[PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow

Suraj Gupta posted 1 patch 2 months ago
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow
Posted by Suraj Gupta 2 months ago
In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
irq handling path.
In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
new BD after processing skb.
This will be problematic if both AXI-DMA and AXI ethernet have same
BD count as all Rx BDs will be allocated initially and it won't be
able to allocate new one after Rx irq. Incrementing head pointer w/o
checking for BD allocation will result in garbage values in skb BD and
cause the below kernel crash:

Unable to handle kernel paging request at virtual address fffffffffffffffa
<snip>
Internal error: Oops: 0000000096000006 [#1]  SMP
pc : axienet_dma_rx_cb+0x78/0x150
lr : axienet_dma_rx_cb+0x78/0x150
 Call trace:
  axienet_dma_rx_cb+0x78/0x150 (P)
  xilinx_dma_do_tasklet+0xdc/0x290
  tasklet_action_common+0x12c/0x178
  tasklet_action+0x30/0x3c
  handle_softirqs+0xf8/0x230
<snip>

Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6011d7eae0c7..acd5be60afec 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1457,7 +1457,6 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
 	if (!skbuf_dma)
 		return;
 
-	lp->rx_ring_head++;
 	skb = netdev_alloc_skb(ndev, lp->max_frm_size);
 	if (!skb)
 		return;
@@ -1482,6 +1481,7 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
 	skbuf_dma->desc = dma_rx_desc;
 	dma_rx_desc->callback_param = lp;
 	dma_rx_desc->callback_result = axienet_dma_rx_cb;
+	lp->rx_ring_head++;
 	dmaengine_submit(dma_rx_desc);
 
 	return;
-- 
2.25.1
Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow
Posted by Sean Anderson 2 months ago
On 8/5/25 15:19, Suraj Gupta wrote:
> In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
> irq handling path.
> In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> new BD after processing skb.
> This will be problematic if both AXI-DMA and AXI ethernet have same
> BD count as all Rx BDs will be allocated initially and it won't be
> able to allocate new one after Rx irq. Incrementing head pointer w/o
> checking for BD allocation will result in garbage values in skb BD and
> cause the below kernel crash:
> 
> Unable to handle kernel paging request at virtual address fffffffffffffffa
> <snip>
> Internal error: Oops: 0000000096000006 [#1]  SMP
> pc : axienet_dma_rx_cb+0x78/0x150
> lr : axienet_dma_rx_cb+0x78/0x150
>  Call trace:
>   axienet_dma_rx_cb+0x78/0x150 (P)
>   xilinx_dma_do_tasklet+0xdc/0x290
>   tasklet_action_common+0x12c/0x178
>   tasklet_action+0x30/0x3c
>   handle_softirqs+0xf8/0x230
> <snip>
> 
> Fixes: 6a91b846af85 ("net: axienet: Introduce dmaengine support")
> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6011d7eae0c7..acd5be60afec 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1457,7 +1457,6 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
>  	if (!skbuf_dma)
>  		return;
>  
> -	lp->rx_ring_head++;
>  	skb = netdev_alloc_skb(ndev, lp->max_frm_size);
>  	if (!skb)
>  		return;
> @@ -1482,6 +1481,7 @@ static void axienet_rx_submit_desc(struct net_device *ndev)
>  	skbuf_dma->desc = dma_rx_desc;
>  	dma_rx_desc->callback_param = lp;
>  	dma_rx_desc->callback_result = axienet_dma_rx_cb;
> +	lp->rx_ring_head++;
>  	dmaengine_submit(dma_rx_desc);
>  
>  	return;

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>
Re: [PATCH net] net: xilinx: axienet: Increment Rx skb ring head pointer after BD is successfully allocated in dmaengine flow
Posted by Jakub Kicinski 1 month, 3 weeks ago
On Wed, 6 Aug 2025 00:49:58 +0530 Suraj Gupta wrote:
> In DMAengine flow, AXI DMA driver invokes callback before freeing BD in
> irq handling path.
> In Rx callback (axienet_dma_rx_cb()), axienet driver tries to allocate
> new BD after processing skb.
> This will be problematic if both AXI-DMA and AXI ethernet have same
> BD count as all Rx BDs will be allocated initially and it won't be
> able to allocate new one after Rx irq. Incrementing head pointer w/o
> checking for BD allocation will result in garbage values in skb BD and
> cause the below kernel crash:
> 
> Unable to handle kernel paging request at virtual address fffffffffffffffa
> <snip>
> Internal error: Oops: 0000000096000006 [#1]  SMP
> pc : axienet_dma_rx_cb+0x78/0x150
> lr : axienet_dma_rx_cb+0x78/0x150
>  Call trace:
>   axienet_dma_rx_cb+0x78/0x150 (P)
>   xilinx_dma_do_tasklet+0xdc/0x290
>   tasklet_action_common+0x12c/0x178
>   tasklet_action+0x30/0x3c
>   handle_softirqs+0xf8/0x230
> <snip>

Do you mean that we're incrementing lp->rx_ring_head before we know
that the submission will succeed? Potentially leaving an uninitialized
entry (say at index n), next attempt will try to use the next entry 
(n + 1) but the completion will not know about the skip so it will
try to complete entry n ?

This is really not coming thru in your explanation.

The fix itself seems incomplete. Even if we correctly skip the increment
we will never try to catch up with the allocations, the ring will have
fewer outstanding Rx skbs until reset, right? Worst case we drop all
the skbs and the ring will be empty, no Rx will happen until reset.
The shutdown path seems to be checking for skb = NULL so I guess it's
correct but good to double check..
-- 
pw-bot: cr