Netfilter ingress support (v3)

Hi,

Another round of the patchset to add Netfilter ingress support. This new patchset introduces the necessary updates in 2 steps:

1) Add minismalistic ingress hook infrastructure that allows to register one client at a time, so you hit -EBUSY in case the hook is in use. Basically, we have a function pointer that is rcu-protected to invoke the corresponding filter framework which has minimal performance impact in the critical ingress path and avoid more pollution in it. This patch also ports the ingress qdisc on top of this.

This also results in most of the qdisc ingress code that used to be embedded into net/core/dev.c can now be placed in net/sched/sch_ingress.c, which should allow to get rid of the Qdisc->enqueue() call.

2) Add Netfilter ingress support using the minimalistic hook infrastructure. There is some extra memory consumption (24 bytes) in net_device but pahole reports here a hole due to ____cacheline_aligned_in_smp to get the transmit path area in a different cache line. So I'm not sure it's worth the effort to reduce this to 8 bytes at the cost of getting the hook code a bit more complicated.

As already said, this opens the window to existing nftables core features that are not present in qdisc ingress and that can be used out-of-the-box, most relevantly:

1) Multi-dimensional key dictionary lookups.
2) Arbitrary stateful flow tables.
3) Transactions.

Among others. You can find more on previous RFCs, see:

http://www.spinics.net/lists/netdev/msg325210.html
http://marc.info/?l=netfilter-devel&m=143033337020328&w=2

In summary, this provides the facility to keep both tc and netfilter in place, while the user can select what they prefer to filter from ingress.

Thanks.

Pablo Neira Ayuso (4):
net: add minimalistic ingress filter hook and port sch_ingress on top of it
netfilter: cleanup struct nf_hook_ops indentation
netfilter: add hook list to nf_hook_state
net: add netfilter ingress hook

 include/linux/netdevice.h         |   16 ++++++++
 include/linux/netfilter.h         |   22 +++++-----
 include/linux/netfilter_ingress.h |   26 ++++++++++++
 include/uapi/linux/netfilter.h    |    6 +++
 net/Kconfig                       |    9 +++++
 net/core/dev.c                    |   81 ++++++++++++++++++-------------------
 net/netfilter/Makefile            |    1 +
 net/netfilter/core.c              |   29 ++++++++++---
 net/netfilter/ingress.c           |   41 +++++++++++++++++++
 net/sched/Kconfig                 |    1 +
 net/sched/sch_ingress.c           |   38 +++++++++++++++--
 11 files changed, 211 insertions(+), 59 deletions(-)
 create mode 100644 include/linux/netfilter_ingress.h
 create mode 100644 net/netfilter/ingress.c

1.7.10.4

This patch adds a minimalistic hook infrastructure in netif_receive_core() that allows you to attach one hook function at a time. In case that is already in use, you will hit -EBUSY. The first client of this is sch_ingress that has been ported on top of it. The abstraction is lightweight to avoid performance concerns, and it is ruled by a global static key.

---
 include/linux/netdevice.h |   13 ++++++++
 net/Kconfig               |    3 ++
 net/core/dev.c            |   79 +++++++++++++++++++++------------------------
 net/sched/Kconfig         |    1 +
 net/sched/sch_ingress.c   |   38 ++++++++++++++++++++--
 5 files changed, 89 insertions(+), 45 deletions(-)

index 1899c74..18e1500 100644
@@ -770,6 +770,15 @@ struct netdev_phys_item_id {
 typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
 				       struct sk_buff *skb);
 
+/* This allows you to register and to unregister a function the hook for
+ * ingress filtering.
+ */
+typedef struct sk_buff *ingress_hook_func_t(struct sk_buff *skb);
+
+int dev_ingress_hook_register(struct net_device *dev,
+			      ingress_hook_func_t *hookfn);
+void dev_ingress_hook_unregister(struct net_device *dev);
+
 /*
  * This structure defines the management hooks for network devices.
  * The following hooks can be defined; unless noted otherwise, they are

@@ -1655,7 +1664,11 @@ struct net_device {
 	rx_handler_func_t __rcu	*rx_handler;
 	void __rcu		*rx_handler_data;
 
+#ifdef CONFIG_NET_INGRESS_HOOK
+	ingress_hook_func_t __rcu *ingress_hook;
+#endif
 	struct netdev_queue __rcu *ingress_queue;
+
 	unsigned char		broadcast[MAX_ADDR_LEN];
 #ifdef CONFIG_RFS_ACCEL
 	struct cpu_rmap		*rx_cpu_rmap;

index 44dd578..f0e2f3f 100644
@@ -230,6 +230,9 @@ source "net/mpls/Kconfig"
 source "net/hsr/Kconfig"
 source "net/switchdev/Kconfig"
 
+config NET_INGRESS_HOOK
+	bool
+
 config RPS
 	bool
 	depends on SMP && SYSFS

index 2274e72..3cef39e 100644
@@ -312,6 +312,7 @@ config NET_SCH_PIE
 config NET_SCH_INGRESS
 	tristate "Ingress Qdisc"
 	depends on NET_CLS_ACT
+	select NET_INGRESS_HOOK
 	---help---
 	  Say Y here if you want to use classifiers for incoming packets.
 	  If unsure, say Y.

index a89cc32..38ddef7 100644
@@ -88,12 +88,44 @@ static int ingress_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 /* ------------------------------------------------------------- */
 
+static int ingress_filter(struct sk_buff *skb, struct netdev_queue *rxq)
+{
+	int result = TC_ACT_OK;
+	struct Qdisc *q;
+
+	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
+
+	q = rcu_dereference(rxq->qdisc);
+	if (q != &noop_qdisc) {
+		if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
+			result = qdisc_enqueue_root(skb, q);
+	}
+
+	return result;
+}
+
+static struct sk_buff *qdisc_ingress_hook(struct sk_buff *skb)
+{
+	struct netdev_queue *rxq = rcu_dereference(skb->dev->ingress_queue);
+
+	if (!rxq || rcu_access_pointer(rxq->qdisc) == &noop_qdisc)
+		return skb;
+
+	switch (ingress_filter(skb, rxq)) {
+	case TC_ACT_SHOT:
+	case TC_ACT_STOLEN:
+		kfree_skb(skb);
+		return 0;
+	}
+
+	return skb;
+}
+
 static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 {
-	net_inc_ingress_queue();
 	sch->flags |= TCQ_F_CPUSTATS;
 
-	return 0;
+	return dev_ingress_hook_register(qdisc_dev(sch), qdisc_ingress_hook);
 }
 
 static void ingress_destroy(struct Qdisc *sch)

@@ -101,7 +133,7 @@ static void ingress_destroy(struct Qdisc *sch)
 	struct ingress_qdisc_data *p = qdisc_priv(sch);
 
 	tcf_destroy_chain(&p->filter_list);
-	net_dec_ingress_queue();
+	dev_ingress_hook_unregister(qdisc_dev(sch));
 }
 
 static int ingress_dump(struct Qdisc *sch, struct sk_buff *skb)

1.7.10.4

---
 include/linux/netfilter.h |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 63560d0..83be4a3 100644
