netdev - Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind

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: Tue, 4 Jul 2023 17:42:29 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Jamal Hadi Salim <hadi@...atatu.com>, Victor Nogueira <victor@...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, 
	pctammela@...atatu.com, kernel@...atatu.com
Subject: Re: [PATCH net 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case
 of an error

On Tue, Jul 4, 2023 at 5:20 PM Simon Horman <simon.horman@...igine.com> wrote:
>
> On Tue, Jul 04, 2023 at 04:55:25PM -0400, Jamal Hadi Salim wrote:
> > On Tue, Jul 4, 2023 at 4:48 PM Simon Horman <simon.horman@...igine.com> wrote:
> > >
> > > On Tue, Jul 04, 2023 at 12:14:52PM -0300, Victor Nogueira wrote:
> > > > If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
> > > > was done in cls_bpf_set_parms.
> > > >
> > > > Fix that by calling tcf_unbind_filter in errout_parms.
> > > >
> > > > Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
> > > >
> > >
> > > nit: no blank line here.
> > >
> > > > Signed-off-by: Victor Nogueira <victor@...atatu.com>
> > > > Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
> > > > Reviewed-by: Pedro Tammela <pctammela@...atatu.com>
> > > > ---
> > > >  net/sched/cls_bpf.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> > > > index 466c26df853a..4d9974b1b29d 100644
> > > > --- a/net/sched/cls_bpf.c
> > > > +++ b/net/sched/cls_bpf.c
> > > > @@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
> > > >  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > >                            struct cls_bpf_prog *prog, unsigned long base,
> > > >                            struct nlattr **tb, struct nlattr *est, u32 flags,
> > > > -                          struct netlink_ext_ack *extack)
> > > > +                          bool *bound_to_filter, struct netlink_ext_ack *extack)
> > > >  {
> > > >       bool is_bpf, is_ebpf, have_exts = false;
> > > >       u32 gen_flags = 0;
> > > > @@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
> > > >       if (tb[TCA_BPF_CLASSID]) {
> > > >               prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
> > > >               tcf_bind_filter(tp, &prog->res, base);
> > > > +             *bound_to_filter = true;
> > > >       }
> > > >
> > > >       return 0;
> > > > @@ -464,6 +465,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
> > > >  {
> > > >       struct cls_bpf_head *head = rtnl_dereference(tp->root);
> > > >       struct cls_bpf_prog *oldprog = *arg;
> > > > +     bool bound_to_filter = false;
> > > >       struct nlattr *tb[TCA_BPF_MAX + 1];
> > > >       struct cls_bpf_prog *prog;
> > > >       int ret;
> > >
> > > Please use reverse xmas tree - longest line to shortest - for
> > > local variable declarations in Networking code.
> > >
> >
> > I think Ed's tool is actually wrong on this Simon.
> > The rule I know of is: initializations first then declarations -
> > unless it is documented elsewhere as not the case.
>
> Hi Jamal,
>
> That is not my understanding of the rule.

Something about mixing assignments and declarations being
cplusplusish. That's always how my fingers think.

So how would this have been done differently? This is the current patch:
----
        struct cls_bpf_head *head = rtnl_dereference(tp->root);
        struct cls_bpf_prog *oldprog = *arg;
+       bool bound_to_filter = false;
        struct nlattr *tb[TCA_BPF_MAX + 1];
        struct cls_bpf_prog *prog;
        int ret;
----

Should the change be?
---
        struct cls_bpf_head *head = rtnl_dereference(tp->root);
        struct cls_bpf_prog *oldprog = *arg;
        struct nlattr *tb[TCA_BPF_MAX + 1];
+      bool bound_to_filter = false;
        struct cls_bpf_prog *prog;
        int ret;
---

I dont think my gut or brain would let me type that - but if those are
them rules then it is Victor doing the typing ;->

cheers,
jamal