netdev - Re: [PATCH v3 net-next 09/12] net: dsa: tag_brcm: let DSA core deal with TX reallocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c9fccf3-c3be-024b-2aae-e27a61d9c8b2@gmail.com>
Date:   Tue, 3 Nov 2020 09:32:38 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Christian Eggers <ceggers@...i.de>,
        Kurt Kanzenbach <kurt@...utronix.de>
Subject: Re: [PATCH v3 net-next 09/12] net: dsa: tag_brcm: let DSA core deal
 with TX reallocation



On 11/3/2020 2:51 AM, Vladimir Oltean wrote:
> On Mon, Nov 02, 2020 at 12:34:11PM -0800, Florian Fainelli wrote:
>> On 11/1/2020 11:16 AM, Vladimir Oltean wrote:
>>> Now that we have a central TX reallocation procedure that accounts for
>>> the tagger's needed headroom in a generic way, we can remove the
>>> skb_cow_head call.
>>>
>>> Cc: Florian Fainelli <f.fainelli@...il.com>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> 
> Florian, I just noticed that tag_brcm.c has an __skb_put_padto call,
> even though it is not a tail tagger. This comes from commit:
> 
> commit bf08c34086d159edde5c54902dfa2caa4d9fbd8c
> Author: Florian Fainelli <f.fainelli@...il.com>
> Date:   Wed Jan 3 22:13:00 2018 -0800
> 
>     net: dsa: Move padding into Broadcom tagger
> 
>     Instead of having the different master network device drivers
>     potentially used by DSA/Broadcom tags, move the padding necessary for
>     the switches to accept short packets where it makes most sense: within
>     tag_brcm.c. This avoids multiplying the number of similar commits to
>     e.g: bgmac, bcmsysport, etc.
> 
>     Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>
> 
> Do you remember why this was needed?

Yes, it is explained in the comment surrounding the padding:

         /* The Ethernet switch we are interfaced with needs packets to
be at
           * least 64 bytes (including FCS) otherwise they will be
discarded when
           * they enter the switch port logic. When Broadcom tags are
enabled, we
           * need to make sure that packets are at least 68 bytes
           * (including FCS and tag) because the length verification is
done after
           * the Broadcom tag is stripped off the ingress packet.


> As far as I understand, either the DSA master driver or the MAC itself
> should pad frames automatically. Is that not happening on Broadcom SoCs,
> or why do you need to pad from DSA?

Some of the Ethernet MACs were not doing that automatic padding and/or
had no option to turn this on. This is true for at least SYSTEMPORT (not
Lite) and BGMAC. GENET is also commonly used but does support automatic
RUNT frame padding.

> How should we deal with this? Having tag_brcm.c still do some potential
> reallocation defeats the purpose of doing it centrally, in a way. I was
> trying to change the prototype of struct dsa_device_ops::xmit to stop
> returning a struct sk_buff *, and I stumbled upon this.
> Should we just go ahead and pad everything unconditionally in DSA?
> 

This is really a problem specific to Broadcom tags and how the switch
operate so it seems reasonable to leave those details down to the tagger.
-- 
Florian