Re: [PATCH bpf-next 19/38] bpf: Fix stale offload->prog pointer after realloc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: unlocked offload access in bpf_prog_realloc() causes use-after-free
--

commit 3691aa3c9c7d9f29e407360a3a32c4201c98c86f
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Fix stale offload->prog pointer after realloc

This commit updates fp->aux->offload->prog in bpf_prog_realloc() when a
device-bound BPF program is reallocated during verifier rewrites so that
netdev unregistration does not pass a stale program pointer to
__bpf_prog_offload_destroy().

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 642f0ab8eb2d8..2154c18a5a4d8 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -269,6 +269,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
>  		memcpy(fp, fp_old, fp_old->pages * PAGE_SIZE);
>  		fp->pages = pages;
>  		fp->aux->prog = fp;
> +		if (fp->aux->offload)
> +			fp->aux->offload->prog = fp;

[Severity: Critical]
Does this access to fp->aux->offload and the subsequent freeing of fp_old
need synchronization with bpf_devs_lock?

During bpf_prog_load(), bpf_prog_dev_bound_init() links offload into
ondev->progs under bpf_devs_lock and then releases the lock before
bpf_check() runs. As noted in kernel/bpf/offload.c:

kernel/bpf/offload.c:bpf_dev_bound_resolve_kfunc() {
	/* We don't hold bpf_devs_lock while resolving several
	 * kfuncs and can race with the unregister_netdevice().
	 * We rely on bpf_dev_bound_match() check at attach
	 * to render this program unusable.
	 */
	down_read(&bpf_devs_lock);
	if (!prog->aux->offload)
		goto out;
    ...
}

When verifier instruction rewrites grow the program via
bpf_check() -> bpf_patch_insn_data() -> bpf_patch_insn_single() ->
bpf_prog_realloc(), bpf_prog_realloc() runs without bpf_devs_lock while
concurrent netdev unregistration can run on another CPU under
down_write(&bpf_devs_lock):

kernel/bpf/offload.c:__bpf_offload_dev_netdev_unregister() {
    ...
		list_for_each_entry_safe(offload, ptmp, &ondev->progs, offloads)
			__bpf_prog_offload_destroy(offload->prog);
    ...
}

kernel/bpf/offload.c:__bpf_prog_offload_destroy() {
	struct bpf_prog_offload *offload = prog->aux->offload;

	if (offload->dev_state)
		offload->offdev->ops->destroy(prog);

	list_del_init(&offload->offloads);
	kfree(offload);
	prog->aux->offload = NULL;
}

Can this race in two ways?

1. bpf_prog_realloc() checks if (fp->aux->offload) and sees a non-NULL
   pointer, while __bpf_prog_offload_destroy() concurrently frees offload
   and sets prog->aux->offload = NULL, causing fp->aux->offload->prog = fp
   to write to freed slab memory or dereference NULL.

2. __bpf_offload_dev_netdev_unregister() reads offload->prog (still fp_old)
   and passes it to __bpf_prog_offload_destroy(fp_old), while
   bpf_prog_realloc() concurrently clears fp_old->aux = NULL and frees
   fp_old via __bpf_prog_free(fp_old), causing __bpf_prog_offload_destroy()
   to dereference a NULL prog->aux or freed fp_old.

>  
>  		/* We keep fp->aux from fp_old around in the new
>  		 * reallocated structure.

—
🤖 Generated bylinux-kernel-bot

Assisted-by: Google Gemini-3.1-Pro

---
Do youhave feedback on this review? Please let us know by replying to this email or filing an issue at https://github.com/google/linux-kernel-bot/issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017012058.355787-1-alexei.starovoitov@gmail.com?part=19