Re: [net-next,v2,8/8] net: macb: introduce xmit support

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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

[ ... ]

> @@ -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);
> +	}
> +}
> +

[ ... ]

> @@ -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.

[ ... ]

> @@ -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;

[ ... ]

> @@ -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;
> +

[ ... ]