netdev - Re: [PATCH bpf-next v5 07/13] bpf: net

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMB2axM-601dn0_vvprHp5qdVPb2-204ZHR8zh2pwo6GHo6UCQ@mail.gmail.com>
Date: Sun, 16 Mar 2025 21:56:22 +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 07/13] bpf: net_sched: Support updating qstats

On Sat, Mar 15, 2025 at 4:24 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Mar 13, 2025 at 12:03 PM Amery Hung <ameryhung@...il.com> wrote:
> >
> > From: Amery Hung <amery.hung@...edance.com>
> >
> > Allow bpf qdisc programs to update Qdisc qstats directly with btf struct
> > access.
> >
> > Signed-off-by: Amery Hung <amery.hung@...edance.com>
> > ---
> >  net/sched/bpf_qdisc.c | 53 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 45 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> > index edf01f3f1c2a..6ad3050275a4 100644
> > --- a/net/sched/bpf_qdisc.c
> > +++ b/net/sched/bpf_qdisc.c
> > @@ -36,6 +36,7 @@ bpf_qdisc_get_func_proto(enum bpf_func_id func_id,
> >         }
> >  }
> >
> > +BTF_ID_LIST_SINGLE(bpf_qdisc_ids, struct, Qdisc)
> >  BTF_ID_LIST_SINGLE(bpf_sk_buff_ids, struct, sk_buff)
> >  BTF_ID_LIST_SINGLE(bpf_sk_buff_ptr_ids, struct, bpf_sk_buff_ptr)
> >
> > @@ -60,20 +61,37 @@ static bool bpf_qdisc_is_valid_access(int off, int size,
> >         return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
> >  }
> >
> > -static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log,
> > -                                       const struct bpf_reg_state *reg,
> > -                                       int off, int size)
> > +static int bpf_qdisc_qdisc_access(struct bpf_verifier_log *log,
> > +                                 const struct bpf_reg_state *reg,
> > +                                 int off, int size)
>
> Introducing this func in patch 3 and refactoring in patch 7 ?
> pls avoid the churn.
> squash it ?
>
> if (off + size > end) check wouldn't need to be duplicated.
> Can get the name of struct from btf for bpf_log() purpose.
>

I will squash this patch to patch 3 and share the check in
bpf_qdisc_btf_struct_access() to avoid duplication.

Thanks,
Amery

> >  {
> > -       const struct btf_type *t, *skbt;
> >         size_t end;
> >
> > -       skbt = btf_type_by_id(reg->btf, bpf_sk_buff_ids[0]);
> > -       t = btf_type_by_id(reg->btf, reg->btf_id);
> > -       if (t != skbt) {
> > -               bpf_log(log, "only read is supported\n");
> > +       switch (off) {
> > +       case offsetof(struct Qdisc, qstats) ... offsetofend(struct Qdisc, qstats) - 1:
> > +               end = offsetofend(struct Qdisc, qstats);
> > +               break;
> > +       default:
> > +               bpf_log(log, "no write support to Qdisc at off %d\n", off);
> > +               return -EACCES;
> > +       }
> > +
> > +       if (off + size > end) {
> > +               bpf_log(log,
> > +                       "write access at off %d with size %d beyond the member of Qdisc ended at %zu\n",
> > +                       off, size, end);
> >                 return -EACCES;
> >         }
> >
> > +       return 0;
> > +}
> > +
> > +static int bpf_qdisc_sk_buff_access(struct bpf_verifier_log *log,
> > +                                   const struct bpf_reg_state *reg,
> > +                                   int off, int size)
> > +{
> > +       size_t end;
> > +
> >         switch (off) {
> >         case offsetof(struct sk_buff, tstamp):
> >                 end = offsetofend(struct sk_buff, tstamp);
> > @@ -115,6 +133,25 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log,
> >         return 0;
> >  }
> >
> > +static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log,
> > +                                      const struct bpf_reg_state *reg,
> > +                                      int off, int size)
> > +{
> > +       const struct btf_type *t, *skbt, *qdisct;
> > +
> > +       skbt = btf_type_by_id(reg->btf, bpf_sk_buff_ids[0]);
> > +       qdisct = btf_type_by_id(reg->btf, bpf_qdisc_ids[0]);
> > +       t = btf_type_by_id(reg->btf, reg->btf_id);
> > +
> > +       if (t == skbt)
> > +               return bpf_qdisc_sk_buff_access(log, reg, off, size);
> > +       else if (t == qdisct)
> > +               return bpf_qdisc_qdisc_access(log, reg, off, size);
> > +
> > +       bpf_log(log, "only read is supported\n");
> > +       return -EACCES;
> > +}
> > +
> >  BTF_ID_LIST(bpf_qdisc_init_prologue_ids)
> >  BTF_ID(func, bpf_qdisc_init_prologue)
> >
> > --
> > 2.47.1
> >