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]

Date: Mon, 5 Jun 2023 11:04:35 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Jamal Hadi Salim <hadi@...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 5, 2023 at 10:32 AM Simon Horman <simon.horman@...igine.com> wrote:
>
> 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.

We will certainly be doing this in the next day or two.

cheers,
jamal