netdev - Re: [patch net-next-2.6 v2] net: vlan: make non-hw-accel rx path similar to hw-accel

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: <4DD8DD06.6070202@gmail.com>
Date:	Sun, 22 May 2011 11:53:10 +0200
From:	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>
To:	Jiri Pirko <jpirko@...hat.com>
CC:	Michał Mirosław <mirqus@...il.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Jesse Gross <jesse@...ira.com>,
	Changli Gao <xiaosuo@...il.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	shemminger@...ux-foundation.org, kaber@...sh.net, fubar@...ibm.com,
	eric.dumazet@...il.com, andy@...yhouse.net
Subject: Re: [patch net-next-2.6 v2] net: vlan: make non-hw-accel rx path
 similar to hw-accel

Le 22/05/2011 11:36, Jiri Pirko a écrit :
> Sun, May 22, 2011 at 11:20:09AM CEST, mirqus@...il.com wrote:
>> W dniu 22 maja 2011 11:10 użytkownik Nicolas de Pesloüan
>> <nicolas.2p.debian@...il.com>  napisał:
>>> Le 22/05/2011 10:52, Michał Mirosław a écrit :
>>>>
>>>> 2011/5/22 Nicolas de Pesloüan<nicolas.2p.debian@...il.com>:
>>>>>
>>>>> Le 22/05/2011 08:34, Eric W. Biederman a écrit :
>>>>>>
>>>>>> Jiri Pirko<jpirko@...hat.com>      writes:
>>>>>>
>>>>>>> Sun, May 22, 2011 at 04:59:49AM CEST, nicolas.2p.debian@...il.com
>>>>>>> wrote:
>>>>>>>
>>>>>>> <snip>
>>>>>>>>
>>>>>>>> And because some setups may still require the skb not to be untagged,
>>>>>>>> may be we need the ability to re-tag the skb in some situations...
>>>>>>>> When a protocol handler or rx_handler is explicitly registered on a
>>>>>>>> net_device which expect to receive tagged skb, we should deliver
>>>>>>>> tagged skb to it... Arguably, this may sound incredible for the
>>>>>>>> general case, but may be required for not-so-special cases like
>>>>>>>> bridge or protocol analyzer.
>>>>>>>
>>>>>>> Wait, what setups/code require the skb not to be untagged? If there's
>>>>>>> such, it should be fixed.
>>>>>>
>>>>>> tcpdump on the non-vlan interface for one.
>>>>>
>>>>> bridge is another. More precisely, there is a difference between the
>>>>> following two setups:
>>>>>
>>>>> 1/ eth0 - eth0.100 - br0 - eth1.200 - eth1
>>>>>
>>>>> 2/ eth0 - br0 - eth1
>>>>>
>>>>> In case 1, it is normal and desirable for the bridge to see untagged skb.
>>>>>
>>>>> In case 2, it is desirable for the bridge to see untouched (possibly
>>>>> tagged)
>>>>> skb. If current bridge implementation is able to handle skb from which we
>>>>> removed a tag, in this situation, it means that bridge currently "fix
>>>>> improper untagging" by itself, by forcing re-tagging on output. I think
>>>>> is
>>>>> should not be the job of protocol handlers to fix this. Again, a generic
>>>>> feature should to it when necessary.
>>>>>
>>>>> Think of the following setups:
>>>>>
>>>>> 3/ eth0 - br0 - eth1.200 - eth1.
>>>>> 4/ eth0 - eth0.100 - br0 - eth1
>>>>>
>>>>> What if one expect this setup to add (3) or remove (4) one level of vlan
>>>>> nesting? This is precisely what this setup suggest. How can we instruct
>>>>> the
>>>>> bridge to do so? It is not the bridge responsibility to do any vlan
>>>>> processing. bridge is expected to... bridge !
>>>>
>>>> I assumed that this untaging Jiri is implementing does not remove the
>>>> tag. It moves the information from skb->data to skb->vlan_tci, but the
>>>> information contained is not otherwise changing. All your examples
>>>> should work regardless of where the tag is stored.
>>>
>>> I assumed (but didn't tested) that this untagging also change the starting
>>> point of the payload of the packet. So protocol handlers expecting to have
>>> the raw packet won't see the vlan header.
>>
>> That would also be the case with hardware stripped tags - they need to
>> look into skb->vlan_tci anyway.
>
> Exactly. Nicolas, I do not see anything wrong on always untagging in all
> your setups. As Michal said, vlan_tci keeps the info.

I understand this.

But I don't understand how the bridge code is expected to know whether it should re-tag the packet 
or not before forwarding and which value to use as the egress vlan tag.

1/ eth0 - br0 - eth1 : the bridge is expected to retag using skb->vlan_tci value.

2/ eth0 - eth0.100 - br0 - eth1.200 - eth1 : the bridge is expected to retag using a different value 
than skb->vlan_tci.

3/ eth0 - eth0.100 - br0 - eth1 : the bridge is expected not to re-tag, because the expected 
behavior of this setup is to untag while crossing the bridge.

4/ eth0 - eth0.100 - eth0.100.300 - br0 - eth1.400 - eth1.200 - eth1 : the bridge is expected to 
retag using a different value than skb->vlan_tci. What value would skb->vlan_tci hold when the skb 
will be delivered to the bridge? 100 or 300?

 From my point of view, in both setup, the bridge will receive a single value in skb->vlan_tci and 
will lack any other indication to help it decide how to retag when forwarding.

I'm not against your idea to mimic hw-accel in software. But I'm concern about troubles for those 
who expect to have access to untouched packet. Your patch didn't cause those troubles to start 
happening, but cause them to always happen. Before your patch, someone had the option to use 
non-hw-accel NIC or to disable hw-accel feature if possible. Now, it's no more possible.

	Nicolas.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html