On Fri, Aug 20, 2021 at 7:56 PM Matteo Croce [off-list ref] wrote:

On Fri, Aug 20, 2021 at 7:51 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 18:35:45 +0100,
Matteo Croce [off-list ref] wrote:

quoted

On Fri, Aug 20, 2021 at 7:24 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 18:14:30 +0100,
Matteo Croce [off-list ref] wrote:

quoted

On Fri, Aug 20, 2021 at 7:09 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 17:38:14 +0100,
Matteo Croce [off-list ref] wrote:

quoted

On Fri, Aug 20, 2021 at 6:26 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 11:37:03 +0100,
Matteo Croce [off-list ref] wrote:

quoted

On Thu, Aug 19, 2021 at 6:29 PM Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index fcdb1d20389b..244aa6579ef4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h

@@ -341,7 +341,7 @@ static inline unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
        if (stmmac_xdp_is_enabled(priv))
                return XDP_PACKET_HEADROOM + NET_IP_ALIGN;

-       return NET_SKB_PAD + NET_IP_ALIGN;
+       return 8 + NET_IP_ALIGN;
 }

 void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue);

I don't see the system corrupting packets anymore. Is that exactly
what you had in mind? This really seems to point to a basic buffer
overflow.

[...]

quoted

Sorry, I meant something like:

-       return NET_SKB_PAD + NET_IP_ALIGN;
+       return 8;

I had some hardware which DMA fails if the receive buffer was not word
aligned, but this seems not the case, as 8 + NET_IP_ALIGN = 10, and
it's not aligned too.

No error in that case either, as expected. Given that NET_SKB_PAD is
likely to expand to 64, it is likely a DMA buffer overflow which
probably only triggers for large-ish packets.

Now, we're almost at -rc7, and we don't have a solution in sight.

Can we please revert this until we have an understanding of what is
happening? I'll hopefully have more cycles to work on the issue once
5.14 is out, and hopefully the maintainers of this driver can chime in
(they have been pretty quiet so far).

Thanks,

        M.

--
Without deviation from the norm, progress is not possible.

Last try, what about adding only NET_IP_ALIGN and leaving NET_SKB_PAD?

-       return NET_SKB_PAD + NET_IP_ALIGN;
+       return NET_IP_ALIGN;

I think that alloc_skb adds another NET_SKB_PAD anyway.

I don't see any packet corruption with this. However, this doesn't
prove that this is correct either. What was the rational for adding
NET_SKB_PAD the first place?

I think it's wrong. The original offset was 0, and to align it to the
boundary we need to add just NET_IP_ALIGN, which is two.
NET_SKB_PAD is a much bigger value, (I think 64), which is used to
reserve space to prepend an header, e.g. with tunnels.

How about the other adjustments that Eric mentioned regarding the size
of the buffer? Aren't they required?

I guess that if stmmac_rx_buf1_len() needed such adjustment, it would
be already broken when XDP is in use.
When you use XDP, stmmac_rx_offset() adds a pretty big headroom of 256
byte, which would easily trigger an overflow if not accounted.
Did you try attaching a simple XDP program on a stock 5.13 kernel?

Yes, as mentioned in [1], to which you replied...

        M.

[1] https://lore.kernel.org/r/87wnohqty1.wl-maz@kernel.org (local)

Great.
So I doubt that the adjustment is needed.
Does it work with all the frame size?

Last check, are you sure that the bpf program was loaded in the driver
and not as generic XDP?
You can force it as native with "xdpdrv":

ip link set eth xdpdrv object kernel_passall.o

-- 
per aspera ad upstream