netdev - Re: [PATCH net] net/sched: act_connmark: handle errno on tcf_idr_check

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:   Fri, 24 Feb 2023 08:04:44 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     Pedro Tammela <pctammela@...atatu.com>, netdev@...r.kernel.org,
        xiyou.wangcong@...il.com, jiri@...nulli.us, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        error27@...il.com
Subject: Re: [PATCH net] net/sched: act_connmark: handle errno on tcf_idr_check_alloc

On Fri, Feb 24, 2023 at 4:05 AM Simon Horman <simon.horman@...igine.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
> >