Re: linux-next: Tree for Aug 7

Re: linux-next: Tree for Aug 7

[Sedat Dilek]

On Wed, Aug 7, 2013 at 7:54 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> Changes since 20130806:
>
> The ext4 tree lost its build failure.
>
> The mvebu tree gained a build failure so I used the version from
> next-20130806.
>
> The akpm tree gained conflicts against the ext4 tree.
>
> ----------------------------------------------------------------------------
>

[ CC some netdev and wireless folks ]

Yesterday, I discovered an issue with net-next.
The patch in [1] fixed the problems in my network/wifi environment.
Hannes confirmed that virtio_net are solved, too.
Today's next-20130807 still needs it for affected people.

- Sedat -

[1] http://marc.info/?l=linux-netdev&m=137582524017840&w=2
[2] http://marc.info/?l=linux-netdev&m=137583048219416&w=2
[3] http://marc.info/?t=137579712800008&r=1&w=2

Re: linux-next: Tree for Aug 7

[Phil Sutter]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Wed, Aug 07, 2013 at 10:29:18AM +0200, Sedat Dilek wrote:
> On Wed, Aug 7, 2013 at 7:54 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi all,
> >
> > Changes since 20130806:
> >
> > The ext4 tree lost its build failure.
> >
> > The mvebu tree gained a build failure so I used the version from
> > next-20130806.
> >
> > The akpm tree gained conflicts against the ext4 tree.
> >
> > ----------------------------------------------------------------------------
> >
> 
> [ CC some netdev and wireless folks ]
> 
> Yesterday, I discovered an issue with net-next.
> The patch in [1] fixed the problems in my network/wifi environment.
> Hannes confirmed that virtio_net are solved, too.
> Today's next-20130807 still needs it for affected people.
> 
> - Sedat -
> 
> [1] http://marc.info/?l=linux-netdev&m=137582524017840&w=2
> [2] http://marc.info/?l=linux-netdev&m=137583048219416&w=2
> [3] http://marc.info/?t=137579712800008&r=1&w=2

Could you please try the attached patch. It limits parsing the ethernet
header (by calling eth_type_trans()) to cases when the configured
protocol is ETH_P_ALL, so at least for 802.1X this should fix the
problem.

The idea behind this patch is that users setting the protocol to
something else probably do know better and so should be left alone.

Best wishes, Phil

[-- Attachment #2: af_packet_fixup.diff --]
[-- Type: text/plain, Size: 1606 bytes --]

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index bbe1ece..66bc79c 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1932,8 +1932,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 
 	ph.raw = frame;
 
-	skb->protocol = proto;
-	skb->dev = dev;
 	skb->priority = po->sk.sk_priority;
 	skb->mark = po->sk.sk_mark;
 	sock_tx_timestamp(&po->sk, &skb_shinfo(skb)->tx_flags);
@@ -2002,13 +2000,18 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err))
 			return err;
 
-		if (dev->type == ARPHRD_ETHER)
-			skb->protocol = eth_type_trans(skb, dev);
-
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
 	}
 
+	if (dev->type == ARPHRD_ETHER &&
+	    proto = htons(ETH_P_ALL)) {
+		skb->protocol = eth_type_trans(skb, dev);
+	} else {
+		skb->protocol = proto;
+		skb->dev = dev;
+	}
+
 	max_frame_len = dev->mtu + dev->hard_header_len;
 	if (skb->protocol == htons(ETH_P_8021Q))
 		max_frame_len += VLAN_HLEN;
