Skip to contentSkip to conversation

xt_cgroups fix

PATCH Superseded Daniel Borkmann 4 participants 12 messages Mar 24, 2015 – Mar 25, 2015

Subsystems (2)
Networking netfilter

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Daniel Borkmann

author
#0 lore raw

cover xt_cgroups fix

Hi Pablo,

here's a possible fix for xt_cgroups that was previously reported by Daniel Mack.

The first patch refactors common helpers, which is later on being used by the actual fix. Please see individual patches for more details.

I have based the changes on nf-next as they're rather big, they are, however, on top of Eric's a94070000388 ("netfilter: xt_socket:
prepare for TCP_NEW_SYN_RECV support") from net-next to avoid ugly merge conflicts in xt_socket.

If you nevertheless think it's more suited for nf, or I should ignore the above conflicting commit, I'd be happy to rebase.

Thanks a lot!

Daniel Borkmann (2):
netfilter: x_tables: refactor lookup helpers from xt_socket
netfilter: x_tables: fix NF_INET_LOCAL_IN sk lookups

 net/netfilter/Kconfig        |   5 +
 net/netfilter/xt_cgroup.c    |  86 ++++++++++---
 net/netfilter/xt_sk_helper.h | 282 +++++++++++++++++++++++++++++++++++++++++
 net/netfilter/xt_socket.c    | 293 +++----------------------------------------
 4 files changed, 373 insertions(+), 293 deletions(-)
 create mode 100644 net/netfilter/xt_sk_helper.h
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH 1/2 netfilter: x_tables: refactor lookup helpers from xt_socket

The socket lookup helpers are also needed for fixing xt_cgroups, therefore refactor them into shareable helper functions.

This simplifies and optimizes the xt_socket code itself a bit as well, i.e. time to verdict for early demux sockets should be much faster than previously:

We've unnecessarily extracted proto, {s,d}addr and {s,d}ports from the skb data, accessing possible conntrack information, etc even though we were not even calling into the socket lookup via xt_socket_get_sock_v4() due to skb->sk hit.

After this patch, we only proceed the slow-path when we have a skb->sk miss.

Signed-off-by Daniel Borkmann <[email protected]>
Cc Daniel Mack <[email protected]>
Cc Florian Westphal <[email protected]>
---
 net/netfilter/xt_sk_helper.h | 282 +++++++++++++++++++++++++++++++++++++++++
 net/netfilter/xt_socket.c    | 293 +++----------------------------------------
 2 files changed, 300 insertions(+), 275 deletions(-)
 create mode 100644 net/netfilter/xt_sk_helper.h
net/netfilter/xt_sk_helper.h new file +282 -0
new file mode 100644
index 0000000..604b7ac
@@ -0,0 +1,282 @@
+/*
+ * Transparent proxy support for Linux/iptables
+ *
+ * Copyright (C) 2007-2008 BalaBit IT Ltd.
+ * Author: Krisztian Kovacs
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/skbuff.h>
+
+#include <net/tcp.h>
+#include <net/udp.h>
+#include <net/icmp.h>
+#include <net/sock.h>
+
+#include <net/netfilter/ipv4/nf_defrag_ipv4.h>
+
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
+#define XT_HAVE_IPV6 1
+#include <linux/netfilter_ipv6/ip6_tables.h>
+#include <net/inet6_hashtables.h>
+#include <net/netfilter/ipv6/nf_defrag_ipv6.h>
+#endif
+
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+#define XT_HAVE_CONNTRACK 1
+#include <net/netfilter/nf_conntrack.h>
+#endif
+
+static int extract_icmp4_fields(const struct sk_buff *skb, u8 *protocol,
+				__be32 *raddr, __be32 *laddr, __be16 *rport,
+				__be16 *lport)
+{
+	unsigned int outside_hdrlen = ip_hdrlen(skb);
+	struct iphdr *inside_iph, _inside_iph;
+	struct icmphdr *icmph, _icmph;
+	__be16 *ports, _ports[2];
+
+	icmph = skb_header_pointer(skb, outside_hdrlen,
+				   sizeof(_icmph), &_icmph);
+	if (icmph == NULL)
+		return 1;
+
+	switch (icmph->type) {
+	case ICMP_DEST_UNREACH:
+	case ICMP_SOURCE_QUENCH:
+	case ICMP_REDIRECT:
+	case ICMP_TIME_EXCEEDED:
+	case ICMP_PARAMETERPROB:
+		break;
+	default:
+		return 1;
+	}
+
+	inside_iph = skb_header_pointer(skb, outside_hdrlen +
+					sizeof(struct icmphdr),
+					sizeof(_inside_iph), &_inside_iph);
+	if (inside_iph == NULL)
+		return 1;
+
+	if (inside_iph->protocol != IPPROTO_TCP &&
+	    inside_iph->protocol != IPPROTO_UDP)
+		return 1;
+
+	ports = skb_header_pointer(skb, outside_hdrlen +
+				   sizeof(struct icmphdr) +
+				   (inside_iph->ihl << 2),
+				   sizeof(_ports), &_ports);
+	if (ports == NULL)
+		return 1;
+
+	/* The inside IP packet is the one quoted from our side, thus
+	 * its saddr is the local address.
+	 */
+
+	*protocol = inside_iph->protocol;
+	*laddr = inside_iph->saddr;
+	*lport = ports[0];
+	*raddr = inside_iph->daddr;
+	*rport = ports[1];
+
+	return 0;
+}
+
+static struct sock *xt_get_sk_v4(struct net *net, const u8 protocol,
+				 const __be32 saddr, const __be32 daddr,
+				 const __be16 sport, const __be16 dport,
+				 const struct net_device *in)
+{
+	switch (protocol) {
+	case IPPROTO_TCP:
+		return __inet_lookup(net, &tcp_hashinfo,
+				     saddr, sport, daddr, dport,
+				     in->ifindex);
+	case IPPROTO_UDP:
+		return udp4_lib_lookup(net, saddr, sport, daddr, dport,
+				       in->ifindex);
+	}
+
+	return NULL;
+}
+
+static struct sock *xt_sk_lookup(const struct sk_buff *skb,
+				 const struct net_device *indev)
+{
+	const struct iphdr *iph = ip_hdr(skb);
+	__be32 uninitialized_var(daddr), uninitialized_var(saddr);
+	__be16 uninitialized_var(dport), uninitialized_var(sport);
+	u8 uninitialized_var(protocol);
+#ifdef XT_HAVE_CONNTRACK
+	struct nf_conn const *ct;
+	enum ip_conntrack_info ctinfo;
+#endif
+
+	if (iph->protocol == IPPROTO_UDP || iph->protocol == IPPROTO_TCP) {
+		struct udphdr _hdr, *hp = NULL;
+
+		hp = skb_header_pointer(skb, ip_hdrlen(skb),
+					sizeof(_hdr), &_hdr);
+		if (hp == NULL)
+			return NULL;
+
+		protocol = iph->protocol;
+
+		saddr = iph->saddr;
+		daddr = iph->daddr;
+
+		sport = hp->source;
+		dport = hp->dest;
+
+	} else if (iph->protocol == IPPROTO_ICMP) {
+		if (extract_icmp4_fields(skb, &protocol, &saddr, &daddr,
+					 &sport, &dport))
+			return NULL;
+	} else {
+		return NULL;
+	}
+
+#ifdef XT_HAVE_CONNTRACK
+	/* Do the lookup with the original socket address in
+	 * case this is a reply packet of an established
+	 * SNAT-ted connection.
+	 */
+	ct = nf_ct_get(skb, &ctinfo);
+	if (ct && !nf_ct_is_untracked(ct) &&
+	    ((iph->protocol != IPPROTO_ICMP &&
+	      ctinfo == IP_CT_ESTABLISHED_REPLY) ||
+	     (iph->protocol == IPPROTO_ICMP &&
+	      ctinfo == IP_CT_RELATED_REPLY)) &&
+	    (ct->status & IPS_SRC_NAT_DONE)) {
+
+		daddr = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.ip;
+		dport = (iph->protocol == IPPROTO_TCP) ?
+			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.tcp.port :
+			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
+	}
+#endif
+	return xt_get_sk_v4(dev_net(skb->dev), protocol, saddr, daddr,
+			    sport, dport, indev);
+}
+
+#ifdef XT_HAVE_IPV6
+static int extract_icmp6_fields(const struct sk_buff *skb,
+				unsigned int outside_hdrlen,
+				int *protocol,
+				const struct in6_addr **raddr,
+				const struct in6_addr **laddr,
+				__be16 *rport, __be16 *lport,
+				struct ipv6hdr *ipv6_var)
+{
+	const struct ipv6hdr *inside_iph;
+	struct icmp6hdr *icmph, _icmph;
+	__be16 *ports, _ports[2];
+	u8 inside_nexthdr;
+	__be16 inside_fragoff;
+	int inside_hdrlen;
+
+	icmph = skb_header_pointer(skb, outside_hdrlen,
+				   sizeof(_icmph), &_icmph);
+	if (icmph == NULL)
+		return 1;
+
+	if (icmph->icmp6_type & ICMPV6_INFOMSG_MASK)
+		return 1;
+
+	inside_iph = skb_header_pointer(skb, outside_hdrlen + sizeof(_icmph),
+					sizeof(*ipv6_var), ipv6_var);
+	if (inside_iph == NULL)
+		return 1;
+
+	inside_nexthdr = inside_iph->nexthdr;
+	inside_hdrlen = ipv6_skip_exthdr(skb, outside_hdrlen + sizeof(_icmph) +
+					      sizeof(*ipv6_var),
+					 &inside_nexthdr, &inside_fragoff);
+	if (inside_hdrlen < 0)
+		return 1; /* hjm: Packet has no/incomplete transport layer headers. */
+
+	if (inside_nexthdr != IPPROTO_TCP &&
+	    inside_nexthdr != IPPROTO_UDP)
+		return 1;
+
+	ports = skb_header_pointer(skb, inside_hdrlen,
+				   sizeof(_ports), &_ports);
+	if (ports == NULL)
+		return 1;
+
+	/* The inside IP packet is the one quoted from our side, thus
+	 * its saddr is the local address.
+	 */
+
+	*protocol = inside_nexthdr;
+	*laddr = &inside_iph->saddr;
+	*lport = ports[0];
+	*raddr = &inside_iph->daddr;
+	*rport = ports[1];
+
+	return 0;
+}
+
+static struct sock *xt_get_sk_v6(struct net *net, const u8 protocol,
+				 const struct in6_addr *saddr,
+				 const struct in6_addr *daddr,
+				 const __be16 sport, const __be16 dport,
+				 const struct net_device *in)
+{
+	switch (protocol) {
+	case IPPROTO_TCP:
+		return inet6_lookup(net, &tcp_hashinfo,
+				    saddr, sport, daddr, dport,
+				    in->ifindex);
+	case IPPROTO_UDP:
+		return udp6_lib_lookup(net, saddr, sport, daddr, dport,
+				       in->ifindex);
+	}
+
+	return NULL;
+}
+
+static struct sock *xt_sk_lookup6(const struct sk_buff *skb,
+				  const struct net_device *indev)
+{
+	__be16 uninitialized_var(dport), uninitialized_var(sport);
+	const struct in6_addr *daddr = NULL, *saddr = NULL;
+	struct ipv6hdr *iph = ipv6_hdr(skb);
+	int thoff = 0, tproto;
+
+	tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL);
+	if (tproto < 0) {
+		pr_debug("unable to find transport header in IPv6 packet, dropping\n");
+		return NULL;
+	}
+
+	if (tproto == IPPROTO_UDP || tproto == IPPROTO_TCP) {
+		struct udphdr _hdr, *hp;
+
+		hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr);
+		if (hp == NULL)
+			return NULL;
+
+		saddr = &iph->saddr;
+		daddr = &iph->daddr;
+
+		sport = hp->source;
+		dport = hp->dest;
+
+	} else if (tproto == IPPROTO_ICMPV6) {
+		struct ipv6hdr ipv6_var;
+
+		if (extract_icmp6_fields(skb, thoff, &tproto, &saddr, &daddr,
+					 &sport, &dport, &ipv6_var))
+			return NULL;
+	} else {
+		return NULL;
+	}
+
+	return xt_get_sk_v6(dev_net(skb->dev), tproto, saddr, daddr,
+			    sport, dport, indev);
+}
+#endif /* XT_HAVE_IPV6 */
net/netfilter/xt_socket.c +18 -275

379 lines not on this page · raw message

Signature · 1 line
1.9.3
author
#2 lore raw

PATCH 2/2 netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

In reply to Daniel Borkmann

While originally only being intended for outgoing traffic, commit a00e76349f35 ("netfilter: x_tables: allow to use cgroup match for LOCAL_IN nf hooks") enabled xt_cgroups for the NF_INET_LOCAL_IN hook as well, in order to allow for nfacct accounting.

This basically was under the assumption that socket early demux will resolve it. It's correct that demux happens after PRE_ROUTING, but before LOCAL_IN.

However, that as-is only partially works, i.e. it works for the case of established TCP and connected UDP sockets when early demux is enabled, but not for various other ingress scenarios e.g. unconnected UDP, request sockets, etc.

Instead of reverting commit a00e76349f35, I think it's worth to fix it up as there are applications requiring xt_cgroup to match on ingress and egress side. In order to do so, we need to perform a full lookup on skb->sk (ingress) miss, similarly as being done in xt_socket.

Therefore, we need to make use of shared helpers xt_sk_lookup() and xt_sk_lookup6(). Thanks to Daniel for the report and also additional testing.

Reported-by Daniel Mack <[email protected]>
Fixes a00e76349f35 ("netfilter: x_tables: allow to use cgroup match for LOCAL_IN nf hooks")

Reference: http://thread.gmane.org/gmane.linux.network/355527

Signed-off-by Daniel Borkmann <[email protected]>
Tested-by Daniel Mack <[email protected]>
Cc Alexey Perevalov <[email protected]>
Cc Florian Westphal <[email protected]>
---
 net/netfilter/Kconfig     |  5 +++
 net/netfilter/xt_cgroup.c | 86 +++++++++++++++++++++++++++++++++++++----------
 2 files changed, 73 insertions(+), 18 deletions(-)
net/netfilter/Kconfig +5 -0
index 971cd75..044bd22 100644
@@ -960,8 +960,13 @@ config NETFILTER_XT_MATCH_BPF
 
 config NETFILTER_XT_MATCH_CGROUP
 	tristate '"control group" match support'
+	depends on NETFILTER_XTABLES
 	depends on NETFILTER_ADVANCED
+	depends on !NF_CONNTRACK || NF_CONNTRACK
+	depends on (IPV6 || IPV6=n)
 	depends on CGROUPS
+	select NF_DEFRAG_IPV4
+	select NF_DEFRAG_IPV6 if IP6_NF_IPTABLES
 	select CGROUP_NET_CLASSID
 	---help---
 	Socket/process control group matching allows you to match locally
net/netfilter/xt_cgroup.c +68 -18

114 lines not on this page · raw message

Signature · 1 line
1.9.3
maintainer
#3 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

Quoted 26 lines · Daniel Borkmann
net/netfilter/xt_cgroup.c +7 -2
index 7198d66..cd2468d 100644
@@ -16,8 +16,11 @@
 #include <linux/module.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_cgroup.h>
+
 #include <net/sock.h>
 
+#include "xt_sk_helper.h"
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Daniel Borkmann <[email protected]>");
 MODULE_DESCRIPTION("Xtables: process control group matching");

@@ -34,38 +37,85 @@ static int cgroup_mt_check(const struct xt_mtchk_param *par)
 	return 0;
 }
 
-static bool
-cgroup_mt(const struct sk_buff *skb, struct xt_action_param *par)
+static bool cgroup_mt(const struct sk_buff *skb,
+		      const struct xt_action_param *par,
+		      struct sock *(*cgroup_mt_slow)(const struct sk_buff *skb,
+						     const struct net_device *indev))

net/netfilter/xt_cgroup.c static int cgroup_mt_check(const struct xt_mtchk_param *par) · line 53

Quoted 10 lines
net/netfilter/xt_cgroup.c +8 -0
 {
 	const struct xt_cgroup_info *info = par->matchinfo;
+	struct sock *sk = skb->sk;
+	u32 sk_classid;
+
+	if (sk) {
+		sk_classid = sk->sk_classid;
+	} else {
+		if (par->in != NULL)
+			sk = cgroup_mt_slow(skb, par->in);

Is this working with timewait sock?

Quoted 9 lines
net/netfilter/xt_cgroup.c +9 -0
+		if (sk == NULL)
+			return false;
+
+		sk_classid = sk->sk_classid;
+		sock_gen_put(sk);
+	}
+
+	return (info->id == sk_classid) ^ info->invert;
+}
author
#4 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

