Re: [nf-next PATCH v4 13/16] netfilter: nf_tables: Handle NETDEV_CHANGENAME events

[Phil Sutter <phil@nwl.cc>]

On Sun, Sep 22, 2024 at 09:32:24AM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > For the sake of simplicity, treat them like consecutive NETDEV_REGISTER
> > and NETDEV_UNREGISTER events. If the new name matches a hook spec and
> > registration fails, escalate the error and keep things as they are.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v3:
> > - Register first and handle errors to avoid having unregistered the
> >   device but registration fails.
> > ---
> >  net/netfilter/nf_tables_api.c    | 5 +++++
> >  net/netfilter/nft_chain_filter.c | 5 +++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 2684990dd3dc..4d40c1905735 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -9371,6 +9371,11 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
> >  	struct nft_table *table;
> >  	struct net *net;
> >  
> > +	if (event == NETDEV_CHANGENAME) {
> > +		if (nf_tables_flowtable_event(this, NETDEV_REGISTER, ptr))
> > +			return NOTIFY_BAD;
> > +		event = NETDEV_UNREGISTER;
> > +	}
> 
> Consider flowtable that should claim devices "pv*".
> You get CHANGENAME, device name is, say, pv5.
> 
> Device name is registered in nf_tables_flowtable_event().
> Then, event is set to UNREGISTER.
> 
> AFAICS this may unreg the device again immediately, as unreg part
> only compares device pointer and we can't be sure the device was
> part of any flowtable when CHANGENAME was triggered.
> 
> So I think nf_tables_flowtable_event() must handle CHANGENAME
> directly, first check if any flowtable holds the device at this time,
> then check if we need to register it with a new name, and do unreg
> only if it was previously part of any flowtable.
> 
> Same logic needed for netdev chains.
> 
> Does that make sense?

Oh, you're right: Registering the device again (with new name) then
searching *all* flowtables for the device and unregistering it will
undo the previous registration, too! This obviously needs proper
testing, too.

Thanks, Phil