[PATCH net 0/2] net: dev_queue_xmit_nit fixes

[PATCH net 0/2] net: dev_queue_xmit_nit fixes

[Daniel Borkmann]

First patch is a fix for the netdev discussion ``PROBLEM: Software injected
vlan tagged packets are unable to be identified using recent BPF
modifications'' and the second one I spotted while doing the first fix.

Daniel Borkmann (2):
  net: dev_queue_xmit_nit: fix skb->vlan_tci field value
  net: dev_queue_xmit_nit: fix potential NULL ptr dereference

 net/core/dev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
1.7.11.7

[PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Daniel Borkmann]

VLAN packets that are locally injected through taps will loose their
skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
meta data is used in Linux socket filtering for VLANs. Tested with a
VLAN ancillary ops filter.

Patch is based on a previous version by Jiri Pirko.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ani Sinha <ani@aristanetworks.com>
Cc: Jiri Pirko <jpirko@redhat.com>
Reported-by: Paul Pearce <pearce@cs.berkeley.edu>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/core/dev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 515473e..723dcd0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1775,6 +1775,19 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 	struct packet_type *ptype;
 	struct sk_buff *skb2 = NULL;
 	struct packet_type *pt_prev = NULL;
+	struct ethhdr *ehdr;
+
+	/* Network taps could make use of skb->vlan_tci, which got wiped
+	 * out. Hence, we need to reset it correctly.
+	 */
+	skb_reset_mac_header(skb);
+	ehdr = eth_hdr(skb);
+
+	if (ehdr->h_proto == __constant_htons(ETH_P_8021Q)) {
+		skb2 = vlan_untag(skb);
+		if (likely(skb2))
+			skb = skb2;
+	}
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
-- 
1.7.11.7

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

Agreed with the fix. This fixes the issue introduced by this change :l


On Tue, Jan 8, 2013 at 10:51 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> VLAN packets that are locally injected through taps will loose their
> skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
> back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
> meta data is used in Linux socket filtering for VLANs. Tested with a
> VLAN ancillary ops filter.
>
> Patch is based on a previous version by Jiri Pirko.
>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Ani Sinha <ani@aristanetworks.com>
> Cc: Jiri Pirko <jpirko@redhat.com>
> Reported-by: Paul Pearce <pearce@cs.berkeley.edu>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Ani Sinha <ani@aristanetworks.com>

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Eric Dumazet]

On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
> VLAN packets that are locally injected through taps will loose their
> skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
> back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
> meta data is used in Linux socket filtering for VLANs. Tested with a
> VLAN ancillary ops filter.
> 
> Patch is based on a previous version by Jiri Pirko.
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Ani Sinha <ani@aristanetworks.com>
> Cc: Jiri Pirko <jpirko@redhat.com>
> Reported-by: Paul Pearce <pearce@cs.berkeley.edu>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/core/dev.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 515473e..723dcd0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1775,6 +1775,19 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  	struct packet_type *ptype;
>  	struct sk_buff *skb2 = NULL;
>  	struct packet_type *pt_prev = NULL;
> +	struct ethhdr *ehdr;
> +
> +	/* Network taps could make use of skb->vlan_tci, which got wiped
> +	 * out. Hence, we need to reset it correctly.
> +	 */
> +	skb_reset_mac_header(skb);
> +	ehdr = eth_hdr(skb);
> +
> +	if (ehdr->h_proto == __constant_htons(ETH_P_8021Q)) {
> +		skb2 = vlan_untag(skb);
> +		if (likely(skb2))
> +			skb = skb2;
> +	}
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {

This patch is wrong (it adds a leak), and not needed.

If a packet has no vlan_tci, its for a good reason.

We want sniffer see the packet content as is.

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Jiri Pirko]