@@ -2331,15 +2334,17 @@ static int packet_snd(struct socket *sock,
 
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
-	if (dev->type == ARPHRD_ETHER) {
+	if (dev->type == ARPHRD_ETHER &&
+	    proto == htons(ETH_P_ALL)) {
 		skb->protocol = eth_type_trans(skb, dev);
-		if (skb->protocol == htons(ETH_P_8021Q))
-			reserve += VLAN_HLEN;
 	} else {
 		skb->protocol = proto;
 		skb->dev = dev;
 	}
 
+	if (skb->protocol == htons(ETH_P_8021Q))
+		reserve += VLAN_HLEN;
+
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
 		err = -EMSGSIZE;
 		goto out_free;

Re: linux-next: Tree for Aug 7

[Johannes Berg]

On Wed, 2013-08-07 at 17:59 +0200, Phil Sutter wrote:

> The idea behind this patch is that users setting the protocol to
> something else probably do know better and so should be left alone.

Regardless of that, I think that still the skb pointers would be changed
by this patch which would confuse the receiver of the SKB (device
driver), no? Has anyone verified that theory? :)

johannes

Re: linux-next: Tree for Aug 7

[Eric Dumazet]

On Wed, 2013-08-07 at 18:12 +0200, Johannes Berg wrote:
> On Wed, 2013-08-07 at 17:59 +0200, Phil Sutter wrote:
> 
> > The idea behind this patch is that users setting the protocol to
> > something else probably do know better and so should be left alone.
> 
> Regardless of that, I think that still the skb pointers would be changed
> by this patch which would confuse the receiver of the SKB (device
> driver), no? Has anyone verified that theory? :)

Maybe receivers made wrong assumptions about some headers being set or
not set ?

A patch can uncover prior bugs.

commit 76fe45812a3b134c3917 is an example of a fix we had to do because
of another fix ;)

Re: linux-next: Tree for Aug 7

[Johannes Berg]

On Wed, 2013-08-07 at 09:17 -0700, Eric Dumazet wrote:
> On Wed, 2013-08-07 at 18:12 +0200, Johannes Berg wrote:
> > On Wed, 2013-08-07 at 17:59 +0200, Phil Sutter wrote:
> > 
> > > The idea behind this patch is that users setting the protocol to
> > > something else probably do know better and so should be left alone.
> > 
> > Regardless of that, I think that still the skb pointers would be changed
> > by this patch which would confuse the receiver of the SKB (device
> > driver), no? Has anyone verified that theory? :)
> 
> Maybe receivers made wrong assumptions about some headers being set or
> not set ?

Maybe. I haven't tested it, but I'm thinking that skb->data doesn't
point to the start of the data frame in this case, since we now call
eth_type_trans() which pulls the ethernet header. So if the device just
transmits skb->len starting from skb->data, it'll be wrong, no? That
seems a basic assumption though.

johannes

Re: linux-next: Tree for Aug 7

[Eric Dumazet]

On Wed, 2013-08-07 at 18:22 +0200, Johannes Berg wrote:

> Maybe. I haven't tested it, but I'm thinking that skb->data doesn't
> point to the start of the data frame in this case, since we now call
> eth_type_trans() which pulls the ethernet header. So if the device just
> transmits skb->len starting from skb->data, it'll be wrong, no? That
> seems a basic assumption though.

Yes, it seems calling eth_type_trans() is not right here, and even could
crash.

Sorry, for being vague, I am a bit busy this morning.

Re: linux-next: Tree for Aug 7

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 07 Aug 2013 09:40:09 -0700

> On Wed, 2013-08-07 at 18:22 +0200, Johannes Berg wrote:
> 
>> Maybe. I haven't tested it, but I'm thinking that skb->data doesn't
>> point to the start of the data frame in this case, since we now call
>> eth_type_trans() which pulls the ethernet header. So if the device just
>> transmits skb->len starting from skb->data, it'll be wrong, no? That
>> seems a basic assumption though.
> 
> Yes, it seems calling eth_type_trans() is not right here, and even could
> crash.
> 
> Sorry, for being vague, I am a bit busy this morning.

Yes, this is absolutely the core problem, you absolute cannot
call eth_type_trans() on the output path, it pulls off the
ethernet header from the packet.  That can't possibly work.

I want a real fix submitted formally for this problem immediately,
or else I'm reverting all of these changes this afternoon.

Thanks.

Re: linux-next: Tree for Aug 7

[Phil Sutter]

