Re: [PATCH bpf-next v5 07/13] bpf: net_sched: Support updating qstats

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Mar 13, 2025 at 12:03 PM Amery Hung <ameryhung@gmail.com> wrote:
>
> From: Amery Hung <amery.hung@bytedance.com>
>
> Allow bpf qdisc programs to update Qdisc qstats directly with btf struct
> access.
>
> Signed-off-by: Amery Hung <amery.hung@bytedance.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.

>  {
> -       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
>