Tue, Jan 08, 2013 at 09:04:52PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
>> VLAN packets that are locally injected through taps will loose their
>> skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
>> back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
>> meta data is used in Linux socket filtering for VLANs. Tested with a
>> VLAN ancillary ops filter.
>> 
>> Patch is based on a previous version by Jiri Pirko.
>> 
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: Ani Sinha <ani@aristanetworks.com>
>> Cc: Jiri Pirko <jpirko@redhat.com>
>> Reported-by: Paul Pearce <pearce@cs.berkeley.edu>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>  net/core/dev.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 515473e..723dcd0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1775,6 +1775,19 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>>  	struct packet_type *ptype;
>>  	struct sk_buff *skb2 = NULL;
>>  	struct packet_type *pt_prev = NULL;
>> +	struct ethhdr *ehdr;
>> +
>> +	/* Network taps could make use of skb->vlan_tci, which got wiped
>> +	 * out. Hence, we need to reset it correctly.
>> +	 */
>> +	skb_reset_mac_header(skb);
>> +	ehdr = eth_hdr(skb);
>> +
>> +	if (ehdr->h_proto == __constant_htons(ETH_P_8021Q)) {
>> +		skb2 = vlan_untag(skb);
>> +		if (likely(skb2))
>> +			skb = skb2;
>> +	}
>>  
>>  	rcu_read_lock();
>>  	list_for_each_entry_rcu(ptype, &ptype_all, list) {
>
>This patch is wrong (it adds a leak), and not needed.
>
>If a packet has no vlan_tci, its for a good reason.
>
>We want sniffer see the packet content as is.


The issue is that for exmaple in af_packet the function packet_rcv()
expects vlan_tci to be filled out as that is on RX path ensured by
__netif_receive_skb(). However on TX path, dev_queue_xmit_nit() is
called with vlan_tci cleaned in case the device does not have TX vlan
accel enabled. This patch is trying to fix this difference.

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

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Eric Dumazet]

On Tue, 2013-01-08 at 21:22 +0100, Jiri Pirko wrote:

> 
> The issue is that for exmaple in af_packet the function packet_rcv()
> expects vlan_tci to be filled out as that is on RX path ensured by
> __netif_receive_skb(). However on TX path, dev_queue_xmit_nit() is
> called with vlan_tci cleaned in case the device does not have TX vlan
> accel enabled. This patch is trying to fix this difference.

I perfectly understood that, and I repeat :

I want to see the difference.

A filter cannot expect skb->vlan_tci is set for all packets.

If a device doesn't have TX vlan accel, vlan_tci is 0.

packet sniffing is already slow, we don't want to force an extra copy
with vlan_untag() killer.

If you want to fix/extend af_packet, do this in af_packet.

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Jiri Pirko]

Tue, Jan 08, 2013 at 07:51:32PM CET, dborkman@redhat.com wrote:
>VLAN packets that are locally injected through taps will loose their
>skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
>back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
>meta data is used in Linux socket filtering for VLANs. Tested with a
>VLAN ancillary ops filter.
>
>Patch is based on a previous version by Jiri Pirko.
>
>Cc: Eric Dumazet <eric.dumazet@gmail.com>
>Cc: Ani Sinha <ani@aristanetworks.com>
>Cc: Jiri Pirko <jpirko@redhat.com>
>Reported-by: Paul Pearce <pearce@cs.berkeley.edu>
>Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>---
> net/core/dev.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 515473e..723dcd0 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -1775,6 +1775,19 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
> 	struct packet_type *ptype;
> 	struct sk_buff *skb2 = NULL;
> 	struct packet_type *pt_prev = NULL;
>+	struct ethhdr *ehdr;
>+
>+	/* Network taps could make use of skb->vlan_tci, which got wiped
>+	 * out. Hence, we need to reset it correctly.
>+	 */
>+	skb_reset_mac_header(skb);
>+	ehdr = eth_hdr(skb);
>+
>+	if (ehdr->h_proto == __constant_htons(ETH_P_8021Q)) {
>+		skb2 = vlan_untag(skb);
>+		if (likely(skb2))
>+			skb = skb2;
>+	}

	Hmm, nitpick, I think that better would be to do:
		skb = vlan_untag(skb);
		if (unlikely(!skb))
			return;
	
	I believe that better is to deliver skbs in consistent way and
	to do not deliver at all in case of -ENOMEM