@@ -79,16 +79,16 @@ typedef unsigned int nf_hookfn(const struct nf_hook_ops *ops,
 			       const struct nf_hook_state *state);
 
 struct nf_hook_ops {
-	struct list_head list;
+	struct list_head 	list;
 
 	/* User fills in from here down. */
-	nf_hookfn	*hook;
-	struct module	*owner;
-	void		*priv;
-	u_int8_t	pf;
-	unsigned int	hooknum;
+	nf_hookfn		*hook;
+	struct module		*owner;
+	void			*priv;
+	u_int8_t		pf;
+	unsigned int		hooknum;
 	/* Hooks are ordered in ascending priority. */
-	int		priority;
+	int			priority;
 };
 
 struct nf_sockopt_ops {

1.7.10.4

---
 include/linux/netfilter.h |    7 +++++--
 net/netfilter/core.c      |    6 ++----
 2 files changed, 7 insertions(+), 6 deletions(-)

index 83be4a3..388ed19 100644
@@ -54,10 +54,12 @@ struct nf_hook_state {
 	struct net_device *in;
 	struct net_device *out;
 	struct sock *sk;
+	struct list_head *hook_list;
 	int (*okfn)(struct sock *, struct sk_buff *);
 };
 
 static inline void nf_hook_state_init(struct nf_hook_state *p,
+				      struct list_head *hook_list,
 				      unsigned int hook,
 				      int thresh, u_int8_t pf,
 				      struct net_device *indev,

@@ -71,6 +73,7 @@ static inline void nf_hook_state_init(struct nf_hook_state *p,
 	p->in = indev;
 	p->out = outdev;
 	p->sk = sk;
+	p->hook_list = hook_list;
 	p->okfn = okfn;
 }
 

@@ -166,8 +169,8 @@ static inline int nf_hook_thresh(u_int8_t pf, unsigned int hook,
 	if (nf_hooks_active(pf, hook)) {
 		struct nf_hook_state state;
 
-		nf_hook_state_init(&state, hook, thresh, pf,
-				   indev, outdev, sk, okfn);
+		nf_hook_state_init(&state, &nf_hooks[pf][hook], hook, thresh,
+				   pf, indev, outdev, sk, okfn);
 		return nf_hook_slow(skb, &state);
 	}
 	return 1;

index e616301..e418cfd 100644
@@ -166,11 +166,9 @@ int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state)
 	/* We may already have this, but read-locks nest anyway */
 	rcu_read_lock();
 
-	elem = list_entry_rcu(&nf_hooks[state->pf][state->hook],
-			      struct nf_hook_ops, list);
+	elem = list_entry_rcu(state->hook_list, struct nf_hook_ops, list);
 next_hook:
-	verdict = nf_iterate(&nf_hooks[state->pf][state->hook], skb, state,
-			     &elem);
+	verdict = nf_iterate(state->hook_list, skb, state, &elem);
 	if (verdict == NF_ACCEPT || verdict == NF_STOP) {
 		ret = 1;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_DROP) {

1.7.10.4

This patch adds a new NFPROTO_NETDEV family that allows you to register per-device hooks from the ingress path. This is built upon the minimalistic ingress hook infrastructure.

The caller is responsible for holding/putting the reference on the net_device that is attached to nf_hook_ops.

---
 include/linux/netdevice.h         |    3 +++
 include/linux/netfilter.h         |    1 +
 include/linux/netfilter_ingress.h |   26 +++++++++++++++++++++++
 include/uapi/linux/netfilter.h    |    6 ++++++
 net/Kconfig                       |    6 ++++++
 net/core/dev.c                    |    3 +++
 net/netfilter/Makefile            |    1 +
 net/netfilter/core.c              |   23 ++++++++++++++++++++-
 net/netfilter/ingress.c           |   41 +++++++++++++++++++++++++++++++++++++
 9 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/netfilter_ingress.h
 create mode 100644 net/netfilter/ingress.c

index 18e1500..8333feb 100644
@@ -1668,6 +1668,9 @@ struct net_device {
 	ingress_hook_func_t __rcu *ingress_hook;
 #endif
 	struct netdev_queue __rcu *ingress_queue;
+#ifdef CONFIG_NETFILTER_INGRESS
+	struct list_head 	nf_hooks_ingress;
+#endif
 
 	unsigned char		broadcast[MAX_ADDR_LEN];
 #ifdef CONFIG_RFS_ACCEL

index 388ed19..f91715a 100644
@@ -90,6 +90,7 @@ struct nf_hook_ops {
 	void			*priv;
 	u_int8_t		pf;
 	unsigned int		hooknum;
+	struct net_device	*dev;
 	/* Hooks are ordered in ascending priority. */
 	int			priority;
 };

new file mode 100644
index 0000000..5d94872
@@ -0,0 +1,26 @@
+#ifndef _NETFILTER_INGRESS_H_
+#define _NETFILTER_INGRESS_H_
+
+#include <linux/netdevice.h>
+
+#ifdef CONFIG_NETFILTER_INGRESS
+struct list_head *nf_register_ingress_hook(struct nf_hook_ops *reg);
+void nf_unregister_ingress_hook(struct nf_hook_ops *reg);
+
+static inline void nf_hook_ingress_init(struct net_device *dev)
+{
+	INIT_LIST_HEAD(&dev->nf_hooks_ingress);
+}
+#else
+static inline struct list_head *
+nf_register_ingress_hook(struct nf_hook_ops *reg)
+{
+	return &nf_hooks[reg->pf][reg->hooknum];
+}
+
+static inline void nf_unregister_ingress_hook(struct nf_hook_ops *reg) {}
+
+static inline void nf_hook_ingress_init(struct net_device *dev) {}
+#endif
+
+#endif /* _NETFILTER_INGRESS_H_ */

index ef1b1f8..177027c 100644
@@ -51,11 +51,17 @@ enum nf_inet_hooks {
 	NF_INET_NUMHOOKS
 };
 
+enum nf_dev_hooks {
+	NF_NETDEV_INGRESS,
+	NF_NETDEV_NUMHOOKS
+};
+
 enum {
 	NFPROTO_UNSPEC =  0,
 	NFPROTO_INET   =  1,
 	NFPROTO_IPV4   =  2,
 	NFPROTO_ARP    =  3,
+	NFPROTO_NETDEV =  5,
 	NFPROTO_BRIDGE =  7,
 	NFPROTO_IPV6   = 10,
 	NFPROTO_DECNET = 12,

index f0e2f3f..78d58c9 100644
@@ -191,6 +191,12 @@ config BRIDGE_NETFILTER
 
 	  If unsure, say N.
 
+config NETFILTER_INGRESS
+	bool "Netfilter ingress hooks"
+	select NET_INGRESS_HOOK
+	help
+	  You can say Y here if you want to enable Netfilter ingress hook.
+
 source "net/netfilter/Kconfig"
 source "net/ipv4/netfilter/Kconfig"
 source "net/ipv6/netfilter/Kconfig"

index 126d0b1..99d8728 100644
@@ -135,6 +135,7 @@
 #include <linux/if_macvlan.h>
 #include <linux/errqueue.h>
 #include <linux/hrtimer.h>
+#include <linux/netfilter_ingress.h>
 
 #include "net-sysfs.h"
 

@@ -6841,6 +6842,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 #ifdef CONFIG_NET_INGRESS_HOOK
 	RCU_INIT_POINTER(dev->ingress_hook, NULL);
 #endif
+	nf_hook_ingress_init(dev);
+
 #ifdef CONFIG_SYSFS
 	dev->num_rx_queues = rxqs;
 	dev->real_num_rx_queues = rxqs;

index a87d8b8..f6923e2 100644
@@ -1,4 +1,5 @@
 netfilter-objs := core.o nf_log.o nf_queue.o nf_sockopt.o
+netfilter-$(CONFIG_NETFILTER_INGRESS) += ingress.o
 
 nf_conntrack-y	:= nf_conntrack_core.o nf_conntrack_standalone.o nf_conntrack_expect.o nf_conntrack_helper.o nf_conntrack_proto.o nf_conntrack_l3proto_generic.o nf_conntrack_proto_generic.o nf_conntrack_proto_tcp.o nf_conntrack_proto_udp.o nf_conntrack_extend.o nf_conntrack_acct.o nf_conntrack_seqadj.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_TIMEOUT) += nf_conntrack_timeout.o

index e418cfd..370ea06 100644
@@ -22,6 +22,7 @@
 #include <linux/proc_fs.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/netfilter_ingress.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
 

@@ -64,10 +65,23 @@ static DEFINE_MUTEX(nf_hook_mutex);
 
 int nf_register_hook(struct nf_hook_ops *reg)
 {
+	struct list_head *nf_hook_list;
 	struct nf_hook_ops *elem;
 
 	mutex_lock(&nf_hook_mutex);
-	list_for_each_entry(elem, &nf_hooks[reg->pf][reg->hooknum], list) {
+	switch (reg->pf) {
+	case NFPROTO_NETDEV:
+		nf_hook_list = nf_register_ingress_hook(reg);
+		if (IS_ERR(nf_hook_list)) {
+			mutex_unlock(&nf_hook_mutex);
+			return PTR_ERR(nf_hook_list);
+		}
+		break;
+	default:
+		nf_hook_list = &nf_hooks[reg->pf][reg->hooknum];
+		break;
+	}
+	list_for_each_entry(elem, nf_hook_list, list) {
 		if (reg->priority < elem->priority)
 			break;
 	}

@@ -84,6 +98,13 @@ void nf_unregister_hook(struct nf_hook_ops *reg)
 {
 	mutex_lock(&nf_hook_mutex);
 	list_del_rcu(&reg->list);
+	switch (reg->pf) {
+	case NFPROTO_NETDEV:
+		nf_unregister_ingress_hook(reg);
+		break;
+	default:
+		break;
+	}
 	mutex_unlock(&nf_hook_mutex);
 #ifdef HAVE_JUMP_LABEL
 	static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]);

1.7.10.4

...

wow, I have to say I'm impressed. That's the most genius way to really kill TC.
Patch 1 looks good, patch 2,3,4 are nicely building on top... until somebody starts asking how patch 5 will look.
In the future netfilter ingress module will be loaded along with other iptables modules just like conntrack is today and users who would want to use ingress tc would have to _unload_ netfilter_ingress module, but if it has interesting dependencies it may mean to unload iptables and the rest.
So at the end the users will have a binary choice either to use iptables/nft or use tc, because they won't be able to co-exist because ingress_hook is the only one.
I don't understand this 'tc hate'. Why go out of the way to make TC more difficult to use ?
Just add _new_ hook for netfilter ingress and both subsystems can happily co-exist.

FWIW while I think this is a valid concern, I believe its unfounded.

netfilter_ingress must not force run-time
dependencies like 'oh, you want tc, too bad, no conntrack for you)'.

(and i don't see any need for such a dependency).

I don't see this having anything to do with 'TC hate'.

Lets face it, tc and netfilter are horrid when it comes to play nice with each other, and functionality provided by both partially overlaps.

Just look at all the hacks we have in tc to use netfilter (iptables, conntrack) functionality, or even iptables CLASSIFY target (aka "tc filters are too hard for me").

In case there is later some really good reason why you need both tc ingress and nft ingress simultaneously then we can always revisit this and add a second hook for coexistence, at the expense of more complexity.

I don't see a use case where you'd need both tc ingress and nft ingress at the same time. Can you elaborate?

At the moment I think having two hooks provides no advantage, it only complicates code.

It is an either-or choice. You cant have both. The _evil genius_ part i think is that distros which ship with iptables rules and conntracking on are going to not even turn on tc and my scripts now have to go unload one.
But even if the scripts worked (perhaps there are plans to make sure all scripts continue to work transparently), i care about performance and youve suddenly taken that away from me.

So i would agree with adding the two hooks. Priority should be given to tc in the code path.

cheers,
jamal

Again, why is there a need to have both (at same time)?

Please don't accuse anyone of being 'evil'.

I think that tc and netfilter are both broken by design in the sense that we have two different systems with partially overlapping functionality while interaction between them consists of hacks.

It works both ways, iptables CLASSIFY target is also not very elegant from a design point of view, it bolts the packet/connection matching functionality provided by iptables to qdisc hierarchy.

You mean add 'rmmod nft_ingress' or something similar?

That might be a problem. One possible way out would be to make 'tc qdisc add dev eth0 ingress' silently unregister nft ingress on kernel side (but not vice versa).

Not nice, but it would keep compatibility with tc ingress scripts.

I don't think thats an unsolveable problem, currently we have the qdisc->enqueue() indirection, now we have hook + qdisc->enqueue() BUT AFAIU the latter could now be avoided by having ingress hook interact with sch_ingress directly.

That being said, I am not opposed to two hooks, I just don't see any technical reason whatsoever why one would need two different ingress classification engines in use at same time.

Pick your favorite distro. Do you seriously believe my tc scripts will work by default as they used to when the distro ships with iptables turned on? Get a freshly minted distro and try to check what the defaults are.
I have no idea what some of these things are for but they are there.

It is a figure of speech and was intended to be humorous. I consider Pablo a friend, sorry if that came out wrong.

Well, from the tc perspective the angle has been one of laziness and avoiding to rewrite any features that netfilter has. Essentially a gateway-to-iptables. It has not been easy.
It does look silly if we copy things netfilter does in our view.

Assuming there is something else using the nft side, now they break because tc has taken over.

What i described above.

cheers,
jamal

Ugh. I'm sorry Jamal, seems I lack a sense of humor. In any case, no offense taken.

Right, I can imagine that.

Sure, that would not be good either (perhaps even worse).

Well, thats true. But I would not care at this point (in the 'nft ingress breaks when tc takes over') case.

I'd first like to see what distros would do (by default, that is).

I'd prefer to not base a 'we need to support both at same time' decision on speculation as to what some distribution might do at some point.

Anything that makes sure that tc ingress doesn't break is fine. If that turns out to be 'two hooks' then, alas, so be it.

I'd just like some evidence first 8-)

It's exactly the opposite which this thread is showing. Single hook creates complicated relationship between tc/nft. There is no code shared. TC critical path becomes slower with extra dereference and indirect jump. We have to think how 'tc qdisc add ingress' can auto-unload nft_ingress, etc That is real complexity that can be avoided with two hooks.

You complained on the performance impact of the initial generic hook infrastructure that allows qdisc ingress and nft to co-exist, fair enough.

I came back with an even more simple hook infrastructure, to address performance concerns, and you indicate that an *extra dereference and indirect jump* makes this slowier and that both cannot co-exist with this.

Well, any kind of hook generalization will need at least those minimal changes. And that allows us to move code that belongs to sch_ingress. That got embedded into net/core/dev.c because at the time this code was made when there was no RCU around.

This sounds as an excuse to me.

Does anything speak against a separate hook with a static key?