netdev - Re: [PATCH bpf-next v5 09/13] bpf: net_sched: Disable attaching bpf qdisc to non root

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]

Message-ID: <CAMB2axNQiayH3_UHYFFi-OZ9tK0KmQ-b-k49ep=mio5dcRTGCQ@mail.gmail.com>
Date: Sun, 16 Mar 2025 21:58:16 +0800
From: Amery Hung <ameryhung@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...nel.org>, Jakub Kicinski <kuba@...nel.org>, 
	Eric Dumazet <edumazet@...gle.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jamal Hadi Salim <jhs@...atatu.com>, Kui-Feng Lee <sinquersw@...il.com>, 
	Toke Høiland-Jørgensen <toke@...hat.com>, 
	Jiri Pirko <jiri@...nulli.us>, Stanislav Fomichev <stfomichev@...il.com>, 
	ekarani.silvestre@....ufcg.edu.br, yangpeihao@...u.edu.cn, 
	Peilin Ye <yepeilin.cs@...il.com>, Kernel Team <kernel-team@...a.com>
Subject: Re: [PATCH bpf-next v5 09/13] bpf: net_sched: Disable attaching bpf
 qdisc to non root

On Sat, Mar 15, 2025 at 4:31 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Mar 13, 2025 at 12:03 PM Amery Hung <ameryhung@...il.com> wrote:
> >
> > Do not allow users to attach bpf qdiscs to classful qdiscs. This is to
> > prevent accidentally breaking existings classful qdiscs if they rely on
> > some data in the child qdisc. This restriction can potentially be lifted
> > in the future. Note that, we still allow bpf qdisc to be attached to mq.
> >
> > Signed-off-by: Amery Hung <ameryhung@...il.com>
> > ---
> >  net/sched/bpf_qdisc.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> > index e4e7a5879869..c2f33cd35674 100644
> > --- a/net/sched/bpf_qdisc.c
> > +++ b/net/sched/bpf_qdisc.c
> > @@ -170,8 +170,11 @@ static int bpf_qdisc_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
> >                 return 0;
> >
> >         *insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
> > +       *insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, 16);
> >         *insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0);
>
> Both loads need a comment.
> It's st_ops callback specific and not obvious what ends up in r1 and r2.
>

Got it. I will clarify this in the comment.

> >         *insn++ = BPF_CALL_KFUNC(0, bpf_qdisc_init_prologue_ids[0]);
> > +       *insn++ = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1);
> > +       *insn++ = BPF_EXIT_INSN();
> >         *insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
> >         *insn++ = prog->insnsi[0];
> >
> > @@ -239,11 +242,26 @@ __bpf_kfunc void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64
> >  }
> >
> >  /* bpf_qdisc_init_prologue - Hidden kfunc called in prologue of .init. */
> > -__bpf_kfunc void bpf_qdisc_init_prologue(struct Qdisc *sch)
> > +__bpf_kfunc int bpf_qdisc_init_prologue(struct Qdisc *sch,
> > +                                       struct netlink_ext_ack *extack)
> >  {
> >         struct bpf_sched_data *q = qdisc_priv(sch);
> > +       struct net_device *dev = qdisc_dev(sch);
> > +       struct Qdisc *p;
> > +
> > +       if (sch->parent != TC_H_ROOT) {
> > +               p = qdisc_lookup(dev, TC_H_MAJ(sch->parent));
> > +               if (!p)
> > +                       return -ENOENT;
> > +
> > +               if (!(p->flags & TCQ_F_MQROOT)) {
> > +                       NL_SET_ERR_MSG(extack, "BPF qdisc only supported on root or mq");
> > +                       return -EINVAL;
> > +               }
> > +       }
> >
> >         qdisc_watchdog_init(&q->watchdog, sch);
> > +       return 0;
> >  }
> >
> >  /* bpf_qdisc_reset_destroy_epilogue - Hidden kfunc called in epilogue of .reset
> > --
> > 2.47.1
> >