[PATCH net 2/2] net: dev_queue_xmit_nit: fix potential NULL ptr dereference

[Daniel Borkmann]

Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
skb->users instead of skb_clone()'') introduced a skb_clone in
dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
be injected into pt_prev->func().

Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 723dcd0..6c35c33 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1827,7 +1827,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 			pt_prev = ptype;
 		}
 	}
-	if (pt_prev)
+	if (skb2 && pt_prev)
 		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
 	rcu_read_unlock();
 }
-- 
1.7.11.7

Re: [PATCH net 2/2] net: dev_queue_xmit_nit: fix potential NULL ptr dereference

[Eric Dumazet]

On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
> Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
> skb->users instead of skb_clone()'') introduced a skb_clone in
> dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
> be injected into pt_prev->func().
> 
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 723dcd0..6c35c33 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1827,7 +1827,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  			pt_prev = ptype;
>  		}
>  	}
> -	if (pt_prev)
> +	if (skb2 && pt_prev)
>  		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>  	rcu_read_unlock();
>  }

My opinion is this patch is not needed.

pt_prev can be set only if skb2 is not NULL

Re: [PATCH net 2/2] net: dev_queue_xmit_nit: fix potential NULL ptr dereference

[Daniel Borkmann]

On 01/08/2013 08:22 PM, Eric Dumazet wrote:
> On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
>> Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
>> skb->users instead of skb_clone()'') introduced a skb_clone in
>> dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
>> be injected into pt_prev->func().
>>
>> Cc: Changli Gao <xiaosuo@gmail.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   net/core/dev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 723dcd0..6c35c33 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1827,7 +1827,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>>   			pt_prev = ptype;
>>   		}
>>   	}
>> -	if (pt_prev)
>> +	if (skb2 && pt_prev)
>>   		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>>   	rcu_read_unlock();
>>   }
>
> My opinion is this patch is not needed.
>
> pt_prev can be set only if skb2 is not NULL

Agreed, I missed that, thanks.

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Paul Pearce]

On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
> VLAN packets that are locally injected through taps will loose their
> skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
> back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
> meta data is used in Linux socket filtering for VLANs. Tested with a
> VLAN ancillary ops filter.
>
> Patch is based on a previous version by Jiri Pirko.

I think there may be issues with the patch beyond Eric's comments. It
seems to trash packet contents.

I applied this patch to Fedora flavored kernel 3.6.11-1.fc16.x86_64.
vlan tagged packets injected via libpcap's pcap_inject() came out
mangled at the packet filters.

The following injected packet:

01:01:01:01:01:01 > 02:02:02:02:02:02, ethertype 802.1Q (0x8100),
length 64: vlan 99, p 0, ethertype ARP, Request who-has 192.168.0.1
tell 192.168.0.1, length 46
0x0000:  0202 0202 0202 0101 0101 0101 8100 0063
0x0010:  0806 0001 0800 0604 0001 0025 6438 8afc
0x0020:  c0a8 0001 0000 0000 0000 c0a8 0001 0000
0x0030:  0000 0000 0000 0000 0000 0000 0000 0000

Arrived as this:

01:01:81:00:00:63 > 02:02:01:01:01:01, ethertype 802.1Q (0x8100),
length 64: vlan 514, p 0, ethertype ARP, Request who-has 192.168.0.1
tell 192.168.0.1, length 46
0x0000:  0202 0101 0101 0101 8100 0063 8100 0202
0x0010:  0806 0001 0800 0604 0001 0025 6438 8afc
0x0020:  c0a8 0001 0000 0000 0000 c0a8 0001 0000
0x0030:  0000 0000 0000 0000 0000 0000 0000 0000

It also might be worth noting the modified libpcap is able to identify
this packet with the filter "vlan" or "vlan 514". Prior to this kernel
patch such a packet could not be identified with any vlan filter.

If this isn't a problem with the patch, perhaps I'm missing a
necessary post-3.6.11 patch?

Thoughts?

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

