Re: [PATCH bpf v2] bpf: cpumap: fix use-after-free of dev_rx on netdev unregister

[bot+bpf-ci]

> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index 5e59ab896f058..0c9647f28034d 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c

[ ... ]

> @@ -832,3 +879,39 @@ void __cpu_map_flush(struct list_head *flush_list)
>  		wake_up_process(bq->obj->kthread);
>  	}
>  }
> +
> +/* Frames in the ring and skbs in GRO hold a raw pointer to the ingress
> + * device, make every kthread consume them before the device is freed.
> + */
                                   ^^^

The BPF subsystem requires multi-line comments to have the opening /*
on its own line, with text starting on the next line. This is a newly
added comment. Should it be reformatted like this?

/*
 * Frames in the ring and skbs in GRO hold a raw pointer to the ingress
 * device, make every kthread consume them before the device is freed.
 */

> +static int cpu_map_netdev_event(struct notifier_block *nb,
> +				unsigned long event, void *ptr)
> +{
> +	struct bpf_cpu_map_entry *rcpu;
> +
> +	if (event != NETDEV_UNREGISTER)
> +		return NOTIFY_OK;
> +
> +	mutex_lock(&cpu_map_mutex);
> +	list_for_each_entry(rcpu, &cpu_map_list, list) {
> +		/* the whole ring, plus a batch already pulled out */
> +		WRITE_ONCE(rcpu->drain_left, rcpu->queue->size + CPUMAP_BATCH);
> +		wake_up_process(rcpu->kthread);
> +	}
> +	list_for_each_entry(rcpu, &cpu_map_list, list)
> +		wait_event(rcpu->drain_wq, !READ_ONCE(rcpu->drain_left));
> +	mutex_unlock(&cpu_map_mutex);
> +
> +	return NOTIFY_OK;
> +}

NETDEV_UNREGISTER notifiers run with RTNL held:
call_netdevice_notifiers_info() contains ASSERT_RTNL(), and the call
site in unregister_netdevice_many_notify() also does ASSERT_RTNL() at
entry.

The wait_event() holds RTNL until every cpumap kthread in the system
has consumed rcpu->queue->size + CPUMAP_BATCH frames. The ring size is
bounded only by the 16384 sanity limit in cpu_map_update_elem(), and
the countdown is decremented by frames actually consumed, so a ring
that is continuously fed by a different, still-registered device forces
the full 16392-frame count to be pushed through before returning.

unregister_netdevice_many_notify() calls the notifier once per device
in the batch, so a batched teardown such as netns exit with many veths
multiplies this: N devices x 16392 frames of RTNL-held work,
serialized.

The commit message addresses the bound ("The work is bounded by the
ring size, and the frames had to be consumed anyway") but not the fact
that RTNL is held for the whole drain. Also, the "frames had to be
consumed anyway" statement is only true for frames already queued - the
size+BATCH countdown can force consumption of many newly produced
frames belonging to unrelated devices.

This is not a deadlock (the drain is bounded and the kthread cannot
block indefinitely since its work runs under local_bh_disable()), but
could this cause latency or stall issues on systems with many devices
or large ring sizes?

[ ... ]

> -    Fixes: 9c270af37bb6 ("bpf: XDP_REDIRECT enable use of cpumap")
> +    Fixes: 70280ed91cb8 ("bpf: cpumap convert to use generic xdp_frame")
>      Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>

The Fixes tag points to commit 9c270af37bb6 ("bpf: XDP_REDIRECT enable
use of cpumap"), but is this the right commit?

That commit (October 2017) introduced the cpu_map_enqueue() function
signature but did not store dev_rx anywhere - it used data_hard_start
as a void pointer with a comment saying "Followup patch will generalize
this".

The actual storage of the device pointer without a reference
(xdpf->dev_rx = dev_rx;) was added 6 months later in commit
70280ed91cb8 ("bpf: cpumap convert to use generic xdp_frame", April
2018) when the code was converted to use the generic xdp_frame
structure. This is where the use-after-free bug was introduced.

Should the Fixes tag be:

  Fixes: 70280ed91cb8 ("bpf: cpumap convert to use generic xdp_frame")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35727192473