Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 12, 2024 at 03:32:55PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Documentation of list_del_rcu() warns callers to not immediately free
> > the deleted list item. While it seems not necessary to use the
> > RCU-variant of list_del() here in the first place, doing so seems to
> > require calling kfree_rcu() on the deleted item as well.
> > 
> > Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nf_tables_api.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index b6547fe22bd8..2982f49b6d55 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
> >  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
> >  					    FLOW_BLOCK_UNBIND);
> >  		list_del_rcu(&hook->list);
> > -		kfree(hook);
> > +		kfree_rcu(hook, rcu);
> >  	}
> >  	kfree(flowtable->name);
> >  	module_put(flowtable->data.type->owner);
> 
> AFAICS its safe to use list_del() everywhere, I can't find a single
> instance where the hooks are iterated without mutex serialization.
> 
> nf_tables_flowtable_destroy() is called after the hook has been
> unregisted (detached from nf_hook list) and rcu grace period elapsed.

Yes, I didn't find a caller which didn't synchronize_rcu() before
calling it. Same applies to chain hooks, right? I'd drop all the _rcu()
calls and give it a try, but the resulting race conditions may be hard
to trigger.

Cheers, Phil