On Tue, Jan 8, 2013 at 9:15 PM, Paul Pearce <pearce@cs.berkeley.edu> wrote:
> On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
>> VLAN packets that are locally injected through taps will loose their
>> skb->vlan_tci value when they pass dev_hard_start_xmit and get looped
>> back to a packet sniffer via dev_queue_xmit_nit. Besides others, this
>> meta data is used in Linux socket filtering for VLANs. Tested with a
>> VLAN ancillary ops filter.
>>
>> Patch is based on a previous version by Jiri Pirko.
>
> I think there may be issues with the patch beyond Eric's comments. It
> seems to trash packet contents.
>
> I applied this patch to Fedora flavored kernel 3.6.11-1.fc16.x86_64.
> vlan tagged packets injected via libpcap's pcap_inject() came out
> mangled at the packet filters.
>

The proposed patch tries to fix the issue that arose after the
following commit :

commit b40863c667c16b7a73d4f034a8eab67029b5b15a
Author: Eric Dumazet <edumazet@google.com>
Date:   Tue Sep 18 20:44:49 2012 +0000

    net: more accurate network taps in transmit path


I do not believe 3.6.11 kernel has this change. 3.6.11 should not need
the patch.

ani

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Eric Dumazet]

On Tue, 2013-01-08 at 22:06 -0800, Ani Sinha wrote:

> The proposed patch tries to fix the issue that arose after the
> following commit :
> 
> commit b40863c667c16b7a73d4f034a8eab67029b5b15a
> Author: Eric Dumazet <edumazet@google.com>
> Date:   Tue Sep 18 20:44:49 2012 +0000
> 
>     net: more accurate network taps in transmit path
> 
> 
> I do not believe 3.6.11 kernel has this change. 3.6.11 should not need
> the patch.

Thats irrelevant. This only shows that user land was depending on a
prior undocumented behavior.

It seems a libpcap issue to me. Kernel side provides all needed bits.

When I want "tcpdump src port 2030", filter is :

(000) ldh      [12]
(001) jeq      #0x86dd          jt 2	jf 8
(002) ldb      [20]
(003) jeq      #0x84            jt 6	jf 4
(004) jeq      #0x6             jt 6	jf 5
(005) jeq      #0x11            jt 6	jf 19
(006) ldh      [54]
(007) jeq      #0x7ee           jt 18	jf 19
(008) jeq      #0x800           jt 9	jf 19
(009) ldb      [23]
(010) jeq      #0x84            jt 13	jf 11
(011) jeq      #0x6             jt 13	jf 12
(012) jeq      #0x11            jt 13	jf 19
(013) ldh      [20]
(014) jset     #0x1fff          jt 19	jf 15
(015) ldxb     4*([14]&0xf)
(016) ldh      [x + 14]
(017) jeq      #0x7ee           jt 18	jf 19
(018) ret      #96
(019) ret      #0

See how it handles both IPv4 and IPv6, and various protocols
automatically ?

If I only wanted "udp and src port 2030" it would give :

(000) ldh      [12]
(001) jeq      #0x86dd          jt 2	jf 6
(002) ldb      [20]
(003) jeq      #0x11            jt 4	jf 15
(004) ldh      [54]
(005) jeq      #0x7ee           jt 14	jf 15
(006) jeq      #0x800           jt 7	jf 15
(007) ldb      [23]
(008) jeq      #0x11            jt 9	jf 15
(009) ldh      [20]
(010) jset     #0x1fff          jt 15	jf 11
(011) ldxb     4*([14]&0xf)
(012) ldh      [x + 14]
(013) jeq      #0x7ee           jt 14	jf 15
(014) ret      #96
(015) ret      #0



So when I want "tcpdump vlan 100" it generates :

(000) ldh      [12]
(001) jeq      #0x8100          jt 2	jf 6
(002) ldh      [14]
(003) and      #0xfff
(004) jeq      #0x64            jt 5	jf 6
(005) ret      #96
(006) ret      #0

