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 for Android: free password hash cracker in your pocket

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

Message-ID: <20201103090411.64f785cc@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Tue, 3 Nov 2020 09:04:11 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        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 Tue, 3 Nov 2020 10:51:00 +0000 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?
> 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?

In a recent discussion I was wondering if it makes sense to add the
padding len to struct net_device, with similar best-effort semantics
to needed_*room. It'd be a u8, so little worry about struct size.

You could also make sure DSA always provisions for padding if it has to
reallocate, you don't need to actually pad:

@@ -568,6 +568,9 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)
                /* No reallocation needed, yay! */
                return 0;
 
+       if (skb->len < ETH_ZLEN)
+               needed_tailroom += ETH_ZLEN;
+
        return pskb_expand_head(skb, needed_headroom, needed_tailroom,
                                GFP_ATOMIC);
 }

That should save the realloc for all reasonable drivers while not
costing anything (other than extra if()) to drivers which don't care.