[RFC,v2,net-next,05/28] net/sched: act_api: introduce tc_lookup_action_byid()

Commit Message

Jamal Hadi Salim May 17, 2023, 11:02 a.m. UTC

Introduce a lookup helper to retrieve the tc_action_ops
instance given its action id.

Co-developed-by: Victor Nogueira <[e-mail]>
Signed-off-by: Victor Nogueira <[e-mail]>
Co-developed-by: Pedro Tammela <[e-mail]>
Signed-off-by: Pedro Tammela <[e-mail]>
Signed-off-by: Jamal Hadi Salim <[e-mail]>
---
 include/net/act_api.h |  1 +
 net/sched/act_api.c   | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Comments

Marcelo Ricardo Leitner June 2, 2023, 7:36 p.m. UTC | #1

On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> +/* 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;
> +
> +	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;

It shouldn't call break here but instead already return NULL:
if id matched, it cannot be present on the dyn list.

Moreover, the search be optimized: now that TCA_ID_ is split between
fixed and dynamic ranges (patch #3), it could jump directly into the
right list. Control path performance is also important..

> +		}
> +	}
> +	read_unlock(&act_mod_lock);
> +
> +	read_lock(&base_net->act_mod_lock);
> +
> +	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);

Jamal Hadi Salim June 3, 2023, 1:10 p.m. UTC | #2

On Fri, Jun 2, 2023 at 3:36 PM Marcelo Ricardo Leitner
<[e-mail]> wrote:
>
> On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> > +/* 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;
> > +
> > +     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;
>
> It shouldn't call break here but instead already return NULL:
> if id matched, it cannot be present on the dyn list.
>
> Moreover, the search be optimized: now that TCA_ID_ is split between
> fixed and dynamic ranges (patch #3), it could jump directly into the
> right list. Control path performance is also important..
>

Good catch again. This is actually a bug we missed in our code review.
We'll fix it in the next update.

cheers,
jamal


> > +             }
> > +     }
> > +     read_unlock(&act_mod_lock);
> > +
> > +     read_lock(&base_net->act_mod_lock);
> > +
> > +     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);
>

Jamal Hadi Salim June 3, 2023, 1:14 p.m. UTC | #3

On Fri, Jun 2, 2023 at 3:36 PM Marcelo Ricardo Leitner
<[e-mail]> wrote:
>
> On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> > +/* 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;
> > +
> > +     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;
>
> It shouldn't call break here but instead already return NULL:
> if id matched, it cannot be present on the dyn list.
>
> Moreover, the search be optimized: now that TCA_ID_ is split between
> fixed and dynamic ranges (patch #3), it could jump directly into the
> right list. Control path performance is also important..



Sorry - didnt respond to this last part: We could use standard tc
actions in a P4 program and we prioritize looking at them first. This
helper is currently only needed for us - so you could argue that we
should only look TCA_ID_DYN onwards but it is useful to be more
generic and since this is a slow path it is not critical. Unless i
misunderstood what you said.

cheers,
jamal

> > +             }
> > +     }
> > +     read_unlock(&act_mod_lock);
> > +
> > +     read_lock(&base_net->act_mod_lock);
> > +
> > +     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);
>

Simon Horman June 5, 2023, 9:55 a.m. UTC | #4

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 <[e-mail]>
> Signed-off-by: Victor Nogueira <[e-mail]>
> Co-developed-by: Pedro Tammela <[e-mail]>
> Signed-off-by: Pedro Tammela <[e-mail]>
> Signed-off-by: Jamal Hadi Salim <[e-mail]>
> ---
>  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.

> +
> +	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.

> +
> +	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
>

Marcelo Ricardo Leitner June 5, 2023, 12:08 p.m. UTC | #5

On Sat, Jun 03, 2023 at 09:14:53AM -0400, Jamal Hadi Salim wrote:
> On Fri, Jun 2, 2023 at 3:36 PM Marcelo Ricardo Leitner
> <[e-mail]> wrote:
> >
> > On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> > > +/* 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;
> > > +
> > > +     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;
> >
> > It shouldn't call break here but instead already return NULL:
> > if id matched, it cannot be present on the dyn list.
> >
> > Moreover, the search be optimized: now that TCA_ID_ is split between
> > fixed and dynamic ranges (patch #3), it could jump directly into the
> > right list. Control path performance is also important..
>
>
>
> Sorry - didnt respond to this last part: We could use standard tc
> actions in a P4 program and we prioritize looking at them first. This
> helper is currently only needed for us - so you could argue that we
> should only look TCA_ID_DYN onwards but it is useful to be more
> generic and since this is a slow path it is not critical. Unless i

Yes, and no. Control path is not as critical as datapath, I agree with
that, but it's also important. It's a small change, but oh well.. it
should accumulate on complex datapaths.

> misunderstood what you said.

I meant something like this:

+     if (act_id < TCA_ID_DYN) {
+         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; /* now break; is okay */
+                 }
+         }
+         read_unlock(&act_mod_lock);
+     } else {
+         read_lock(&base_net->act_mod_lock);
+
+         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);
+     }