On Wed, Aug 07, 2013 at 10:47:13AM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 07 Aug 2013 09:40:09 -0700
> 
> > On Wed, 2013-08-07 at 18:22 +0200, Johannes Berg wrote:
> > 
> >> Maybe. I haven't tested it, but I'm thinking that skb->data doesn't
> >> point to the start of the data frame in this case, since we now call
> >> eth_type_trans() which pulls the ethernet header. So if the device just
> >> transmits skb->len starting from skb->data, it'll be wrong, no? That
> >> seems a basic assumption though.
> > 
> > Yes, it seems calling eth_type_trans() is not right here, and even could
> > crash.
> > 
> > Sorry, for being vague, I am a bit busy this morning.
> 
> Yes, this is absolutely the core problem, you absolute cannot
> call eth_type_trans() on the output path, it pulls off the
> ethernet header from the packet.  That can't possibly work.
> 
> I want a real fix submitted formally for this problem immediately,
> or else I'm reverting all of these changes this afternoon.

One could simply call skb_push(skb, ETH_HLEN) right after calling
eth_type_trans(skb, dev) in order to undo the 'data' and 'len'
adjustment. Not sure if this kind of hack is the right way to go here,
or if the whole af_packet parses ethernet header discussion should be
opened again instead.

Best wishes, Phil

Re: linux-next: Tree for Aug 7

[David Miller]

From: Phil Sutter <phil@nwl.cc>
Date: Wed, 7 Aug 2013 20:37:58 +0200

> One could simply call skb_push(skb, ETH_HLEN) right after calling
> eth_type_trans(skb, dev) in order to undo the 'data' and 'len'
> adjustment. Not sure if this kind of hack is the right way to go here,
> or if the whole af_packet parses ethernet header discussion should be
> opened again instead.

That's completely pointless work.

Without that header pull, the only two things left that
eth_type_trans() does is set the skb->protocol field and
set skb->dev.

And even the latter has to be done already in an else
branch in the suspect AF_PACKET code.

So this eth_type_trans() call is 2/3 duplicate or unnecessary work,
it's the completely the wrong thing to do.

Look, I'm going to fix this myself, because I'm pretty tired of
waiting for the obvious fix.

Re: linux-next: Tree for Aug 7

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)

> Look, I'm going to fix this myself, because I'm pretty tired of
> waiting for the obvious fix.

Someone please test this:

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index c623861..afc02a6 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -29,6 +29,7 @@
 
 #ifdef __KERNEL__
 extern __be16		eth_type_trans(struct sk_buff *skb, struct net_device *dev);
+extern __be16		__eth_type_trans(struct sk_buff *skb, struct net_device *dev);
 extern const struct header_ops eth_header_ops;
 
 extern int eth_header(struct sk_buff *skb, struct net_device *dev,
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index be1f64d..35dc1be 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
 EXPORT_SYMBOL(eth_rebuild_header);
 
 /**
+ * __eth_type_trans - only determine the packet's protocol ID.
+ * @skb: packet
+ * @dev: device
+ */
+__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
+{
+	struct ethhdr *eth = (struct ethhdr *) skb->data;
+
+	/*
+	 * Some variants of DSA tagging don't have an ethertype field
+	 * at all, so we check here whether one of those tagging
+	 * variants has been configured on the receiving interface,
+	 * and if so, set skb->protocol without looking at the packet.
+	 */
+	if (netdev_uses_dsa_tags(dev))
+		return htons(ETH_P_DSA);
+	if (netdev_uses_trailer_tags(dev))
+		return htons(ETH_P_TRAILER);
+
+	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
+		return eth->h_proto;
+
+	/*
+	 *      This is a magic hack to spot IPX packets. Older Novell breaks
+	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
+	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
+	 *      won't work for fault tolerant netware but does for the rest.
+	 */
+	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
+		return htons(ETH_P_802_3);
+
+	/*
+	 *      Real 802.2 LLC
+	 */
+	return htons(ETH_P_802_2);
+}
+EXPORT_SYMBOL(__eth_type_trans);
+
+/**
  * eth_type_trans - determine the packet's protocol ID.
  * @skb: received socket data
  * @dev: receiving network device
@@ -184,33 +223,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 			skb->pkt_type = PACKET_OTHERHOST;
 	}
 
-	/*
-	 * Some variants of DSA tagging don't have an ethertype field
-	 * at all, so we check here whether one of those tagging
-	 * variants has been configured on the receiving interface,
-	 * and if so, set skb->protocol without looking at the packet.
-	 */
-	if (netdev_uses_dsa_tags(dev))
-		return htons(ETH_P_DSA);
-	if (netdev_uses_trailer_tags(dev))
-		return htons(ETH_P_TRAILER);
-
-	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
-		return eth->h_proto;
-
-	/*
-	 *      This is a magic hack to spot IPX packets. Older Novell breaks
-	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
-	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
-	 *      won't work for fault tolerant netware but does for the rest.
-	 */
-	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
-		return htons(ETH_P_802_3);
-
-	/*
-	 *      Real 802.2 LLC
-	 */
-	return htons(ETH_P_802_2);
+	return __eth_type_trans(skb, dev);
 }
 EXPORT_SYMBOL(eth_type_trans);
 
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0c0f6c9..ec8e1c3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2003,7 +2003,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 			return err;
 
 		if (dev->type == ARPHRD_ETHER)
