On Tue, Aug 6, 2024 at 6:24 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> > Jiri,
> >
> > the verifier removes the check because it assumes that pointers
> > passed by the kernel into tracepoint are valid and trusted.
> > In this case:
> > trace_sched_pi_setprio(p, pi_task);
> >
> > pi_task can be NULL.
> >
> > We cannot make all tracepoint pointers to be PTR_TRUSTED | PTR_MAYBE_NULL
> > by default, since it will break a bunch of progs.
> > Instead we can annotate this tracepoint arg as __nullable and
> > teach the verifier to recognize such special arguments of tracepoints.
>
> ok, so you mean to be able to mark it in event header like:
>
> TRACE_EVENT(sched_pi_setprio,
> TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task __nullable),
>
> I guess we could make pahole to emit DECL_TAG for that argument,
> but I'm not sure how to propagate that __nullable info to pahole
>
> while wondering about that, I tried the direct fix below ;-)
We don't need to rush such a hack below.
No need to add decl_tag and change pahole either.
The arg name is already vmlinux BTF:
[51371] FUNC_PROTO '(anon)' ret_type_id=0 vlen=3
'__data' type_id=61
'tsk' type_id=77
'pi_task' type_id=77
[51372] FUNC '__bpf_trace_sched_pi_setprio' type_id=51371 linkage=static
just need to rename "pi_task" to "pi_task__nullable"
and teach the verifier.
> jirka
>
>
> ---
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 95426d5b634e..1a20bbdead64 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6377,6 +6377,25 @@ int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto,
> return off;
> }
>
> +static bool is_tracing_prog_raw_tp(const struct bpf_prog *prog, const char *name)
> +{
> + struct btf *btf = prog->aux->attach_btf;
> + const struct btf_type *t;
> + const char *tname;
> +
> + if (prog->expected_attach_type != BPF_TRACE_RAW_TP)
> + return false;
> +
> + t = btf_type_by_id(btf, prog->aux->attach_btf_id);
> + if (!t)
> + return false;
> +
> + tname = btf_name_by_offset(btf, t->name_off);
> + if (!tname)
> + return false;
> + return !strcmp(tname, name);
> +}
> +
> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const struct bpf_prog *prog,
> struct bpf_insn_access_aux *info)
> @@ -6544,6 +6563,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> }
> }
>
> + /* Second argument of sched_pi_setprio tracepoint can be null */
> + if (is_tracing_prog_raw_tp(prog, "btf_trace_sched_pi_setprio") && arg == 1)
> + info->reg_type |= PTR_MAYBE_NULL;
> +
> info->btf = btf;
> info->btf_id = t->type;
> t = btf_type_by_id(btf, t->type);
