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]

Date:   Tue, 3 Nov 2020 10:51:00 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Florian Fainelli <f.fainelli@...il.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 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?
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?
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?