Re: [PATCH net] net: ethernet: cadence: fix socket buffer corruption problem

On 30.10.2018 21:36, Tristram Ha - C24268 wrote:

quoted

Could you check on your side that applying this on top of your patch, your
scenario is still working?

diff --git a/drivers/net/ethernet/cadence/macb_main.c

b/drivers/net/ethernet/cadence/macb_main.c
index 1d86b4d5645a..e1347d6d1b50 100644

--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -1702,12 +1702,8 @@ static int macb_pad_and_fcs(struct sk_buff **skb,

struct net_device *ndev)
                *skb = nskb;
        }

-       if (padlen) {
-               if (padlen >= ETH_FCS_LEN)
-                       skb_put_zero(*skb, padlen - ETH_FCS_LEN);
-               else
-                       skb_trim(*skb, ETH_FCS_LEN - padlen);
-       }
+       if (padlen > ETH_FCS_LEN)
+               skb_put_zero(*skb, padlen - ETH_FCS_LEN);

I think it is okay but I need to check all paths are covered.

On my side I checked with pktgen generating packets with sizes starting
from 1-MTU. Same scripts I also used when I first implemented this but it
seems that your scenario wasn't covered. Please have a look and let me know
how it works on your side.

quoted

It was reported in [1] that UDP checksum is offloaded to hardware no matter
the application previously computed it.

The code should be executed only for packets that has checksum computed
by
applications ((*skb)->ip_summed != CHECKSUM_PARTIAL). The idea was to
not
recompute checksum for packets with checksum already computed. To do
so,
while hardware checksum is enabled (NETIF_F_HW_CSUM), TX_NOCRC bit
should
be set on buffer descriptor. But to do so, packets must have a minimum size
of 64 and FCS to be computed.

The NETIF_F_HW_CSUM check was placed there because the issue
described in
[1] is reproducible because hardware checksum is enabled and overrides the
checksum provided by applications.

[1] https://www.spinics.net/lists/netdev/msg505065.html

I understand the issue now.  It is weird that the transmit descriptor does not
have direct control over turning on checksum generation or not, but it wastes
3 bits returning the error codes of such generation.  What can the driver do
with such information?

Yep, from my POV it would have been nice if it could have been able to do
the pad an FCS even when hardware checksum is enabled and TX_NOCRC bit is
set in descriptor. For cases when hardware checksum is disable it seems
that it could handle it.

In my opinion then hardware transmit checksumming cannot be supported
In Linux.

quoted

quoted

NETIF_F_SG is not enabled in the MAC I used, so enabling

NETIF_IF_HW_CSUM

quoted

is rather pointless.  With the padding code the transmit throughput cannot

get

quoted

higher than 100Mbps in a gigabit connection.

I would recommend to add this option to disable manual padding in one of

those

quoted

macb_config structures.

In this way the user would have to know from the beginning what kind of
packets are used.

The kernel already does a good job of calculating checksum.  Using hardware to
do that does not improve performance much.

Alternative is to use ethtool to disable hardware tx checksum so that software can
intentionally send wrong checksums.