On Sun, Feb 26, 2023 at 01:28:17PM +0100, Julian Anastasov wrote:
>
> Hello,
>
> On Sun, 26 Feb 2023, Sven Auhagen wrote:
>
> > On Sun, Feb 26, 2023 at 10:53:58AM +0200, Julian Anastasov wrote:
> > >
> > > > diff --git a/net/netfilter/nf_flow_table_procfs.c b/net/netfilter/nf_flow_table_procfs.c
> > > > index 159b033a43e6..cff9ad58c7c9 100644
> > > > --- a/net/netfilter/nf_flow_table_procfs.c
> > > > +++ b/net/netfilter/nf_flow_table_procfs.c
> > > > @@ -64,6 +64,16 @@ static const struct seq_operations nf_flow_table_cpu_seq_ops = {
> > > > .show = nf_flow_table_cpu_seq_show,
> > > > };
> > > >
> > > > +static int nf_flow_table_counter_show(struct seq_file *seq, void *v)
> > > > +{
> > > > + struct net *net = seq_file_net(seq);
> > > > +
> > > > + seq_printf(seq, "%lld\n",
> > > > + atomic64_read(&net->ft.count_flowoffload)
> > > > + );
> > > > + return 0;
> > > > +}
> > > > +
> > > > int nf_flow_table_init_proc(struct net *net)
> > > > {
> > > > struct proc_dir_entry *pde;
> > > > @@ -71,6 +81,9 @@ int nf_flow_table_init_proc(struct net *net)
> > > > pde = proc_create_net("nf_flowtable", 0444, net->proc_net_stat,
> > > > &nf_flow_table_cpu_seq_ops,
> > > > sizeof(struct seq_net_private));
> > >
> > > Result should be checked, pde is not needed:
> > >
> >
> > pde is needed to free the per cpu stat structure in nf_flow_table_fini_net
>
> Nothing is needed to add in nf_flow_table_fini_net(),
> only in nf_flow_table_fini_proc() which is called by
> nf_flow_table_pernet_exit().
>
> > in the error case. This was implemented before this patch though.
> > I understand that some kind of error check would be good.
> > Since there is no consequence when this does not work I did not implement one
> > and let the old error checking go along.
>
> The checking tools will warn for such leaks, so
> every module should cleanup everything on init error.
>
> > Do you have any ideas how to solve the problem more?
>
> Nothing more is needed except the missing
> remove_proc_entry("nf_flowtable_counter", net->proc_net_stat);
> in nf_flow_table_fini_proc(). The var 'pde' is not needed.
> On error in nf_flow_table_init_proc() it should free everything
> allocated there and nf_flow_table_fini_proc() should free
> everything that is allocated in nf_flow_table_init_proc().
Got it and you are correct about nf_flow_table_fini_proc but it is also
called from nf_flow_table_pernet_init in case of a nf_flow_table_init_proc
error which is checking the return of the pde variable.
That what I was referring to.
I will send a v2 with your proposed changes.
>
> > Best
> > Sven
> >
> > > if (!proc_create_net())
> > > goto err;
> > >
> > > > + proc_create_net_single("nf_flowtable_counter", 0444,
> > > > + net->proc_net, nf_flow_table_counter_show, NULL);
> > >
> > > if (!proc_create_net_single())
> > > goto err_net;
> > > return 0;
> > >
> > > err_net:
> > > remove_proc_entry("nf_flowtable", net->proc_net_stat);
> > >
> > > err:
> > > return -ENOMEM;
> > >
> > > > +
> > > > return pde ? 0 : -ENOMEM;
> > > > }
>
> Regards
>
> --
> Julian Anastasov <ja@xxxxxx>
>
