From: Vlad Yasevich <vyasevich@gmail•com>
To: Jiri Pirko <jiri@resnulli•us>
Cc: netdev@vger•kernel.org, Vladislav Yasevich <vyasevic@redhat•com>,
Florian Zumbiehl <florz@florz•de>,
Eric Dumazet <eric.dumazet@gmail•com>,
Matthew Rosato <mjrosato@linux•vnet.ibm.com>
Subject: Re: [PATCH net] core: Untag packets after rx_handler has run.
Date: Thu, 04 Sep 2014 15:29:00 -0400 [thread overview]
Message-ID: <5408BD7C.7030805@gmail.com> (raw)
In-Reply-To: <20140904190524.GA1938@nanopsycho.lan>
On 09/04/2014 03:05 PM, Jiri Pirko wrote:
> Thu, Sep 04, 2014 at 08:40:43PM CEST, vyasevich@gmail•com wrote:
>> Currently, we attempt to remove the vlan informaion from the packet
>> before passing it to the rx_handler. In most situations this works
>> just fine since the rx_handlers are usually installed for the
>> lower device and thus vlan device isn't found. However, macvtap
>> device is a bit different as it installs an rx_handler on top
>> of a macvlan device. As a result, if someone was define a vlan
>> device on top of a macvap (for the purposes of enabling a VM
>> to use vlans with macvtap), then the current code will result
>> in passing an untagged packet to the macvtap rx_handler and the
>> VM will not receive tagged traffic.
>
> skb->vlan_tci is set. macvlan should work with that to pass the frame
> correctly. This should be handled in macvtap code.
OK. Consider a configuration.
vlan10
vlan10 |
| VM1:eth0
v |
macvtap0 <---+
|
V
eth0
On the host, vlan10 can't really send and receive traffic. It's only
there to add a vlan filter to eth0 so that packets marked with vlan10
can be received.
When traffic is processed by __netif_receive_skb_core(), skb->dev is eth0 and we
have the tci, so that when vlan_do_receive() is called, we can't find the vlan
device and call the rx_handler macvlan_handle_frame().
That handler calls netif_rx() with skb->dev set to macvtap0.
This time through the receive path, vlan_tci is still set and we
do find the vlan device which is on top of macvtap0, so we set the tci to 0
and then pass it to the rx_handler macvtap_handle_frame().
As a result, we pass an untagged frame to the VM.
>
> btw can you give me an example of setup where rx_handler is set for
> macvlan device? I wonder what is could be.
>
>>
>> This patch moves the untaggin code to after the rx_handler for
>> the current device has been called. This still works for the
>> existing rx_handlers (like bonding/teaming/bridging/etc) and
>> also makes vlans on top of macvtap work as before.
>>
>> Fixes: 6acf54f1cf (macvtap: Add support of packet capture on macvtap device)
>> Reported-by: Matthew Rosato <mjrosato@linux•vnet.ibm.com>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat•com>
>> ---
>> net/core/dev.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index ab9a165..54691d1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3642,17 +3642,6 @@ ncls:
>> if (pfmemalloc && !skb_pfmemalloc_protocol(skb))
>> goto drop;
>>
>> - if (vlan_tx_tag_present(skb)) {
>> - if (pt_prev) {
>> - ret = deliver_skb(skb, pt_prev, orig_dev);
>> - pt_prev = NULL;
>> - }
>> - if (vlan_do_receive(&skb))
>> - goto another_round;
>> - else if (unlikely(!skb))
>> - goto unlock;
>> - }
>> -
>> rx_handler = rcu_dereference(skb->dev->rx_handler);
>> if (rx_handler) {
>> if (pt_prev) {
>> @@ -3674,6 +3663,17 @@ ncls:
>> }
>> }
>>
>> + if (vlan_tx_tag_present(skb)) {
>> + if (pt_prev) {
>> + ret = deliver_skb(skb, pt_prev, orig_dev);
>> + pt_prev = NULL;
>> + }
>> + if (vlan_do_receive(&skb))
>> + goto another_round;
>> + else if (unlikely(!skb))
>> + goto unlock;
>> + }
>> +
>
> nack. This will definitelly break several stacked setups.
Which ones? The only thing I can see that would behave differently
is something like:
vlan0 bridge0
| |
+-------- eth0
In this case, the old code would give an untagged packet to the bridge
and the new code would give a tagged packet.
This set-up is a bit ambiguous. Remove the vlan, and bridge gets a tagged
traffic even though the vlan has no relationship to the bridge.
I've tested a couple of different stacked setups and they all seem to work.
Thanks
-vlad
>
>
>> if (unlikely(vlan_tx_tag_present(skb))) {
>> if (vlan_tx_tag_get_id(skb))
>> skb->pkt_type = PACKET_OTHERHOST;
>> --
>> 1.9.3
>>
next prev parent reply other threads:[~2014-09-04 19:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-04 18:40 [PATCH net] core: Untag packets after rx_handler has run Vladislav Yasevich
2014-09-04 19:05 ` Jiri Pirko
2014-09-04 19:29 ` Vlad Yasevich [this message]
2014-09-04 20:43 ` Alexei Starovoitov
2014-09-04 21:01 ` Vlad Yasevich
2014-09-04 21:54 ` Alexei Starovoitov
2014-09-04 23:48 ` Vlad Yasevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5408BD7C.7030805@gmail.com \
--to=vyasevich@gmail$(echo .)com \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=florz@florz$(echo .)de \
--cc=jiri@resnulli$(echo .)us \
--cc=mjrosato@linux$(echo .)vnet.ibm.com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=vyasevic@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox