Message-ID: <AANLkTi=Vdcn7xzJMPxkugvEVy32N7Bp=KVtir6NESnDF@mail.gmail.com>
Date:	Thu, 30 Sep 2010 19:37:13 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Roger Luethi <rl@...lgate.ch>, netdev@...r.kernel.org,
	Patrick McHardy <kaber@...sh.net>
Subject: Re: VLAN packets silently dropped in promiscuous mode

On Thu, Sep 30, 2010 at 3:04 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> Le jeudi 30 septembre 2010 à 14:21 -0700, Jesse Gross a écrit :
>> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@...lgate.ch> wrote:
>> > On Wed, 29 Sep 2010 10:44:26 -0700, Jesse Gross wrote:
>> >> On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@...lgate.ch> wrote:
>> >> > I noticed packets for unknown VLANs getting silently dropped even in
>> >> > promiscuous mode (this is true only for the hardware accelerated path).
>> >> > netif_nit_deliver was introduced specifically to prevent that, but the
>> >> > function gets called only _after_ packets from unknown VLANs have been
>> >> > dropped.
>> >>
>> >> Some drivers are fixing this on a case by case basis by disabling
>> >> hardware accelerated VLAN stripping when in promiscuous mode, i.e.:
>> >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5f6c01819979afbfec7e0b15fe52371b8eed87e8
>> >>
>> >> However, at this point it is more or less random which drivers do
>> >> this.  It would obviously be much better if it were consistent.
>> >
>> > My understanding is this. Hardware VLAN tagging and stripping can always be
>> > enabled. The kernel passes 802.1Q information along with the stripped
>> > header to libpcap which reassembles the original header where necessary.
>> > Works for me.
>>
>> Sorry, I misread your original post as saying that the VLAN header
>> gets dropped, rather than the entire packet.  I agree that this is how
>> it should work but not necessarily how it does work (again, depending
>> on the driver).  Here's the problem that I was talking about:
>>
>> Most drivers have a snippet of code that looks something like this
>> (taken from ixgbe):
>>
>> if (adapter->vlgrp && is_vlan && (tag & VLAN_VID_MASK))
>>       vlan_gro_receive(napi, adapter->vlgrp, tag, skb);
>> else
>>       napi_gro_receive(napi, skb);
>>
>> At this point the VLAN has already been stripped in hardware.  If
>> there is no VLAN group configured on the device then we hit the second
>> case.  The VLAN header was removed from the SKB and the tag variable
>> is unused.  It is no longer possible for libpcap to reconstruct the
>> header because the information was thrown away (even the fact that
>> there was a VLAN tag at all).
>>
>> There are a couple ways to fix this:
>>
>> * Turn off VLAN stripping when in promiscuous mode (as done by the ixgbe driver)
>> * Reconstruct the VLAN header when there is no VLAN group (as done by
>> the tg3 driver)
>>
>> A bunch of drivers do neither (bnx2x, for example) and exhibit this
>> problem.  It's getting better but it seems like a common issue.
>
> tg3 is not perfect, because it does the reconstruction of VLAN header
> even if device is not in promiscuous mode.
>
> It could drop the frame instead.
>
> I wonder which SNMP counter is incremented in this case.
>
> Apparently, none :(

That's true.  Dropping here seems roughly equivalent to the effects of
a hardware VLAN filter, which will also not be tracked by a counter,
so that seems not too bad to me.

The thing that concerns me though is why so many drivers seem to have
this problem with completely dropping the VLAN header.  I know that
even several of the ones that work now were broken initially and had
to be fixed.  Seeing as the driver drops the VLAN information before
it gets to the general networking code I don't see a generic fix to
this as it is currently setup.  However, perhaps we could make it so
that it is harder to get wrong.  Something like this:

* Allow vlan_gro_receive() to take a NULL VLAN group and a tag of 0
(and do the same thing for vlan_hwaccel_rx())
* Now that the vlan functions can deal with non-VLAN packets, merge
them into their non-VLAN counterparts.
* We can now demultiplex between the VLAN/non-VLAN case in core
networking.  This is done anyways, it just prevents every driver from
needing that code block I copied above and allows us to fix these
types of problems centrally.
* Dump the VLAN tag into the SKB and hand off the packet to the
various consumers: VLAN devices, libpcap, bridge hook (not currently
done but should be for trunking).

I see a number of advantages of this:
* Fixes all the problems with cards dropping VLAN headers at once.
* Avoids having to disable VLAN acceleration when in promiscuous mode
(good for bridging since it always puts devices in promiscuous mode).
* Keeps VLAN tag separate until given to ultimate consumer, which
avoids needing to do header reconstruction as in tg3 unless absolutely
necessary.
* Consolidates common driver code in core networking.
--
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

Powered by blists - more mailing lists