VLAN packets silently dropped in promiscuous mode

VLAN packets silently dropped in promiscuous mode

[Roger Luethi]

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.

Presumably this has been the case since

commit e1c096e251e52773afeffbbcb74d0a072be47ea3
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Tue Jan 6 10:50:09 2009 -0800
    vlan: Add GRO interfaces

I didn't find any indication that the change in behavior is intentional. Is
it?

Roger

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@hellgate.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.

Re: VLAN packets silently dropped in promiscuous mode

[Roger Luethi]

On Wed, 29 Sep 2010 10:44:26 -0700, Jesse Gross wrote:
> On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@hellgate.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.

Hardware VLAN filtering, on the other hand, must be disabled in promiscuous
mode. But doing that in the driver makes no difference now as the current
VLAN code drops the packets so preserved before they are passed to the pcap
interface. That appears to be a bug.

Roger

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 10:07 +0200, Roger Luethi a écrit :
> On Wed, 29 Sep 2010 10:44:26 -0700, Jesse Gross wrote:
> > On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@hellgate.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.
> 
> Hardware VLAN filtering, on the other hand, must be disabled in promiscuous
> mode. But doing that in the driver makes no difference now as the current
> VLAN code drops the packets so preserved before they are passed to the pcap
> interface. That appears to be a bug.

Agreed

Could you try following patch, based on net-next-2.6 ?

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 0eb486d..fabdedb 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -101,7 +101,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 
 	if (vlan_dev)
 		skb->dev = vlan_dev;
