| Message ID | 20230517110232.29349-4-jhs@mojatatu.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing P4TC | expand |
CC: Dan Carpenter On Wed, May 17, 2023 at 07:02:08AM -0400, Jamal Hadi Salim wrote: > The initialisation of P4TC action instances require access to a struct p4tc_act > (which appears in later patches) to help us to retrieve information like the > dynamic action parameters etc. In order to retrieve struct p4tc_act we need the > pipeline name or id and the action name or id. Also recall that P4TC > action IDs are dynamic and are net namespace specific. The init callback from > tc_action_ops parameters had no way of supplying us that information. To solve > this issue, we decided to create a new tc_action_ops callback (init_ops), that > provides us with the tc_action_ops struct which then provides us with the > pipeline and action name. In addition we add a new refcount to struct > tc_action_ops called dyn_ref, which accounts for how many action instances we > have of a specific dynamic action. > > Co-developed-by: Victor Nogueira <victor@mojatatu.com> > Signed-off-by: Victor Nogueira <victor@mojatatu.com> > Co-developed-by: Pedro Tammela <pctammela@mojatatu.com> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Hi Jamal, Victor and Pedro, > index bc4e178873e4..0ba5a4b5db6f 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -1006,7 +1006,7 @@ int tcf_register_action(struct tc_action_ops *act, > struct tc_action_ops *a; > int ret; > > - if (!act->act || !act->dump || !act->init) > + if (!act->act || !act->dump || (!act->init && !act->init_ops)) > return -EINVAL; > > /* We have to register pernet ops before making the action ops visible, > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > } > } > > - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > - userflags.value | flags, extack); > + if (a_o->init) > + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > + userflags.value | flags, extack); > + else if (a_o->init_ops) > + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, > + tp, a_o, userflags.value | flags, > + extack); By my reading the initialisation of a occurs here. Which is now conditional. > } else { > err = a_o->init(net, nla, est, &a, tp, userflags.value | flags, > extack); A bit further down, outside of the else clause above, the code looks like this. if (!police && tb[TCA_ACT_COOKIE]) tcf_set_action_cookie(&a->user_cookie, user_cookie); if (!police) a->hw_stats = hw_stats; Which causes Smatch to complain that a may be used uninitialised. Is this the case?
On Mon, Jun 05, 2023 at 11:51:14AM +0200, Simon Horman wrote: > > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > > } > > } > > > > - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > - userflags.value | flags, extack); > > + if (a_o->init) > > + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > + userflags.value | flags, extack); > > + else if (a_o->init_ops) > > + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, > > + tp, a_o, userflags.value | flags, > > + extack); > > By my reading the initialisation of a occurs here. > Which is now conditional. > Right. Presumably the author knows that one (and only one) of the ->init or ->init_ops pointers is set. This kind of relationship between two variables is something that Smatch tries to track inside a function but outside of functions, like here, then Smatch doesn't track it. I can't really think of a scalable way to track this. So there are a couple options: 1) Ignore the warning. 2) Remove the second if. if (a_o->init) err = a_o->init(); else err = a_o->init_ops(); I kind of like this, because I think it communicates the if ->init() isn't set then ->init_ops() must be. 3) Add a return. if (a_o->init) { err = a_o->init(); } else if (a_o->init_ops) { err = a_o->init_ops(); } else { WARN_ON(1); return ERR_PTR(-EINVAL); } 4) Add an unreachable. But the last time I suggested this it led to link errors and I didn't get a chance to investigate so probably don't do this: if (a_o->init) { err = a_o->init(); } else if (a_o->init_ops) { err = a_o->init_ops(); } else { unreachable(); } regards, dan carpenter
On Mon, Jun 5, 2023 at 7:39 AM Dan Carpenter via p4tc-discussions <p4tc-discussions@netdevconf.info> wrote: > > On Mon, Jun 05, 2023 at 11:51:14AM +0200, Simon Horman wrote: > > > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > > > } > > > } > > > > > > - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > - userflags.value | flags, extack); > > > + if (a_o->init) > > > + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > + userflags.value | flags, extack); > > > + else if (a_o->init_ops) > > > + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, > > > + tp, a_o, userflags.value | flags, > > > + extack); > > > > By my reading the initialisation of a occurs here. > > Which is now conditional. > > > > Right. Presumably the author knows that one (and only one) of the > ->init or ->init_ops pointers is set. Yes, this is correct and the code above checks i.e - if (!act->act || !act->dump || !act->init) + if (!act->act || !act->dump || (!act->init && !act->init_ops)) return -EINVAL; > This kind of relationship between > two variables is something that Smatch tries to track inside a function > but outside of functions, like here, then Smatch doesn't track it. > I can't really think of a scalable way to track this. Could you have used the statement i referred to above as part of the state? > So there are a couple options: > > 1) Ignore the warning. > 2) Remove the second if. > > if (a_o->init) > err = a_o->init(); > else > err = a_o->init_ops(); > > I kind of like this, because I think it communicates the if ->init() > isn't set then ->init_ops() must be. I like this approach - we'll refactor to remove the !police. (note police using some old tc versions is still a pariah and has typically to be checked separately, at some point we should audit the code and remove any police specific checks). cheers, jamal > 3) Add a return. > > if (a_o->init) { > err = a_o->init(); > } else if (a_o->init_ops) { > err = a_o->init_ops(); > } else { > WARN_ON(1); > return ERR_PTR(-EINVAL); > } > > 4) Add an unreachable. But the last time I suggested this it led to > link errors and I didn't get a chance to investigate so probably don't > do this: > > if (a_o->init) { > err = a_o->init(); > } else if (a_o->init_ops) { > err = a_o->init_ops(); > } else { > unreachable(); > } > > regards, > dan carpenter > > _______________________________________________ > p4tc-discussions mailing list -- p4tc-discussions@netdevconf.info > To unsubscribe send an email to p4tc-discussions-leave@netdevconf.info
On Mon, Jun 05, 2023 at 10:01:44AM -0400, Jamal Hadi Salim wrote: > On Mon, Jun 5, 2023 at 7:39 AM Dan Carpenter via p4tc-discussions > <p4tc-discussions@netdevconf.info> wrote: > > > > On Mon, Jun 05, 2023 at 11:51:14AM +0200, Simon Horman wrote: > > > > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > > > > } > > > > } > > > > > > > > - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > > - userflags.value | flags, extack); > > > > + if (a_o->init) > > > > + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > > + userflags.value | flags, extack); > > > > + else if (a_o->init_ops) > > > > + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, > > > > + tp, a_o, userflags.value | flags, > > > > + extack); > > > > > > By my reading the initialisation of a occurs here. > > > Which is now conditional. > > > > > > > Right. Presumably the author knows that one (and only one) of the > > ->init or ->init_ops pointers is set. > > Yes, this is correct and the code above checks i.e > - if (!act->act || !act->dump || !act->init) > + if (!act->act || !act->dump || (!act->init && !act->init_ops)) > return -EINVAL; > Ah. Right. > > This kind of relationship between > > two variables is something that Smatch tries to track inside a function > > but outside of functions, like here, then Smatch doesn't track it. > > I can't really think of a scalable way to track this. > > Could you have used the statement i referred to above as part of the state? > If the if statement were in the same function then Smatch would be able to parse this but that relationship information doesn't carry across the function boundary. It's actually quite a bit more complicated than just the function boundary even... I don't know if this is even possible but if it were then it would be like a 5-7 year time frame to make it work... regards, dan carpenter
On Mon, Jun 5, 2023 at 10:59 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Mon, Jun 05, 2023 at 10:01:44AM -0400, Jamal Hadi Salim wrote: > > On Mon, Jun 5, 2023 at 7:39 AM Dan Carpenter via p4tc-discussions > > <p4tc-discussions@netdevconf.info> wrote: > > > > > > On Mon, Jun 05, 2023 at 11:51:14AM +0200, Simon Horman wrote: > > > > > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > > > > > } > > > > > } > > > > > > > > > > - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > > > - userflags.value | flags, extack); > > > > > + if (a_o->init) > > > > > + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, > > > > > + userflags.value | flags, extack); > > > > > + else if (a_o->init_ops) > > > > > + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, > > > > > + tp, a_o, userflags.value | flags, > > > > > + extack); > > > > > > > > By my reading the initialisation of a occurs here. > > > > Which is now conditional. > > > > > > > > > > Right. Presumably the author knows that one (and only one) of the > > > ->init or ->init_ops pointers is set. > > > > Yes, this is correct and the code above checks i.e > > - if (!act->act || !act->dump || !act->init) > > + if (!act->act || !act->dump || (!act->init && !act->init_ops)) > > return -EINVAL; > > > > Ah. Right. > > > > This kind of relationship between > > > two variables is something that Smatch tries to track inside a function > > > but outside of functions, like here, then Smatch doesn't track it. > > > I can't really think of a scalable way to track this. > > > > Could you have used the statement i referred to above as part of the state? > > > > If the if statement were in the same function then Smatch would be able > to parse this but that relationship information doesn't carry across the > function boundary. It's actually quite a bit more complicated than just > the function boundary even... I don't know if this is even possible but > if it were then it would be like a 5-7 year time frame to make it work... Understood. I guess this semantically is at least a layer above, so it owuld be complex. cheers, jamal > regards, > dan carpenter > >
diff --git a/include/net/act_api.h b/include/net/act_api.h index a414c0f94ff1..363f7f8b5586 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -108,6 +108,7 @@ struct tc_action_ops { char kind[ACTNAMSIZ]; enum tca_id id; /* identifier should match kind */ unsigned int net_id; + refcount_t dyn_ref; size_t size; struct module *owner; int (*act)(struct sk_buff *, const struct tc_action *, @@ -119,6 +120,11 @@ struct tc_action_ops { struct nlattr *est, struct tc_action **act, struct tcf_proto *tp, u32 flags, struct netlink_ext_ack *extack); + /* This should be merged with the original init action */ + int (*init_ops)(struct net *net, struct nlattr *nla, + struct nlattr *est, struct tc_action **act, + struct tcf_proto *tp, struct tc_action_ops *ops, + u32 flags, struct netlink_ext_ack *extack); int (*walk)(struct net *, struct sk_buff *, struct netlink_callback *, int, const struct tc_action_ops *, diff --git a/net/sched/act_api.c b/net/sched/act_api.c index bc4e178873e4..0ba5a4b5db6f 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1006,7 +1006,7 @@ int tcf_register_action(struct tc_action_ops *act, struct tc_action_ops *a; int ret; - if (!act->act || !act->dump || !act->init) + if (!act->act || !act->dump || (!act->init && !act->init_ops)) return -EINVAL; /* We have to register pernet ops before making the action ops visible, @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, } } - err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, - userflags.value | flags, extack); + if (a_o->init) + err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp, + userflags.value | flags, extack); + else if (a_o->init_ops) + err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a, + tp, a_o, userflags.value | flags, + extack); } else { err = a_o->init(net, nla, est, &a, tp, userflags.value | flags, extack);