Re: [net-next,v2,8/8] net: macb: introduce xmit support
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-02-24 00:09:03
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- net: macb: introduce xmit support This commit adds XDP_TX verdict support and introduces ndo_xdp_xmit function for redirection. It updates macb_tx_unmap() to handle both skbs and xdp frames advertising NETDEV_XDP_ACT_NDO_XMIT capability.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 50646ee90672..69392ec0065f 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -967,6 +967,17 @@ static int macb_halt_tx(struct macb *bp) bp, TSR); } +static void macb_tx_release_buff(void *buff, enum macb_tx_buff_type type, int budget) +{ + if (type == MACB_TYPE_SKB) { + napi_consume_skb(buff, budget); + } else if (type == MACB_TYPE_XDP_TX) { + xdp_return_frame_rx_napi(buff);
^^^^
Can xdp_return_frame_rx_napi() be called in workqueue context?
This function is only safe in NAPI or softirq context because it calls
__xdp_return() with napi_direct=true, which eventually calls
bpf_net_ctx_get_ri() -> bpf_net_ctx_get(), dereferencing
current->bpf_net_context without a NULL check.
In NAPI context, bpf_net_context is set up by bpf_net_ctx_set() in
net_rx_action/napi_poll. In workqueue context, current->bpf_net_context
is NULL.
macb_tx_release_buff() is called from macb_tx_unmap(), which is called
from macb_tx_error_task(). macb_tx_error_task() runs as a workqueue
task, scheduled by:
macb_interrupt() ->
schedule_work(&queue->tx_error_task) ->
macb_tx_error_task() ->
macb_tx_unmap() ->
macb_tx_release_buff(ptr, MACB_TYPE_XDP_TX, 0) ->
xdp_return_frame_rx_napi() ->
__xdp_return(napi_direct=true) ->
xdp_return_frame_no_direct() ->
bpf_net_ctx_get_ri() ->
NULL deref crash
Should this use xdp_return_frame() instead of xdp_return_frame_rx_napi()
when not in NAPI context, similar to how napi_consume_skb() uses the
budget parameter to distinguish contexts (budget=0 means non-NAPI)?
+ } else {
+ xdp_return_frame(buff);
+ }
+}
+[ ... ]
quoted hunk ↗ jump to hunk
@@ -1069,6 +1080,10 @@ static void macb_tx_error_task(struct work_struct *work) desc = macb_tx_desc(queue, tail); ctrl = desc->ctrl; tx_buff = macb_tx_buff(queue, tail); + + if (tx_buff->type != MACB_TYPE_SKB) + goto unmap; + skb = tx_buff->ptr;
Does macb_tx_error_task() hold the necessary locks when accessing the TX
ring?
macb_tx_error_task accesses queue->tx_head (loop boundary in line 1077)
and queue->tx_tail without holding queue->tx_ptr_lock. It only holds
bp->lock.
The new macb_xdp_submit_frame modifies queue->tx_head (line 1461) under
tx_ptr_lock but BEFORE acquiring the nested bp->lock (line 1464).
macb_tx_error_task only calls napi_disable(&queue->napi_tx), so RX NAPI
(which calls gem_xdp_run -> macb_xdp_submit_frame for XDP_TX) remains
active. netif_tx_stop_all_queues does not stop ndo_xdp_xmit (gem_xdp_xmit
-> macb_xdp_submit_frame).
The race can occur:
CPU 0: macb_tx_error_task
- holds bp->lock
- iterates ring using tx_head as boundary
CPU 1: macb_xdp_submit_frame
- acquires tx_ptr_lock
- submits a frame
- updates tx_head
- blocks on bp->lock
CPU 0:
- sees updated tx_head
- iterates into newly submitted frame
- frees its buffer via macb_tx_unmap
- resets tx_head=0, tx_tail=0
CPU 1:
- acquires bp->lock
- kicks TSTART on reset ring
This results in a freed DMA buffer in an active ring, and inconsistent
tx_head/tx_tail state.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1184,6 +1200,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) spin_lock_irqsave(&queue->tx_ptr_lock, flags); head = queue->tx_head; for (tail = queue->tx_tail; tail != head && packets < budget; tail++) { + void *data = NULL; struct macb_tx_buff *tx_buff; struct sk_buff *skb; struct macb_dma_desc *desc;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1206,11 +1223,16 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) for (;; tail++) { tx_buff = macb_tx_buff(queue, tail); - if (tx_buff->type == MACB_TYPE_SKB) - skb = tx_buff->ptr; + if (tx_buff->type != MACB_TYPE_SKB) { + data = tx_buff->ptr; + goto unmap; + } /* First, update TX stats if needed */ - if (skb) { + if (tx_buff->type == MACB_TYPE_SKB && tx_buff->ptr) {
^^^^ This isn't a bug, but could the type check here be simplified? At line 1226, non-SKB types are filtered out with 'if (tx_buff->type != MACB_TYPE_SKB) goto unmap'. At line 1232, the condition checks the type again with 'if (tx_buff->type == MACB_TYPE_SKB && tx_buff->ptr)', but at this point the type is always MACB_TYPE_SKB. The condition could be simplified to 'if (tx_buff->ptr)' for clarity.
+ data = tx_buff->ptr; + skb = tx_buff->ptr; +
[ ... ]