From: Josef Bacik A frame that is redirected to another CPU waits in the queue of the cpumap entry with a plain pointer to the device it came from, and an skb from generic XDP with skb->dev. Nothing keeps that device. Closing and unregistering it waits for its NAPI and for RCU, but not for the kthread of the entry. If the kthread is late by more than that, since its CPU is busy with something of a higher priority, it builds skbs for a device that has been freed and hands them to the stack: BUG: KASAN: use-after-free in dev_gro_receive+0x1f51/0x2b30 Read of size 8 at addr ffff88800a7340b0 by task cpumap/1/map:1/54 Take a reference to the device for each entry of the queue. The kthread cannot drop it at the end of the batch, since its GRO node may keep the skb for longer. Let it note the devices and drop the references behind a full GRO flush, which it does whenever the queue is empty, and from now on also when it has noted about 2048 of them. A device whose frames are stuck in a queue is now unregistered only when the kthread has got to them. RTNL is not held meanwhile, but if the device goes with its network namespace, the cleanup of all namespaces waits. Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation") Fixes: 11941f8a8536 ("bpf: cpumap: Implement generic cpumap") Signed-off-by: Josef Bacik Signed-off-by: Alexei Starovoitov --- kernel/bpf/cpumap.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index 5e59ab896f05..eb87131b1bef 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -45,6 +45,7 @@ */ #define CPU_MAP_BULK_SIZE 8 /* 8 == one cacheline on 64-bit archs */ +#define CPU_MAP_HELD_DEVS 2048 struct bpf_cpu_map_entry; struct bpf_cpu_map; @@ -74,6 +75,13 @@ struct bpf_cpu_map_entry { struct completion kthread_running; struct rcu_work free_work; + + /* + * Every frame and skb in the queue holds a reference to its device. + * The kthread keeps it until nothing is left in GRO. + */ + struct net_device *held[CPU_MAP_HELD_DEVS]; + u32 nr_held; }; struct bpf_cpu_map { @@ -129,9 +137,11 @@ static void __cpu_map_ring_cleanup(struct ptr_ring *ring) WARN_ON_ONCE(1); if (unlikely(__ptr_test_bit(0, &ptr))) { __ptr_clear_bit(0, &ptr); + dev_put(((struct sk_buff *)ptr)->dev); kfree_skb(ptr); continue; } + dev_put(((struct xdp_frame *)ptr)->dev_rx); xdp_return_frame(ptr); } } @@ -311,7 +321,7 @@ static int cpu_map_kthread_run(void *data) void *frames[CPUMAP_BATCH]; void *skbs[CPUMAP_BATCH]; u32 i, n, m; - bool empty; + bool empty, full; /* Release CPU reschedule checks */ if (__ptr_ring_empty(rcpu->queue)) { @@ -344,10 +354,12 @@ static int cpu_map_kthread_run(void *data) struct sk_buff *skb = f; __ptr_clear_bit(0, &skb); + rcpu->held[rcpu->nr_held++] = skb->dev; skbs[ret.skb_n++] = skb; continue; } + rcpu->held[rcpu->nr_held++] = ((struct xdp_frame *)f)->dev_rx; frames[ret.xdp_n++] = f; page = virt_to_page(f); @@ -398,10 +410,13 @@ static int cpu_map_kthread_run(void *data) /* Flush either every 64 packets or in case of empty ring */ packets += n; empty = __ptr_ring_empty(rcpu->queue); - if (packets >= NAPI_POLL_WEIGHT || empty) { - cpu_map_gro_flush(rcpu, empty); + full = empty || rcpu->nr_held > CPU_MAP_HELD_DEVS - CPUMAP_BATCH; + if (packets >= NAPI_POLL_WEIGHT || full) { + cpu_map_gro_flush(rcpu, full); packets = 0; } + while (full && rcpu->nr_held) + dev_put(rcpu->held[--rcpu->nr_held]); local_bh_enable(); /* resched point, may call do_softirq() */ } @@ -738,12 +753,14 @@ static void bq_flush_to_queue(struct xdp_bulk_queue *bq) for (i = 0; i < bq->count; i++) { struct xdp_frame *xdpf = bq->q[i]; - int err; - err = __ptr_ring_produce(q, xdpf); - if (err) { + if (__ptr_ring_full(q)) { drops++; xdp_return_frame_rx_napi(xdpf); + } else { + /* The device cannot go before the flush, from now on it can */ + dev_hold(xdpf->dev_rx); + __ptr_ring_produce(q, xdpf); } processed++; } @@ -807,11 +824,15 @@ int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu, __skb_pull(skb, skb->mac_len); skb_set_redirected(skb, false); + dev_hold(skb->dev); __ptr_set_bit(0, &skb); ret = ptr_ring_produce(rcpu->queue, skb); - if (ret < 0) + if (ret < 0) { + __ptr_clear_bit(0, &skb); + dev_put(skb->dev); goto trace; + } wake_up_process(rcpu->kthread); trace: -- 2.55.0