[libvirt] [PATCH] nwfilter: probe for inverted ctdir

Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instread and vice-versa. We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 123 ++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -27,6 +27,10 @@ #include <string.h> #include <sys/stat.h> #include <fcntl.h> +#include <arpa/inet.h> +#include <sys/select.h> +#include <sys/time.h> +#include <unistd.h> #include "internal.h" @@ -85,6 +89,12 @@ static char *iptables_cmd_path; static char *ip6tables_cmd_path; static char *grep_cmd_path; +/* + * --ctdir original vs. reply's meaning was inverted in the netfilter + * at some point. We probe for it. + */ +static bool iptables_ctdir_corrected = false; + #define PRINT_ROOT_CHAIN(buf, prefix, ifname) \ snprintf(buf, sizeof(buf), "libvirt-%c-%s", prefix, ifname) #define PRINT_CHAIN(buf, prefix, ifname, suffix) \ @@ -1262,6 +1272,9 @@ iptablesEnforceDirection(int directionIn virNWFilterRuleDefPtr rule, virBufferPtr buf) { + if (iptables_ctdir_corrected) + directionIn = !directionIn; + if (rule->tt != VIR_NWFILTER_RULE_DIRECTION_INOUT) virBufferAsprintf(buf, " -m conntrack --ctdir %s", (directionIn) ? "Original" @@ -4304,6 +4317,113 @@ ebiptablesDriverTestCLITools(void) return ret; } +static void +ebiptablesDriverProbeCtdir(void) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + static const char cmdline[] = + "$IPT -%c INPUT %c -i lo -p udp --dport %hu " + "-m state --state ESTABLISHED -j ACCEPT " CMD_SEPARATOR + "$IPT -%c INPUT %c -i lo -p udp --dport %hu " + "-m conntrack --ctdir original -j ACCEPT " CMD_SEPARATOR + "$IPT -%c INPUT %c -i lo -p udp --dport %hu -j DROP"; + /* + * Above '--ctdir original' gets this test to receive a message on + * 'fixed' netfilter. + */ + unsigned short port; + int ssockfd = -1, csockfd = -1; + struct sockaddr_in serveraddr = { + .sin_family = AF_INET, + }; + fd_set readfds; + struct timeval timeout = { + .tv_sec = 0, + .tv_usec = 1000 * 200, + }; + int n; + + if (inet_aton("127.0.0.1", &serveraddr.sin_addr) == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "inet_aton failed"); + goto cleanup; + } + + if ((ssockfd = socket(AF_INET, SOCK_DGRAM, 0)) < 0 || + (csockfd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { + VIR_ERROR(_("Could not open UDP socket")); + goto cleanup; + } + + for (port = 0xffff; port > 1024; port--) { + serveraddr.sin_port = htons(port); + if (bind(ssockfd, (struct sockaddr *)&serveraddr, + sizeof(serveraddr)) == 0) + break; + } + if (port == 1024) { + VIR_ERROR(_("Could not bind to any UDP socket")); + goto cleanup; + } + + NWFILTER_SET_IPTABLES_SHELLVAR(&buf); + virBufferAsprintf(&buf, cmdline, + 'I', '1', port, + 'I', '2', port, + 'I', '3', port); + + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + + if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) { + VIR_ERROR(_("Could not apply iptables rules")); + goto cleanup_iptables; + } + + if (sendto(csockfd, cmdline, 1, 0, (struct sockaddr *)&serveraddr, + sizeof(serveraddr)) < 0) { + VIR_ERROR(_("Could not send to UDP socket")); + goto cleanup_iptables; + } + + FD_ZERO(&readfds); + FD_SET(ssockfd, &readfds); + + while (true) { + n = select(ssockfd + 1, &readfds, NULL, NULL, &timeout); + if (n < 0) { + if (errno == EINTR) + continue; + VIR_ERROR(_("Select failed")); + goto cleanup_iptables; + } + if (n == 0) { + VIR_INFO("Ctdir probing received no data -- 'old' netfilter"); + goto cleanup_iptables; + } + VIR_INFO("Ctdir probing received data -- 'fixed' netfilter"); + iptables_ctdir_corrected = true; + break; + } + +cleanup_iptables: + virBufferFreeAndReset(&buf); + + NWFILTER_SET_IPTABLES_SHELLVAR(&buf); + virBufferAsprintf(&buf, cmdline, + 'D', ' ', port, + 'D', ' ', port, + 'D', ' ', port); + ebiptablesExecCLI(&buf, NULL, NULL); + +cleanup: + virBufferFreeAndReset(&buf); + VIR_FORCE_CLOSE(ssockfd); + VIR_FORCE_CLOSE(csockfd); +} + static int ebiptablesDriverInit(bool privileged) { @@ -4341,6 +4461,9 @@ ebiptablesDriverInit(bool privileged) return -ENOTSUP; } + if (iptables_cmd_path) + ebiptablesDriverProbeCtdir(); + ebiptables_driver.flags = TECHDRV_FLAG_INITIALIZED; return 0;

On 03/21/2013 04:04 PM, Stefan Berger wrote:
Linux netfilter at some point inverted the meaning of the '--ctdir reply' and newer netfilter implementations now expect '--ctdir original' instread and vice-versa.
s/instread/instead/
We probe for this netfilter change via a UDP message over loopback and 3 filtering rules applied to INPUT. If the sent byte arrives, the newer netfilter implementation has been detected.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- src/nwfilter/nwfilter_ebiptables_driver.c | 123 ++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)
+/* + * --ctdir original vs. reply's meaning was inverted in the netfilter + * at some point. We probe for it. + */ +static bool iptables_ctdir_corrected = false;
C guarantees that this is initialized to false without having to explicitly state that. Looks big, but it's a one-time probe done at initialization, and seems like it does the trick. You may want to wait for a review from Laine, but I didn't spot anything else wrong. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/21/2013 06:26 PM, Eric Blake wrote:
On 03/21/2013 04:04 PM, Stefan Berger wrote: C guarantees that this is initialized to false without having to explicitly state that.
Looks big, but it's a one-time probe done at initialization, and seems like it does the trick. You may want to wait for a review from Laine, but I didn't spot anything else wrong.
One problem: src/nwfilter/nwfilter_ebiptables_driver.c:4346: if (inet_aton("127.0.0.1", &serveraddr.sin_addr) == 0) { maint.mk: use inet_aton_r, not inet_aton I don't think this function exists...

On 03/21/2013 06:40 PM, Stefan Berger wrote:
On 03/21/2013 06:26 PM, Eric Blake wrote:
On 03/21/2013 04:04 PM, Stefan Berger wrote: C guarantees that this is initialized to false without having to explicitly state that.
Looks big, but it's a one-time probe done at initialization, and seems like it does the trick. You may want to wait for a review from Laine, but I didn't spot anything else wrong.
One problem:
src/nwfilter/nwfilter_ebiptables_driver.c:4346: if (inet_aton("127.0.0.1", &serveraddr.sin_addr) == 0) { maint.mk: use inet_aton_r, not inet_aton
I don't think this function exists...
Indeed not the best of messages from cfg.mk; but the explanation lies in Makefile.nonreentrant - we are explicitly rejecting inet_aton() in favor of getaddrinfo(). For that matter, src/util/virsocketaddr.c:virSocketAddrParse() might make your attempt more compact. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Stefan Berger