On Fri, Dec 06, 2024 at 04:36:03PM -0800, Martin KaFai Lau wrote:
> On 12/6/24 8:24 AM, Maciej Fijalkowski wrote:
> > > I think we can remove the projection_of call from the
> > > bpf_is_prog_ctx_type() such that it honors the exact argument
> > > type written in the kernel source code. Add this particular projection_of
> > > check (renamed to bpf_is_kern_ctx in the diff) to the other callers for
> > > backward compat such that the caller can selectively translate
> > > the argument of a subprog to the corresponding prog ctx type.
> > >
> > > Lightly tested only:
> > I tried the kernel diff on my side and it addressed my needs. Will you
> > send a patch?
>
> There is no real kfunc taking the "struct sk_buff *" now. It is better to
> make this change together with your skb acquire/release kfunc introduction.
> You can include this patch in your future set.
Ack, good point. Will do as suggested.
>
> >
> > > diff --git i/kernel/bpf/btf.c w/kernel/bpf/btf.c
> > > index e7a59e6462a9..2d39f91617fb 100644
> > > --- i/kernel/bpf/btf.c
> > > +++ w/kernel/bpf/btf.c
> > > @@ -5914,6 +5914,26 @@ bool btf_is_projection_of(const char *pname, const char *tname)
> > > return false;
> > > }
> > > +static bool btf_is_kern_ctx(const struct btf *btf,
> > > + const struct btf_type *t,
> > > + enum bpf_prog_type prog_type)
> > > +{
> > > + const struct btf_type *ctx_type;
> > > + const char *tname, *ctx_tname;
> > > +
> > > + t = btf_type_skip_modifiers(btf, t->type, NULL);
> > > + if (!btf_type_is_struct(t))
> > > + return false;
> > > +
> > > + tname = btf_name_by_offset(btf, t->name_off);
> > > + if (!tname)
> > > + return false;
> > > +
> > > + ctx_type = find_canonical_prog_ctx_type(prog_type);
> > > + ctx_tname = btf_name_by_offset(btf_vmlinux, ctx_type->name_off);
> > > + return btf_is_projection_of(ctx_tname, tname);
> > We're sort of doubling the work that btf_is_prog_ctx_type() is doing also,
> > maybe add a flag to btf_is_prog_ctx_type() that will allow us to skip
> > btf_is_projection_of() call when needed? e.g. in get_kfunc_ptr_arg_type().
>
> It is pretty cheap to do the btf_is_kern_ctx().
>
> I don't have a strong opinion on either way. may be a "bool check_kern_ctx".
It's just that this flow to retrieve both struct names is duplicated,
below would serve the same purpose...but I don't have a strong opinion
either:) anyways, thanks for discussion!
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 4214e76c9168..3c6acb993d5b 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -577,7 +577,7 @@ struct btf_struct_meta *btf_find_struct_meta(const struct btf *btf, u32 btf_id);
bool btf_is_projection_of(const char *pname, const char *tname);
bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
const struct btf_type *t, enum bpf_prog_type prog_type,
- int arg);
+ int arg, bool check_proj);
int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type);
bool btf_types_are_same(const struct btf *btf1, u32 id1,
const struct btf *btf2, u32 id2);
@@ -653,7 +653,7 @@ static inline struct btf_struct_meta *btf_find_struct_meta(const struct btf *btf
static inline bool
btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
const struct btf_type *t, enum bpf_prog_type prog_type,
- int arg)
+ int arg, bool check_proj)
{
return false;
}
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e7a59e6462a9..dd27eb35b827 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5916,7 +5916,7 @@ bool btf_is_projection_of(const char *pname, const char *tname)
bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
const struct btf_type *t, enum bpf_prog_type prog_type,
- int arg)
+ int arg, bool check_proj)
{
const struct btf_type *ctx_type;
const char *tname, *ctx_tname;
@@ -5976,8 +5976,9 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
* int socket_filter_bpf_prog(struct __sk_buff *skb)
* { // no fields of skb are ever used }
*/
- if (btf_is_projection_of(ctx_tname, tname))
- return true;
+ if (check_proj)
+ if (btf_is_projection_of(ctx_tname, tname))
+ return true;
if (strcmp(ctx_tname, tname)) {
/* bpf_user_pt_regs_t is a typedef, so resolve it to
* underlying struct and check name again
@@ -6140,7 +6141,7 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
enum bpf_prog_type prog_type,
int arg)
{
- if (!btf_is_prog_ctx_type(log, btf, t, prog_type, arg))
+ if (!btf_is_prog_ctx_type(log, btf, t, prog_type, arg, true))
return -ENOENT;
return find_kern_ctx_type_id(prog_type);
}
@@ -7505,7 +7506,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
if (!btf_type_is_ptr(t))
goto skip_pointer;
- if ((tags & ARG_TAG_CTX) || btf_is_prog_ctx_type(log, btf, t, prog_type, i)) {
+ if ((tags & ARG_TAG_CTX) || btf_is_prog_ctx_type(log, btf, t, prog_type, i, true)) {
if (tags & ~ARG_TAG_CTX) {
bpf_log(log, "arg#%d has invalid combination of tags\n", i);
return -EINVAL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 60938b136365..bb636f53dfc3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11604,7 +11604,8 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
* type to our caller. When a set of conditions hold in the BTF type of
* arguments, we resolve it to a known kfunc_ptr_arg_type.
*/
- if (btf_is_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
+ if (btf_is_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog),
+ argno, false))
return KF_ARG_PTR_TO_CTX;
if (is_kfunc_arg_nullable(meta->btf, &args[argno]) && register_is_null(reg))
--
2.34.1