Quoted 31 lines · Pablo Neira Ayuso
Quoted 30 lines · Daniel Borkmann
net/netfilter/xt_cgroup.c +9 -2
index 7198d66..cd2468d 100644
@@ -16,8 +16,11 @@
  #include <linux/module.h>
  #include <linux/netfilter/x_tables.h>
  #include <linux/netfilter/xt_cgroup.h>
+
  #include <net/sock.h>
 
+#include "xt_sk_helper.h"
+
  MODULE_LICENSE("GPL");
  MODULE_AUTHOR("Daniel Borkmann <[email protected]>");
  MODULE_DESCRIPTION("Xtables: process control group matching");

@@ -34,38 +37,85 @@ static int cgroup_mt_check(const struct xt_mtchk_param *par)
  	return 0;
  }
 
-static bool
-cgroup_mt(const struct sk_buff *skb, struct xt_action_param *par)
+static bool cgroup_mt(const struct sk_buff *skb,
+		      const struct xt_action_param *par,
+		      struct sock *(*cgroup_mt_slow)(const struct sk_buff *skb,
+						     const struct net_device *indev))
  {
  	const struct xt_cgroup_info *info = par->matchinfo;
+	struct sock *sk = skb->sk;
+	u32 sk_classid;
Quoted 8 lines

net/netfilter/xt_cgroup.c static int cgroup_mt_check(const struct xt_mtchk_param *par) · line 53

Quoted 6 lines
net/netfilter/xt_cgroup.c +6 -0
+
+	if (sk) {
+		sk_classid = sk->sk_classid;
+	} else {
+		if (par->in != NULL)
+			sk = cgroup_mt_slow(skb, par->in);

Is this working with timewait sock?

Yes, all socket objects that are allocated (sk_alloc()) get a sk_classid of the current task. Given that both share the same lookup handler, we don't ignore them here as some xt_socket flags could after the lookup optionally do.

Quoted 9 lines
Quoted 9 lines
net/netfilter/xt_cgroup.c +9 -0
+		if (sk == NULL)
+			return false;
+
+		sk_classid = sk->sk_classid;
+		sock_gen_put(sk);
+	}
+
+	return (info->id == sk_classid) ^ info->invert;
+}
maintainer
#5 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

Quoted 41 lines · Daniel Borkmann
Quoted 31 lines · Pablo Neira Ayuso
Quoted 30 lines · Daniel Borkmann
net/netfilter/xt_cgroup.c +9 -2
index 7198d66..cd2468d 100644
@@ -16,8 +16,11 @@
 #include <linux/module.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_cgroup.h>
+
 #include <net/sock.h>
 
+#include "xt_sk_helper.h"
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Daniel Borkmann <[email protected]>");
 MODULE_DESCRIPTION("Xtables: process control group matching");

@@ -34,38 +37,85 @@ static int cgroup_mt_check(const struct xt_mtchk_param *par)
 	return 0;
 }
 
-static bool
-cgroup_mt(const struct sk_buff *skb, struct xt_action_param *par)
+static bool cgroup_mt(const struct sk_buff *skb,
+		      const struct xt_action_param *par,
+		      struct sock *(*cgroup_mt_slow)(const struct sk_buff *skb,
+						     const struct net_device *indev))
 {
 	const struct xt_cgroup_info *info = par->matchinfo;
+	struct sock *sk = skb->sk;
+	u32 sk_classid;
Quoted 8 lines

net/netfilter/xt_cgroup.c static int cgroup_mt_check(const struct xt_mtchk_param *par) · line 53

Quoted 6 lines
net/netfilter/xt_cgroup.c +6 -0
+
+	if (sk) {
+		sk_classid = sk->sk_classid;
+	} else {
+		if (par->in != NULL)
+			sk = cgroup_mt_slow(skb, par->in);

Is this working with timewait sock?

Quoted 4 lines

Yes, all socket objects that are allocated (sk_alloc()) get a sk_classid of the current task. Given that both share the same lookup handler, we don't ignore them here as some xt_socket flags could after the lookup optionally do.

I mean, we may get a packet from the input path while in TIME_WAIT, and sk will be actually a inet_timewait_sock, which has a different layout (no sk_classid).

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
net/netfilter/xt_cgroup.c +9 -0
+		if (sk == NULL)
+			return false;
+
+		sk_classid = sk->sk_classid;
+		sock_gen_put(sk);
+	}
+
+	return (info->id == sk_classid) ^ info->invert;
+}
author
#6 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

On 03/25/2015 06:17 PM, Pablo Neira Ayuso wrote:
...

Quoted 3 lines

I mean, we may get a packet from the input path while in TIME_WAIT, and sk will be actually a inet_timewait_sock, which has a different layout (no sk_classid).

Sorry, you are correct, thanks for the hint. We would actually need to test if we deal with a full socket, iow sk_fullsock(sk).

maintainer
#7 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

In reply to Daniel Borkmann
Quoted 20 lines · Daniel Borkmann

While originally only being intended for outgoing traffic, commit a00e76349f35 ("netfilter: x_tables: allow to use cgroup match for LOCAL_IN nf hooks") enabled xt_cgroups for the NF_INET_LOCAL_IN hook as well, in order to allow for nfacct accounting.

This basically was under the assumption that socket early demux will resolve it. It's correct that demux happens after PRE_ROUTING, but before LOCAL_IN.

However, that as-is only partially works, i.e. it works for the case of established TCP and connected UDP sockets when early demux is enabled, but not for various other ingress scenarios e.g. unconnected UDP, request sockets, etc.

Instead of reverting commit a00e76349f35, I think it's worth to fix it up as there are applications requiring xt_cgroup to match on ingress and egress side. In order to do so, we need to perform a full lookup on skb->sk (ingress) miss, similarly as being done in xt_socket.

Quoted 3 lines

Therefore, we need to make use of shared helpers xt_sk_lookup() and xt_sk_lookup6(). Thanks to Daniel for the report and also additional testing.

So this is basically needed when early demux is disabled?

This is a rather large rework, I would like to know what scenarios we're not currently catching with the existing code.

author
#8 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

Quoted 21 lines · Pablo Neira Ayuso
Quoted 20 lines · Daniel Borkmann

While originally only being intended for outgoing traffic, commit a00e76349f35 ("netfilter: x_tables: allow to use cgroup match for LOCAL_IN nf hooks") enabled xt_cgroups for the NF_INET_LOCAL_IN hook as well, in order to allow for nfacct accounting.

This basically was under the assumption that socket early demux will resolve it. It's correct that demux happens after PRE_ROUTING, but before LOCAL_IN.

However, that as-is only partially works, i.e. it works for the case of established TCP and connected UDP sockets when early demux is enabled, but not for various other ingress scenarios e.g. unconnected UDP, request sockets, etc.

Instead of reverting commit a00e76349f35, I think it's worth to fix it up as there are applications requiring xt_cgroup to match on ingress and egress side. In order to do so, we need to perform a full lookup on skb->sk (ingress) miss, similarly as being done in xt_socket.

Quoted 8 lines
Quoted 3 lines

Therefore, we need to make use of shared helpers xt_sk_lookup() and xt_sk_lookup6(). Thanks to Daniel for the report and also additional testing.

So this is basically needed when early demux is disabled?

This is a rather large rework, I would like to know what scenarios we're not currently catching with the existing code.

Hm, perhaps Daniel can elaborate better, what I have seen in my testing when xt_cgroup fails to match the cgroup on ingress traffic is i) early demux sysctl disabled, ii) udp on unconnected sockets (which I understand is the majority of udp traffic), iii) tcp and udp (any kind) on localhost communications. Daniel's original report can be found here [1].

