Hi Phil,
On Tue, Jul 08, 2025 at 04:38:44PM +0200, Phil Sutter wrote:
> Hi Pablo,
>
> 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@xxxxxx> 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.
This would also provide a generic infrastructure to report hook
registration and unregistration, as a side effect.
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.
> 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. :)
