Re: [PATCH net] net: airoha: Fix memory leak in airoha_qdma_rx_process()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says: "more thing to fix.."
---
net: airoha: Fix memory leak in airoha_qdma_rx_process()

This commit fixes a memory leak in the error path of
airoha_qdma_rx_process() by partially reverting a previous commit and
always calling page_pool_put_full_page().

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 95ba99b89428e..91cb63a32d990 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -697,9 +697,8 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)

This problem wasn't introduced by this patch, but while reviewing this
function I noticed the descriptor field msg1 is read from memory before
the ownership bit is validated:

airoha_qdma_rx_process() {
        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);

                if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
                        break;

Could this lead to processing stale descriptor metadata? On weakly-ordered
architectures, the CPU might reorder these reads before the device commits
the new descriptor data. 

Are we missing a dma_rmb() after the ownership check?

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

This is also an existing issue, but does jumping to this error path on a
scattered packet fragment leave the receive loop in an inconsistent state?

If an error occurs while processing a middle fragment of a packet, q->skb
is freed and set to NULL here. However, the loop continues to the next
descriptor.

Since q->skb is now NULL, won't the next fragment of the same corrupted
packet be erroneously parsed as the start of a brand new Ethernet frame:

                if (!q->skb) { /* first buffer */
                        q->skb = napi_build_skb(e->buf, q->buf_size);

Could this desynchronization allow arbitrary payload to be injected into
the network stack as a legitimate packet? The driver may need to discard
all subsequent fragments until a descriptor without QDMA_DESC_MORE_MASK is
encountered.

> +		page_pool_put_full_page(q->page_pool, page, true);
>  	}
>  	airoha_qdma_fill_rx_queue(q);
>