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]
Date:   Fri, 16 Oct 2020 11:52:20 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Christian Eggers <ceggers@...i.de>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: ksz: fix padding size of skb

On Fri, 16 Oct 2020 21:13:19 +0300 Vladimir Oltean wrote:
> On Fri, Oct 16, 2020 at 11:03:11AM -0700, Jakub Kicinski wrote:
> > On Fri, 16 Oct 2020 18:56:45 +0300 Vladimir Oltean wrote:  
> > > > 3. "Manually" unsharing in dsa_slave_xmit(), reserving enough tailroom
> > > > for the tail tag (and ETH_ZLEN?). Would moving the "else" clause from
> > > > ksz_common_xmit()  to dsa_slave_xmit() do the job correctly?    
> > > 
> > > I was thinking about something like that, indeed. DSA knows everything
> > > about the tagger: its overhead, whether it's a tail tag or not. The xmit
> > > callback of the tagger should only be there to populate the tag where it
> > > needs to be. But reallocation, padding, etc etc, should all be dealt
> > > with by the common DSA xmit procedure. We want the taggers to be simple
> > > and reuse as much logic as possible, not to be bloated.  
> > 
> > FWIW if you want to avoid the reallocs you may want to set
> > needed_tailroom on the netdev.  
> 
> Tell me more about that, I've been meaning since forever to try it out.
> I read about needed_headroom and needed_tailroom, and it's one of the
> reasons why I added the .tail_tag option in the DSA tagger (to
> distinguish whether a switch needs headroom or tailroom), but I can't
> figure out, just from static analysis of the code, where exactly is the
> needed tailroom being accounted for. 

My understanding that the tail tag use case matches pretty well,
needed_tailroom is supposed to be a hit to the higher layers of 
the stack to leave some extra space at the end.

Now, it's probably of limited use in practice since I'd imagine 
most data comes in fragments. 

> For example, if I'm in Christian's
> situation, e.g. I have a packet smaller than ETH_ZLEN, would the
> tailroom be enough to hold just the dev->needed_tailroom, or would there
> be enough space in the skb for the entire ETH_ZLEN + dev->needed_tailroom?

I don't think we account for padding in general. Also looking at
__pskb_pull_tail() we're already seem to be provisioning some extra 
128B. So I guess it won't matter and the DSA tags are too small to
need the head/tailroom hints anyway..

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