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 10:17:57 -0400
From: Jamal Hadi Salim <hadi@...atatu.com>
To: Simon Horman <simon.horman@...igine.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()

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.

cheers,
jamal

> > +
> > +     base_net = net_generic(net, dyn_act_net_id);
> > +     a = idr_find(&base_net->act_base, act_id);
> > +     if (a && try_module_get(a->owner))
> > +             res = a;
> > +
> > +     read_unlock(&base_net->act_mod_lock);
> > +
> > +     return res;
> > +}
> > +EXPORT_SYMBOL(tc_lookup_action_byid);
> > +
> >  /* lookup by name */
> >  static struct tc_action_ops *tc_lookup_action_n(struct net *net, char *kind)
> >  {
> > --
> > 2.25.1
> >
> _______________________________________________
> p4tc-discussions mailing list -- p4tc-discussions@...devconf.info
> To unsubscribe send an email to p4tc-discussions-leave@...devconf.info