-	else if (vlan_id)
+	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
 		goto drop;
 
 	for (p = napi->gro_list; p; p = p->next) {

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :

> Agreed
> 
> Could you try following patch, based on net-next-2.6 ?

Here is the official patch I cooked for net-2.6 (linux-2.6)

I tested it successfully with a tg3 NIC.

Thanks

[PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode

Roger Luethi noticed packets for unknown VLANs getting silently dropped
even in promiscuous mode.

Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
before drops.

Reported-by: Roger Luethi <rl@hellgate.ch>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/8021q/vlan_core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..ba502b4 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 
 	if (vlan_dev)
 		skb->dev = vlan_dev;
-	else if (vlan_id)
+	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
 		goto drop;
 
 	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
@@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 
 	if (vlan_dev)
 		skb->dev = vlan_dev;
-	else if (vlan_id)
+	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
 		goto drop;
 
 	for (p = napi->gro_list; p; p = p->next) {

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 11:55 +0200, Eric Dumazet a écrit :
> Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :
> 
> > Agreed
> > 
> > Could you try following patch, based on net-next-2.6 ?
> 
> Here is the official patch I cooked for net-2.6 (linux-2.6)
> 
> I tested it successfully with a tg3 NIC.
> 
> Thanks
> 
> [PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
> 
> Roger Luethi noticed packets for unknown VLANs getting silently dropped
> even in promiscuous mode.
> 
> Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
> before drops.
> 
> Reported-by: Roger Luethi <rl@hellgate.ch>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/8021q/vlan_core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 01ddb04..ba502b4 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  
>  	if (vlan_dev)
>  		skb->dev = vlan_dev;
> -	else if (vlan_id)
> +	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>  		goto drop;
>  
>  	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> @@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
>  
>  	if (vlan_dev)
>  		skb->dev = vlan_dev;
> -	else if (vlan_id)
> +	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>  		goto drop;
>  
>  	for (p = napi->gro_list; p; p = p->next) {
> 


Hmm, packets are delivered not only to tcpdump but also on other stacks,
on ethX.

So this is a domain violation.

Patch is not applicable as is.

Re: VLAN packets silently dropped in promiscuous mode

[Patrick McHardy]

On 30.09.2010 12:40, Eric Dumazet wrote:
> Le jeudi 30 septembre 2010 à 11:55 +0200, Eric Dumazet a écrit :
>> Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :
>>
>>> Agreed
>>>
>>> Could you try following patch, based on net-next-2.6 ?
>>
>> Here is the official patch I cooked for net-2.6 (linux-2.6)
>>
>> I tested it successfully with a tg3 NIC.
>>
>> Thanks
>>
>> [PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
>>
>> Roger Luethi noticed packets for unknown VLANs getting silently dropped
>> even in promiscuous mode.
>>
>> Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
>> before drops.
>>
>> Reported-by: Roger Luethi <rl@hellgate.ch>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>  net/8021q/vlan_core.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 01ddb04..ba502b4 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>>  
>>  	if (vlan_dev)
>>  		skb->dev = vlan_dev;
>> -	else if (vlan_id)
>> +	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>>  		goto drop;
>>  
>>  	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>> @@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
>>  
>>  	if (vlan_dev)
>>  		skb->dev = vlan_dev;
>> -	else if (vlan_id)
>> +	else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>>  		goto drop;
>>  
>>  	for (p = napi->gro_list; p; p = p->next) {
>>
> 
> 
> Hmm, packets are delivered not only to tcpdump but also on other stacks,
> on ethX.
> 
> So this is a domain violation.

This should be fine as long as the packets are properly marked
with PACKET_OTHERHOST.

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 12:50 +0200, Patrick McHardy a écrit :

> This should be fine as long as the packets are properly marked
> with PACKET_OTHERHOST.

Ah thanks Patrick for the tip !

I tested following patch on tg3 and it is doing the right thing this
time.


[PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode

Roger Luethi noticed packets for unknown VLANs getting silently dropped
even in promiscuous mode.

Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
before drops.

As suggested by Patrick, mark such packets to have skb->pkt_type set to
PACKET_OTHERHOST to make sure they are dropped by IP stack.

Reported-by: Roger Luethi <rl@hellgate.ch>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
---
 net/8021q/vlan_core.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..0eb96f7 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -24,8 +24,11 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 
 	if (vlan_dev)
 		skb->dev = vlan_dev;
-	else if (vlan_id)
-		goto drop;
+	else if (vlan_id) {
+		if (!(skb->dev->flags & IFF_PROMISC))
+			goto drop;
+		skb->pkt_type = PACKET_OTHERHOST;
+	}
 
 	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
 
@@ -102,8 +105,11 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 
 	if (vlan_dev)
 		skb->dev = vlan_dev;
-	else if (vlan_id)
-		goto drop;
+	else if (vlan_id) {
+		if (!(skb->dev->flags & IFF_PROMISC))
+			goto drop;
+		skb->pkt_type = PACKET_OTHERHOST;
+	}
 
 	for (p = napi->gro_list; p; p = p->next) {
 		NAPI_GRO_CB(p)->same_flow =

Re: VLAN packets silently dropped in promiscuous mode

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 30 Sep 2010 14:16:44 +0200

> [PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
> 
> Roger Luethi noticed packets for unknown VLANs getting silently dropped
> even in promiscuous mode.
> 
> Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
> before drops.
> 
> As suggested by Patrick, mark such packets to have skb->pkt_type set to
> PACKET_OTHERHOST to make sure they are dropped by IP stack.
> 
> Reported-by: Roger Luethi <rl@hellgate.ch>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Patrick McHardy <kaber@trash.net>

Applied and queued up for -stable, thanks everyone!

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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.

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 14:21 -0700, Jesse Gross a écrit :
> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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 :(

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Thu, Sep 30, 2010 at 3:04 PM, Eric Dumazet <eric.dumazet@gmail.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@hellgate.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@hellgate.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.

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le jeudi 30 septembre 2010 à 19:37 -0700, Jesse Gross a écrit :

> 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.

This seems very reasonable ;)

I'll add a counter, a core generalization of 
commit 8990f468a (net: rx_dropped accounting)

Because we can drop packets _after_ netif_rx() if RPS is in action
anyway.

[PATCH net-next] net: add a core netdev->rx_dropped counter

[Eric Dumazet]

Le vendredi 01 octobre 2010 à 07:10 +0200, Eric Dumazet a écrit :

> This seems very reasonable ;)
> 
> I'll add a counter, a core generalization of 
> commit 8990f468a (net: rx_dropped accounting)
> 
> Because we can drop packets _after_ netif_rx() if RPS is in action
> anyway.
> 
> 

In this patch I fold the additional dev->rx_dropped into get_stats()
structure. We might chose to not fold it, and provides this counter in a
new /proc/net/dev column, a new rtnetlink attribute (and appropriate
iproute2 change)

What do you think ?

[PATCH net-next] net: add a core netdev->rx_dropped counter

In various situations, a device provides a packet to our stack and we
drop it before it enters protocol stack :
- softnet backlog full (accounted in /proc/net/softnet_stat)
- bad vlan tag (not accounted)
- unknown/unregistered protocol (not accounted)

We can handle a per-device counter of such dropped frames at core level,
and automatically adds it to the device provided stats (rx_dropped), so
that standard tools can be used (ifconfig, ip link, cat /proc/net/dev)

This is a generalization of commit 8990f468a (net: rx_dropped
accounting), thus reverting it.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/loopback.c    |    8 +-------
 include/linux/netdevice.h |    3 +++
 net/8021q/vlan.h          |    2 --
 net/8021q/vlan_core.c     |    2 ++
 net/8021q/vlan_dev.c      |   11 ++++-------
 net/core/dev.c            |   19 +++++++++++--------
 net/ipv4/ip_gre.c         |    3 +--
 net/ipv4/ipip.c           |    3 +--
 net/ipv6/ip6_tunnel.c     |    3 +--
 net/ipv6/ip6mr.c          |    3 +--
 net/ipv6/sit.c            |    3 +--
 11 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 4b0e30b..2d9663a 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -64,7 +64,6 @@ struct pcpu_lstats {
 	u64			packets;
 	u64			bytes;
 	struct u64_stats_sync	syncp;
-	unsigned long		drops;
 };
 
 /*
@@ -90,8 +89,7 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
 		lb_stats->bytes += len;
 		lb_stats->packets++;
 		u64_stats_update_end(&lb_stats->syncp);
-	} else
-		lb_stats->drops++;
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -101,7 +99,6 @@ static struct rtnl_link_stats64 *loopback_get_stats64(struct net_device *dev,
 {
 	u64 bytes = 0;
 	u64 packets = 0;
-	u64 drops = 0;
 	int i;
 
 	for_each_possible_cpu(i) {
@@ -115,14 +112,11 @@ static struct rtnl_link_stats64 *loopback_get_stats64(struct net_device *dev,
 			tbytes = lb_stats->bytes;
 			tpackets = lb_stats->packets;
 		} while (u64_stats_fetch_retry(&lb_stats->syncp, start));
-		drops   += lb_stats->drops;
 		bytes   += tbytes;
 		packets += tpackets;
 	}
 	stats->rx_packets = packets;
 	stats->tx_packets = packets;
-	stats->rx_dropped = drops;
-	stats->rx_errors  = drops;
 	stats->rx_bytes   = bytes;
 	stats->tx_bytes   = bytes;
 	return stats;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ceed347..444f042 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -884,6 +884,9 @@ struct net_device {
 	int			iflink;
 
 	struct net_device_stats	stats;
+	atomic_long_t		rx_dropped; /* dropped packets by core network
+					     * Do not use this in drivers.
+					     */
 
 #ifdef CONFIG_WIRELESS_EXT
 	/* List of functions to handle Wireless Extensions (instead of ioctl).
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index b26ce34..8d9503a 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -25,7 +25,6 @@ struct vlan_priority_tci_mapping {
  *	@rx_multicast: number of received multicast packets
  *	@syncp: synchronization point for 64bit counters
  *	@rx_errors: number of errors
- *	@rx_dropped: number of dropped packets
  */
 struct vlan_rx_stats {
 	u64			rx_packets;
@@ -33,7 +32,6 @@ struct vlan_rx_stats {
 	u64			rx_multicast;
 	struct u64_stats_sync	syncp;
 	unsigned long		rx_errors;
-	unsigned long		rx_dropped;
 };
 
 /**
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 0eb486d..35a04a1 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -30,6 +30,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	return polling ? netif_receive_skb(skb) : netif_rx(skb);
 
 drop:
+	atomic_long_inc(&skb->dev->rx_dropped);
 	dev_kfree_skb_any(skb);
 	return NET_RX_DROP;
 }
@@ -117,6 +118,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 	return dev_gro_receive(napi, skb);
 
 drop:
+	atomic_long_inc(&skb->dev->rx_dropped);
 	return GRO_DROP;
 }
 
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index f6fbcc0..f54251e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -225,16 +225,15 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	if (unlikely(netif_rx(skb) == NET_RX_DROP)) {
-		if (rx_stats)
-			rx_stats->rx_dropped++;
-	}
+	netif_rx(skb);
+
 	rcu_read_unlock();
 	return NET_RX_SUCCESS;
 
 err_unlock:
 	rcu_read_unlock();
 err_free:
+	atomic_long_inc(&dev->rx_dropped);
 	kfree_skb(skb);
 	return NET_RX_DROP;
 }
@@ -846,15 +845,13 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st
 			accum.rx_packets += rxpackets;
 			accum.rx_bytes   += rxbytes;
 			accum.rx_multicast += rxmulticast;
-			/* rx_errors, rx_dropped are ulong, not protected by syncp */
+			/* rx_errors is ulong, not protected by syncp */
 			accum.rx_errors  += p->rx_errors;
-			accum.rx_dropped += p->rx_dropped;
 		}
 		stats->rx_packets = accum.rx_packets;
 		stats->rx_bytes   = accum.rx_bytes;
 		stats->rx_errors  = accum.rx_errors;
 		stats->multicast  = accum.rx_multicast;
-		stats->rx_dropped = accum.rx_dropped;
 	}
 	return stats;
 }
diff --git a/net/core/dev.c b/net/core/dev.c
index a313bab..5143663 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1483,8 +1483,9 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	if (!(dev->flags & IFF_UP) ||
-	    (skb->len > (dev->mtu + dev->hard_header_len))) {
+	if (unlikely(!(dev->flags & IFF_UP) ||
+		     (skb->len > (dev->mtu + dev->hard_header_len)))) {
+		atomic_long_inc(&dev->rx_dropped);
 		kfree_skb(skb);
 		return NET_RX_DROP;
 	}
@@ -2548,6 +2549,7 @@ enqueue:
 
 	local_irq_restore(flags);
 
+	atomic_long_inc(&skb->dev->rx_dropped);
 	kfree_skb(skb);
 	return NET_RX_DROP;
 }
@@ -2996,6 +2998,7 @@ ncls:
 	if (pt_prev) {
 		ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 	} else {
+		atomic_long_inc(&skb->dev->rx_dropped);
 		kfree_skb(skb);
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)
@@ -5431,14 +5434,14 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 
 	if (ops->ndo_get_stats64) {
 		memset(storage, 0, sizeof(*storage));
-		return ops->ndo_get_stats64(dev, storage);
-	}
-	if (ops->ndo_get_stats) {
+		ops->ndo_get_stats64(dev, storage);
+	} else if (ops->ndo_get_stats) {
 		netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
-		return storage;
+	} else {
+		netdev_stats_to_stats64(storage, &dev->stats);
+		dev_txq_stats_fold(dev, storage);
 	}
-	netdev_stats_to_stats64(storage, &dev->stats);
-	dev_txq_stats_fold(dev, storage);
+	storage->rx_dropped += atomic_long_read(&dev->rx_dropped);
 	return storage;
 }
 EXPORT_SYMBOL(dev_get_stats);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index fbe2c47..9d421f4 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -679,8 +679,7 @@ static int ipgre_rcv(struct sk_buff *skb)
 		skb_reset_network_header(skb);
 		ipgre_ecn_decapsulate(iph, skb);
 
-		if (netif_rx(skb) == NET_RX_DROP)
-			tunnel->dev->stats.rx_dropped++;
+		netif_rx(skb);
 
 		rcu_read_unlock();
 		return 0;
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 6ad46c2..e9b816e 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -414,8 +414,7 @@ static int ipip_rcv(struct sk_buff *skb)
 
 		ipip_ecn_decapsulate(iph, skb);
 
-		if (netif_rx(skb) == NET_RX_DROP)
-			tunnel->dev->stats.rx_dropped++;
+		netif_rx(skb);
 
 		rcu_read_unlock();
 		return 0;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 8be3c45..c2c0f89 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -768,8 +768,7 @@ static int ip6_tnl_rcv(struct sk_buff *skb, __u16 protocol,
 
 		dscp_ecn_decapsulate(t, ipv6h, skb);
 
-		if (netif_rx(skb) == NET_RX_DROP)
-			t->dev->stats.rx_dropped++;
+		netif_rx(skb);
 
 		rcu_read_unlock();
 		return 0;
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 2640c9b..6f32ffc 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -666,8 +666,7 @@ static int pim6_rcv(struct sk_buff *skb)
 
 	skb_tunnel_rx(skb, reg_dev);
 
-	if (netif_rx(skb) == NET_RX_DROP)
-		reg_dev->stats.rx_dropped++;
+	netif_rx(skb);
 
 	dev_put(reg_dev);
 	return 0;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index d770178..367a6cc 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -600,8 +600,7 @@ static int ipip6_rcv(struct sk_buff *skb)
 
 		ipip6_ecn_decapsulate(iph, skb);
 
-		if (netif_rx(skb) == NET_RX_DROP)
-			tunnel->dev->stats.rx_dropped++;
+		netif_rx(skb);
 
 		rcu_read_unlock();
 		return 0;

Re: [PATCH net-next] net: add a core netdev->rx_dropped counter

[Jesse Gross]

On Fri, Oct 1, 2010 at 12:06 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 01 octobre 2010 à 07:10 +0200, Eric Dumazet a écrit :
>
>> This seems very reasonable ;)
>>
>> I'll add a counter, a core generalization of
>> commit 8990f468a (net: rx_dropped accounting)
>>
>> Because we can drop packets _after_ netif_rx() if RPS is in action
>> anyway.
>>
>>
>
> In this patch I fold the additional dev->rx_dropped into get_stats()
> structure. We might chose to not fold it, and provides this counter in a
> new /proc/net/dev column, a new rtnetlink attribute (and appropriate
> iproute2 change)
>
> What do you think ?

This patch makes sense to me.  I think combining the dropped stats
with those from the driver makes sense, at least at the moment, since
both the counters from both sources are from a combination of
different causes anyways.

Re: [PATCH net-next] net: add a core netdev->rx_dropped counter

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 01 Oct 2010 09:06:55 +0200

> [PATCH net-next] net: add a core netdev->rx_dropped counter
> 
> In various situations, a device provides a packet to our stack and we
> drop it before it enters protocol stack :
> - softnet backlog full (accounted in /proc/net/softnet_stat)
> - bad vlan tag (not accounted)
> - unknown/unregistered protocol (not accounted)
> 
> We can handle a per-device counter of such dropped frames at core level,
> and automatically adds it to the device provided stats (rx_dropped), so
> that standard tools can be used (ifconfig, ip link, cat /proc/net/dev)
> 
> This is a generalization of commit 8990f468a (net: rx_dropped
> accounting), thus reverting it.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Looks good to me, applied, thanks.

Re: VLAN packets silently dropped in promiscuous mode

[Eric Dumazet]

Le vendredi 01 octobre 2010 à 07:10 +0200, Eric Dumazet a écrit :
> Le jeudi 30 septembre 2010 à 19:37 -0700, Jesse Gross a écrit :
> 
> > 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.
> 
> This seems very reasonable ;)

Jesse, do you plan to work on this stuff yourself in a near future ?

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Fri, Oct 1, 2010 at 1:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 01 octobre 2010 à 07:10 +0200, Eric Dumazet a écrit :
>> Le jeudi 30 septembre 2010 à 19:37 -0700, Jesse Gross a écrit :
>>
>> > 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.
>>
>> This seems very reasonable ;)
>
> Jesse, do you plan to work on this stuff yourself in a near future ?

I'm a little bit busy at the moment but I'm hoping to have some time
to work on it towards the end of this week/beginning of next week.

Thanks for looking over the idea.

Re: VLAN packets silently dropped in promiscuous mode

[Guillaume Gaudonville]

Jesse Gross wrote:
> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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)
>   
This is not totally true: if changing the MTU ixgbe_change_mtu will call:
 ixgbe_reinit_locked--> ixgbe_up --> ixgbe_configure:
                 --> ixgbe_set_rx_mode: flag IFF_PROMISC is tested 
ixgbe_vlan_filter_enable is not called
                 --> ixgbe_restore_vlan --> ixgbe_vlan_rx_register: flag 
IFF_PROMISC is not tested ixgbe_vlan_filter_enable
                      will be called.

In fact it should happen each time we configure something which needs a 
reset of the device. Why don't add a test
on flag promiscuous directly in ixgbe_vlan_filter_enable? Or do it on 
each call, if we want to allow a device in promiscuous
mode to enable this feature.

What do you think?

> * 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.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


-- 
Guillaume Gaudonville
6WIND
Software Engineer

Tel: +33 1 39 30 92 63
Mob: +33 6 47 85 34 33
Fax: +33 1 39 30 92 11
guillaume.gaudonville@6wind.com
www.6wind.com
Join the Multicore Packet Processing Forum: www.multicorepacketprocessing.com

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues

This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Fri, Oct 15, 2010 at 2:16 AM, Guillaume Gaudonville
<guillaume.gaudonville@6wind.com> wrote:
> Jesse Gross wrote:
>>
>> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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)
>>
>
> This is not totally true: if changing the MTU ixgbe_change_mtu will call:
> ixgbe_reinit_locked--> ixgbe_up --> ixgbe_configure:
>                --> ixgbe_set_rx_mode: flag IFF_PROMISC is tested
> ixgbe_vlan_filter_enable is not called
>                --> ixgbe_restore_vlan --> ixgbe_vlan_rx_register: flag
> IFF_PROMISC is not tested ixgbe_vlan_filter_enable
>                     will be called.
>
> In fact it should happen each time we configure something which needs a
> reset of the device. Why don't add a test
> on flag promiscuous directly in ixgbe_vlan_filter_enable? Or do it on each
> call, if we want to allow a device in promiscuous
> mode to enable this feature.
>
> What do you think?

I can believe that there are paths that lead to this not working
correctly.  That was actually my larger point: this is something that
is commonly not implemented correctly in drivers.  Rather than try to
study every driver my goal is to just avoid the problem completely by
handling vlan acceleration centrally in the networking core.  I sent
out an RFC patch series a few days ago that should solve this problem:

http://marc.info/?l=linux-netdev&m=128700022614170&w=3

Re: VLAN packets silently dropped in promiscuous mode

[Guillaume Gaudonville]

Jesse Gross wrote:
> On Fri, Oct 15, 2010 at 2:16 AM, Guillaume Gaudonville
> <guillaume.gaudonville@6wind.com> wrote:
>   
>> Jesse Gross wrote:
>>     
>>> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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)
>>>
>>>       
>> This is not totally true: if changing the MTU ixgbe_change_mtu will call:
>> ixgbe_reinit_locked--> ixgbe_up --> ixgbe_configure:
>>                --> ixgbe_set_rx_mode: flag IFF_PROMISC is tested
>> ixgbe_vlan_filter_enable is not called
>>                --> ixgbe_restore_vlan --> ixgbe_vlan_rx_register: flag
>> IFF_PROMISC is not tested ixgbe_vlan_filter_enable
>>                     will be called.
>>
>> In fact it should happen each time we configure something which needs a
>> reset of the device. Why don't add a test
>> on flag promiscuous directly in ixgbe_vlan_filter_enable? Or do it on each
>> call, if we want to allow a device in promiscuous
>> mode to enable this feature.
>>
>> What do you think?
>>     
>
> I can believe that there are paths that lead to this not working
> correctly.  That was actually my larger point: this is something that
> is commonly not implemented correctly in drivers.  Rather than try to
> study every driver my goal is to just avoid the problem completely by
> handling vlan acceleration centrally in the networking core.  I sent
> out an RFC patch series a few days ago that should solve this problem:
>
> http://marc.info/?l=linux-netdev&m=128700022614170&w=3
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   
Thank you, I'm going to check these patches and try to apply them in our 
kernel.

Best Regards,

-- 
Guillaume Gaudonville
6WIND
Software Engineer

Tel: +33 1 39 30 92 63
Mob: +33 6 47 85 34 33
Fax: +33 1 39 30 92 11
guillaume.gaudonville@6wind.com
www.6wind.com
Join the Multicore Packet Processing Forum: www.multicorepacketprocessing.com

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues

This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.

Re: VLAN packets silently dropped in promiscuous mode

[Jesse Gross]

On Mon, Oct 25, 2010 at 6:48 AM, Guillaume Gaudonville
<guillaume.gaudonville@6wind.com> wrote:
> Jesse Gross wrote:
>>
>> On Fri, Oct 15, 2010 at 2:16 AM, Guillaume Gaudonville
>> <guillaume.gaudonville@6wind.com> wrote:
>>
>>>
>>> Jesse Gross wrote:
>>>
>>>>
>>>> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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)
>>>>
>>>>
>>>
>>> This is not totally true: if changing the MTU ixgbe_change_mtu will call:
>>> ixgbe_reinit_locked--> ixgbe_up --> ixgbe_configure:
>>>               --> ixgbe_set_rx_mode: flag IFF_PROMISC is tested
>>> ixgbe_vlan_filter_enable is not called
>>>               --> ixgbe_restore_vlan --> ixgbe_vlan_rx_register: flag
>>> IFF_PROMISC is not tested ixgbe_vlan_filter_enable
>>>                    will be called.
>>>
>>> In fact it should happen each time we configure something which needs a
>>> reset of the device. Why don't add a test
>>> on flag promiscuous directly in ixgbe_vlan_filter_enable? Or do it on
>>> each
>>> call, if we want to allow a device in promiscuous
>>> mode to enable this feature.
>>>
>>> What do you think?
>>>
>>
>> I can believe that there are paths that lead to this not working
>> correctly.  That was actually my larger point: this is something that
>> is commonly not implemented correctly in drivers.  Rather than try to
>> study every driver my goal is to just avoid the problem completely by
>> handling vlan acceleration centrally in the networking core.  I sent
>> out an RFC patch series a few days ago that should solve this problem:
>>
>> http://marc.info/?l=linux-netdev&m=128700022614170&w=3
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Thank you, I'm going to check these patches and try to apply them in our
> kernel.

