Re: [nf-next RFC] netfilter: nf_tables: Feature ifname-based hook registration

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Thu, Jul 10, 2025 at 12:43:03AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 08, 2025 at 04:38:44PM +0200, Phil Sutter wrote:
> > On Mon, Jul 07, 2025 at 09:25:50PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 04, 2025 at 04:04:39PM +0200, Florian Westphal wrote:
> > > > Phil Sutter <phil@nwl.cc> wrote:
> > > > > Please keep in mind we already have 'nft list hooks' which provides
> > > > > hints in that direction. It does not show which flowtable/chain actually
> > > > > binds to a given device, though.
> > > > 
> > > > Its possible to extend it:
> > > > - add NF_HOOK_OP_NFT_FT to enum nf_hook_ops_type
> > > > - add
> > > > 
> > > > static int nfnl_hook_put_nft_ft_info(struct sk_buff *nlskb,
> > > >                                    const struct nfnl_dump_hook_data *ctx,
> > > >                                    unsigned int seq,
> > > >                                    struct nf_flowtable *ft)
> > > > 
> > > > to nfnetlink_hook.c
> > > > 
> > > > it can use container_of to get to the nft_flowtable struct.
> > > > It might be possibe to share some code with nfnl_hook_put_nft_chain_info
> > > > and reuse some of the same netlink attributes.
> > > > 
> > > > - call it from nfnl_hook_dump_one.
> > > > 
> > > > I think it would use useful to have, independent of "eth*" support.
> > > 
> > > This is a good idea to place this in nfnetlink_hook, that
> > > infrastructure is for debugging purpose indeed.
> > > 
> > > If this update is made, I also think it makes sense to remove the
> > > netlink event notification code for devices, I don't have a use case
> > > for that code in the new device group other than debugging.
> > > 
> > > If Phil's intention is to make code savings, then extending
> > > nfnetlink_hook and removing the existing device notification group
> > > make sense to me.
> > > 
> > > User can simply resort to check via dump if a matching hook is
> > > registered for eth* in nfnetlink_hook.
> > 
> > What is the downside of having it? Are you concerned about the need to
> > maintain it or something else (as well)?
> 
> I was considering that nfnetlink_hook is a better fit for this
> purpose, these event notifications that report new devices could come
> from net/netfilter/core.c instead. That is, nf_register_net_hook() and
> nf_tables_unregister_hook().
> 
> You also mentioned you originally used this syntax:
> 
>         nft monitor hooks
> 
> which, after Florian's suggestion, made me think all this belongs to
> nfnetlink_hook.
> 
> This would avoid an asymmetry in the API. At this moment, new device
> hooks are reported via nftables, but listing will be retrieved via
> nfnetlink_hook.

Ah, I see.

> This would also provide a generic infrastructure to report hook
> registration and unregistration, as a side effect.

OK, fair with me!

> If you accept this suggestion, it is a matter of:
> 
> #1 revert the patch in nf.git for the incomplete event notification
>    (you have three more patches pending for nf-next to complete this
>     for control plane notifications).
> #2 add event notifications to net/netfilter/core.c and nfnetlink_hook.
> 
> Only -rc kernels have been release containing the incomplete device
> event notification. It is a bit late to revert to be honest, but
> better late than never. This infrastructure is triggering more debate
> than expected.
> 
> And that would be more work on your pile to respin, which is always a
> hard sell.

No problem. I'll quickly submit a revert for nf.git and attempt an
implementation in nfnetlink or core code "later" - I assume the
flowtable support in 'nft list hooks' output is fine to satisfy the
traceability requirement for name-based hooks and so we're good to go
with the user space implementation?

> > I had extended the monitor testsuite to assert correct behaviour wrt.
> > adding/removing devices. Implementing this is in a shell test is
> > trivial, but still work to be done. :)

I'll then drop the 'nft monitor' addon for now and write a shell test
using 'nft list hooks' to validate correct behaviour instead.

Thanks, Phil