Re: [PATCH net-next v2 3/3] netfilter: nf_flow_table: count pending offload workqueue tasks

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 17 May 2022 at 13:20, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, May 16, 2022 at 10:10:32PM +0300, Vlad Buslov wrote:
> [...]
>> diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
>> index ddc54b6d18ee..c8fc5c7ef04a 100644
>> --- a/net/netfilter/Kconfig
>> +++ b/net/netfilter/Kconfig
>> @@ -734,6 +734,14 @@ config NF_FLOW_TABLE
>>  
>>  	  To compile it as a module, choose M here.
>>  
>> +config NF_FLOW_TABLE_PROCFS
>> +	bool "Supply flow table statistics in procfs"
>> +	default y
>> +	depends on PROC_FS
>> +	help
>> +	  This option enables for the flow table offload statistics
>> +	  to be shown in procfs under net/netfilter/nf_flowtable.
>
> This belongs to patch 2/3.
>
> Then, use NF_FLOW_TABLE_PROCFS to conditionally add it to
> nf_flow_table if this is enabled in .config? To honor this new Kconfig
> toggle.
>
> I mean instead of:
>
> obj-$(CONFIG_NF_FLOW_TABLE)    += nf_flow_table.o
>  nf_flow_table-objs             := nf_flow_table_core.o nf_flow_table_ip.o \
> -                                  nf_flow_table_offload.o
> +                                  nf_flow_table_offload.o \
> +                                  nf_flow_table_sysctl.o
>
> this?
>
> nf_flow_table-$(CONFIG_NF_FLOW_TABLE_SYSCTL)    += nf_flow_table_sysctl.o

In V2 I have both sysctl and procfs implementations in single file.
As I replied for previous patch in series: Should I split those in two
separate files (nf_flow_table_sysctl.c and nf_flow_table_procfs.c) that
both could be conditionally compiled depending on their respective
configs?

>
>>  config NETFILTER_XTABLES
>>  	tristate "Netfilter Xtables support (required for ip_tables)"
>>  	default m if NETFILTER_ADVANCED=n
>> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
>> index e2598f98017c..c86dd627ef42 100644
>> --- a/net/netfilter/nf_flow_table_core.c
>> +++ b/net/netfilter/nf_flow_table_core.c
>> @@ -662,17 +662,51 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
>>  }
>>  EXPORT_SYMBOL_GPL(nf_flow_table_free);
>>  
>> +static int nf_flow_table_init_net(struct net *net)
>> +{
>> +	net->ft.stat = alloc_percpu(struct nf_flow_table_stat);
>
> Missing check for NULL in case alloc_percpu() fails?
>

I might be missing something, but why isn't NULL check in following line
sufficient?

>> +	return net->ft.stat ? 0 : -ENOMEM;
>> +}