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] [day] [month] [year] [list]

Message-ID: <20201103103436.486e9339@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Tue, 3 Nov 2020 10:34:36 -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 18:15:29 +0000 Vladimir Oltean wrote:
> On Tue, Nov 03, 2020 at 09:04:11AM -0800, Jakub Kicinski wrote:
> > 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.  
> 
> What would that mean in practice? Modify the existing alloc_skb calls
> which have an expression e that depends on LL_RESERVED_SPACE(dev), into
> max(e, dev->padding_len)? There's a lot of calls to alloc_skb to modify
> though...

Yeah, separate helper would probably be warranted, so we don't have to
touch multiple sites every time we adjust things.

> > 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.  
> 
> DSA does already provision for padding if it has to reallocate, but only
> for the case where it needs to add a frame header at the end of the skb
> (i.e. "tail taggers"). My question here was whether there would be any
> drawback to doing that for all types of switches, including ones that
> might deal with padding in some other way (i.e. in hardware).

Well, we may re-alloc unnecessarily if we provision for padding of all
frames.

So what I was trying to achieve was to add the padding space _after_
the "do we need to realloc" check.

	/* over-provision space for pad, if we realloc anyway */
	if (!needed_tailroom && skb->len < ETH_ZLEN)
		needed_tailroom = ETH_ZLEN - skb->len;