Re: [nf-next PATCH v4 15/16] netfilter: nf_tables: Add notications for hook changes

[Phil Sutter <phil@nwl.cc>]

On Sat, Sep 21, 2024 at 11:10:34AM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Notify user space if netdev hooks are updated due to netdev add/remove
> > events. Send minimal notification messages by introducing
> > NFT_MSG_NEWDEV/DELDEV message types describing a single device only.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/net/netfilter/nf_tables.h        |  2 +
> >  include/uapi/linux/netfilter/nf_tables.h |  5 +++
> >  net/netfilter/nf_tables_api.c            | 56 ++++++++++++++++++++++++
> >  net/netfilter/nft_chain_filter.c         |  1 +
> >  4 files changed, 64 insertions(+)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index eaf2f5184bdf..f8da38e45277 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -1132,6 +1132,8 @@ int nft_setelem_validate(const struct nft_ctx *ctx, struct nft_set *set,
> >  int nft_set_catchall_validate(const struct nft_ctx *ctx, struct nft_set *set);
> >  int nf_tables_bind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
> >  void nf_tables_unbind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
> > +void nf_tables_chain_device_notify(const struct nft_chain *chain,
> > +				   const struct net_device *dev, int event);
> >  
> >  enum nft_chain_types {
> >  	NFT_CHAIN_T_DEFAULT = 0,
> > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > index d6476ca5d7a6..3a874febf1ac 100644
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -142,6 +142,8 @@ enum nf_tables_msg_types {
> >  	NFT_MSG_DESTROYOBJ,
> >  	NFT_MSG_DESTROYFLOWTABLE,
> >  	NFT_MSG_GETSETELEM_RESET,
> > +	NFT_MSG_NEWDEV,
> > +	NFT_MSG_DELDEV,
> 
> This relies on implicit NFNL_CB_UNSPEC == 0 and nfnetlink
> bailing out whe NFT_MSG_NEWDEV appears in a netlink message
> coming from userspace.

I guess with 'implicit NFNL_CB_UNSPEC == 0' you mean the extra
nf_tables_cb array fields' 'type' value being 0 (nfnetlink.h explicitly
defines NFNL_CB_UNSPEC value as 0). I don't see the connection here
though, probably I miss nfnetlink_rcv_msg() relying on that field value
or so.

I do see implicit dependency on attr_count field being 0 via
nla_parse_deprecated().

> Is there precedence for this?
> If not, maybe better to add explicit entries to the
> nf_tables_cb[] array?
> 
> AFAICS its fine as-is, nfnetlink won't blindly invoke
> NULL ->call() pointer, but I'm not sure this was designed
> to be this way or if this is a coincidence.

I see at least NFNL_MSG_ACCT_OVERQUOTA missing from nfnl_acct_cb. The
former was introduced in 2014. May I claim grandfathering? ;)

Cheers, Phil