Re: [PATCH bpf-next 5/6] bpf: defer registrations until the vmlinux BTF is available

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf: UAF in sysfs reader on module BTF alloc ID failure
- [High] bpf: data corruption via un-synchronized kfunc registrations
- [Medium] bpf: race condition causes transient BPF program load failures
- [Medium] bpf: memory leak of vmlinux deferred queue on alloc ID failure
--

commit 7468b94a7c4356b1f1498b95a9030eec1830ffa1
Author: Jay Wang <wanjay@amazon.com>
Subject: bpf: defer registrations until the vmlinux BTF is available

This commit changes the BPF subsystem to defer kfunc, dtor kfunc, and
struct_ops registrations until the vmlinux BTF is parsed. This allows
configuring CONFIG_DEBUG_INFO_BTF as a module so it doesn't need to be parsed
at boot if not required.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 50eb7a95fd825..cbba20a908e9d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
[ ... ]
> @@ -6581,7 +6583,15 @@ struct btf *btf_parse_vmlinux(void)
>  	if (err) {
>  		btf_free(btf);
>  		btf = ERR_PTR(err);
> +		goto err_out;
>  	}
> +
> +	/*
> +	 * With CONFIG_DEBUG_INFO_BTF=m, kfunc, dtor kfunc and struct_ops
> +	 * registrations for vmlinux made before the BTF was available were
> +	 * queued; apply them now, before the BTF becomes visible to anyone.
> +	 */
> +	btf_apply_deferred_vmlinux_regs(btf);
>  err_out:
>  	btf_verifier_env_free(env);
>  	return btf;

[Severity: Medium]
When btf_alloc_id() fails, the code jumps to err_out, which bypasses the call
to btf_apply_deferred_vmlinux_regs(). Does this leave the items in the
btf_vmlinux_deferred_regs queue stranded, causing a memory leak for those
dynamically allocated deferred registrations? Also, since
btf_vmlinux_regs_closed is never set, would subsequent registrations continue
to queue infinitely?

[ ... ]
> @@ -8905,6 +9033,87 @@ static int __init btf_module_init(void)
>  }
>  
>  fs_initcall(btf_module_init);
> +
> +#if IS_MODULE(CONFIG_DEBUG_INFO_BTF)
> +/*
> + * CONFIG_DEBUG_INFO_BTF=m: the vmlinux BTF has just become available.  Parse
> + * the BTF of the modules that were loaded before it, and apply the
> + * registrations that waited for them.  Called from bpf_get_btf_vmlinux()
> + * once btf_vmlinux is published, with no locks held.
> + */
> +void btf_parse_deferred_modules(void)
> +{
[ ... ]
> +		err = PTR_ERR_OR_ZERO(btf);
> +		if (!err) {
> +			err = btf_alloc_id(btf);
> +			if (err) {
> +				/* btf owns the data now, btf_free() drops it */
> +				btf_mod->data = NULL;
> +				btf_free(btf);
> +			}
> +		}
> +		if (err) {
> +			/*
> +			 * The module is loaded and stays.  Unlike at load time
> +			 * there is no way to reject it, so drop its BTF.
> +			 */
> +			pr_warn("failed to validate module [%s] BTF: %d\n",
> +				btf_mod->module->name, err);
> +			list_del(&btf_mod->list);
> +			btf_module_free(btf_mod);
> +			continue;
> +		}

[Severity: High]
Could the btf_free() call above introduce a use-after-free window for
concurrent sysfs readers? The backing buffer btf->data (which was originally
btf_mod->data) is freed by btf_free(). Even though btf_mod->data is set to
NULL, the sysfs file for the module remains active until btf_module_free()
removes it. If there is a concurrent read to /sys/kernel/btf/<module>, can it
dereference the freed attr->private pointer?

[ ... ]
> +		/* btf->data is btf_mod->data now, the sysfs file keeps working */
> +		btf_mod->data = NULL;
> +		kvfree(btf_mod->base_data);
> +		btf_mod->base_data = NULL;
> +		btf_mod->btf = btf;
> +		parsed = true;

[Severity: Medium]
Here btf_mod->btf is assigned, which makes the module BTF visible globally.
Since this happens before the deferred kfuncs and struct_ops are actually
registered, is there a race condition where a concurrent BPF program load
attempts to use this BTF, but fails verification because the kfuncs are not
yet in the module's kfunc_set_tab?

> +
> +		/*
> +		 * Registrations are applied after dropping the mutex (they
> +		 * walk btf_modules); pin what they need until then.
> +		 */
> +		list_for_each_entry_safe(reg, rtmp, &btf_mod->deferred_regs, list) {
> +			list_del(&reg->list);
> +			if (!try_module_get(btf_mod->module)) {
> +				btf_free_deferred_reg(reg);
> +				continue;
> +			}
> +			btf_get(btf);
> +			reg->btf = btf;
> +			reg->module = btf_mod->module;
> +			list_add_tail(&reg->list, &regs);
> +		}
> +	}
> +	mutex_unlock(&btf_module_mutex);
> +
> +	if (parsed)
> +		purge_cand_cache(NULL);
> +	btf_apply_deferred_regs(&regs);
> +}
> +#endif /* IS_MODULE(CONFIG_DEBUG_INFO_BTF) */

[Severity: High]
Is it possible for data corruption to occur here due to concurrent kfunc
registrations? Because the btf_module_mutex is dropped and btf_mod->btf has
already been set, a concurrent module initcall could perform a new
register_btf_kfunc_id_set(). Since btf_mod->btf is no longer NULL, it would
bypass the deferral queue and proceed to btf_kfunc_id_set_add() directly.

If btf_apply_deferred_regs() is running simultaneously, both paths could
concurrently invoke btf_populate_kfunc_set(), which mutates btf->kfunc_set_tab
without synchronization:

kernel/bpf/btf.c:btf_populate_kfunc_set() {
    ...
	/* Grow set */
	set = krealloc(tab->sets[hook],
		       struct_size(set, pairs, set_cnt + add_set->cnt),
		       GFP_KERNEL | __GFP_NOWARN);
	if (!set) {
		ret = -ENOMEM;
		goto end;
	}

	/* For newly allocated set, initialize set->cnt to 0 */
	if (!tab->sets[hook])
		set->cnt = 0;
	tab->sets[hook] = set;
    ...
}

Would this result in double-free or use-after-free on the krealloc, or lost
kfunc registrations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923053948.30617-1-wanjay@amazon.com?part=5