Thanks,
Daniel

[1] http://thread.gmane.org/gmane.linux.network/355527

#9 lore raw

Re: [PATCH nf-next 2/2] netfilter: x_tables: fix cgroup's NF_INET_LOCAL_IN sk lookups

Quoted 7 lines · Daniel Borkmann
Quoted 4 lines · Pablo Neira Ayuso

So this is basically needed when early demux is disabled?

This is a rather large rework, I would like to know what scenarios we're not currently catching with the existing code.

Quoted 6 lines

Hm, perhaps Daniel can elaborate better, what I have seen in my testing when xt_cgroup fails to match the cgroup on ingress traffic is i) early demux sysctl disabled, ii) udp on unconnected sockets (which I understand is the majority of udp traffic), iii) tcp and udp (any kind) on localhost communications. Daniel's original report can be found here [1].

Currently, ingress matching fails if the xt_cgroup module's match callback is called with skb->sk == NULL, which is the case in the scenarios described above. Also, according to Cong, this is as well always the case if the ingress network device is 'lo'.

We want to use xt_cgroup to realize a per-application firewall for both filtering and accounting. For this, being able to catch every network packet that is destined for or originated by a task that is assigned to a certain net_cls CGroup controller is essential. Also, the match has to be effective regardless of the network interface in use.

In my tests, Daniel's patches work perfectly fine.

Thanks,
Daniel

maintainer
#10 lore raw
In reply to Daniel Borkmann
Quoted 7 lines · Daniel Borkmann

here's a possible fix for xt_cgroups that was previously reported by Daniel Mack.

The first patch refactors common helpers, which is later on being used by the actual fix. Please see individual patches for more details.

Quoted 7 lines

I have based the changes on nf-next as they're rather big, they are, however, on top of Eric's a94070000388 ("netfilter: xt_socket:
prepare for TCP_NEW_SYN_RECV support") from net-next to avoid ugly merge conflicts in xt_socket.

If you nevertheless think it's more suited for nf, or I should ignore the above conflicting commit, I'd be happy to rebase.

My main problem with these patches is that we're now paving the way for skb->sk testing in input, i.e. doing protocol demuxing steps in iptables modules.

E.g. why not accept similar patch for xt_owner?
What about sctp (or any other protocol) support?

I don't see anything wrong with the implementation per se but I'm afraid we're starting down a slippery slope here.

author
#11 lore raw
Quoted 13 lines · Florian Westphal
Quoted 12 lines · Daniel Borkmann

here's a possible fix for xt_cgroups that was previously reported by Daniel Mack.

The first patch refactors common helpers, which is later on being used by the actual fix. Please see individual patches for more details.

I have based the changes on nf-next as they're rather big, they are, however, on top of Eric's a94070000388 ("netfilter: xt_socket:
prepare for TCP_NEW_SYN_RECV support") from net-next to avoid ugly merge conflicts in xt_socket.

Quoted 6 lines
Quoted 2 lines

If you nevertheless think it's more suited for nf, or I should ignore the above conflicting commit, I'd be happy to rebase.

My main problem with these patches is that we're now paving the way for skb->sk testing in input, i.e. doing protocol demuxing steps in iptables modules.

Well, we're doing this in xt_socket already, here it's just fixing a real issue in xt_cgroup. The only alternative I currently see would be to revert Alexey's commit, but that would limit the scope of possibilities for xt_cgroup quite a lot. :(

Quoted 2 lines

E.g. why not accept similar patch for xt_owner?
What about sctp (or any other protocol) support?

Right, I see your concern, but at the same time I think that i.e. SCTP has much more severe problems, i) being the horrible state of the stack implementation itself ;), ii) being the lack of more important features in iptables (i.e. NAT on multi-homing). Anyway, taken that aside, you could restrict that support, as we currently do only for available early demuxes, but given that activity on SCTP I truly doubt that's coming any time soon.

Quoted 2 lines

I don't see anything wrong with the implementation per se but I'm afraid we're starting down a slippery slope here.