* Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value
2013-01-08 18:51 ` [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value Daniel Borkmann
@ 2013-01-08 19:54 ` Ani Sinha
2013-01-08 20:04 ` Eric Dumazet
2013-01-08 20:14 ` Jiri Pirko
2 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2013-01-08 19:54 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, netdev, Eric Dumazet, Jiri Pirko
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value
2013-01-08 18:51 ` [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value Daniel Borkmann
2013-01-08 19:54 ` Ani Sinha
@ 2013-01-08 20:04 ` Eric Dumazet
2013-01-08 20:22 ` Jiri Pirko
2013-01-08 20:14 ` Jiri Pirko
2 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2013-01-08 20:04 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, netdev, Ani Sinha, Jiri Pirko
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value
2013-01-08 20:04 ` Eric Dumazet
@ 2013-01-08 20:22 ` Jiri Pirko
2013-01-08 20:42 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Pirko @ 2013-01-08 20:22 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Daniel Borkmann, David Miller, netdev, Ani Sinha, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value
2013-01-08 20:22 ` Jiri Pirko
@ 2013-01-08 20:42 ` Eric Dumazet
0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2013-01-08 20:42 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Daniel Borkmann, David Miller, netdev, Ani Sinha, Jiri Pirko
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value
2013-01-08 18:51 ` [PATCH net 1/2] net: dev_queue_xmit_nit: fix skb->vlan_tci field value Daniel Borkmann
2013-01-08 19:54 ` Ani Sinha
2013-01-08 20:04 ` Eric Dumazet
@ 2013-01-08 20:14 ` Jiri Pirko
2 siblings, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2013-01-08 20:14 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, netdev, Eric Dumazet, Ani Sinha, 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
^ permalink raw reply [flat|nested] 10+ messages in thread