-			skb->protocol = eth_type_trans(skb, dev);
+			skb->protocol = __eth_type_trans(skb, dev);
 
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
@@ -2332,13 +2332,13 @@ static int packet_snd(struct socket *sock,
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
 	if (dev->type == ARPHRD_ETHER) {
-		skb->protocol = eth_type_trans(skb, dev);
+		skb->protocol = __eth_type_trans(skb, dev);
 		if (skb->protocol == htons(ETH_P_8021Q))
 			reserve += VLAN_HLEN;
 	} else {
 		skb->protocol = proto;
-		skb->dev = dev;
 	}
+	skb->dev = dev;
 
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
 		err = -EMSGSIZE;

Re: linux-next: Tree for Aug 7

[Sedat Dilek]

On Thu, Aug 8, 2013 at 1:36 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)
>
>> Look, I'm going to fix this myself, because I'm pretty tired of
>> waiting for the obvious fix.
>
> Someone please test this:
>

Your patch on top of next-20130807 does not fix the issue in my wifi/network.
No working Internet connection (ping etc.).

- Sedat -

> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index c623861..afc02a6 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -29,6 +29,7 @@
>
>  #ifdef __KERNEL__
>  extern __be16          eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> +extern __be16          __eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>  extern const struct header_ops eth_header_ops;
>
>  extern int eth_header(struct sk_buff *skb, struct net_device *dev,
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index be1f64d..35dc1be 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
>  EXPORT_SYMBOL(eth_rebuild_header);
>
>  /**
> + * __eth_type_trans - only determine the packet's protocol ID.
> + * @skb: packet
> + * @dev: device
> + */
> +__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
> +{
> +       struct ethhdr *eth = (struct ethhdr *) skb->data;
> +
> +       /*
> +        * Some variants of DSA tagging don't have an ethertype field
> +        * at all, so we check here whether one of those tagging
> +        * variants has been configured on the receiving interface,
> +        * and if so, set skb->protocol without looking at the packet.
> +        */
> +       if (netdev_uses_dsa_tags(dev))
> +               return htons(ETH_P_DSA);
> +       if (netdev_uses_trailer_tags(dev))
> +               return htons(ETH_P_TRAILER);
> +
> +       if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> +               return eth->h_proto;
> +
> +       /*
> +        *      This is a magic hack to spot IPX packets. Older Novell breaks
> +        *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> +        *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> +        *      won't work for fault tolerant netware but does for the rest.
> +        */
> +       if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> +               return htons(ETH_P_802_3);
> +
> +       /*
> +        *      Real 802.2 LLC
> +        */
> +       return htons(ETH_P_802_2);
> +}
> +EXPORT_SYMBOL(__eth_type_trans);
> +
> +/**
>   * eth_type_trans - determine the packet's protocol ID.
>   * @skb: received socket data
>   * @dev: receiving network device
> @@ -184,33 +223,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>                         skb->pkt_type = PACKET_OTHERHOST;
>         }
>
> -       /*
> -        * Some variants of DSA tagging don't have an ethertype field
> -        * at all, so we check here whether one of those tagging
> -        * variants has been configured on the receiving interface,
> -        * and if so, set skb->protocol without looking at the packet.
> -        */
> -       if (netdev_uses_dsa_tags(dev))
> -               return htons(ETH_P_DSA);
> -       if (netdev_uses_trailer_tags(dev))
> -               return htons(ETH_P_TRAILER);
> -
> -       if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> -               return eth->h_proto;
> -
> -       /*
> -        *      This is a magic hack to spot IPX packets. Older Novell breaks
> -        *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> -        *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> -        *      won't work for fault tolerant netware but does for the rest.
> -        */
> -       if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> -               return htons(ETH_P_802_3);
> -
> -       /*
> -        *      Real 802.2 LLC
> -        */
> -       return htons(ETH_P_802_2);
> +       return __eth_type_trans(skb, dev);
>  }
>  EXPORT_SYMBOL(eth_type_trans);
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 0c0f6c9..ec8e1c3 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2003,7 +2003,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>                         return err;
>
>                 if (dev->type == ARPHRD_ETHER)
> -                       skb->protocol = eth_type_trans(skb, dev);
> +                       skb->protocol = __eth_type_trans(skb, dev);
>
>                 data += dev->hard_header_len;
>                 to_write -= dev->hard_header_len;
> @@ -2332,13 +2332,13 @@ static int packet_snd(struct socket *sock,
>         sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>
>         if (dev->type == ARPHRD_ETHER) {
> -               skb->protocol = eth_type_trans(skb, dev);
> +               skb->protocol = __eth_type_trans(skb, dev);
>                 if (skb->protocol == htons(ETH_P_8021Q))
>                         reserve += VLAN_HLEN;
>         } else {
>                 skb->protocol = proto;
> -               skb->dev = dev;
>         }
> +       skb->dev = dev;
>
>         if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
>                 err = -EMSGSIZE;

Re: linux-next: Tree for Aug 7

[Eric Dumazet]

On Wed, 2013-08-07 at 16:36 -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)
> 
> > Look, I'm going to fix this myself, because I'm pretty tired of
> > waiting for the obvious fix.
> 
> Someone please test this:
> 
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index c623861..afc02a6 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -29,6 +29,7 @@
>  
>  #ifdef __KERNEL__
>  extern __be16		eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> +extern __be16		__eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>  extern const struct header_ops eth_header_ops;
>  
>  extern int eth_header(struct sk_buff *skb, struct net_device *dev,
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index be1f64d..35dc1be 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
>  EXPORT_SYMBOL(eth_rebuild_header);
>  
>  /**
> + * __eth_type_trans - only determine the packet's protocol ID.
> + * @skb: packet
> + * @dev: device
> + */
> +__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
> +{

We probably want an inline here

Or else, we are adding an extra function call in rx fast path.

(At least my compiler did this)


0000000000000470 <eth_type_trans>:
 470:	e8 00 00 00 00       	callq  475 <eth_type_trans+0x5>
			471: R_X86_64_PC32	__fentry__-0x4
 475:	48 8b 8f d0 00 00 00 	mov    0xd0(%rdi),%rcx
 47c:	48 8b 87 c8 00 00 00 	mov    0xc8(%rdi),%rax
 483:	44 8b 47 60          	mov    0x60(%rdi),%r8d
 487:	55                   	push   %rbp
 488:	48 89 77 20          	mov    %rsi,0x20(%rdi)
 48c:	48 89 ca             	mov    %rcx,%rdx
 48f:	48 89 e5             	mov    %rsp,%rbp
 492:	48 29 c2             	sub    %rax,%rdx
 495:	41 83 f8 0d          	cmp    $0xd,%r8d
 499:	66 89 97 b8 00 00 00 	mov    %dx,0xb8(%rdi)
 4a0:	76 19                	jbe    4bb <eth_type_trans+0x4b>
 4a2:	41 83 e8 0e          	sub    $0xe,%r8d
 4a6:	44 3b 47 64          	cmp    0x64(%rdi),%r8d
 4aa:	44 89 47 60          	mov    %r8d,0x60(%rdi)
 4ae:	72 36                	jb     4e6 <eth_type_trans+0x76>
 4b0:	48 83 c1 0e          	add    $0xe,%rcx
 4b4:	48 89 8f d0 00 00 00 	mov    %rcx,0xd0(%rdi)
 4bb:	81 e2 ff ff 00 00    	and    $0xffff,%edx
 4c1:	48 01 d0             	add    %rdx,%rax
 4c4:	f6 00 01             	testb  $0x1,(%rax)
 4c7:	75 2e                	jne    4f7 <eth_type_trans+0x87>
 4c9:	48 8b 96 88 02 00 00 	mov    0x288(%rsi),%rdx
 4d0:	48 8b 00             	mov    (%rax),%rax
 4d3:	48 33 02             	xor    (%rdx),%rax
 4d6:	48 c1 e0 10          	shl    $0x10,%rax
 4da:	48 85 c0             	test   %rax,%rax
 4dd:	75 09                	jne    4e8 <eth_type_trans+0x78>
 4df:	e8 00 00 00 00       	callq  4e4 <eth_type_trans+0x74>
			4e0: R_X86_64_PC32	__eth_type_trans-0x4
 4e4:	5d                   	pop    %rbp
 4e5:	c3                   	retq   
 4e6:	0f 0b                	ud2    
 4e8:	0f b6 47 75          	movzbl 0x75(%rdi),%eax
 4ec:	83 e0 f8             	and    $0xfffffff8,%eax
 4ef:	83 c8 03             	or     $0x3,%eax
 4f2:	88 47 75             	mov    %al,0x75(%rdi)
 4f5:	eb e8                	jmp    4df <eth_type_trans+0x6f>
 4f7:	48 8b 00             	mov    (%rax),%rax
 4fa:	48 33 86 b8 02 00 00 	xor    0x2b8(%rsi),%rax
 501:	48 c1 e0 10          	shl    $0x10,%rax
 505:	48 85 c0             	test   %rax,%rax
 508:	0f b6 47 75          	movzbl 0x75(%rdi),%eax
 50c:	75 0b                	jne    519 <eth_type_trans+0xa9>
 50e:	83 e0 f8             	and    $0xfffffff8,%eax
 511:	83 c8 01             	or     $0x1,%eax
 514:	88 47 75             	mov    %al,0x75(%rdi)
 517:	eb c6                	jmp    4df <eth_type_trans+0x6f>
 519:	83 e0 f8             	and    $0xfffffff8,%eax
 51c:	83 c8 02             	or     $0x2,%eax
 51f:	88 47 75             	mov    %al,0x75(%rdi)
 522:	eb bb                	jmp    4df <eth_type_trans+0x6f>

Re: linux-next: Tree for Aug 7

[David Miller]

From: Sedat Dilek <sedat.dilek@gmail.com>
Date: Thu, 8 Aug 2013 02:02:48 +0200

> On Thu, Aug 8, 2013 at 1:36 AM, David Miller <davem@davemloft.net> wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)
>>
>>> Look, I'm going to fix this myself, because I'm pretty tired of
>>> waiting for the obvious fix.
>>
>> Someone please test this:
> 
> Your patch on top of next-20130807 does not fix the issue in my wifi/network.
> No working Internet connection (ping etc.).

Ok, I'm going to simply revert all of these changes, thanks for testing.

Re: linux-next: Tree for Aug 7

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 07 Aug 2013 17:09:26 -0700 (PDT)

> Ok, I'm going to simply revert all of these changes, thanks for testing.

Here is what I just pushed to net-next:

====================
>From 09effa67a18d893fc4e6f81a3659fc9efef1445e Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Wed, 7 Aug 2013 17:11:00 -0700
Subject: [PATCH] packet: Revert recent header parsing changes.

This reverts commits:

0f75b09c798ed00c30d7d5551b896be883bc2aeb
cbd89acb9eb257ed3b2be867142583fdcf7fdc5b
c483e02614551e44ced3fe6eedda8e36d3277ccc

Amongst other things, it's modifies the SKB header
to pull the ethernet headers off via eth_type_trans()
on the output path which is bogus.

It's causing serious regressions for people.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/packet/af_packet.c |   53 ++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0c0f6c9..4cb28a7 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -88,7 +88,6 @@
 #include <linux/virtio_net.h>
 #include <linux/errqueue.h>
 #include <linux/net_tstamp.h>
-#include <linux/if_arp.h>
 
 #ifdef CONFIG_INET
 #include <net/inet_common.h>
@@ -1924,7 +1923,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		__be16 proto, unsigned char *addr, int hlen)
 {
 	union tpacket_uhdr ph;
-	int to_write, offset, len, tp_len, nr_frags, len_max, max_frame_len;
+	int to_write, offset, len, tp_len, nr_frags, len_max;
 	struct socket *sock = po->sk.sk_socket;
 	struct page *page;
 	void *data;
@@ -1947,6 +1946,10 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		tp_len = ph.h1->tp_len;
 		break;
 	}
