Re: [PATCH nf-next 1/8] netfilter: flowtable: move nf_flowtable out of container structures

[Simon Horman <horms@kernel.org>]

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@strlen.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:

...