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: <3ae92181-b80c-c152-4274-50711c3ed062@gmail.com>
Date:   Thu, 10 Sep 2020 12:55:46 -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 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]


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

-- 
Florian