What's wrong instructing libpcap to extend the filter to be able to 
get the correct result, vlan id being in skb->vlan_id (vlan accel on),
or in the packet itself (vlan accel off)

This way, you could chose if you want to get only accelerated vlan,
or non accelerated vlan, or both. And you need no kernel hacking.

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

+tcpdump-workers


On Tue, Jan 8, 2013 at 10:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2013-01-08 at 22:06 -0800, Ani Sinha wrote:
>
>> The proposed patch tries to fix the issue that arose after the
>> following commit :
>>
>> commit b40863c667c16b7a73d4f034a8eab67029b5b15a
>> Author: Eric Dumazet <edumazet@google.com>
>> Date:   Tue Sep 18 20:44:49 2012 +0000
>>
>>     net: more accurate network taps in transmit path
>>
>>
>> I do not believe 3.6.11 kernel has this change. 3.6.11 should not need
>> the patch.
>
> Thats irrelevant. This only shows that user land was depending on a
> prior undocumented behavior.
>
> It seems a libpcap issue to me. Kernel side provides all needed bits.
>
> When I want "tcpdump src port 2030", filter is :
>
> (000) ldh      [12]
> (001) jeq      #0x86dd          jt 2    jf 8
> (002) ldb      [20]
> (003) jeq      #0x84            jt 6    jf 4
> (004) jeq      #0x6             jt 6    jf 5
> (005) jeq      #0x11            jt 6    jf 19
> (006) ldh      [54]
> (007) jeq      #0x7ee           jt 18   jf 19
> (008) jeq      #0x800           jt 9    jf 19
> (009) ldb      [23]
> (010) jeq      #0x84            jt 13   jf 11
> (011) jeq      #0x6             jt 13   jf 12
> (012) jeq      #0x11            jt 13   jf 19
> (013) ldh      [20]
> (014) jset     #0x1fff          jt 19   jf 15
> (015) ldxb     4*([14]&0xf)
> (016) ldh      [x + 14]
> (017) jeq      #0x7ee           jt 18   jf 19
> (018) ret      #96
> (019) ret      #0
>
> See how it handles both IPv4 and IPv6, and various protocols
> automatically ?
>
> If I only wanted "udp and src port 2030" it would give :
>
> (000) ldh      [12]
> (001) jeq      #0x86dd          jt 2    jf 6
> (002) ldb      [20]
> (003) jeq      #0x11            jt 4    jf 15
> (004) ldh      [54]
> (005) jeq      #0x7ee           jt 14   jf 15
> (006) jeq      #0x800           jt 7    jf 15
> (007) ldb      [23]
> (008) jeq      #0x11            jt 9    jf 15
> (009) ldh      [20]
> (010) jset     #0x1fff          jt 15   jf 11
> (011) ldxb     4*([14]&0xf)
> (012) ldh      [x + 14]
> (013) jeq      #0x7ee           jt 14   jf 15
> (014) ret      #96
> (015) ret      #0
>
>
>
> So when I want "tcpdump vlan 100" it generates :
>
> (000) ldh      [12]
> (001) jeq      #0x8100          jt 2    jf 6
> (002) ldh      [14]
> (003) and      #0xfff
> (004) jeq      #0x64            jt 5    jf 6
> (005) ret      #96
> (006) ret      #0
>
> What's wrong instructing libpcap to extend the filter to be able to
> get the correct result, vlan id being in skb->vlan_id (vlan accel on),
> or in the packet itself (vlan accel off)
>
> This way, you could chose if you want to get only accelerated vlan,
> or non accelerated vlan, or both. And you need no kernel hacking.
>
>
>

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

>> Thats irrelevant. This only shows that user land was depending on a
>> prior undocumented behavior.

Why do you say that? The following patch from Pirko ensured that on
both RX and TX regardless whether the driver/hw supported vlan
acceleration, the outermost vlan tags will always be extracted out of
the packet and put in skb aux data :

commit bcc6d47903612c3861201cc3a866fb604f26b8b2
Author: Jiri Pirko <jpirko@redhat.com>
Date:   Thu Apr 7 19:48:33 2011 +0000

    net: vlan: make non-hw-accel rx path similar to hw-accel

