On Wed, Aug 7, 2013 at 7:54 AM, Stephen Rothwell <[email protected]> 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
From: David Miller <[email protected]>
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" <[email protected]>
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 <[email protected]>
---
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
On Wed, 2013-08-07 at 16:36 -0700, David Miller wrote:
> From: David Miller <[email protected]>
> 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>
On Wed, Aug 07, 2013 at 10:47:13AM -0700, David Miller wrote:
> From: Eric Dumazet <[email protected]>
> 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
On Wed, Aug 07, 2013 at 04:36:21PM -0700, David Miller wrote:
> From: David Miller <[email protected]>
> 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
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.
On Thu, Aug 8, 2013 at 1:36 AM, David Miller <[email protected]> wrote:
> From: David Miller <[email protected]>
> 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;
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
On Wed, Aug 07, 2013 at 10:29:18AM +0200, Sedat Dilek wrote:
> On Wed, Aug 7, 2013 at 7:54 AM, Stephen Rothwell <[email protected]> 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
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
From: Phil Sutter <[email protected]>
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.
From: Sedat Dilek <[email protected]>
Date: Thu, 8 Aug 2013 02:02:48 +0200
> On Thu, Aug 8, 2013 at 1:36 AM, David Miller <[email protected]> wrote:
>> From: David Miller <[email protected]>
>> 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.
From: Eric Dumazet <[email protected]>
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.
From: David Miller <[email protected]>
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;
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 ;)