netdev - Re: [p4tc-discussions] Re: [PATCH RFC v2 net-next 05/28] net/sched: act_api: introduce tc_lookup_action

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZH3x3mT+80K1BR1O@corigine.com>
Date: Mon, 5 Jun 2023 16:31:58 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Jamal Hadi Salim <hadi@...atatu.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org,
	deb.chatterjee@...el.com, tom@...anda.io,
	p4tc-discussions@...devconf.info, Mahesh.Shirshyad@....com,
	Vipin.Jain@....com, tomasz.osinski@...el.com,
	xiyou.wangcong@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, khalidm@...dia.com,
	toke@...hat.com
Subject: Re: [p4tc-discussions] Re: [PATCH RFC v2 net-next 05/28] net/sched:
 act_api: introduce tc_lookup_action_byid()

On Mon, Jun 05, 2023 at 10:17:57AM -0400, Jamal Hadi Salim wrote:
> Hi Simon,
> Thanks for the reviews.
> 
> On Mon, Jun 5, 2023 at 5:56 AM Simon Horman via p4tc-discussions
> <p4tc-discussions@...devconf.info> wrote:
> >
> > On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> > > Introduce a lookup helper to retrieve the tc_action_ops
> > > instance given its action id.
> > >
> > > Co-developed-by: Victor Nogueira <victor@...atatu.com>
> > > Signed-off-by: Victor Nogueira <victor@...atatu.com>
> > > Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
> > > Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> > > Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> > > ---
> > >  include/net/act_api.h |  1 +
> > >  net/sched/act_api.c   | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 36 insertions(+)
> > >
> > > diff --git a/include/net/act_api.h b/include/net/act_api.h
> > > index 363f7f8b5586..34b9a9ff05ee 100644
> > > --- a/include/net/act_api.h
> > > +++ b/include/net/act_api.h
> > > @@ -205,6 +205,7 @@ int tcf_idr_release(struct tc_action *a, bool bind);
> > >
> > >  int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
> > >  int tcf_register_dyn_action(struct net *net, struct tc_action_ops *act);
> > > +struct tc_action_ops *tc_lookup_action_byid(struct net *net, u32 act_id);
> > >  int tcf_unregister_action(struct tc_action_ops *a,
> > >                         struct pernet_operations *ops);
> > >  int tcf_unregister_dyn_action(struct net *net, struct tc_action_ops *act);
> > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> > > index 0ba5a4b5db6f..101c6debf356 100644
> >
> > > --- a/net/sched/act_api.c
> > > +++ b/net/sched/act_api.c
> > > @@ -1084,6 +1084,41 @@ int tcf_unregister_dyn_action(struct net *net, struct tc_action_ops *act)
> > >  }
> > >  EXPORT_SYMBOL(tcf_unregister_dyn_action);
> > >
> > > +/* lookup by ID */
> > > +struct tc_action_ops *tc_lookup_action_byid(struct net *net, u32 act_id)
> > > +{
> > > +     struct tcf_dyn_act_net *base_net;
> > > +     struct tc_action_ops *a, *res = NULL;
> >
> > Hi Jamal, Victor and Pedro,
> >
> > A minor nit from my side: as this is networking code, please use reverse
> > xmas tree - longest line to shortest - for local variable declarations.
> >
> 
> Will do in the next update.
> 
> > > +
> > > +     if (!act_id)
> > > +             return NULL;
> > > +
> > > +     read_lock(&act_mod_lock);
> > > +
> > > +     list_for_each_entry(a, &act_base, head) {
> > > +             if (a->id == act_id) {
> > > +                     if (try_module_get(a->owner)) {
> > > +                             read_unlock(&act_mod_lock);
> > > +                             return a;
> > > +                     }
> > > +                     break;
> > > +             }
> > > +     }
> > > +     read_unlock(&act_mod_lock);
> > > +
> > > +     read_lock(&base_net->act_mod_lock);
> >
> > base_net does not appear to be initialised here.
> 
> Yayawiya. Excellent catch. Not sure how even coverity didnt catch this
> or our own internal review. I am guessing you either caught this by
> eyeballing or some tool. If it is a tool we should add it to our CICD.
> We have the clang static analyser but that thing produces so many
> false positives that it is intense labor to review some of the
> nonsense it spews - so it may have caught it and we missed it.

Hi Jamal,

My eyes are not so good these days, so I use tooling.

In this case it is caught by a W=1 build using both gcc-12 and clang-16,
and by Smatch. I would also recommend running Sparse, Coccinelle,
and the xmastree check from Edward Cree [1].

[1] https://github.com/ecree-solarflare/xmastree

FWIIW, I only reviewed the first 12 patches of this series.
If you could run the above mentioned tools over the remaining patches you
may find some more things of interest.