>
> cheers,
> jamal
>
> > > +             }
> > > +     }
> > > +     read_unlock(&act_mod_lock);
> > > +
> > > +     read_lock(&base_net->act_mod_lock);
> > > +
> > > +     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);
> >
>

Jamal Hadi Salim June 5, 2023, 2:17 p.m. UTC | #6

Hi Simon,
Thanks for the reviews.

On Mon, Jun 5, 2023 at 5:56 AM Simon Horman via p4tc-discussions
<[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Victor Nogueira <[e-mail]>
> > Co-developed-by: Pedro Tammela <[e-mail]>
> > Signed-off-by: Pedro Tammela <[e-mail]>
> > Signed-off-by: Jamal Hadi Salim <[e-mail]>
> > ---
> >  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 -- [e-mail]
> To unsubscribe send an email to [e-mail]

Simon Horman June 5, 2023, 2:31 p.m. UTC | #7

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
> <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Victor Nogueira <[e-mail]>
> > > Co-developed-by: Pedro Tammela <[e-mail]>
> > > Signed-off-by: Pedro Tammela <[e-mail]>
> > > Signed-off-by: Jamal Hadi Salim <[e-mail]>
> > > ---
> > >  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.

Jamal Hadi Salim June 5, 2023, 2:55 p.m. UTC | #8

On Mon, Jun 5, 2023 at 8:08 AM Marcelo Ricardo Leitner via
p4tc-discussions <[e-mail]> wrote:
>
> On Sat, Jun 03, 2023 at 09:14:53AM -0400, Jamal Hadi Salim wrote:
> > On Fri, Jun 2, 2023 at 3:36 PM Marcelo Ricardo Leitner
> > <[e-mail]> wrote:
> > >
> > > On Wed, May 17, 2023 at 07:02:09AM -0400, Jamal Hadi Salim wrote:
> > > > +/* 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;
> > > > +
> > > > +     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;
> > >
> > > It shouldn't call break here but instead already return NULL:
> > > if id matched, it cannot be present on the dyn list.
> > >
> > > Moreover, the search be optimized: now that TCA_ID_ is split between
> > > fixed and dynamic ranges (patch #3), it could jump directly into the
> > > right list. Control path performance is also important..
> >
> >
> >
> > Sorry - didnt respond to this last part: We could use standard tc
> > actions in a P4 program and we prioritize looking at them first. This
> > helper is currently only needed for us - so you could argue that we
> > should only look TCA_ID_DYN onwards but it is useful to be more
> > generic and since this is a slow path it is not critical. Unless i
>
> Yes, and no. Control path is not as critical as datapath, I agree with
> that, but it's also important. It's a small change, but oh well.. it
> should accumulate on complex datapaths.
>
> > misunderstood what you said.
>
> I meant something like this:
>
> +     if (act_id < TCA_ID_DYN) {
> +         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; /* now break; is okay */
> +                 }
> +         }
> +         read_unlock(&act_mod_lock);
> +     } else {
> +         read_lock(&base_net->act_mod_lock);
> +
> +         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);
> +     }

Reasonable. We will update with this approach. Thanks Marcelo.

cheers,
jamal

> >
> > cheers,
> > jamal
> >
> > > > +             }
> > > > +     }
> > > > +     read_unlock(&act_mod_lock);
> > > > +
> > > > +     read_lock(&base_net->act_mod_lock);
> > > > +
> > > > +     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);
> > >
> >
>
> _______________________________________________
> p4tc-discussions mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

Jamal Hadi Salim June 5, 2023, 3:04 p.m. UTC | #9

On Mon, Jun 5, 2023 at 10:32 AM Simon Horman <[e-mail]> 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
> > <[e-mail]> 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 <[e-mail]>
> > > > Signed-off-by: Victor Nogueira <[e-mail]>
> > > > Co-developed-by: Pedro Tammela <[e-mail]>
> > > > Signed-off-by: Pedro Tammela <[e-mail]>
> > > > Signed-off-by: Jamal Hadi Salim <[e-mail]>
> > > > ---
> > > >  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

Patch

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;
+
+	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 = 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)
 {