| Message ID | 20230223141639.13491-1-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: act_connmark: handle errno on tcf_idr_check_alloc | expand |
On Thu, Feb 23, 2023 at 9:17 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > Smatch reports that 'ci' can be used uninitialized. > The current code ignores errno coming from tcf_idr_check_alloc, which > will lead to the incorrect usage of 'ci'. Handle the errno as it should. > > Fixes: 288864effe33 ("net/sched: act_connmark: transition to percpu stats and rcu") > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > --- > net/sched/act_connmark.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c > index 8dabfb52ea3d..cf4086a9e3c0 100644 > --- a/net/sched/act_connmark.c > +++ b/net/sched/act_connmark.c > @@ -125,6 +125,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > if (!nparms) > return -ENOMEM; > > + ci = to_connmark(*a); > parm = nla_data(tb[TCA_CONNMARK_PARMS]); > index = parm->index; > ret = tcf_idr_check_alloc(tn, &index, a, bind); > @@ -137,14 +138,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > goto out_free; > } > > - ci = to_connmark(*a); > - > nparms->net = net; > nparms->zone = parm->zone; > > ret = ACT_P_CREATED; > } else if (ret > 0) { > - ci = to_connmark(*a); > if (bind) { > err = 0; > goto out_free; > @@ -158,6 +156,9 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > nparms->zone = parm->zone; > > ret = 0; > + } else { > + err = ret; > + goto out_free; > } > > err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); > -- > 2.34.1 >
On Thu, Feb 23, 2023 at 11:16:39AM -0300, Pedro Tammela wrote: > Smatch reports that 'ci' can be used uninitialized. > The current code ignores errno coming from tcf_idr_check_alloc, which > will lead to the incorrect usage of 'ci'. Handle the errno as it should. > > Fixes: 288864effe33 ("net/sched: act_connmark: transition to percpu stats and rcu") > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- > net/sched/act_connmark.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c > index 8dabfb52ea3d..cf4086a9e3c0 100644 > --- a/net/sched/act_connmark.c > +++ b/net/sched/act_connmark.c > @@ -125,6 +125,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > if (!nparms) > return -ENOMEM; > > + ci = to_connmark(*a); > parm = nla_data(tb[TCA_CONNMARK_PARMS]); > index = parm->index; > ret = tcf_idr_check_alloc(tn, &index, a, bind); > @@ -137,14 +138,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > goto out_free; > } > > - ci = to_connmark(*a); > - > nparms->net = net; > nparms->zone = parm->zone; > > ret = ACT_P_CREATED; > } else if (ret > 0) { > - ci = to_connmark(*a); > if (bind) { > err = 0; > goto out_free; Hi Pedro, I think the issue here isn't so much that there may be incorrect usage of ci - although that can happen - but rather that an error condition - the failure of tcf_idr_check_alloc is ignored. Viewed through this lens I think it becomes clear that the hunk below fixes the problem. While the hunks above are cleanups. A nice cleanup. But still a cleanup. I think that as a fix for 'net' a minimal approach is best and thus the patch below. I'd also like to comment that the usual style for kernel code is to handle error cases in conditions - typically immediately after the condition arises. While non-error cases follow, outside of condtions. F.e. err = do_something(with_something); if (err) { /* handle error */ ... } /* proceed with non-error case here */ ... In the code at hand this is complicate by there being two non-error cases, and it thus being logical to treat them conditionally. Even so, i do wonder if there is value in treating the error case first, right next to the code that might cause the error, in order to make it clearer that the error is being handled (as normal). And in saying so, I do realise it contradicts my statement about minimal changes to some extent. i.e. (*completely untested*) ret = tcf_idr_check_alloc(tn, &index, a, bind); if (ret < 0) { err = ret; goto out_free; } else if (!ret) { ... } else { ... } > @@ -158,6 +156,9 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > nparms->zone = parm->zone; > > ret = 0; > + } else { > + err = ret; > + goto out_free; > } > > err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); > -- > 2.34.1 >
On Fri, Feb 24, 2023 at 4:05 AM Simon Horman <simon.horman@corigine.com> wrote: > [..] > Hi Pedro, > > I think the issue here isn't so much that there may be incorrect usage of > ci - although that can happen - but rather that an error condition - the > failure of tcf_idr_check_alloc is ignored. > > Viewed through this lens I think it becomes clear that the hunk > below fixes the problem. While the hunks above are cleanups. > A nice cleanup. But still a cleanup. > I agree with your analysis. The initialization could be left as is and the else being an error condition would cover it (although it is one less line with Pedro's approach). However, on that "else" train of thought - more comment below: > I think that as a fix for 'net' a minimal approach is best and thus > the patch below. > > I'd also like to comment that the usual style for kernel code is to handle > error cases in conditions - typically immediately after the condition > arises. While non-error cases follow, outside of condtions. > > F.e. > > err = do_something(with_something); > if (err) { > /* handle error */ > ... > } > > /* proceed with non-error case here */ > ... > > In the code at hand this is complicate by there being two non-error cases, > and it thus being logical to treat them conditionally. since 0190c1d452a91 unfortunately given we have the potential of 3 possible return codes (where's before it was either 0 or 1) and it would help to have consistent code in the spirit of if/else if/else - gact is probably the best example for this. My opinion is we should fix all the action init()s to follow that pattern. There are like 3 different patterns and the danger of making a mistake with clever manipulation of the return code (as is done for example in mpls or vlan) is susceptible to breakage once someone submits a patch that is not properly reviewed. cheers, jamal > Even so, i do wonder if there is value in treating the error case first, > right next to the code that might cause the error, in order to make it > clearer that the error is being handled (as normal). > > And in saying so, I do realise it contradicts my statement > about minimal changes to some extent. > > i.e. (*completely untested*) > > ret = tcf_idr_check_alloc(tn, &index, a, bind); > if (ret < 0) { > err = ret; > goto out_free; > } else if (!ret) { > ... > } else { > ... > } > > > @@ -158,6 +156,9 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, > > nparms->zone = parm->zone; > > > > ret = 0; > > + } else { > > + err = ret; > > + goto out_free; > > } > > > > err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack); > > -- > > 2.34.1 > >
Hi Jamal, On Fri, Feb 24, 2023 at 08:04:44AM -0500, Jamal Hadi Salim wrote: > On Fri, Feb 24, 2023 at 4:05 AM Simon Horman <simon.horman@corigine.com> wrote: > > > > [..] > > > Hi Pedro, > > > > I think the issue here isn't so much that there may be incorrect usage of > > ci - although that can happen - but rather that an error condition - the > > failure of tcf_idr_check_alloc is ignored. > > > > Viewed through this lens I think it becomes clear that the hunk > > below fixes the problem. While the hunks above are cleanups. > > A nice cleanup. But still a cleanup. > > > > I agree with your analysis. The initialization could be left as is and > the else being an error condition would cover it > (although it is one less line with Pedro's approach). > However, on that "else" train of thought - more comment below: > > > I think that as a fix for 'net' a minimal approach is best and thus > > the patch below. > > > > I'd also like to comment that the usual style for kernel code is to handle > > error cases in conditions - typically immediately after the condition > > arises. While non-error cases follow, outside of condtions. > > > > F.e. > > > > err = do_something(with_something); > > if (err) { > > /* handle error */ > > ... > > } > > > > /* proceed with non-error case here */ > > ... > > > > In the code at hand this is complicate by there being two non-error cases, > > and it thus being logical to treat them conditionally. > > since 0190c1d452a91 unfortunately given we have the potential of 3 > possible return codes (where's before it was either 0 or 1) and it > would help to have consistent code in the spirit of if/else if/else - > gact is probably the best example for this. > My opinion is we should fix all the action init()s to follow that > pattern. There are like 3 different patterns and the danger of making > a mistake with clever manipulation of the return code (as is done for > example in mpls or vlan) is susceptible to breakage once someone > submits a patch that is not properly reviewed. I agree with that line of thought.
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c index 8dabfb52ea3d..cf4086a9e3c0 100644 --- a/net/sched/act_connmark.c +++ b/net/sched/act_connmark.c @@ -125,6 +125,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, if (!nparms) return -ENOMEM; + ci = to_connmark(*a); parm = nla_data(tb[TCA_CONNMARK_PARMS]); index = parm->index; ret = tcf_idr_check_alloc(tn, &index, a, bind); @@ -137,14 +138,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, goto out_free; } - ci = to_connmark(*a); - nparms->net = net; nparms->zone = parm->zone; ret = ACT_P_CREATED; } else if (ret > 0) { - ci = to_connmark(*a); if (bind) { err = 0; goto out_free; @@ -158,6 +156,9 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla, nparms->zone = parm->zone; ret = 0; + } else { + err = ret; + goto out_free; } err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
Smatch reports that 'ci' can be used uninitialized. The current code ignores errno coming from tcf_idr_check_alloc, which will lead to the incorrect usage of 'ci'. Handle the errno as it should. Fixes: 288864effe33 ("net/sched: act_connmark: transition to percpu stats and rcu") Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_connmark.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)