+	if (unlikely(tp_len > size_max)) {
+		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
+		return -EMSGSIZE;
+	}
 
 	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
@@ -2002,25 +2005,10 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err))
 			return err;
 
-		if (dev->type == ARPHRD_ETHER)
-			skb->protocol = eth_type_trans(skb, dev);
-
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
 	}
 
-	max_frame_len = dev->mtu + dev->hard_header_len;
-	if (skb->protocol == htons(ETH_P_8021Q))
-		max_frame_len += VLAN_HLEN;
-
-	if (size_max > max_frame_len)
-		size_max = max_frame_len;
-
-	if (unlikely(tp_len > size_max)) {
-		pr_err("packet size is too long (%d > %d)\n", tp_len, size_max);
-		return -EMSGSIZE;
-	}
-
 	offset = offset_in_page(data);
 	len_max = PAGE_SIZE - offset;
 	len = ((to_write > len_max) ? len_max : to_write);
@@ -2059,7 +2047,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	struct net_device *dev;
 	__be16 proto;
 	bool need_rls_dev = false;
-	int err;
+	int err, reserve = 0;
 	void *ph;
 	struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
 	int tp_len, size_max;
@@ -2092,6 +2080,8 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	if (unlikely(dev == NULL))
 		goto out;
 
+	reserve = dev->hard_header_len;
+
 	err = -ENETDOWN;
 	if (unlikely(!(dev->flags & IFF_UP)))
 		goto out_put;
