Re: [PATCH net v2] net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@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.
Full review at: https://sashiko.dev
---
net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()

This commit adds a missing dma_rmb() to ensure DMA read operations are
completed and uses READ_ONCE() to prevent compiler reordering when accessing
descriptor fields.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 95ba99b89428e..f1843bc5b9914 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -612,15 +611,17 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  	while (done < budget) {
>  		struct airoha_queue_entry *e = &q->entry[q->tail];
>  		struct airoha_qdma_desc *desc = &q->desc[q->tail];
> -		u32 hash, reason, msg1 = le32_to_cpu(desc->msg1);
> -		struct page *page = virt_to_head_page(e->buf);
> -		u32 desc_ctrl = le32_to_cpu(desc->ctrl);
> +		u32 hash, reason, msg1, desc_ctrl;
>  		struct airoha_gdm_port *port;
>  		int data_len, len, p;
> +		struct page *page;
>  
> +		desc_ctrl = le32_to_cpu(READ_ONCE(desc->ctrl));
>  		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
>  			break;
[ ... ]
> @@ -633,10 +634,12 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  		if (!len || data_len < len)
>  			goto free_frag;

Could this jump lead to an uninitialized pointer dereference?

The initialization of page was removed from the declarations above, so it
is uninitialized at this point.

Later in the function, the error path looks like this:

free_frag:
		if (q->skb) {
			dev_kfree_skb(q->skb);
			q->skb = NULL;
		} else {
			page_pool_put_full_page(q->page_pool, page, true);
		}

If q->skb is NULL, passing the uninitialized page to
page_pool_put_full_page() might cause a crash.

Also, this isn't a bug introduced by this patch, but does this error path
leak the page associated with the current descriptor?

If we hit an error on a scattered fragment while q->skb is not NULL, the
SKB is freed, but the current page (which hasn't been attached to the SKB
yet) bypasses the else branch and isn't returned to the page pool.

>  
> -		p = airoha_qdma_get_gdm_port(eth, desc);
> +		msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
> +		p = airoha_qdma_get_gdm_port(eth, msg1);
>  		if (p < 0 || !eth->ports[p])
>  			goto free_frag;

Similarly, taking this jump leaves page uninitialized before reaching the
error handling path.

>  
> +		page = virt_to_head_page(e->buf);
>  		port = eth->ports[p];
>  		if (!q->skb) { /* first buffer */