[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231123135213.GE6339@kernel.org>
Date: Thu, 23 Nov 2023 13:52:13 +0000
From: Simon Horman <horms@...nel.org>
To: Florian Westphal <fw@...len.de>
Cc: netfilter-devel@...r.kernel.org, lorenzo@...nel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH nf-next 1/8] netfilter: flowtable: move nf_flowtable out
of container structures
On Tue, Nov 21, 2023 at 01:27:44PM +0100, Florian Westphal wrote:
> struct nf_flowtable is currently wholly embedded in either nft_flowtable
> or tcf_ct_flow_table.
>
> In order to allow flowtable acceleration via XDP, the XDP program will
> need to map struct net_device to struct nf_flowtable.
>
> To make this work reliably, make a clear separation of the frontend
> (nft, tc) and backend (nf_flowtable) representation.
>
> In this first patch, amke it so nft_flowtable and tcf_ct_flow_table
> only store pointers to an nf_flowtable structure.
>
> The main goal is to have follow patches that allow us to keep the
> nf_flowtable structure around for a bit longer (e.g. until after
> an rcu grace period has elapesed) when the frontend(s) are tearing the
> structures down.
>
> At this time, things are fine, but when xdp programs might be using
> the nf_flowtable structure as well we will need a way to ensure that
> no such users exist anymore.
>
> Right now there is inufficient guarantee: nftables only ensures
> that the netfilter hooks are unregistered, and tc only ensures the
> tc actions have been removed.
>
> Any future kfunc might still be called in parallel from an XDP
> program. The easies way to resolve this is to let the nf_flowtable
> core handle release and module reference counting itself.
>
> Signed-off-by: Florian Westphal <fw@...len.de>
...
> @@ -312,24 +313,29 @@ static int tcf_ct_flow_table_get(struct net *net, struct tcf_ct_params *params)
> if (err)
> goto err_insert;
>
> - ct_ft->nf_ft.type = &flowtable_ct;
> - ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD |
> - NF_FLOWTABLE_COUNTER;
> - err = nf_flow_table_init(&ct_ft->nf_ft);
> + ct_ft->nf_ft = kzalloc(sizeof(*ct_ft->nf_ft), GFP_KERNEL);
> + if (!ct_ft->nf_ft)
> + goto err_alloc;
Hi Florian,
This branch will cause the function to return err, but err is 0 here.
Perhaps it should be set to a negative error value instead?
Flagged by Smatch.
> +
> + ct_ft->nf_ft->type = &flowtable_ct;
> + ct_ft->nf_ft->flags |= NF_FLOWTABLE_HW_OFFLOAD |
> + NF_FLOWTABLE_COUNTER;
> + err = nf_flow_table_init(ct_ft->nf_ft);
> if (err)
> goto err_init;
> - write_pnet(&ct_ft->nf_ft.net, net);
> + write_pnet(&ct_ft->nf_ft->net, net);
>
> __module_get(THIS_MODULE);
> out_unlock:
> params->ct_ft = ct_ft;
> - params->nf_ft = &ct_ft->nf_ft;
> + params->nf_ft = ct_ft->nf_ft;
> mutex_unlock(&zones_mutex);
>
> return 0;
>
> err_init:
> rhashtable_remove_fast(&zones_ht, &ct_ft->node, zones_params);
> + kfree(ct_ft->nf_ft);
> err_insert:
> kfree(ct_ft);
> err_alloc:
...
Powered by blists - more mailing lists