Re: [External] Storing sk_buffs as kptrs in map

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

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