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

[Kris Van Hees <kris.van.hees@oracle.com>]

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).