From: Aaro Koskinen <[email protected]>
When using 16K DMA buffers and ring mode, the DES3 refill is not working
correctly as the function is using a bogus pointer for checking the
private data. As a result stale pointers will remain in the RX descriptor
ring, so DMA will now likely overwrite/corrupt some already freed memory.
As simple reproducer, just receive some UDP traffic:
# ifconfig eth0 down; ifconfig eth0 mtu 9000; ifconfig eth0 up
# iperf3 -c 192.168.253.40 -u -b 0 -R
If you didn't crash by now check the RX descriptors to find non-contiguous
RX buffers:
cat /sys/kernel/debug/stmmaceth/eth0/descriptors_status
[...]
1 [0x2be5020]: 0xa3220321 0x9ffc1ffc 0x72d70082 0x130e207e
^^^^^^^^^^^^^^^^^^^^^
2 [0x2be5040]: 0xa3220321 0x9ffc1ffc 0x72998082 0x1311a07e
^^^^^^^^^^^^^^^^^^^^^
A simple ping test will now report bad data:
# ping -s 8200 192.168.253.40
PING 192.168.253.40 (192.168.253.40) 8200(8228) bytes of data.
8208 bytes from 192.168.253.40: icmp_seq=1 ttl=64 time=1.00 ms
wrong data byte #8144 should be 0xd0 but was 0x88
Fix the wrong pointer. Also we must refill DES3 only if the DMA buffer
size is 16K.
Fixes: 54139cf3bb33 ("net: stmmac: adding multiple buffers for rx")
Signed-off-by: Aaro Koskinen <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/ring_mode.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
index f936166d8910..4d9bcb4d0378 100644
--- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
@@ -113,10 +113,11 @@ static unsigned int is_jumbo_frm(int len, int enh_desc)
static void refill_desc3(void *priv_ptr, struct dma_desc *p)
{
- struct stmmac_priv *priv = (struct stmmac_priv *)priv_ptr;
+ struct stmmac_rx_queue *rx_q = priv_ptr;
+ struct stmmac_priv *priv = rx_q->priv_data;
/* Fill DES3 in case of RING mode */
- if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
+ if (priv->dma_buf_sz == BUF_SIZE_16KiB)
p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
}
--
2.17.0
On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
> /* Fill DES3 in case of RING mode */
> - if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
> + if (priv->dma_buf_sz == BUF_SIZE_16KiB)
Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?
Thanks,
Jose Miguel Abreu
Hi,
From: Jose Abreu [[email protected]]:
> On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
> > /* Fill DES3 in case of RING mode */
> > - if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
> > + if (priv->dma_buf_sz == BUF_SIZE_16KiB)
>
> Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?
I think it should be the same as in stmmac_init_rx_buffers():
if (priv->dma_buf_sz == BUF_SIZE_16KiB)
stmmac_init_desc3(priv, p);
A.
On 3/19/2019 11:07 AM, Koskinen, Aaro (Nokia - FI/Espoo) wrote:
> Hi,
>
> From: Jose Abreu [[email protected]]:
>> On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
>>> /* Fill DES3 in case of RING mode */
>>> - if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
>>> + if (priv->dma_buf_sz == BUF_SIZE_16KiB)
>>
>> Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?
>
> I think it should be the same as in stmmac_init_rx_buffers():
>
> if (priv->dma_buf_sz == BUF_SIZE_16KiB)
> stmmac_init_desc3(priv, p);
>
> A.
>
Hmm, yeah makes sense. I was under impression that XGMAC had a >
16KB buffer but I was wrong.
This change looks okay then.
Acked-by: Jose Abreu <[email protected]>
Thanks,
Jose Miguel Abreu
From: Aaro Koskinen <[email protected]>
Date: Mon, 18 Mar 2019 23:36:08 +0200
> From: Aaro Koskinen <[email protected]>
>
> When using 16K DMA buffers and ring mode, the DES3 refill is not working
> correctly as the function is using a bogus pointer for checking the
> private data. As a result stale pointers will remain in the RX descriptor
> ring, so DMA will now likely overwrite/corrupt some already freed memory.
>
> As simple reproducer, just receive some UDP traffic:
...
> Fixes: 54139cf3bb33 ("net: stmmac: adding multiple buffers for rx")
> Signed-off-by: Aaro Koskinen <[email protected]>
Applied and queued up for -stable, thank you.