netdev - Re: [PATCH RESEND net] net: handle 802.1P vlan 0 packets properly

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:   Wed, 12 Jun 2019 15:19:46 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     "Govindarajulu Varadarajan (gvaradar)" <gvaradar@...co.com>,
        "Christian Benvenuti (benve)" <benve@...co.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "govind.varadar@...il.com" <govind.varadar@...il.com>,
        "ssuryaextr@...il.com" <ssuryaextr@...il.com>
Subject: Re: [PATCH RESEND net] net: handle 802.1P vlan 0 packets properly

On 2019/06/12 6:57, Govindarajulu Varadarajan (gvaradar) wrote:
> @On Tue, 2019-06-11 at 13:34 +0900, Toshiaki Makita wrote:
>> On 2019/06/11 3:31, Govindarajulu Varadarajan wrote:
>>> When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
>>> vlan_do_receive() returns false if it does not find vlan_dev. Later
>>> __netif_receive_skb_core() fails to find packet type handler for
>>> skb->protocol 801.1AD and drops the packet.
>>>
>>> 801.1P header with vlan id 0 should be handled as untagged packets.
>>> This patch fixes it by checking if vlan_id is 0 and processes next vlan
>>> header.
>>>
>>> Signed-off-by: Govindarajulu Varadarajan <gvaradar@...co.com>
>>> ---
>>>    net/8021q/vlan_core.c | 24 +++++++++++++++++++++---
>>>    1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>> index a313165e7a67..0cde54c02c3f 100644
>>> --- a/net/8021q/vlan_core.c
>>> +++ b/net/8021q/vlan_core.c
>>> @@ -9,14 +9,32 @@
>>>    bool vlan_do_receive(struct sk_buff **skbp)
>>>    {
>>>    	struct sk_buff *skb = *skbp;
>>> -	__be16 vlan_proto = skb->vlan_proto;
>>> -	u16 vlan_id = skb_vlan_tag_get_id(skb);
>>> +	__be16 vlan_proto;
>>> +	u16 vlan_id;
>>>    	struct net_device *vlan_dev;
>>>    	struct vlan_pcpu_stats *rx_stats;
>>>    
>>> +again:
>>> +	vlan_proto = skb->vlan_proto;	
>>> +	vlan_id = skb_vlan_tag_get_id(skb);
>>>    	vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
>>> -	if (!vlan_dev)
>>> +	if (!vlan_dev) {
>>> +		/* Incase of 802.1P header with vlan id 0, continue if
>>> +		 * vlan_dev is not found.
>>> +		 */
>>> +		if (unlikely(!vlan_id)) {
>>> +			__vlan_hwaccel_clear_tag(skb);
>>
>> Looks like this changes existing behavior. Priority-tagged packets will be
>> untagged
>> before bridge, etc. I think priority-tagged packets should be forwarded as
>> priority-tagged
>> (iff bridge is not vlan-aware), not untagged.
> 
> Makes sense to me. If rx_handler is registered to the device, pkt should be sent
> untagged to rx_handler.
> 
> 
> I would like to get some clarification on few more cases before I change the
> code. In the setup:
> 
>                  br0
>                   |
>    vlan 100       |
>     |(802.1AD)    |
>     |             |
> +--------------------+
> |        eth0        |
> +--------------------+
> 
> Case 1: [802.1P vlan0] [IP]
> Current behaviour: pkt is sent to br0 with priority tag. i.e we should not remove
> the 802.1P tag.
> This patch: removes the 802.1P tag and br0 receives untagged packet. This is
> wrong.
> Expected behaviour: Should be same as current behaviour.
> 
> Case 2: [802.1AD vlan 100] [IP]
> Current behaviour: pkt is sent to vlan 100 device.
> This patch: same as current behaviour.
> Expected behaviour: same as current behaviour
> 
> Case 3: [802.1P vlan 0] [802.1AD vlan 100] [IP]
> Current behaviour: Pkt is sent to br0 rx_handler. This happens because
> vlan_do_receive() returns false (vlan 0 device is not present). Stack does not go
> through inner headers.
> This patch: pkt is sent to vlan 100 device. Because vlan_do_receive() strips vlan
> 0 header and finds vlan 100 device.
> Expected behaviour: ?
> IMO: Pkt should be sent to vlan 100 device because 802.1P should be treated as
> priority tag and not as vlan tagged pkt. Since vlan 100 device is present, it
> should be sent to vlan 100 device.

Maybe yes, maybe no. There is no standard about that. Your opinion is consistent
behavior between untagged and priority-tagged. OTOH, it changes existing behavior.
We basically try to keep existing behavior even if the behavior looks odd in some
way in order not to break existing users. So I would choose the other option, send
packets to br0.

> 
> Case 4: [802.1AD vlan 200] [802.1AD vlan 100] [IP]
> Current behaviour: Pkt is sent to br0 since vlan 200 device is not found.
> This patch: same as current behaviour.
> Expected behaviour: Same as current behaviour.
> 
> Is my understanding correct?

Agree except case 3.

Toshiaki Makita