netdev - Re: [RFC PATCH] __netif_receive_skb_core: don't untag vlan from skb on DSA master

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: <20200910200121.uaqhi2y7m3k2jssg@skbuf>
Date:   Thu, 10 Sep 2020 23:01:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     kuba@...nel.org, andrew@...n.ch, willemdebruijn.kernel@...il.com,
        edumazet@...gle.com, netdev@...r.kernel.org
Subject: Re: [RFC PATCH] __netif_receive_skb_core: don't untag vlan from skb
 on DSA master

On Thu, Sep 10, 2020 at 12:55:46PM -0700, Florian Fainelli wrote:
> On 9/10/2020 12:46 PM, Florian Fainelli wrote:
> > On 9/10/2020 9:22 AM, Vladimir Oltean wrote:
> > > A DSA master interface has upper network devices, each representing an
> > > Ethernet switch port attached to it. Demultiplexing the source ports and
> > > setting skb->dev accordingly is done through the catch-all ETH_P_XDSA
> > > packet_type handler. Catch-all because DSA vendors have various header
> > > implementations, which can be placed anywhere in the frame: before the
> > > DMAC, before the EtherType, before the FCS, etc. So, the ETH_P_XDSA
> > > handler acts like an rx_handler more than anything.
> > >
> > > It is unlikely for the DSA master interface to have any other upper than
> > > the DSA switch interfaces themselves. Only maybe a bridge upper*, but it
> > > is very likely that the DSA master will have no 8021q upper. So
> > > __netif_receive_skb_core() will try to untag the VLAN, despite the fact
> > > that the DSA switch interface might have an 8021q upper. So the skb will
> > > never reach that.
> > >
> > > So far, this hasn't been a problem because most of the possible
> > > placements of the DSA switch header mentioned in the first paragraph
> > > will displace the VLAN header when the DSA master receives the frame, so
> > > __netif_receive_skb_core() will not actually execute any VLAN-specific
> > > code for it. This only becomes a problem when the DSA switch header does
> > > not displace the VLAN header (for example with a tail tag).
> > >
> > > What the patch does is it bypasses the untagging of the skb when there
> > > is a DSA switch attached to this net device. So, DSA is the only
> > > packet_type handler which requires seeing the VLAN header. Once skb->dev
> > > will be changed, __netif_receive_skb_core() will be invoked again and
> > > untagging, or delivery to an 8021q upper, will happen in the RX of the
> > > DSA switch interface itself.
> > >
> > > *see commit 9eb8eff0cf2f ("net: bridge: allow enslaving some DSA master
> > > network devices". This is actually the reason why I prefer keeping DSA
> > > as a packet_type handler of ETH_P_XDSA rather than converting to an
> > > rx_handler. Currently the rx_handler code doesn't support chaining, and
> > > this is a problem because a DSA master might be bridged.
> > >
> > > Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> > > ---
> > > Resent, sorry, I forgot to copy the list.
> > >
> > >   net/core/dev.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 152ad3b578de..952541ce1d9d 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -98,6 +98,7 @@
> > >   #include <net/busy_poll.h>
> > >   #include <linux/rtnetlink.h>
> > >   #include <linux/stat.h>
> > > +#include <net/dsa.h>
> > >   #include <net/dst.h>
> > >   #include <net/dst_metadata.h>
> > >   #include <net/pkt_sched.h>
> > > @@ -5192,7 +5193,7 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
> > >           }
> > >       }
> > > -    if (unlikely(skb_vlan_tag_present(skb))) {
> > > +    if (unlikely(skb_vlan_tag_present(skb)) && !netdev_uses_dsa(skb->dev)) {
> >
> > Not that I have performance numbers to claim  this, but we would
> > probably want:
> >
> > && likely(!netdev_uses_dsa(skb->dev))
> >
> > as well?
>
> And #include <net/dsa.h> as it does not look like there is any implicit
> header inclusion that provides that definition:
>
> net/core/dev.c: In function '__netif_receive_skb_core':
> net/core/dev.c:5196:46: error: implicit declaration of function
> 'netdev_uses_dsa'; did you mean 'netdev_reset_tc'?
> [-Werror=implicit-function-declaration]
>

Uhm, it's right there? Not sure how you ended up with that warning.

And I know little about how branch prediction works, I thought it's
enough that the netdev_uses_dsa() check is already following an unlikely
path.

Thanks,
-Vladimir