Re: [PATCH net v2 1/2] net: stmmac: Prevent NULL deref when RX memory exhausted

[Sam Edwards]

On Thu, Mar 19, 2026 at 2:59 PM Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:

Hi Russell,

> Isn't the limit equivalent to:
>
>         space = CIRC_SPACE(rx_q->cur_rx, rx_q->dirty_rx,
>                            priv->dma_conf.dma_rx_size);
>         limit = min_t(unsigned int, space, limit);
>

I had to think about the edge cases for a few minutes -- yes, you're
right. However...

> (Think of the "full" and "empty" cases as "space" which can be
> consumed to provide entries for the refiller to action.)

...this seems like a double negative: We're looking at the ring from
the refiller's perspective (i.e. considering "dirty" the meaningful
state), and *then* looking at the "unused space" (i.e. "not dirty").
If I used it, I'd at least add a comment explaining that CIRC_SPACE
is, counterintuitively, not measuring "space."

After the bugs are fixed, I'll follow up with a net-next patch [1]
that removes the `min` altogether and makes the loop check for 'dirty'
directly, so my focus here is "minimal, obviously correct" and not
necessarily "tidiest." Would it be acceptable if I simplify
stmmac_rx_dirty() to use CIRC_CNT() then, instead of using
CIRC_SPACE() now?

> I think the same applies for patch 2 - when the above returns zero
> it means we have no entries in the ring that aren't due for refill.

I'm not completely satisfied with the `stmmac_rx_dirty() ==
dma_rx_size - 1` expression either. One might argue we should keep
trying until dirty==0, because tolerating *any* dirtiness risks not
having enough buffers for the next traffic burst, causing avoidable rx
drops. But that's a discussion for patch 2. :)

> Have you checked whether stmmac_rx_zc() also buggy in this respect?

I skimmed it. I saw that it checks the return status of
stmmac_rx_refill_zc() and on failure, returns the NAPI budget to keep
polling -- which told me that the ZC author(s) at least considered
these problems.

Looking at it more thoroughly now, there are a few code smells around
the unconditional `dirty = 0;`, and `dirty` being used as the "budget"
for stmmac_rx_refill_zc() (why have a budget at all then?) so it looks
like something's there, but it's a separate bug if so.

Best,
Sam

[1] https://lore.kernel.org/netdev/20260316021009.262358-4-CFSworks@xxxxxxxxx/