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

Message-ID: <ea9437b4-e7ba-e31c-0576-36eaeee806a1@gmail.com>
Date:   Thu, 10 Sep 2020 12:46:30 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, kuba@...nel.org,
        andrew@...n.ch, willemdebruijn.kernel@...il.com,
        edumazet@...gle.com
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC PATCH] __netif_receive_skb_core: don't untag vlan from skb
 on DSA master



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?

>   check_vlan_id:
>   		if (skb_vlan_tag_get_id(skb)) {
>   			/* Vlan id is non 0 and vlan_do_receive() above couldn't
> 

-- 
Florian