* NFS over NAT causes e1000e transmit hangs
From: Florian Fainelli @ 2017-04-18 18:18 UTC (permalink / raw)
To: intel-wired-lan, jeffrey.t.kirsher; +Cc: netdev
Hi,
I am using NFS over a NAT with two e1000e adapters and with eth1 being
the LAN interface and eth0 the WAN interface. The kernel is Ubuntu's
16.10 kernel: 4.8.0-46-generic. The device doing NAT over NFS is just
mounting a remote folder and doing normal execution/file accesses. It's
enough to untar a file from this device onto a NFS share to expose the
problem.
The transmit hangs look like the ones below, doing a rmmod/insmod does
not help eliminated the problem, nor does a power cycle. Stopping the
NFS over NAT definitively does let the adapter recover.
Happy to test any patches/newer kernels if you think there is something
obviously wrong. It *seems* to have started when I updated to 4.8.x, and
I was not able to see this under 4.4, so first things could be to try a
bisection, time permitting.
The two devices involved in the NAT are:
fainelli@fainelli-desktop:[~/../linux]$ lspci -s 0000:09:00.0 -v
09:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network
Connection
Subsystem: Intel Corporation Gigabit CT Desktop Adapter
Flags: bus master, fast devsel, latency 0, IRQ 17
Memory at ef6c0000 (32-bit, non-prefetchable) [size=128K]
Memory at ef600000 (32-bit, non-prefetchable) [size=512K]
I/O ports at b000 [size=32]
Memory at ef6e0000 (32-bit, non-prefetchable) [size=16K]
Expansion ROM at ef680000 [disabled] [size=256K]
Capabilities: <access denied>
Kernel driver in use: e1000e
Kernel modules: e1000e
fainelli@fainelli-desktop:[~/../linux]$ lspci -s 0000:00:19.0 -v
00:19.0 Ethernet controller: Intel Corporation 82579LM Gigabit Network
Connection (rev 05)
Subsystem: Dell 82579LM Gigabit Network Connection
Flags: bus master, fast devsel, latency 0, IRQ 43
Memory at ef900000 (32-bit, non-prefetchable) [size=128K]
Memory at ef929000 (32-bit, non-prefetchable) [size=4K]
I/O ports at f040 [size=32]
Capabilities: <access denied>
Kernel driver in use: e1000e
Kernel modules: e1000e
[516481.589090] e1000e 0000:00:19.0 eth0: Detected Hardware Unit Hang:
TDH <9b>
TDT <b0>
next_to_use <b0>
next_to_clean <96>
buffer_info[next_to_clean]:
time_stamp <107b0fc76>
next_to_watch <9b>
jiffies <107b10048>
next_to_watch.status <0>
MAC Status <40080083>
PHY Status <796d>
PHY 1000BASE-T Status <3c00>
PHY Extended Status <3000>
PCI Status <10>
[516483.573120] e1000e 0000:00:19.0 eth0: Detected Hardware Unit Hang:
TDH <9b>
TDT <b0>
next_to_use <b0>
next_to_clean <96>
buffer_info[next_to_clean]:
time_stamp <107b0fc76>
next_to_watch <9b>
jiffies <107b10238>
next_to_watch.status <0>
MAC Status <40080083>
PHY Status <796d>
PHY 1000BASE-T Status <3c00>
PHY Extended Status <3000>
PCI Status <10>
[516485.589452] e1000e 0000:00:19.0 eth0: Detected Hardware Unit Hang:
TDH <9b>
TDT <b0>
next_to_use <b0>
next_to_clean <96>
buffer_info[next_to_clean]:
time_stamp <107b0fc76>
next_to_watch <9b>
jiffies <107b10430>
next_to_watch.status <0>
MAC Status <40080083>
PHY Status <796d>
PHY 1000BASE-T Status <3c00>
PHY Extended Status <3000>
PCI Status <10>
[516487.573397] e1000e 0000:00:19.0 eth0: Detected Hardware Unit Hang:
TDH <9b>
TDT <b0>
next_to_use <b0>
next_to_clean <96>
buffer_info[next_to_clean]:
time_stamp <107b0fc76>
next_to_watch <9b>
jiffies <107b10620>
next_to_watch.status <0>
MAC Status <40080083>
PHY Status <796d>
PHY 1000BASE-T Status <3c00>
PHY Extended Status <3000>
PCI Status <10>
[516487.700509] e1000e 0000:00:19.0 eth0: Reset adapter unexpectedly
[516491.526799] e1000e: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow
Control: Rx/Tx
Thanks for reading, here is a virtual potato: 0.
--
Florian
^ permalink raw reply
* Re: [PATCH v3 0/9] ftgmac100: Rework batch 5 - Features
From: David Miller @ 2017-04-18 18:11 UTC (permalink / raw)
To: benh; +Cc: netdev
In-Reply-To: <20170417223706.16483-1-benh@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 18 Apr 2017 08:36:57 +1000
> This is the third spin of the fifth and last batch of
> updates to the ftgmac100 driver.
>
> This contains a few additional "features" such as:
>
> - Support for ethtool n-way reset
> - Multicast filtering & promisc support
> - Vlan offload
> - netpoll
>
> And a couple of misc bits. This also adds the device-tree binding
> documentation.
>
> v2. - Addresses review comments and adds a new patch fixing a
> theorical ordering issue in my new NAPI poll implementation
> - Add a bug fix (Patch 8/9) for a potential ordering issue
> in the new NAPI poll code.
>
> v3. - Rebase on net-next (fix conflict with an unrelated #include
> change series)
> - Update DT bindings better describing accepted phy-mode values
Series applied, thanks Ben.
^ permalink raw reply
* Re: [PATCH 00/25] Ethernet-Marvell: Fine-tuning for several function implementations
From: David Miller @ 2017-04-18 18:09 UTC (permalink / raw)
To: elfring
Cc: netdev, f.fainelli, jarod, jszhang, mlindner, tremyfr, rmk+kernel,
sergei.shtylyov, stephen, thomas.petazzoni, linux-kernel,
kernel-janitors
In-Reply-To: <d6f26bba-c678-5b66-4f5d-85f56d895190@users.sourceforge.net>
From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 17 Apr 2017 17:03:33 +0200
> Several update suggestions were taken into account
> from static source code analysis.
Series applied.
^ permalink raw reply
* Re: [PATCH net-next] net: ipv6: Add early demux handler for UDP unicast
From: Hannes Frederic Sowa @ 2017-04-18 18:09 UTC (permalink / raw)
To: David Miller, simon.horman; +Cc: eric.dumazet, subashab, netdev, netdev-owner
In-Reply-To: <20170418.111633.1943544161712972224.davem@davemloft.net>
On Tue, Apr 18, 2017, at 17:16, David Miller wrote:
> From: Simon Horman <simon.horman@netronome.com>
> Date: Tue, 18 Apr 2017 17:09:04 +0900
>
> > On Wed, Mar 08, 2017 at 11:22:01AM -0800, Eric Dumazet wrote:
> >> On Wed, 2017-03-08 at 12:11 -0700, Subash Abhinov Kasiviswanathan wrote:
> >> > On 2017-03-08 11:40, Eric Dumazet wrote:
> >> > > Well, this 'optimization' actually hurts when UDP sockets are not
> >> > > connected, since this adds an extra cache line miss per incoming
> >> > > packet.
> >> > >
> >> > > (DNS servers for example)
> >> >
> >> > Hi Eric
> >> >
> >> > Thanks for your comments. Would it be preferable to disable early demux
> >> > for the
> >> > servers with large unconnected workloads in that case?
> >>
> >> Well, many servers handle both TCP and UDP.
> >>
> >> For TCP, there is no question about early demux, this is definitely a
> >> win.
> >>
> >> We probably should have one sysctl to enable TCP early demux, one for
> >> UDP early demux.
> >
> > If early demux is a clear win for TCP then I wonder if it is
> > unnecessary and by some leap also undesirable to have a configuration
> > knob for that case.
>
> For forwarding workloads it is pure overhead since the early demux will
> never find a local socket, and therefore it is wasted work.
Also for some more complicated fib rules setups the early demux logic
could end up causing doing wrong lookups, because some route might not
be actually local in one fib rule but it is in another one.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH net-next] bpf: remove reference to sock_filter_ext from kerneldoc comment
From: Alexei Starovoitov @ 2017-04-18 17:50 UTC (permalink / raw)
To: Daniel Borkmann, Tobias Klauser, netdev
In-Reply-To: <58F5DDB2.8050308@iogearbox.net>
On 4/18/17 2:34 AM, Daniel Borkmann wrote:
> On 04/18/2017 11:27 AM, Tobias Klauser wrote:
>> struct sock_filter_ext didn't make it into the tree and is now called
>> struct bpf_insn. Reword the kerneldoc comment for bpf_convert_filter()
>> accordingly.
>>
>> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
>
> Good point, thanks!
>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
good ol' days :)
let it rip.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH -next] rhashtable: remove insecure_elasticity
From: David Miller @ 2017-04-18 17:49 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20170416005509.31784-1-fw@strlen.de>
From: Florian Westphal <fw@strlen.de>
Date: Sun, 16 Apr 2017 02:55:09 +0200
> commit 83e7e4ce9e93c3 ("mac80211: Use rhltable instead of rhashtable")
> removed the last user that made use of 'insecure_elasticity' parameter,
> i.e. the default of 16 is used everywhere.
>
> Replace it with a constant.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Yeah if it's not used, we can kill it. If someone needs it again it is
trivial to add it back if necessary.
Applied, thanks Florian.
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: remove poll() flakes when receiving RST
From: Soheil Hassas Yeganeh @ 2017-04-18 17:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
In-Reply-To: <20170418164552.29261-2-edumazet@google.com>
On Tue, Apr 18, 2017 at 12:45 PM, Eric Dumazet <edumazet@google.com> wrote:
> When a RST packet is processed, we send two wakeup events to interested
> polling users.
>
> First one by a sk->sk_error_report(sk) from tcp_reset(),
> followed by a sk->sk_state_change(sk) from tcp_done().
>
> Depending on machine load and luck, poll() can either return POLLERR,
> or POLLIN|POLLOUT|POLLERR|POLLHUP (this happens on 99 % of the cases)
>
> This is probably fine, but we can avoid the confusion by reordering
> things so that we have more TCP fields updated before the first wakeup.
>
> This might even allow us to remove some barriers we added in the past.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> ---
> net/ipv4/tcp_input.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index a5838858c362cd3270296001ceaae341e9e9bf01..37e2aa925f62395cfb48145cd3a76b6afebb64b1 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4008,10 +4008,10 @@ void tcp_reset(struct sock *sk)
> /* This barrier is coupled with smp_rmb() in tcp_poll() */
> smp_wmb();
>
> + tcp_done(sk);
> +
> if (!sock_flag(sk, SOCK_DEAD))
> sk->sk_error_report(sk);
> -
> - tcp_done(sk);
> }
>
> /*
> --
> 2.12.2.762.g0e3151a226-goog
>
Thanks, Eric. Nice improvement!
^ permalink raw reply
* [PATCH] net/packet: initialize val in packet_getsockopt()
From: Alexander Potapenko @ 2017-04-18 17:47 UTC (permalink / raw)
To: dvyukov, kcc, edumazet, davem, kuznet; +Cc: linux-kernel, netdev
In the case getsockopt() is called with PACKET_HDRLEN and zero length,
|val| remains uninitialized and the syscall may behave differently
depending on its value. This doesn't have security consequences (as the
uninit bytes aren't copied back), but it's still cleaner to initialize
|val|.
This bug has been detected with KMSAN.
Signed-off-by: Alexander Potapenko <glider@google.com>
---
KMSAN report below:
==================================================================
BUG: KMSAN: use of unitialized memory in packet_getsockopt+0xb9b/0xbe0
inter: 0
CPU: 0 PID: 1036 Comm: probe Tainted: G B 4.11.0-rc5+ #2444
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x143/0x1b0 lib/dump_stack.c:52
kmsan_report+0x16b/0x1e0 mm/kmsan/kmsan.c:1078
__kmsan_warning_32+0x5c/0xa0 mm/kmsan/kmsan_instr.c:510
packet_getsockopt+0xb9b/0xbe0 net/packet/af_packet.c:3839
SYSC_getsockopt+0x495/0x540 net/socket.c:1829
SyS_getsockopt+0xb0/0xd0 net/socket.c:1811
entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
RIP: 0033:0x436d8a
RSP: 002b:00007ffce54e52c8 EFLAGS: 00000203 ORIG_RAX: 0000000000000037
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000436d8a
RDX: 000000000000000b RSI: 0000000000000107 RDI: 0000000000000003
RBP: 00007ffce54e52b0 R08: 00007ffce54e52d8 R09: 0000000000000004
R10: 00007ffce54e52d4 R11: 0000000000000203 R12: 00007ffce54e53c8
R13: 00007ffce54e53d8 R14: 0000000000000002 R15: 0000000000000000
origin description: ----val@packet_getsockopt (origin=00000000f6600052)
local variable created at:
packet_getsockopt+0xcd/0xbe0 net/packet/af_packet.c:3789
SYSC_getsockopt+0x495/0x540 net/socket.c:1829
==================================================================
---
net/packet/af_packet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 8489beff5c25..09398454ec66 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3787,7 +3787,7 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
char __user *optval, int __user *optlen)
{
int len;
- int val, lv = sizeof(val);
+ int val = 0, lv = sizeof(val);
struct sock *sk = sock->sk;
struct packet_sock *po = pkt_sk(sk);
void *data = &val;
--
2.12.2.816.g2cccc81164-goog
^ permalink raw reply related
* Re: net/ipv4: use-after-free in ip_queue_xmit
From: Andrey Konovalov @ 2017-04-18 17:45 UTC (permalink / raw)
To: Cong Wang
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Eric Dumazet,
Dmitry Vyukov, Kostya Serebryany, syzkaller
In-Reply-To: <CAM_iQpVXKA6S2Nc1+UAW6q5X-iiSCU3UQaAsCm2SAkA5Axct+A@mail.gmail.com>
On Tue, Apr 18, 2017 at 6:05 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Apr 18, 2017 at 5:15 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> Yes, I don't have this field in the rtable struct.
>>
>> I'm on 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
>>
>> I also don't see it in the cross reference:
>> http://lxr.free-electrons.com/source/include/net/route.h#L51
>>
>
> It is provided by my patch: https://patchwork.ozlabs.org/patch/747556/
> which means you applied an incomplete patch... :-/
Oops, my bad.
Applied the patch.
Thanks!
^ permalink raw reply
* [PATCH net-next v3] net: ipv6: Fix UDP early demux lookup with udp_l3mdev_accept=0
From: Subash Abhinov Kasiviswanathan @ 2017-04-18 17:39 UTC (permalink / raw)
To: dsa, davem, netdev, rshearma, eric.dumazet
Cc: Subash Abhinov Kasiviswanathan, Eric Dumazet
David Ahern reported that 5425077d73e0c ("net: ipv6: Add early demux
handler for UDP unicast") breaks udp_l3mdev_accept=0 since early
demux for IPv6 UDP was doing a generic socket lookup which does not
require an exact match. Fix this by making UDPv6 early demux match
connected sockets only.
v1->v2: Take reference to socket after match as suggested by Eric
v2->v3: Add comment before break
Fixes: 5425077d73e0c ("net: ipv6: Add early demux handler for UDP unicast")
Reported-by: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Eric Dumazet <edumazet@google.com>
Acked-by: David Ahern <dsa@cumulusnetworks.com>
Tested-by: David Ahern <dsa@cumulusnetworks.com>
---
net/ipv6/udp.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index b793ed1..b615e7a 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -46,6 +46,7 @@
#include <net/tcp_states.h>
#include <net/ip6_checksum.h>
#include <net/xfrm.h>
+#include <net/inet_hashtables.h>
#include <net/inet6_hashtables.h>
#include <net/busy_poll.h>
#include <net/sock_reuseport.h>
@@ -864,21 +865,26 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
return 0;
}
+
static struct sock *__udp6_lib_demux_lookup(struct net *net,
__be16 loc_port, const struct in6_addr *loc_addr,
__be16 rmt_port, const struct in6_addr *rmt_addr,
int dif)
{
+ unsigned short hnum = ntohs(loc_port);
+ unsigned int hash2 = udp6_portaddr_hash(net, loc_addr, hnum);
+ unsigned int slot2 = hash2 & udp_table.mask;
+ struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
+ const __portpair ports = INET_COMBINED_PORTS(rmt_port, hnum);
struct sock *sk;
- rcu_read_lock();
- sk = __udp6_lib_lookup(net, rmt_addr, rmt_port, loc_addr, loc_port,
- dif, &udp_table, NULL);
- if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
- sk = NULL;
- rcu_read_unlock();
-
- return sk;
+ udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
+ if (INET6_MATCH(sk, net, rmt_addr, loc_addr, ports, dif))
+ return sk;
+ /* Only check first socket in chain */
+ break;
+ }
+ return NULL;
}
static void udp_v6_early_demux(struct sk_buff *skb)
@@ -903,7 +909,7 @@ static void udp_v6_early_demux(struct sk_buff *skb)
else
return;
- if (!sk)
+ if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
return;
skb->sk = sk;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next 0/3] sctp: add proper process for duplicated stream reconf requests
From: David Miller @ 2017-04-18 17:40 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman
In-Reply-To: <cover.1492264734.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Sat, 15 Apr 2017 22:00:26 +0800
> Now sctp stream reconf will process a request again even if it's seqno
> is less than asoc->strreset_inseq. It may cause a replay attack.
>
> This patchset is to avoid it by add proper process for all duplicated
> stream reconf requests.
Series applied, thanks.
^ permalink raw reply
* question about size of sk_buff and skb_shared_info
From: Code Soldier1 @ 2017-04-18 17:34 UTC (permalink / raw)
To: netdev
Hi Folks,
I am sure there is a reason for the current sizes of these structures,
However the reason is not obvious to me. So please help me understand.
Currently the size of sk_buff on an x86_64 system is 232 bytes -- Why
is that. I expected it to be a multiple of 32/64 as they are the most
common cache lines. Since the alignment calculation will align the
structure with the hw cache line, it seems like we might be wasting
space ?
skb_shared_info on the other hand is perfectly aligned with a size of 320 bytes.
Thanks,
--
CS1
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: Eric Dumazet @ 2017-04-18 17:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: James Hughes, David Miller, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <eb8f4cda-d1b1-1038-513b-08ef016042ae@gmail.com>
On Tue, 2017-04-18 at 10:10 -0700, Florian Fainelli wrote:
> BTW, this pattern of using skb_headroom() ... skb_copy_expand() seems to
> be recurring in pretty much all USB network drivers that have a tx_fixup
> callback set. The problem is that each driver needs its own
> headroom/tailroom so the fix is not as simple as putting the
> skb_cow_head() before the call to the drivers' tx_fixup...
>
> I wonder if a coccinelle patch would be able to do that for us?
Not sure about coccinelle, because some drivers also need some tailroom.
^ permalink raw reply
* Re: IGMP on IPv6
From: Murali Karicheri @ 2017-04-18 17:20 UTC (permalink / raw)
To: Cong Wang; +Cc: Hangbin Liu, open list:TI NETCP ETHERNET DRIVER, David Miller
In-Reply-To: <58F648FB.10606@ti.com>
On 04/18/2017 01:12 PM, Murali Karicheri wrote:
> On 04/17/2017 05:38 PM, Cong Wang wrote:
>> Hello,
>>
>> On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote:
>>> On 03/22/2017 11:04 AM, Murali Karicheri wrote:
>>>> This is going directly to the slave Ethernet interface.
>>>>
>>>> When I put a WARN_ONCE, I found this is coming directly from
>>>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output()
>>>>
>>>> Do you think this is fixed in latest kernel at master? If so, could
>>>> you point me to some commits.
>>>>
>>>>
>>> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if
>>> this is fixed by some commit or I need to debug? I see IGMPv6 has some
>>> fixes on the list to make it similar to IGMPv4. So can someone clarify this is
>>> is a bug at IGMPv6 code or I need to look into the HSR driver code?
>>> Since IGMPv4 is going over the HSR interface I am assuming this is a
>>> bug in the IGMPv6 code. But since I have not experience with this code
>>> can some expert comment please?
>>>
>>
>> How did you configure your network interfaces and IPv4/IPv6 multicast?
>> IOW, how did you reproduce this? For example, did you change your
>> HSR setup when this happened since you mentioned
>> NETDEV_CHANGEUPPER?
>>
> Thanks for responding! Really appreciate.
>
> I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of
> my testing, I dump the packets going through the slave interfaces attached
> to the hsr interface (for example my Ethernet interfaces eth2 and eth3
> are attached to the hsr interface and I dump the packets at the egress
> of eth2 and eth3 in my driver along with that at hsr xmit function). As
> soon as I create the hsr interface, I see a bunch of packets going directly
> through the lower interface, not through the upper one (i.e hsr interface)
> and these are of eth_type = 86 dd. Please ignore my reference to
> NETDEV_CHANGEUPPER for now as it was wild guess.
>
> I have not done any debugging, but the WARN_ONCE which I have placed
> in the lower level driver looking for eth_type = 86 dd provided the
> above trace.
>
Here is the command I have used to create the hsr interface...
ip link add name hsr0 type hsr slave1 eth2 slave2 eth3 supervision 45 version 1
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
From: James Hughes @ 2017-04-18 17:17 UTC (permalink / raw)
To: netdev, Steve Glendinning, Microchip Linux Driver Support; +Cc: James Hughes
The driver was failing to check that the SKB wasn't cloned
before adding checksum data.
Replace existing handling to extend/copy the header buffer
with skb_cow_head.
Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Changes in v2
- Changed skb_cow to skb_cow_head as suggested by netdev list
drivers/net/usb/smsc95xx.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index df60c98..094f0ee 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
- if (skb_headroom(skb) < overhead) {
- struct sk_buff *skb2 = skb_copy_expand(skb,
- overhead, 0, flags);
- dev_kfree_skb_any(skb);
- skb = skb2;
- if (!skb)
- return NULL;
+ /* Make writable and expand header space by overhead if required */
+ if (skb_cow_head(skb, overhead)) {
+ return NULL;
}
if (csum) {
--
2.9.3
^ permalink raw reply related
* Re: [PATCH next RFC] net: sched: mirred: move xmit to tasklet
From: David Miller @ 2017-04-18 17:15 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20170418130331.14268-1-fw@strlen.de>
From: Florian Westphal <fw@strlen.de>
Date: Tue, 18 Apr 2017 15:03:31 +0200
> mirred is prone to deadlocks as it invokes dev_queue_xmit while
> holding one or more qdisc locks.
>
> Avoid lock recursions by moving tx context to a tasklet.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> This a stab at removing the lock recursions discussed during netconf.
>
> Taking the cost of the tasklet appears to be the only solution;
> i tried to use a percpu 'history' instead but its not clear to
> me that this avoids all corner cases.
>
> While this patch doesn't avoid loops we don't hang the kernel
> anymore and removing the 'looping' filter makes things calm
> down again (there are also other ways to create such loops anyway,
> including use of a cable... )
The thing is, those qdisc's are per-device.
So we could return the locking cpu and back out of deadlocks just like
we do for the netdev tx lock.
Given that, maybe the remaining part of the equation could possibly be
handled by a ttl.
^ permalink raw reply
* Re: [PATCH][net-next] esp6: fix incorrect null pointer check on xo
From: David Miller @ 2017-04-18 17:12 UTC (permalink / raw)
To: colin.king; +Cc: netdev, steffen.klassert
In-Reply-To: <20170418140653.8839-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Tue, 18 Apr 2017 15:06:53 +0100
Trimming the CC: list down to something that actually makes
sense.
> From: Colin Ian King <colin.king@canonical.com>
>
> The check for xo being null is incorrect, currently it is checking
> for non-null, it should be checking for null.
>
> Detected with CoverityScan, CID#1429349 ("Dereference after null check")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
At a minimum you should work to check that you are CC:'ing the person
who added the code you are fixing. You can use "git blame" for this.
Also, Steffen, you need to add the new IPSEC offload files to the
MAINTAINERS entry for iPSEC.
Thank you.
^ permalink raw reply
* Re: IGMP on IPv6
From: Murali Karicheri @ 2017-04-18 17:12 UTC (permalink / raw)
To: Cong Wang; +Cc: Hangbin Liu, open list:TI NETCP ETHERNET DRIVER, David Miller
In-Reply-To: <CAM_iQpWYuQdxF0AOEMHRy7BSmxMVUb-7w7OdgYeOKe6atATsFQ@mail.gmail.com>
On 04/17/2017 05:38 PM, Cong Wang wrote:
> Hello,
>
> On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote:
>> On 03/22/2017 11:04 AM, Murali Karicheri wrote:
>>> This is going directly to the slave Ethernet interface.
>>>
>>> When I put a WARN_ONCE, I found this is coming directly from
>>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output()
>>>
>>> Do you think this is fixed in latest kernel at master? If so, could
>>> you point me to some commits.
>>>
>>>
>> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if
>> this is fixed by some commit or I need to debug? I see IGMPv6 has some
>> fixes on the list to make it similar to IGMPv4. So can someone clarify this is
>> is a bug at IGMPv6 code or I need to look into the HSR driver code?
>> Since IGMPv4 is going over the HSR interface I am assuming this is a
>> bug in the IGMPv6 code. But since I have not experience with this code
>> can some expert comment please?
>>
>
> How did you configure your network interfaces and IPv4/IPv6 multicast?
> IOW, how did you reproduce this? For example, did you change your
> HSR setup when this happened since you mentioned
> NETDEV_CHANGEUPPER?
>
Thanks for responding! Really appreciate.
I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of
my testing, I dump the packets going through the slave interfaces attached
to the hsr interface (for example my Ethernet interfaces eth2 and eth3
are attached to the hsr interface and I dump the packets at the egress
of eth2 and eth3 in my driver along with that at hsr xmit function). As
soon as I create the hsr interface, I see a bunch of packets going directly
through the lower interface, not through the upper one (i.e hsr interface)
and these are of eth_type = 86 dd. Please ignore my reference to
NETDEV_CHANGEUPPER for now as it was wild guess.
I have not done any debugging, but the WARN_ONCE which I have placed
in the lower level driver looking for eth_type = 86 dd provided the
above trace.
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: Florian Fainelli @ 2017-04-18 17:10 UTC (permalink / raw)
To: Eric Dumazet, James Hughes
Cc: David Miller, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <1492534347.10587.130.camel@edumazet-glaptop3.roam.corp.google.com>
On 04/18/2017 09:52 AM, Eric Dumazet wrote:
> On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
>> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date: Tue, 18 Apr 2017 08:51:51 -0700
>>>
>>>> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>>>>> The driver was failing to check that the SKB wasn't cloned
>>>>> before adding checksum data or adding header data.
>>>>> Replace existing handling to extend the buffer with
>>>>> skb_cow. Don't use skb_cow_head as the sw checksum
>>>>> code modifies the data portion.
>>>>>
>>>>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>>>>> ---
>>>>> drivers/net/usb/smsc95xx.c | 10 +++-------
>>>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>>>>> index df60c98..04f6397 100644
>>>>> --- a/drivers/net/usb/smsc95xx.c
>>>>> +++ b/drivers/net/usb/smsc95xx.c
>>>>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>>>>> /* We do not advertise SG, so skbs should be already linearized */
>>>>> BUG_ON(skb_shinfo(skb)->nr_frags);
>>>>>
>>>>> - if (skb_headroom(skb) < overhead) {
>>>>> - struct sk_buff *skb2 = skb_copy_expand(skb,
>>>>> - overhead, 0, flags);
>>>>> - dev_kfree_skb_any(skb);
>>>>> - skb = skb2;
>>>>> - if (!skb)
>>>>> - return NULL;
>>>>> + /* Make writable and expand space by overhead if required */
>>>>> + if (skb_cow(skb, overhead)) {
>>>>> + return NULL;
>>>>> }
>>>>
>>>> Note that this patch will probably force a copy of all locally generated
>>>> TCP packets.
>>>>
>>>> For them skb_cloned(skb) is true.
>>>>
>>>> I do believe skb_cow_head() would be better, since TCP stack uses the
>>>> __skb_header_release() helper to tell lower stacks they can write the
>>>> header part, even on a clone.
>>>
>>> Agreed.
>>
>> I'm happy to work it as you see fit - you know this code far better than I do.
>>
>> Our reading of the code is that the software checksum path is
>> modifying the data rather than just adding a header. Based on the
>> description of skb_cow_head it therefore isn't appropriate. If that
>> isn't a concern in reality then skb_cow_head is fine and I'll make a
>> V2 patchset.
>> Or do we need to skb_cow if doing the software checksum, but
>> skb_cow_head normally? That can be done instead but requires a
>> slightly larger change.
>>
>> The failure case we were seeing was with a bridged network using
>> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
>> making an SKB clone of broadcasts for the two interfaces, and then
>> both drivers were adding headers without checking skb_cloned(skb)
>> first, hence trampling on each other. For small packets the SMSC95xx
>> driver will be computing the software checksum and writing it in to
>> the data, so the wifi driver will also be seeing it. For many drivers
>> that probably won't matter, but is that always true?
>>
>> (Patches for the Broadcom wifi driver will be coming once we've worked
>> out the best way of fixing this - there is no error path easily
>> available if the skb_cow_head call fails).
>>
>
> You misread what the driver does.
>
> The TCP data (payload) is _not_ modified.
>
> Only additional headers are pushed in front of the existing (Ethernet,
> IP, TCP) headers.
>
> For this, skb_cow_head() is the perfect solution.
BTW, this pattern of using skb_headroom() ... skb_copy_expand() seems to
be recurring in pretty much all USB network drivers that have a tx_fixup
callback set. The problem is that each driver needs its own
headroom/tailroom so the fix is not as simple as putting the
skb_cow_head() before the call to the drivers' tx_fixup...
I wonder if a coccinelle patch would be able to do that for us?
--
Florian
^ permalink raw reply
* Re: bluetooth 6lowpan interfaces are not virtual anymore
From: Michael Richardson @ 2017-04-18 16:59 UTC (permalink / raw)
To: Alexander Aring
Cc: Network Development, Jukka Rissanen, Luiz Augusto von Dentz,
linux-wpan@vger.kernel.org, Linux Bluetooth
In-Reply-To: <2d178eb8-3fbc-3385-6e0c-fa9941713959@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]
Alexander Aring <aar@pengutronix.de> wrote:
> What does the 6LoWPAN interface?
> It will do a protocol change (an adaptation, because 6LoWPAN should
> provide the same functionality as IPv6) from IPv6 to 6LoWPAN (tx) and
> vice versa for (rx). In my opinion this should be handled as a virtual
> interface and not as an interface with a queue.
I wonder if modeling all the 6lowpan work as a virtual interface is even
the right abstraction anymore. I think that it was certainly a good model at
the time the interface was created, given no other clear thing to do.
We don't model IPv6 ND (or IPv4 ARP) or fragmentation in general as a
virtual interface on top of a raw interface.
Really, it's a set of operations that happens on a packet.
802.15.4 is notable for it's current lack of an ethertype (IEEE is fixing
that though), so you can't actually run different protocols on the
same PANID.
BT does have a variety of different protocols, and IPv6 is only one.
In a classic SVR4 STREAMS works, it would have been just another module.
(No, I'm not a fan of *STREAMS* or of SVR4 in general, although I liked
some of the ideas).
At this time, things like PANID and channel are set on the wpanX interface.
If they were set on the 6lowpan interface, such that one could (in theory,
assuming the hardware could do it, which some can, and some can not) then
one could have multiple 6lowpan interfaces on top of the same wpanX.
Or one could run some non-IP protocol like pre-IP Zigbee on one PANID
while one runs 6lowpan on another. THEN, a virtual interface would make
sense for the same reason VLAN interfaces make sense.
^ permalink raw reply
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Cong Wang @ 2017-04-18 17:03 UTC (permalink / raw)
To: Wolfgang Bumiller
Cc: Linux Kernel Network Developers, Jamal Hadi Salim,
David S. Miller
In-Reply-To: <20170418101322.27666-2-w.bumiller@proxmox.com>
On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
<w.bumiller@proxmox.com> wrote:
> Whether the reference count has to be decremented depends
> on whether the policy was created. If TCA_ACT_COOKIE is
> passed and an error occurs there, the same condition still
> has to be honored.
>
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>
> I did not include the Acked-bys here because I've noticed that this
> is still wrong. After reading a bit more and doing more tests with
> different filters I realized that the `name != NULL` case is specific
> to the police filter only. For the other filters this patch here breaks
> refcounting in the error case (I included it for reference only).
police action...That is why I said we may need a TCA_POLICE_COOKIE.
> I'm thinking the first patch should be enough. (I've tested forcing the
> other filters into the error path *without* this patch and couldn't
> produce crashes or reference count problems (while with this patch
> applied it was leaking reference counts on creation (which makes sense
> considering tcf_hash_release is used and the ACT_P_CREATED case will
> keep repeating)). (Whereas without both patches simply looking through
> creating and deleting a policing filter pretty much always resulted in
> crashes with various different backtraces.)
>
The action API's suck here.
The idea is we should rollback everything when cookie setup fails.
Taking another look, it seems the current code (without this patch) is
correct:
1) When ->init() returns ACT_P_CREATED, we should rollback both
act creation and module refcnt, the former is already taken care by
tcf_hash_release(), the latter is at err_mod.
2) When ->init() returns !ACT_P_CREATED, we should rollback the
the modification to the existing action and module refcnt, the former is
impossible with current code (because we don't do copy and update)
so we only do tcf_hash_release(), module refcnt needs to rollback
like normal path.
Ideally, these action API's should handle it nicely, exposing the
module_put()/module_get() is ugly and confusing.
^ permalink raw reply
* Re: [PATCH net] selftests/net: Fixes psock_fanout CBPF test case
From: Sowmini Varadhan @ 2017-04-18 17:03 UTC (permalink / raw)
To: Mike Maloney; +Cc: netdev, davem, Mike Maloney
In-Reply-To: <CAJegeYmU4ntddE8cHFZT5QM0=aOzu0s1pYkQMehREv56JorEog@mail.gmail.com>
On (04/18/17 11:56), Mike Maloney wrote:
> I am not 100% sure what you are asking for, as the instructions you
> can feed to bpf_asm are already commented to the right of the program.
oh, right, I missed that you had commented the BPF_STMT macros
with the equivalent bpf_asm inovcation.
Looks good to me.
--Sowmini
^ permalink raw reply
* Re: [Patch net-next v3] net_sched: move the empty tp check from ->destroy() to ->delete()
From: Daniel Borkmann @ 2017-04-18 17:01 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: John Fastabend, Jamal Hadi Salim, lucasb
In-Reply-To: <1492453840-15816-1-git-send-email-xiyou.wangcong@gmail.com>
Hi Cong,
sorry for the late reply. Generally the patch looks good to me, just
a few comments inline:
On 04/17/2017 08:30 PM, Cong Wang wrote:
> Roi reported we could have a race condition where in ->classify() path
> we dereference tp->root and meanwhile a parallel ->destroy() makes it
> a NULL.
Correct.
> This is possible because ->destroy() could be called when deleting
> a filter to check if we are the last one in tp, this tp is still
> linked and visible at that time.
>
> Daniel fixed this in commit d936377414fa
> ("net, sched: respect rcu grace period on cls destruction"), but
> the root cause of this problem is the semantic of ->destroy(), it
> does two things (for non-force case):
>
> 1) check if tp is empty
> 2) if tp is empty we could really destroy it
>
> and its caller, if cares, needs to check its return value to see if
> it is really destroyed. Therefore we can't unlink tp unless we know
> it is empty.
>
> As suggested by Daniel, we could actually move the test logic to ->delete()
> so that we can safely unlink tp after ->delete() tells us the last one is
> just deleted and before ->destroy().
>
> What's more, even we unlink it before ->destroy(), it could still have
> readers since we don't wait for a grace period here, we should not modify
> tp->root in ->destroy() either.
Here seems to be a bit of a mixup in this analysis, imo. The issue
that Roi reported back then was exactly the one that d936377414fa ("net,
sched: respect rcu grace period on cls destruction") fixed, which
affected flower and other classifiers:
Roi reported a crash in flower where tp->root was NULL in ->classify()
callbacks. Reason is that in ->destroy() tp->root is set to NULL via
RCU_INIT_POINTER(). It's problematic for some of the classifiers, because
this doesn't respect RCU grace period for them, and as a result, still
outstanding readers from tc_classify() will try to blindly dereference
a NULL tp->root.
The ->delete() callback was never used by Roi back then, he said that
he just removed the ingress qdisc in his test, which implicitly purges
all cls attached to it via tcf_destroy_chain(). So the above description
with regards to the "root cause" of Roi's reported issue is not correct.
The issue that this patch fixes is an _independent_ race that we found
while auditing the code when looking into Roi's report back then. It
fixes commit 1e052be69d04 ("net_sched: destroy proto tp when all filters
are gone"), which added the RCU_INIT_POINTER() after the tcf_destroy() in
RTM_DELTFILTER case. That part of the description looks good, where you
describe that "[...] we need to move the test logic to ->delete(), so
that we can safely unlink tp after ->delete() tells us the last one is
just deleted and before ->destroy()."
Please also add Fixes tag, so it can be better tracked for backports.
Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone")
> Reported-by: Roi Dayan <roid@mellanox.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
[...]
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index 9962090..d388536 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
[...]
> @@ -150,17 +144,17 @@ static bool fw_destroy(struct tcf_proto *tp, bool force)
> call_rcu(&f->rcu, fw_delete_filter);
> }
> }
> - RCU_INIT_POINTER(tp->root, NULL);
> kfree_rcu(head, rcu);
> - return true;
> }
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index a371075..8e2baf8 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -312,12 +305,10 @@ static bool route4_destroy(struct tcf_proto *tp, bool force)
> kfree_rcu(b, rcu);
> }
> }
> - RCU_INIT_POINTER(tp->root, NULL);
> kfree_rcu(head, rcu);
> - return true;
> }
> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
> index d7f2923..9e3748b 100644
> --- a/net/sched/cls_rsvp.h
> +++ b/net/sched/cls_rsvp.h
> @@ -302,22 +302,13 @@ static void rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
[...]
> - }
> -
> - RCU_INIT_POINTER(tp->root, NULL);
> + return;
>
> for (h1 = 0; h1 < 256; h1++) {
> struct rsvp_session *s;
> @@ -337,10 +328,9 @@ static bool rsvp_destroy(struct tcf_proto *tp, bool force)
[...]
The above three RCU_INIT_POINTER(tp->root, NULL) are independent
of the fix and actually do no harm right now. I described that in
d936377414fa ("net, sched: respect rcu grace period on cls destruction")
as well, meaning that they each handle tp->root being NULL in ->classify()
path (for historic reasons), so this is handled gracefully, readers use
rcu_dereference_bh(tp->root) and test for this being NULL.
But I agree that this could be cleaned up along with the check in the
->classify() callbacks for these three (not sure if really worth it,
though). However, such cleanup should be a separate patch and not
included in this fix.
Other than the mentioned things, this looks good to me.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: James Hughes @ 2017-04-18 16:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <1492534347.10587.130.camel@edumazet-glaptop3.roam.corp.google.com>
On 18 April 2017 at 17:52, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
>> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
>> > From: Eric Dumazet <eric.dumazet@gmail.com>
>> > Date: Tue, 18 Apr 2017 08:51:51 -0700
>> >
>> >> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>> >>> The driver was failing to check that the SKB wasn't cloned
>> >>> before adding checksum data or adding header data.
>> >>> Replace existing handling to extend the buffer with
>> >>> skb_cow. Don't use skb_cow_head as the sw checksum
>> >>> code modifies the data portion.
>> >>>
>> >>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>> >>> ---
>> >>> drivers/net/usb/smsc95xx.c | 10 +++-------
>> >>> 1 file changed, 3 insertions(+), 7 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> >>> index df60c98..04f6397 100644
>> >>> --- a/drivers/net/usb/smsc95xx.c
>> >>> +++ b/drivers/net/usb/smsc95xx.c
>> >>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>> >>> /* We do not advertise SG, so skbs should be already linearized */
>> >>> BUG_ON(skb_shinfo(skb)->nr_frags);
>> >>>
>> >>> - if (skb_headroom(skb) < overhead) {
>> >>> - struct sk_buff *skb2 = skb_copy_expand(skb,
>> >>> - overhead, 0, flags);
>> >>> - dev_kfree_skb_any(skb);
>> >>> - skb = skb2;
>> >>> - if (!skb)
>> >>> - return NULL;
>> >>> + /* Make writable and expand space by overhead if required */
>> >>> + if (skb_cow(skb, overhead)) {
>> >>> + return NULL;
>> >>> }
>> >>
>> >> Note that this patch will probably force a copy of all locally generated
>> >> TCP packets.
>> >>
>> >> For them skb_cloned(skb) is true.
>> >>
>> >> I do believe skb_cow_head() would be better, since TCP stack uses the
>> >> __skb_header_release() helper to tell lower stacks they can write the
>> >> header part, even on a clone.
>> >
>> > Agreed.
>>
>> I'm happy to work it as you see fit - you know this code far better than I do.
>>
>> Our reading of the code is that the software checksum path is
>> modifying the data rather than just adding a header. Based on the
>> description of skb_cow_head it therefore isn't appropriate. If that
>> isn't a concern in reality then skb_cow_head is fine and I'll make a
>> V2 patchset.
>> Or do we need to skb_cow if doing the software checksum, but
>> skb_cow_head normally? That can be done instead but requires a
>> slightly larger change.
>>
>> The failure case we were seeing was with a bridged network using
>> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
>> making an SKB clone of broadcasts for the two interfaces, and then
>> both drivers were adding headers without checking skb_cloned(skb)
>> first, hence trampling on each other. For small packets the SMSC95xx
>> driver will be computing the software checksum and writing it in to
>> the data, so the wifi driver will also be seeing it. For many drivers
>> that probably won't matter, but is that always true?
>>
>> (Patches for the Broadcom wifi driver will be coming once we've worked
>> out the best way of fixing this - there is no error path easily
>> available if the skb_cow_head call fails).
>>
>
> You misread what the driver does.
>
> The TCP data (payload) is _not_ modified.
>
> Only additional headers are pushed in front of the existing (Ethernet,
> IP, TCP) headers.
>
> For this, skb_cow_head() is the perfect solution.
>
OK, will modify the patch, commit and resubmit. Thanks
James
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: Eric Dumazet @ 2017-04-18 16:52 UTC (permalink / raw)
To: James Hughes
Cc: David Miller, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <CAE_XsMJZGPvENhCgyvuz48PyLk=T7L-tGoktzVBLPPYDP7oR8A@mail.gmail.com>
On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 18 Apr 2017 08:51:51 -0700
> >
> >> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
> >>> The driver was failing to check that the SKB wasn't cloned
> >>> before adding checksum data or adding header data.
> >>> Replace existing handling to extend the buffer with
> >>> skb_cow. Don't use skb_cow_head as the sw checksum
> >>> code modifies the data portion.
> >>>
> >>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> >>> ---
> >>> drivers/net/usb/smsc95xx.c | 10 +++-------
> >>> 1 file changed, 3 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> >>> index df60c98..04f6397 100644
> >>> --- a/drivers/net/usb/smsc95xx.c
> >>> +++ b/drivers/net/usb/smsc95xx.c
> >>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> >>> /* We do not advertise SG, so skbs should be already linearized */
> >>> BUG_ON(skb_shinfo(skb)->nr_frags);
> >>>
> >>> - if (skb_headroom(skb) < overhead) {
> >>> - struct sk_buff *skb2 = skb_copy_expand(skb,
> >>> - overhead, 0, flags);
> >>> - dev_kfree_skb_any(skb);
> >>> - skb = skb2;
> >>> - if (!skb)
> >>> - return NULL;
> >>> + /* Make writable and expand space by overhead if required */
> >>> + if (skb_cow(skb, overhead)) {
> >>> + return NULL;
> >>> }
> >>
> >> Note that this patch will probably force a copy of all locally generated
> >> TCP packets.
> >>
> >> For them skb_cloned(skb) is true.
> >>
> >> I do believe skb_cow_head() would be better, since TCP stack uses the
> >> __skb_header_release() helper to tell lower stacks they can write the
> >> header part, even on a clone.
> >
> > Agreed.
>
> I'm happy to work it as you see fit - you know this code far better than I do.
>
> Our reading of the code is that the software checksum path is
> modifying the data rather than just adding a header. Based on the
> description of skb_cow_head it therefore isn't appropriate. If that
> isn't a concern in reality then skb_cow_head is fine and I'll make a
> V2 patchset.
> Or do we need to skb_cow if doing the software checksum, but
> skb_cow_head normally? That can be done instead but requires a
> slightly larger change.
>
> The failure case we were seeing was with a bridged network using
> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
> making an SKB clone of broadcasts for the two interfaces, and then
> both drivers were adding headers without checking skb_cloned(skb)
> first, hence trampling on each other. For small packets the SMSC95xx
> driver will be computing the software checksum and writing it in to
> the data, so the wifi driver will also be seeing it. For many drivers
> that probably won't matter, but is that always true?
>
> (Patches for the Broadcom wifi driver will be coming once we've worked
> out the best way of fixing this - there is no error path easily
> available if the skb_cow_head call fails).
>
You misread what the driver does.
The TCP data (payload) is _not_ modified.
Only additional headers are pushed in front of the existing (Ethernet,
IP, TCP) headers.
For this, skb_cow_head() is the perfect solution.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox