Re: [PATCH 5/7] net: add netfilter ingress hook

[Patrick McHardy <kaber@trash.net>]

On 11.04, Pablo Neira Ayuso wrote:
> On Fri, Apr 10, 2015 at 10:33:12PM +0100, Patrick McHardy wrote:
> > On 10.04, Pablo Neira Ayuso wrote:
> > > On Fri, Apr 10, 2015 at 02:36:11PM +0100, Patrick McHardy wrote:
> > We do support all families using the regular NF_QUEUE verdict of course.
> > But yes, nf_queue.c will simply drop packets that don't have a netfilter
> > AF registered.
> > 
> > But my question is whether queueing is something that is even worth
> > considering for the NFPROTO_NETDEV family. As I said, it will at best
> > work for ingress anyways and that will actually be more tricky than just
> > calling skb_share_check(), we need to take care of keeping valid
> > references to all the data you currently store in the CB, including the
> > packet_type, the device, things attached to the skb at this point to
> > the stack etc.
> 
> I think we only need to hold the reference on orig_dev. The pt_prev
> pointer in skb CB can actually be removed. Other things attached to
> the skb we already handle this from nf_queue to make sure they don't
> vanish.

Are you sure? What about removable protocols or packet sockets?

> > If we decide not to support queueing for this family we don't have to
> > use netfilter hooks for this and all the refactoring for async resume
> > becomes unnecessary.
> 
> I think the refactoring is worth. Have a look at the current state of
> this function. It has grown with features along time and it got many
> gotos that force you travel back and forth when reading this code.
> 
> Regarding the nf_queue support at ingress, I don't see any major
> technical obstacule at this moment to support this and I think that
> existing programs that inspect traffic from userspace can benefit from
> this feature (eg. IPS).

Yeah, that might be useful, although they seem to be pretty fine with
getting only IPv4 and IPv6. I guess ARP might be interesting as well,
but we also have hooks for that already.

Regarding the refactoring, there seem to be concerns about performance
impact. My suggestions would be to use nf_hook(), make sure no queueing
can happen and therefore no okfn invocations and then you can simply
add this as a function call to the existing code without the need for
any refactoring or storing state.

You don't loose anything, it only massively simplifies the patches. If
queuing supported is added, you can still change it.