On Fri, 17 May 2024 at 03:22, Amery Hung <ameryhung@xxxxxxxxx> wrote:
>
> On Thu, May 16, 2024 at 5:24 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > On Fri, 17 May 2024 at 02:17, Amery Hung <ameryhung@xxxxxxxxx> wrote:
> > >
> > > On Thu, May 16, 2024 at 4:59 PM Kumar Kartikeya Dwivedi
> > > <memxor@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, 10 May 2024 at 21:24, Amery Hung <ameryhung@xxxxxxxxx> wrote:
> > > > >
> > > > > This patch supports struct_ops programs that acqurie referenced kptrs
> > > > > throguh arguments. In Qdisc_ops, an skb is passed to ".enqueue" in the
> > > > > first argument. The qdisc becomes the sole owner of the skb and must
> > > > > enqueue or drop the skb. This matches the referenced kptr semantic
> > > > > in bpf. However, the existing practice of acquiring a referenced kptr via
> > > > > a kfunc with KF_ACQUIRE does not play well in this case. Calling kfuncs
> > > > > repeatedly allows the user to acquire multiple references, while there
> > > > > should be only one reference to a unique skb in a qdisc.
> > > > >
> > > > > The solutioin is to make a struct_ops program automatically acquire a
> > > > > referenced kptr through a tagged argument in the stub function. When
> > > > > tagged with "__ref_acquired" (suggestion for a better name?), an
> > > > > reference kptr (ref_obj_id > 0) will be acquired automatically when
> > > > > entering the program. In addition, only the first read to the arguement
> > > > > is allowed and it will yeild a referenced kptr.
> > > > >
> > > > > Signed-off-by: Amery Hung <amery.hung@xxxxxxxxxxxxx>
> > > > > ---
> > > > > include/linux/bpf.h | 3 +++
> > > > > kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++----
> > > > > kernel/bpf/btf.c | 10 +++++++++-
> > > > > kernel/bpf/verifier.c | 16 +++++++++++++---
> > > > > 4 files changed, 38 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > > index 9c6a7b8ff963..6aabca1581fe 100644
> > > > > --- a/include/linux/bpf.h
> > > > > +++ b/include/linux/bpf.h
> > > > > @@ -914,6 +914,7 @@ struct bpf_insn_access_aux {
> > > > > struct {
> > > > > struct btf *btf;
> > > > > u32 btf_id;
> > > > > + u32 ref_obj_id;
> > > > > };
> > > > > };
> > > > > struct bpf_verifier_log *log; /* for verbose logs */
> > > > > @@ -1416,6 +1417,8 @@ struct bpf_ctx_arg_aux {
> > > > > enum bpf_reg_type reg_type;
> > > > > struct btf *btf;
> > > > > u32 btf_id;
> > > > > + u32 ref_obj_id;
> > > > > + bool ref_acquired;
> > > > > };
> > > > >
> > > > > struct btf_mod_pair {
> > > > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > > > > index 86c7884abaf8..bca8e5936846 100644
> > > > > --- a/kernel/bpf/bpf_struct_ops.c
> > > > > +++ b/kernel/bpf/bpf_struct_ops.c
> > > > > @@ -143,6 +143,7 @@ void bpf_struct_ops_image_free(void *image)
> > > > > }
> > > > >
> > > > > #define MAYBE_NULL_SUFFIX "__nullable"
> > > > > +#define REF_ACQUIRED_SUFFIX "__ref_acquired"
> > > > > #define MAX_STUB_NAME 128
> > > > >
> > > > > /* Return the type info of a stub function, if it exists.
> > > > > @@ -204,6 +205,7 @@ static int prepare_arg_info(struct btf *btf,
> > > > > struct bpf_struct_ops_arg_info *arg_info)
> > > > > {
> > > > > const struct btf_type *stub_func_proto, *pointed_type;
> > > > > + bool is_nullable = false, is_ref_acquired = false;
> > > > > const struct btf_param *stub_args, *args;
> > > > > struct bpf_ctx_arg_aux *info, *info_buf;
> > > > > u32 nargs, arg_no, info_cnt = 0;
> > > > > @@ -240,8 +242,11 @@ static int prepare_arg_info(struct btf *btf,
> > > > > /* Skip arguments that is not suffixed with
> > > > > * "__nullable".
> > > > > */
> > > > > - if (!btf_param_match_suffix(btf, &stub_args[arg_no],
> > > > > - MAYBE_NULL_SUFFIX))
> > > > > + is_nullable = btf_param_match_suffix(btf, &stub_args[arg_no],
> > > > > + MAYBE_NULL_SUFFIX);
> > > > > + is_ref_acquired = btf_param_match_suffix(btf, &stub_args[arg_no],
> > > > > + REF_ACQUIRED_SUFFIX);
> > > > > + if (!(is_nullable || is_ref_acquired))
> > > > > continue;
> > > > >
> > > > > /* Should be a pointer to struct */
> > > > > @@ -269,11 +274,15 @@ static int prepare_arg_info(struct btf *btf,
> > > > > }
> > > > >
> > > > > /* Fill the information of the new argument */
> > > > > - info->reg_type =
> > > > > - PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > > > > info->btf_id = arg_btf_id;
> > > > > info->btf = btf;
> > > > > info->offset = offset;
> > > > > + if (is_nullable) {
> > > > > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > > > > + } else if (is_ref_acquired) {
> > > > > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID;
> > > > > + info->ref_acquired = true;
> > > > > + }
> > > > >
> > > > > info++;
> > > > > info_cnt++;
> > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > > index 8c95392214ed..e462fb4a4598 100644
> > > > > --- a/kernel/bpf/btf.c
> > > > > +++ b/kernel/bpf/btf.c
> > > > > @@ -6316,7 +6316,8 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > > >
> > > > > /* this is a pointer to another type */
> > > > > for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
> > > > > - const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
> > > > > + struct bpf_ctx_arg_aux *ctx_arg_info =
> > > > > + (struct bpf_ctx_arg_aux *)&prog->aux->ctx_arg_info[i];
> > > > >
> > > > > if (ctx_arg_info->offset == off) {
> > > > > if (!ctx_arg_info->btf_id) {
> > > > > @@ -6324,9 +6325,16 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > > > return false;
> > > > > }
> > > > >
> > > > > + if (ctx_arg_info->ref_acquired && !ctx_arg_info->ref_obj_id) {
> > > > > + bpf_log(log, "cannot acquire a reference to context argument offset %u\n", off);
> > > > > + return false;
> > > > > + }
> > > > > +
> > > > > info->reg_type = ctx_arg_info->reg_type;
> > > > > info->btf = ctx_arg_info->btf ? : btf_vmlinux;
> > > > > info->btf_id = ctx_arg_info->btf_id;
> > > > > + info->ref_obj_id = ctx_arg_info->ref_obj_id;
> > > > > + ctx_arg_info->ref_obj_id = 0;
> > > > > return true;
> > > >
> > > > I think this is fragile. What if the compiler produces two independent
> > > > paths in the program which read the skb pointer once?
> > > > Technically, the program is still reading the skb pointer once at runtime.
> > > > Then you will reset ref_obj_id to 0 when exploring one, and assign as
> > > > 0 in the other one, causing errors.
> > > > ctx_arg_info appears to be global for the program.
> > > >
> > > > I think the better way would be to check if ref_obj_id is still part
> > > > of the reference state.
> > > > If the ref_obj_id has already been dropped from reference_state, then
> > > > any loads should get ref_obj_id = 0.
> > > > That would happen when dropping or enqueueing the skb into qdisc,
> > > > which would (I presume) do release_reference_state(ref_obj_id).
> > > > Otherwise, all of them can share the same ref_obj_id. You won't have
> > > > to implement "can only read once" logic,
> > > > and when you enqueue stuff in the qdisc, all identical copies produced
> > > > from different load instructions will be invalidated.
> > > > Same ref_obj_id == unique ownership of the same object.
> > > > You can already have multiple copies through rX = rY, multiple ctx
> > > > loads of skb will produce a similar verifier state.
> > > >
> > > > So, on entry, assign ctx_arg_info->ref_obj_id uniquely, then on each load:
> > > > if reference_state.find(ctx_arg_info->ref_obj_id) == true; then
> > > > info->ref_obj_id = ctx_arg_info->ref_obj_id; else info->ref_obj_id =
> > > > 0;
> > > >
> > > > Let me know if I missed something.
> > >
> > > You are right. The current approach will falsely reject valid programs,
> > > and your suggestion makes sense.
> >
> > Also, I wonder whether when ref_obj_id has been released, we should
> > mark the loaded register as unknown scalar, vs skb with ref_obj_id =
> > 0?
> > Otherwise right now it will take PTR_TO_BTF_ID | PTR_TRUSTED as
> > reg_type, and I think verifier will permit reads even if ref_obj_id =
> > 0.
>
> If reference_state.find(ctx_arg_info->ref_obj_id) == false, I think we
> should just return false from btf_ctx_access and reject the program
> right away.
>
Hm, yeah, that could be another option as well.
Might be better than returning a scalar and confusing people on usage later.
