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

[<Claudiu.Beznea@microchip.com>]

On 29.10.2018 23:40, Tristram Ha - C24268 wrote:
>> Could you, please, tell me if the above variable was false in your case?
>>
>> bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb);
>>
>> If yes, then, the proper fix would be as follows:
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 8f5bf9166c11..492a8e1a34cd 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1690,7 +1690,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb,
>> struct net_device *ndev)
>>                 padlen += ETH_FCS_LEN;
>>         }
>>
>> -       if (!cloned && headroom + tailroom >= padlen) {
>> +       if (!cloned && headroom + tailroom >= ETH_FCS_LEN) {
>>                 (*skb)->data = memmove((*skb)->head, (*skb)->data, (*skb)->len);
>>                 skb_set_tail_pointer(*skb, (*skb)->len);
>>         } else {
>>
>> Could you please check if it works in your case (and without your patch)?
>>
> 
> Actually doing that reveals another bug:
> 
>         if (padlen) {
>                 if (padlen >= ETH_FCS_LEN)
>                         skb_put_zero(*skb, padlen - ETH_FCS_LEN);
>                 else
>                         skb_trim(*skb, ETH_FCS_LEN - padlen);
>         }
> 
> My fix calls skb_put_zero with zero length.  Your change calls skb_trim which
> actually sets the socket buffer length to 1!
> 
> When this problem happens headroom is 2, tailroom is 1, skb->len is 61, and
> padlen is 3.
> 

Ok, I see now. Looking again, your fix is good. But, as you said, there is
the skb_trim() in this function that is wrong from the beginning (my bad).
I propose to remove it since, with your fix is not even reached anymore.

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);


> DSA driver is being used.  That is why the length is already padded to 60 bytes
> and 1-byte tail tag is added.

Ok, I see, I didn't test with such configurations.

> 
> BTW, I am not sure while this macb_pad_and_fcs function was added.  Is it to
> workaround some hardware bugs?  The code is executed only when
> NETIF_IF_HW_CSUM is used.  But if hardware tx checksumming is enabled why
> not also use the hardware to calculate CRC?

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
> 
> NETIF_F_SG is not enabled in the MAC I used, so enabling NETIF_IF_HW_CSUM
> is rather pointless.  With the padding code the transmit throughput cannot get
> higher than 100Mbps in a gigabit connection.
> 
> I would recommend to add this option to disable manual padding in one of those
> macb_config structures.

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

Thank you,
Claudiu Beznea

>