[v4,net-next,7/9] net: ipv4: listified version of ip_rcv

Also involved adding a way to run a netfilter hook over a list of packets.
 Rather than attempting to make netfilter know about lists (which would be
 a major project in itself) we just let it call the regular okfn (in this
 case ip_rcv_finish()) for any packets it steals, and have it give us back
 a list of packets it's synchronously accepted (which normally NF_HOOK
 would automatically call okfn() on, but we want to be able to potentially
 pass the list to a listified version of okfn().)
The netfilter hooks themselves are indirect calls that still happen per-
 packet (see nf_hook_entry_hookfn()), but again, changing that can be left
 for future work.

There is potential for out-of-order receives if the netfilter hook ends up
 synchronously stealing packets, as they will be processed before any
 accepts earlier in the list.  However, it was already possible for an
 asynchronous accept to cause out-of-order receives, so presumably this is
 considered OK.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/linux/netdevice.h |  3 +++
 include/linux/netfilter.h | 22 +++++++++++++++
 include/net/ip.h          |  2 ++
 net/core/dev.c            |  8 +++---
 net/ipv4/af_inet.c        |  1 +
 net/ipv4/ip_input.c       | 68 ++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 94 insertions(+), 10 deletions(-)

On Mon, Jul 02, 2018 at 04:14:12PM +0100, Edward Cree wrote:
> Also involved adding a way to run a netfilter hook over a list of packets.
>  Rather than attempting to make netfilter know about lists (which would be
>  a major project in itself) we just let it call the regular okfn (in this
>  case ip_rcv_finish()) for any packets it steals, and have it give us back
>  a list of packets it's synchronously accepted (which normally NF_HOOK
>  would automatically call okfn() on, but we want to be able to potentially
>  pass the list to a listified version of okfn().)
> The netfilter hooks themselves are indirect calls that still happen per-
>  packet (see nf_hook_entry_hookfn()), but again, changing that can be left
>  for future work.
> 
> There is potential for out-of-order receives if the netfilter hook ends up
>  synchronously stealing packets, as they will be processed before any
>  accepts earlier in the list.  However, it was already possible for an
>  asynchronous accept to cause out-of-order receives, so presumably this is
>  considered OK.

I think we can simplify things if these chained packets don't follow
the standard forwarding path, this would require to revisit many
subsystems to handle these new chained packets - potentially a lot of
work and likely breaking many things - and I would expect we (and
other subsystems too) will not get very much benefits from these
chained packets.

In general I like this infrastructure, but I think we can get
something simpler if we combine it with the flowtable idea, so chained
packets follow the non-standard flowtable forwarding path as described
in [1].

We could generalize and place the flowtable code in the core if
needed, and make it not netfilter dependent if that's a problem.

Thanks.

[1] https://marc.info/?l=netfilter-devel&m=152898601419841&w=2

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jul 02, 2018 at 04:14:12PM +0100, Edward Cree wrote:
> > Also involved adding a way to run a netfilter hook over a list of packets.
> >  Rather than attempting to make netfilter know about lists (which would be
> >  a major project in itself) we just let it call the regular okfn (in this
> >  case ip_rcv_finish()) for any packets it steals, and have it give us back
> >  a list of packets it's synchronously accepted (which normally NF_HOOK
> >  would automatically call okfn() on, but we want to be able to potentially
> >  pass the list to a listified version of okfn().)
> > The netfilter hooks themselves are indirect calls that still happen per-
> >  packet (see nf_hook_entry_hookfn()), but again, changing that can be left
> >  for future work.
> > 
> > There is potential for out-of-order receives if the netfilter hook ends up
> >  synchronously stealing packets, as they will be processed before any
> >  accepts earlier in the list.  However, it was already possible for an
> >  asynchronous accept to cause out-of-order receives, so presumably this is
> >  considered OK.
> 
> I think we can simplify things if these chained packets don't follow
> the standard forwarding path, this would require to revisit many
> subsystems to handle these new chained packets - potentially a lot of
> work and likely breaking many things - and I would expect we (and
> other subsystems too) will not get very much benefits from these
> chained packets.

I guess it depends on what type of 'bundles' to expect on the wire.
For an average netfilter ruleset it will require more unbundling
because we might have

ipv4/udp -> ipv4/tcp -> ipv6/udp -> ipv4/tcp -> ipv4/udp

From Eds patch set, this would be rebundled to

ipv4/udp -> ipv4/tcp -> ipv4/tcp -> ipv4/udp
and
ipv6/udp

nf ipv4 rule eval has to untangle again, to
ipv4/udp -> ipv4/udp
ipv4/tcp -> ipv4/tcp

HOWEVER, there are hooks that are L4 agnostic, such as defragmentation,
so we might still be able to take advantage.

Defrag is extra silly at the moment, we eat the indirect call cost
only to return NF_ACCEPT without doing anything in 99.99% of all cases.

So I still think there is value in exploring to pass the bundle
into the nf core, via new nf_hook_slow_list() that can be used
from forward path (ingress, prerouting, input, forward, postrouting).

We can make this step-by-step, first splitting everything in
nf_hook_slow_list() and just calling hooksfns for each skb in list.

> In general I like this infrastructure, but I think we can get
> something simpler if we combine it with the flowtable idea, so chained
> packets follow the non-standard flowtable forwarding path as described
> in [1].

Yes, in case all packets would be in the flow table (offload software
fallback) we might be able to keep list as-is in case everything has
same nexthop.

However, I don't see any need to make changes to this series for this
now, it can be added on top.

Did i miss anything?

I do agree that from netfilter p.o.v. ingress hook looks like a good
initial candidate for a listification.

On 02/07/18 16:14, Edward Cree wrote:
> +/* Receive a list of IP packets */
> +void ip_list_rcv(struct list_head *head, struct packet_type *pt,
> +		 struct net_device *orig_dev)
> +{
> +	struct net_device *curr_dev = NULL;
> +	struct net *curr_net = NULL;
> +	struct sk_buff *skb, *next;
> +	struct list_head sublist;
> +
> +	list_for_each_entry_safe(skb, next, head, list) {
> +		struct net_device *dev = skb->dev;
> +		struct net *net = dev_net(dev);
> +
> +		skb = ip_rcv_core(skb, net);
> +		if (skb == NULL)
> +			continue;
I've spotted a bug here, in that if ip_rcv_core() eats the skb (e.g. by
 freeing it) it won't list_del() it, so when we process the sublist we'll
 end up trying to process this skb anyway.
Thus, places where an skb could get freed (possibly remotely, as in nf
 hooks that can steal packets) need to use the dequeue/enqueue model
 rather than the list_cut_before() approach.
Followup patches soon.

-Ed