On Sat, Mar 15, 2025 at 4:24 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Mar 13, 2025 at 12:03 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > From: Amery Hung <amery.hung@xxxxxxxxxxxxx>
> >
> > Allow bpf qdisc programs to update Qdisc qstats directly with btf struct
> > access.
> >
> > Signed-off-by: Amery Hung <amery.hung@xxxxxxxxxxxxx>
> > ---
> > 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
> >
