[PATCH] btf: fix symbol BTF ID lookup

[PATCH] btf: fix symbol BTF ID lookup

[Kris Van Hees]

The logic to perform a BTF ID lookup for a symbol did not guard against
BTF data not having been loaded for the module that contains the symbol.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_bpf.c      |  2 +-
 libdtrace/dt_btf.c      | 32 +++++++++++++++++---------------
 libdtrace/dt_btf.h      |  2 +-
 libdtrace/dt_prov_fbt.c |  3 +--
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
index 2ae7fed1..ccec8b43 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -523,7 +523,7 @@ dt_bpf_init_features(dtrace_hdl_t *dtp)
 {
 	uint32_t	btf_id;
 
-	btf_id = dt_btf_lookup_name_kind(dtp, dtp->dt_shared_btf,
+	btf_id = dt_btf_lookup_name_kind(dtp, dtp->dt_exec,
 					 "bpf_get_btf_vmlinux", BTF_KIND_FUNC);
 	if (btf_id >= 0 &&
 	    have_attach_type(BPF_PROG_TYPE_TRACING, BPF_TRACE_FENTRY, btf_id))
diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
index e9ead435..b84bba73 100644
--- a/libdtrace/dt_btf.c
+++ b/libdtrace/dt_btf.c
@@ -867,29 +867,32 @@ ok:
 }
 
 int32_t
-dt_btf_lookup_name_kind(dtrace_hdl_t *dtp, dt_btf_t *btf, const char *name,
+dt_btf_lookup_name_kind(dtrace_hdl_t *dtp, dt_module_t *dmp, const char *name,
 			uint32_t kind)
 {
-	int32_t	i, base = 0;
+	dt_btf_t	*btf = dmp->dm_btf;
+	int32_t		i, base = 0;
 
 	if (kind == BTF_KIND_UNKN)
 		return -ENOENT;
 	if (strcmp(name, "void") == 0)
 		return 0;
 
-	/*
-	 * Ensure the shared BTF is loaded, and if no BTF is given, use the
-	 * shared one.
-	 */
-	 if (!dtp->dt_shared_btf) {
-		  dt_btf_load_module(dtp, dtp->dt_exec);
+	/* Ensure the shared BTF is loaded. */
+	if (!dtp->dt_shared_btf)
+		dt_btf_load_module(dtp, dtp->dt_exec);
 
-		  if (!btf)
-			   btf = dtp->dt_shared_btf;
-	 }
+	/* If the module does not have BTF data yet, try to load it. */
+	if (!btf) {
+		btf = dt_btf_load_module(dtp, dmp);
 
-	 if (!btf)
-		  return -ENOENT;
+		/* If no BTF momdule data was found, use the shared BTF. */
+		if (!btf)
+			btf = dtp->dt_shared_btf;
+
+		if (!btf)
+			return -ENOENT;
+	}
 
 	/*
 	 * Any module other than 'vmlinux' inherits the types from 'vmlinux'.
@@ -915,8 +918,7 @@ dt_btf_lookup_name_kind(dtrace_hdl_t *dtp, dt_btf_t *btf, const char *name,
 	}
 
 	if (base > 0)
-		return dt_btf_lookup_name_kind(dtp, dtp->dt_shared_btf,
-					       name, kind);
+		return dt_btf_lookup_name_kind(dtp, dtp->dt_exec, name, kind);
 
 	return -ENOENT;
 }
diff --git a/libdtrace/dt_btf.h b/libdtrace/dt_btf.h
index e05b30d0..18fc70b5 100644
--- a/libdtrace/dt_btf.h
+++ b/libdtrace/dt_btf.h
@@ -21,7 +21,7 @@ extern void dt_btf_destroy(dtrace_hdl_t *, dt_btf_t *);
 extern dt_btf_t *dt_btf_load_module(dtrace_hdl_t *, dt_module_t *);
 extern ctf_dict_t *dt_btf_module_ctf(dtrace_hdl_t *, dt_module_t *);
 extern const char *dt_btf_get_string(dtrace_hdl_t *, dt_btf_t *, uint32_t);
-extern int32_t dt_btf_lookup_name_kind(dtrace_hdl_t *, dt_btf_t *,
+extern int32_t dt_btf_lookup_name_kind(dtrace_hdl_t *, dt_module_t *,
 				       const char *, uint32_t);
 extern int dt_btf_func_argc(dtrace_hdl_t *dtp, const dt_btf_t *btf,
 			    uint32_t id);
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 2dd2b7f6..0ba20988 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -350,8 +350,7 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 	if (dmp == NULL)
 		goto done;
 
-	btf_id = dt_btf_lookup_name_kind(dtp, dmp->dm_btf, desc->fun,
-					 BTF_KIND_FUNC);
+	btf_id = dt_btf_lookup_name_kind(dtp, dmp, desc->fun, BTF_KIND_FUNC);
 	if (btf_id <= 0)
 		goto done;
 
-- 
2.43.5

Re: [PATCH] btf: fix symbol BTF ID lookup

[Nick Alcock]

On 24 Jul 2025, Kris Van Hees said:

> The logic to perform a BTF ID lookup for a symbol did not guard against
> BTF data not having been loaded for the module that contains the symbol.

Oops!

> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>

This seems not to do what it says on the tin (though what it does
doesn't look bad).

It's not guarding against a module not being loaded: it's doing all
lookups in vmlinux rather than the shared repo if the module isn't
found. I guess this is desirable (it does increase the set of available
types), but it might be worth mentioning that we're doing this
somewhere, given that at no point does what is actually implemented
guard against BTF not having been loaded for the module that contains
the symbol :) maybe "push the code that decides which BTF to use down
next to the code that loads it" or something?

There are a few rather odd hunks in here, notably:

> -	/*
> -	 * Ensure the shared BTF is loaded, and if no BTF is given, use the
> -	 * shared one.
> -	 */
> -	 if (!dtp->dt_shared_btf) {
> -		  dt_btf_load_module(dtp, dtp->dt_exec);
> +	/* Ensure the shared BTF is loaded. */
> +	if (!dtp->dt_shared_btf)
> +		dt_btf_load_module(dtp, dtp->dt_exec);


If we don't have the dt_shared_btf... load the dt_exec? I mean yes this
will probably load dt_shared_btf as a side-effect, but why not check
dtp->dt_exec? (Hell, why not check both?)

> +	/* If the module does not have BTF data yet, try to load it. */
> +	if (!btf) {
> +		btf = dt_btf_load_module(dtp, dmp);

That looks good though.

> +		/* If no BTF momdule data was found, use the shared BTF. */
> +		if (!btf)
> +			btf = dtp->dt_shared_btf;

s/momdule/module/

>  	if (base > 0)
> -		return dt_btf_lookup_name_kind(dtp, dtp->dt_shared_btf,
> -					       name, kind);
> +		return dt_btf_lookup_name_kind(dtp, dtp->dt_exec, name, kind);

This definitely looks wrong. The intent here is seemingly to climb to
the parent (the shared BTF) if a lookup in the child fails, since you
don't have anything like libctf here to do that for you. Replacing it
with a lookup of dt_exec isn't going to do the same thing at all!

Re: [PATCH] btf: fix symbol BTF ID lookup

[Kris Van Hees]

On Fri, Jul 25, 2025 at 07:37:45PM +0100, Nick Alcock wrote:
> On 24 Jul 2025, Kris Van Hees said:
> 
> > The logic to perform a BTF ID lookup for a symbol did not guard against
> > BTF data not having been loaded for the module that contains the symbol.
> 
> Oops!
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> 
> This seems not to do what it says on the tin (though what it does
> doesn't look bad).
> 
> It's not guarding against a module not being loaded: it's doing all
> lookups in vmlinux rather than the shared repo if the module isn't
> found. I guess this is desirable (it does increase the set of available
> types), but it might be worth mentioning that we're doing this
> somewhere, given that at no point does what is actually implemented
> guard against BTF not having been loaded for the module that contains
> the symbol :) maybe "push the code that decides which BTF to use down
> next to the code that loads it" or something?

Yes, it does, in the sense that it forces the the data to be loaded if it
is not loaded yet.

> There are a few rather odd hunks in here, notably:
> 
> > -	/*
> > -	 * Ensure the shared BTF is loaded, and if no BTF is given, use the
> > -	 * shared one.
> > -	 */
> > -	 if (!dtp->dt_shared_btf) {
> > -		  dt_btf_load_module(dtp, dtp->dt_exec);
> > +	/* Ensure the shared BTF is loaded. */
> > +	if (!dtp->dt_shared_btf)
> > +		dt_btf_load_module(dtp, dtp->dt_exec);
> 
> 
> If we don't have the dt_shared_btf... load the dt_exec? I mean yes this
> will probably load dt_shared_btf as a side-effect, but why not check
> dtp->dt_exec? (Hell, why not check both?)

dt_exec is the dt_module_t representing vmlinux, and thus its dm_btf *is*
shared_btf.  And if you look at the load code, this *will* initialize the
shared_btf (not *will probably*).

> > +	/* If the module does not have BTF data yet, try to load it. */
> > +	if (!btf) {
> > +		btf = dt_btf_load_module(dtp, dmp);
> 
> That looks good though.
> 
> > +		/* If no BTF momdule data was found, use the shared BTF. */
> > +		if (!btf)
> > +			btf = dtp->dt_shared_btf;
> 
> s/momdule/module/

Thanks.

> >  	if (base > 0)
> > -		return dt_btf_lookup_name_kind(dtp, dtp->dt_shared_btf,
> > -					       name, kind);
> > +		return dt_btf_lookup_name_kind(dtp, dtp->dt_exec, name, kind);
> 
> This definitely looks wrong. The intent here is seemingly to climb to
> the parent (the shared BTF) if a lookup in the child fails, since you
> don't have anything like libctf here to do that for you. Replacing it
> with a lookup of dt_exec isn't going to do the same thing at all!

dt_exec == vmlinux, so this will trigger a lookup in shared_btf (which is the
BTF of vmlinux aka dt_exec).

Re: [PATCH] btf: fix symbol BTF ID lookup

[Nick Alcock]

On 25 Jul 2025, Kris Van Hees said:

> On Fri, Jul 25, 2025 at 07:37:45PM +0100, Nick Alcock wrote:
>> On 24 Jul 2025, Kris Van Hees said:
>> > -	/*
>> > -	 * Ensure the shared BTF is loaded, and if no BTF is given, use the
>> > -	 * shared one.
>> > -	 */
>> > -	 if (!dtp->dt_shared_btf) {
>> > -		  dt_btf_load_module(dtp, dtp->dt_exec);
>> > +	/* Ensure the shared BTF is loaded. */
>> > +	if (!dtp->dt_shared_btf)
>> > +		dt_btf_load_module(dtp, dtp->dt_exec);
>> 
>> 
>> If we don't have the dt_shared_btf... load the dt_exec? I mean yes this
>> will probably load dt_shared_btf as a side-effect, but why not check
>> dtp->dt_exec? (Hell, why not check both?)
>
> dt_exec is the dt_module_t representing vmlinux, and thus its dm_btf *is*
> shared_btf.  And if you look at the load code, this *will* initialize the
> shared_btf (not *will probably*).

... yeah, I forgot that dt_exec was a module, not a CTF. It makes sense
given that.

>> >  	if (base > 0)
>> > -		return dt_btf_lookup_name_kind(dtp, dtp->dt_shared_btf,
>> > -					       name, kind);
>> > +		return dt_btf_lookup_name_kind(dtp, dtp->dt_exec, name, kind);
>> 
>> This definitely looks wrong. The intent here is seemingly to climb to
>> the parent (the shared BTF) if a lookup in the child fails, since you
>> don't have anything like libctf here to do that for you. Replacing it
>> with a lookup of dt_exec isn't going to do the same thing at all!
>
> dt_exec == vmlinux, so this will trigger a lookup in shared_btf (which is the
> BTF of vmlinux aka dt_exec).

Aha. My brain was operating in CTF mode, in which of course vmlinux has
its own dict distinct from the shared parent. That's OK, then.

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

... and I shouldn't do these when I'm this tired, this was all obvious.

-- 
NULL && (void)