Now this meant that the filter code should always look into the aux
data for vlan tagging, not in the packet, regardless of hw
acceleration availability. Your patch
b40863c667c16b7a73d4f034a8eab67029b5b15a broke this symmetric
semantics - now on TX on the network tap, we do not have the vlan tags
in the skb aux data.

In my opinion, for a given kernel, the filter code should either look
into the packet offset or in the packet aux data for vlan tags but not
both. Otherwise the filter code becomes incredibly complex since an
inline vlan tag in the packet changes offsets of all headers coming
afterwords and I don't even know if filter code can be correctly
generated in this case. tcpdump-workers folks are CC's here and they
clearly have more experience with libpcap filter code that I do. Hence
I leave it up to them to provide inputs here.

>> What's wrong instructing libpcap to extend the filter to be able to
>> get the correct result, vlan id being in skb->vlan_id (vlan accel on),
>> or in the packet itself (vlan accel off)
>>
>> This way, you could chose if you want to get only accelerated vlan,
>> or non accelerated vlan, or both. And you need no kernel hacking.

This is wrong. Accelerated or not, the kernel code was organized to
have the tags in the packet aux data. So I think this is how user land
should be coded as well.

ani
_______________________________________________
tcpdump-workers mailing list
tcpdump-workers@lists.tcpdump.org
https://lists.sandelman.ca/mailman/listinfo/tcpdump-workers

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Eric Dumazet]

On Wed, 2013-01-09 at 11:27 -0800, Ani Sinha wrote:

> This is wrong. Accelerated or not, the kernel code was organized to
> have the tags in the packet aux data. So I think this is how user land
> should be coded as well.

You have your opinion, thats good.

My opinion as a kernel developer is that the network tap is here to have
a copy of the exact frame given to the _device_.

Because in the end, users will complain to netdev, giving us tcpdump
traces. And if these traces have nothing to do with what is given to the
device, they are almost useless.

If you want other taps, and catch frames before/after various netfilter
hooks, segmentations, vlan accel, tunnels, or before GRO layer, thats a
totally different request.

A packet can be modified by a lot of layers in the kernel.

And yes, BPF filters can be incredibly complex, but it appears kernel is
not a piece of cake.

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

On Wed, Jan 9, 2013 at 11:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-01-09 at 11:27 -0800, Ani Sinha wrote:
>
>> This is wrong. Accelerated or not, the kernel code was organized to
>> have the tags in the packet aux data. So I think this is how user land
>> should be coded as well.
>
> You have your opinion, thats good.
>
> My opinion as a kernel developer is that the network tap is here to have
> a copy of the exact frame given to the _device_.
>

It is fine by me if that is how you see it. In that case. the
behaviour can me made symmetric on both TX and RX. Tap processing in
__netif_receive_skb() can be done before vlan_untag() so that taps see
the exact frame received from the _device_ as you put it.

ani

Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value

[Ani Sinha]

On Wed, Jan 9, 2013 at 12:01 PM, Ani Sinha <ani@aristanetworks.com> wrote:
> On Wed, Jan 9, 2013 at 11:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Wed, 2013-01-09 at 11:27 -0800, Ani Sinha wrote:
>>
>>> This is wrong. Accelerated or not, the kernel code was organized to
>>> have the tags in the packet aux data. So I think this is how user land
>>> should be coded as well.
>>
>> You have your opinion, thats good.
>>
>> My opinion as a kernel developer is that the network tap is here to have
>> a copy of the exact frame given to the _device_.
>>
>
> It is fine by me if that is how you see it. In that case. the
> behaviour can me made symmetric on both TX and RX. Tap processing in
> __netif_receive_skb() can be done before vlan_untag() so that taps see
> the exact frame received from the _device_ as you put it.

Although for accelerated vlan tags, it will be in the meta data
anyways. All I am asking is, let's have the same behaviour on both TX
and RX. If  the tag in the packet let's have it that way in both ways
in what the tap sees.