On Tue, May 17, 2022 at 02:16:04PM +0300, Vlad Buslov wrote:
>
> On Tue 17 May 2022 at 13:20, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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?
Same file is fine.
Probably instead ?
nf_flow_table-$(CONFIG_SYSCTL) += nf_flow_table_sysctl.o
so the #ifdef CONFIG_SYSCTL in nf_flow_table_sysctl.c can go away.
you would need to move:
unsigned int nf_ft_hw_max __read_mostly;
to nf_flow_table_offload.c
Make sense?
> >> 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?
I overlook the return is check for NULL, sorry for the noise.
> >> + return net->ft.stat ? 0 : -ENOMEM;
> >> +}
>