An updated set of patches has been merged into net-2.6, so you might
want to try that instead.

Re: VLAN packets silently dropped in promiscuous mode

[Guillaume Gaudonville]

Jesse Gross wrote:
> On Mon, Oct 25, 2010 at 6:48 AM, Guillaume Gaudonville
> <guillaume.gaudonville@6wind.com> wrote:
>   
>> Jesse Gross wrote:
>>     
>>> On Fri, Oct 15, 2010 at 2:16 AM, Guillaume Gaudonville
>>> <guillaume.gaudonville@6wind.com> wrote:
>>>
>>>       
>>>> Jesse Gross wrote:
>>>>
>>>>         
>>>>> On Thu, Sep 30, 2010 at 1:07 AM, Roger Luethi <rl@hellgate.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@hellgate.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)
>>>>>
>>>>>
>>>>>           
>>>> This is not totally true: if changing the MTU ixgbe_change_mtu will call:
>>>> ixgbe_reinit_locked--> ixgbe_up --> ixgbe_configure:
>>>>               --> ixgbe_set_rx_mode: flag IFF_PROMISC is tested
>>>> ixgbe_vlan_filter_enable is not called
>>>>               --> ixgbe_restore_vlan --> ixgbe_vlan_rx_register: flag
>>>> IFF_PROMISC is not tested ixgbe_vlan_filter_enable
>>>>                    will be called.
>>>>
>>>> In fact it should happen each time we configure something which needs a
>>>> reset of the device. Why don't add a test
>>>> on flag promiscuous directly in ixgbe_vlan_filter_enable? Or do it on
>>>> each
>>>> call, if we want to allow a device in promiscuous
>>>> mode to enable this feature.
>>>>
>>>> What do you think?
>>>>
>>>>         
>>> I can believe that there are paths that lead to this not working
>>> correctly.  That was actually my larger point: this is something that
>>> is commonly not implemented correctly in drivers.  Rather than try to
>>> study every driver my goal is to just avoid the problem completely by
>>> handling vlan acceleration centrally in the networking core.  I sent
>>> out an RFC patch series a few days ago that should solve this problem:
>>>
>>> http://marc.info/?l=linux-netdev&m=128700022614170&w=3
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>       
>> Thank you, I'm going to check these patches and try to apply them in our
>> kernel.
>>     
>
> An updated set of patches has been merged into net-2.6, so you might
> want to try that instead.
>   
Ok I will, thank you.

-- 
Guillaume Gaudonville
6WIND
Software Engineer

Tel: +33 1 39 30 92 63
Mob: +33 6 47 85 34 33
Fax: +33 1 39 30 92 11
guillaume.gaudonville@6wind.com
www.6wind.com
Join the Multicore Packet Processing Forum: www.multicorepacketprocessing.com

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues

This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.