Re: [PATCH net-next v6 1/3] net: ethernet: mtk_eth_soc: fix the RX

Re: [PATCH net-next v6 1/3] net: ethernet: mtk_eth_soc: fix the RX

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account.

*IMPORTANT*: If you do repost (for other reasons) - please include
the relevant info in the cover letter or commit messages.
-- 
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next v6 1/3] net: ethernet: mtk_eth_soc: fix the RX

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: ethernet: mtk_eth_soc: fix the RX"
  gets the history and cause backwards.
- [Low] The subject line "net: ethernet: mtk_eth_soc: fix the RX" does
  not say what is being fixed.

--

net: ethernet: mtk_eth_soc: fix the RX

This patch resets dma_addr to DMA_MAPPING_ERROR at the start of every
loop iteration in mtk_poll_rx(). Without it, a descriptor that goes to
release_desc without a new mapping can pick up high address bits left
over from an earlier iteration. That matters on MTK_36BIT_DMA SoCs with
DRAM above 4 GiB.

> That protection was added by commit 6e0490fc36cd ("net: ethernet:
> mtk_eth_soc: fix SER panic with 4GB+ RAM") for the MTK_RESETTING path,
> but commit e10a35abb3da ("net: ethernet: mtk_eth_soc: fix
> uninitialized variable") moved the DMA_MAPPING_ERROR initialisation
> out of the loop, so it only holds for the first iteration.

[Severity: Low]
Is this history accurate? e10a35abb3da is dated 2023-09-10 and
6e0490fc36cd is dated 2025-04-27, so e10a35abb3da came first.

e10a35abb3da also does not appear to move an initialiser out of the
loop. Before it, mtk_poll_rx() declared dma_addr inside the while loop
with no initialiser:

    dma_addr_t dma_addr;

e10a35abb3da moved the declaration to function scope:

    dma_addr_t dma_addr = DMA_MAPPING_ERROR;

It also added the first dma_addr != DMA_MAPPING_ERROR check at
release_desc. Later, 6e0490fc36cd replaced that check with the FIELD_GET()
branch:

    if (unlikely(dma_addr == DMA_MAPPING_ERROR))
        addr64 = FIELD_GET(RX_DMA_ADDR64_MASK,
                           rxd->rxd2);
    else
        addr64 = RX_DMA_PREP_ADDR64(dma_addr);

So that branch only ever worked for the first loop iteration. Nothing
broke it later. Could the message give the order that way?

The message also does not say that this fix depends on the FIELD_GET()
branch from 6e0490fc36cd. Both hunks touch only the declaration and the
top of the loop, which have looked the same since e10a35abb3da. So the
patch would apply cleanly to a tree that has e10a35abb3da but not
6e0490fc36cd.

In that tree, every descriptor reaching release_desc would hit the
dma_addr == DMA_MAPPING_ERROR case and skip RX_DMA_PREP_ADDR64(). Its
high address bits would then be cleared. That is the original SER
failure, now on every iteration instead of only the first.

6e0490fc36cd carries Fixes: 2d75891ebc09, so most stable trees probably
have it already. Would it still help to name 6e0490fc36cd as a
prerequisite?

> net: ethernet: mtk_eth_soc: fix the RX

[Severity: Low]
This isn't a bug, but the subject does not say what is being fixed.
Something like "reset dma_addr per RX descriptor" or "keep descriptor
high address bits on release" would be more specific.

> With MTK_36BIT_DMA and DRAM above 4 GiB the hardware then DMAs the next
> frame to an address that is neither buffer, and the driver passes the
> stale contents of the untouched buffer up the stack.

Could the consequence be stated more directly? Before this patch, a
stale dma_addr from an earlier iteration reaches release_desc in
mtk_poll_rx() and goes through:

    addr64 = RX_DMA_PREP_ADDR64(dma_addr);
    ...
    rxd->rxd2 = RX_DMA_PREP_PLEN0(ring->buf_size) | addr64;

RX_DMA_PREP_ADDR64() fills RX_DMA_ADDR64_MASK, which holds physical
address bits 35:32. These bits are combined with the current buffer's
low bits in rxd1. The device then writes received frames to the same
offset in a different 4 GiB window. That is silent DMA corruption of
memory the driver does not own.

The mismatched address is also passed to dma_unmap_single() later. One
caller is mtk_poll_rx() itself, on the non-page_pool path:

    if (MTK_HAS_CAPS(eth->soc->caps, MTK_36BIT_DMA))
        addr64 = RX_DMA_GET_ADDR64(trxd.rxd2);

    dma_unmap_single(eth->dma_dev, ((u64)trxd.rxd1 | addr64),
                     ring->buf_size, DMA_FROM_DEVICE);

The other is mtk_rx_clean() during SER.

Would it be worth calling this memory corruption in the message, so
stable and security triage can judge how urgent it is?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/patch-4b4c5003bcf028fafd094e719c5e910b%40loljews.com