@@ -2099,6 +2089,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	size_max = po->tx_ring.frame_size
 		- (po->tp_hdrlen - sizeof(struct sockaddr_ll));
 
+	if (size_max > dev->mtu + reserve)
+		size_max = dev->mtu + reserve;
+
 	do {
 		ph = packet_current_frame(po, &po->tx_ring,
 				TP_STATUS_SEND_REQUEST);
@@ -2331,20 +2324,22 @@ static int packet_snd(struct socket *sock,
 
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
-	if (dev->type == ARPHRD_ETHER) {
-		skb->protocol = eth_type_trans(skb, dev);
-		if (skb->protocol == htons(ETH_P_8021Q))
-			reserve += VLAN_HLEN;
-	} else {
-		skb->protocol = proto;
-		skb->dev = dev;
-	}
-
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
-		err = -EMSGSIZE;
-		goto out_free;
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * packet in hand.
+		 */
+		struct ethhdr *ehdr;
+		skb_reset_mac_header(skb);
+		ehdr = eth_hdr(skb);
+		if (ehdr->h_proto != htons(ETH_P_8021Q)) {
+			err = -EMSGSIZE;
+			goto out_free;
+		}
 	}
 
+	skb->protocol = proto;
+	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
 
-- 
1.7.10.4

Re: linux-next: Tree for Aug 7

[Phil Sutter]

On Wed, Aug 07, 2013 at 04:36:21PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)
> 
> > Look, I'm going to fix this myself, because I'm pretty tired of
> > waiting for the obvious fix.
> 
> Someone please test this:
> 
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index c623861..afc02a6 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -29,6 +29,7 @@
>  
>  #ifdef __KERNEL__
>  extern __be16		eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> +extern __be16		__eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>  extern const struct header_ops eth_header_ops;
>  
>  extern int eth_header(struct sk_buff *skb, struct net_device *dev,
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index be1f64d..35dc1be 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
>  EXPORT_SYMBOL(eth_rebuild_header);
>  
>  /**
> + * __eth_type_trans - only determine the packet's protocol ID.
> + * @skb: packet
> + * @dev: device
> + */
> +__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct ethhdr *eth = (struct ethhdr *) skb->data;
> +
> +	/*
> +	 * Some variants of DSA tagging don't have an ethertype field
> +	 * at all, so we check here whether one of those tagging
> +	 * variants has been configured on the receiving interface,
> +	 * and if so, set skb->protocol without looking at the packet.
> +	 */
> +	if (netdev_uses_dsa_tags(dev))
> +		return htons(ETH_P_DSA);
> +	if (netdev_uses_trailer_tags(dev))
> +		return htons(ETH_P_TRAILER);
> +
> +	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> +		return eth->h_proto;
> +
> +	/*
> +	 *      This is a magic hack to spot IPX packets. Older Novell breaks
> +	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> +	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> +	 *      won't work for fault tolerant netware but does for the rest.
> +	 */
> +	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> +		return htons(ETH_P_802_3);
> +
> +	/*
> +	 *      Real 802.2 LLC
> +	 */
> +	return htons(ETH_P_802_2);
> +}
> +EXPORT_SYMBOL(__eth_type_trans);
> +
> +/**
>   * eth_type_trans - determine the packet's protocol ID.
>   * @skb: received socket data
>   * @dev: receiving network device
> @@ -184,33 +223,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>  			skb->pkt_type = PACKET_OTHERHOST;
>  	}
>  
> -	/*
> -	 * Some variants of DSA tagging don't have an ethertype field
> -	 * at all, so we check here whether one of those tagging
> -	 * variants has been configured on the receiving interface,
> -	 * and if so, set skb->protocol without looking at the packet.
> -	 */
> -	if (netdev_uses_dsa_tags(dev))
> -		return htons(ETH_P_DSA);
> -	if (netdev_uses_trailer_tags(dev))
> -		return htons(ETH_P_TRAILER);
> -
> -	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> -		return eth->h_proto;
> -
> -	/*
> -	 *      This is a magic hack to spot IPX packets. Older Novell breaks
> -	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> -	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> -	 *      won't work for fault tolerant netware but does for the rest.
> -	 */
> -	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> -		return htons(ETH_P_802_3);
> -
> -	/*
> -	 *      Real 802.2 LLC
> -	 */
> -	return htons(ETH_P_802_2);
> +	return __eth_type_trans(skb, dev);
>  }
>  EXPORT_SYMBOL(eth_type_trans);
>  
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 0c0f6c9..ec8e1c3 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2003,7 +2003,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>  			return err;
>  
>  		if (dev->type == ARPHRD_ETHER)
> -			skb->protocol = eth_type_trans(skb, dev);
> +			skb->protocol = __eth_type_trans(skb, dev);
>  
>  		data += dev->hard_header_len;
>  		to_write -= dev->hard_header_len;
> @@ -2332,13 +2332,13 @@ static int packet_snd(struct socket *sock,
>  	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>  
>  	if (dev->type == ARPHRD_ETHER) {
> -		skb->protocol = eth_type_trans(skb, dev);
> +		skb->protocol = __eth_type_trans(skb, dev);
>  		if (skb->protocol == htons(ETH_P_8021Q))
>  			reserve += VLAN_HLEN;
>  	} else {
>  		skb->protocol = proto;
> -		skb->dev = dev;
>  	}
> +	skb->dev = dev;
>  
>  	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
>  		err = -EMSGSIZE;

The problem with this patch is __eth_type_trans() assuming the MAC
header at skb->data which might be correct in the most cases, but not
when called from eth_type_trans() as the later sets skb->data to after
the ethernet header (which was the problem from the beginning).

Best wishes, Phil