On Mon, Sep 16, 2024 at 02:00:59AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 12, 2024 at 03:32:55PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@xxxxxx> 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@xxxxxx>
> > > ---
> > > 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);
>
> This looks correct to me.
>
> > > }
> > > 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.
>
> Netlink dump path is lockless.
>
> nft_dump_basechain_hook() is missing list_for_each_entry_rcu() for
> list iteration, that needs a fix.
>
> nf_tables_fill_flowtable_info() does use list_for_each_entry_rcu().
I'd suggest to start by sending fixes for nf.git to address these two
issues.
> > nf_tables_flowtable_destroy() is called after the hook has been
> > unregisted (detached from nf_hook list) and rcu grace period elapsed.
