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@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. 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
