* Re: [PATCH v5 05/10] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY
From: Andrew Lunn @ 2017-09-14 19:19 UTC (permalink / raw)
To: Corentin Labbe
Cc: Rob Herring, Maxime Ripard, mark.rutland, wens, linux,
catalin.marinas, will.deacon, peppe.cavallaro, alexandre.torgue,
f.fainelli, netdev, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170914185301.GB4021@Red>
> > Is the MDIO controller "allwinner,sun8i-h3-emac" or "snps,dwmac-mdio"?
> > If the latter, then I think the node is fine, but then the mux should be
> > a child node of it. IOW, the child of an MDIO controller should either
> > be a mux node or slave devices.
Hi Rob
Up until now, children of an MDIO bus have been MDIO devices. Those
MDIO devices are either Ethernet PHYs, Ethernet Switches, or the
oddball devices that Broadcom iProc has, like generic PHYs.
We have never had MDIO-muxes as MDIO children. A Mux is not an MDIO
device, and does not have the properties of an MDIO device. It is not
addressable on the MDIO bus. The current MUXes are addressed via GPIOs
or MMIO.
There other similar cases. i2c-mux-gpio is not a child of an i2c bus,
nor i2c-mux-reg or gpio-mux. nxp,pca9548 is however a child of the i2c
bus, because it is an i2c device itself...
If the MDIO mux was an MDIO device, i would agree with you. Bit it is
not, so lets not make it a child.
Andrew
^ permalink raw reply
* feature request for kernel module 8021q
From: Pierre Colombier @ 2017-09-14 19:14 UTC (permalink / raw)
To: netdev
Hello
One big problem with vlans on linux is that the native interface always
get all vlans.
This can cause problem with bridges.
One interesting feature would be to have a special vlan number
(let's say 0 or -1 or 4097 ) designed so that it gets all the untagged
trafic and only the untagged trafic.
what do you think about this ?
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Maxim Uvarov @ 2017-09-14 19:59 UTC (permalink / raw)
To: Vivien Didelot
Cc: Greg KH, netdev, linux-kernel, kernel, David S. Miller,
Florian Fainelli, Andrew Lunn, Egil Hjelmeland, John Crispin,
Woojung Huh, Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <87h8wdb8bj.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
debugfs here is very very useful to read registers directly and
compare what use space tools see. Cool feature to get regs by port and
use standard tools to diff and print them. Even might be better to
allow drivers to decode register names and bits values. Once that is
done driver mainaince will be much easy. I.e. you need only match regs
with spec from one side and regs with user space tools from other
side. Of course it's needed only for debuging, not for production. But
even for production regs dump on something wrong might tell a lot.
Maxim.
2017-09-08 16:58 GMT+03:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> Hi Greg,
>
> Greg KH <gregkh@linuxfoundation.org> writes:
>
>> I agree you shouldn't be using debugfs for this, but in the future, if
>> you do write debugfs code, please take the following review into
>> account:
>
> Humm sorry I may not have given enough details. This was really meant
> for debug and dev only, because DSA makes it hard to query directly the
> hardware (some switch ports are not exposed to userspace as well.)
>
> This is not meant to be used for anything real at all, or even be
> compiled-in in a production kernel. That's why I found it appropriate.
>
> So I am still wondering why it doesn't fit here, can you tell me why?
>
>> You should _never_ care about the return value of a debugfs call, and
>> you should not need to ever propagate the error upward. The api was
>> written to not need this.
>>
>> Just call the function, and return, that's it. If you need to save the
>> return value (i.e. it's a dentry), you also don't care, just save it and
>> pass it to some other debugfs call, and all will still be fine. Your
>> code should never do anything different if a debugfs call succeeds or
>> fails.
>
> Thank for your interesting review! I'll cleanup my out-of-tree patches.
>
>
> Vivien
--
Best regards,
Maxim Uvarov
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Alexander Duyck @ 2017-09-14 20:12 UTC (permalink / raw)
To: Maxim Uvarov
Cc: Vivien Didelot, Greg KH, netdev, linux-kernel@vger.kernel.org,
kernel, David S. Miller, Florian Fainelli, Andrew Lunn,
Egil Hjelmeland, John Crispin, Woojung Huh, Sean Wang,
Nikita Yushchenko, Chris Healy
In-Reply-To: <CAJGZr0KdOPW5_V3kvWutwTY=BJhfd-=yRDPnvbDBOCuMYWj9OQ@mail.gmail.com>
On Thu, Sep 14, 2017 at 12:59 PM, Maxim Uvarov <muvarov@gmail.com> wrote:
> debugfs here is very very useful to read registers directly and
> compare what use space tools see. Cool feature to get regs by port and
> use standard tools to diff and print them. Even might be better to
> allow drivers to decode register names and bits values. Once that is
> done driver mainaince will be much easy. I.e. you need only match regs
> with spec from one side and regs with user space tools from other
> side. Of course it's needed only for debuging, not for production. But
> even for production regs dump on something wrong might tell a lot.
>
> Maxim.
Can you clarify what type of registers it is you are wanting to read?
We already have ethtool which is meant to allow reading the device
registers for a given netdev. As long as the port has a netdev
associated it then there is no need to be getting into debugfs since
we should probably just be using ethtool.
Also as Jiri pointed out there is already devlink which would probably
be a better way to get the associated information for those pieces
that don't have a netdev associated with them.
- Alex
^ permalink raw reply
* Re: Page allocator bottleneck
From: Andi Kleen @ 2017-09-14 20:19 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Jesper Dangaard Brouer, Mel Gorman, Eric Dumazet,
Alexei Starovoitov, Saeed Mahameed, Eran Ben Elisha,
Linux Kernel Network Developers, Andrew Morton, Michal Hocko,
linux-mm
In-Reply-To: <cef85936-10b2-5d76-9f97-cb03b418fd94@mellanox.com>
Tariq Toukan <tariqt@mellanox.com> writes:
>
> Congestion in this case is very clear.
> When monitored in perf top:
> 85.58% [kernel] [k] queued_spin_lock_slowpath
Please look at the callers. Spinlock profiles without callers
are usually useless because it's just blaming the messenger.
Most likely the PCP lists are too small for your extreme allocation
rate, so it goes back too often to the shared pool.
You can play with the vm.percpu_pagelist_fraction setting.
-Andi
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-14 21:01 UTC (permalink / raw)
To: Alexander Duyck
Cc: Maxim Uvarov, Vivien Didelot, Greg KH, netdev,
linux-kernel@vger.kernel.org, kernel, David S. Miller,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <CAKgT0Uf8--Yh9tfkTrh9_dfEYMX5jR6j4unxwxM+WD6E4jk9pA@mail.gmail.com>
> Can you clarify what type of registers it is you are wanting to read?
> We already have ethtool which is meant to allow reading the device
> registers for a given netdev. As long as the port has a netdev
> associated it then there is no need to be getting into debugfs since
> we should probably just be using ethtool.
Not all ports of a DSA switch have a netdev. This is by design. The
presentation we gave to Netdev 2.1 gives some of the background.
Plus a switch has a lot of registers not associated to port. Often a
switch has more global registers than port registers.
> Also as Jiri pointed out there is already devlink which would probably
> be a better way to get the associated information for those pieces
> that don't have a netdev associated with them.
We have looked at the devlink a few times. The current dpipe code is
not generic enough. It makes assumptions about the architecture of the
switch, that it is all match/action based. The niche of top of rack
switches might be like that, but average switches are not.
If dpipe was to support simple generic two dimensional tables, we
probably would use it.
David suggested making a class device for DSA. It is not ideal, but we
are probably going to go that way.
Andrew
^ permalink raw reply
* [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Willem de Bruijn @ 2017-09-14 21:14 UTC (permalink / raw)
To: netdev; +Cc: davem, nixiaoming, Willem de Bruijn
Packet socket bind operations must hold the po->bind_lock. This keeps
po->running consistent with whether the socket is actually on a ptype
list to receive packets.
fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
binds the fanout object to receive through packet_rcv_fanout.
Make it hold the po->bind_lock when testing po->running and rebinding.
Else, it can race with other rebind operations, such as that in
packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
can result in a socket being added to a fanout group twice, causing
use-after-free KASAN bug reports, among others.
Reported independently by both trinity and syzkaller.
Verified that the syzkaller reproducer passes after this patch.
Reported-by: nixioaming <nixiaoming@huawei.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/packet/af_packet.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c26172995511..d288f52c53f7 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
mutex_lock(&fanout_mutex);
- err = -EINVAL;
- if (!po->running)
- goto out;
-
err = -EALREADY;
if (po->fanout)
goto out;
@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
list_add(&match->list, &fanout_list);
}
err = -EINVAL;
- if (match->type == type &&
+
+ spin_lock(&po->bind_lock);
+ if (po->running &&
+ match->type == type &&
match->prot_hook.type == po->prot_hook.type &&
match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
err = 0;
}
}
+ spin_unlock(&po->bind_lock);
+
+ if (err && !refcount_read(&match->sk_ref)) {
+ list_del(&match->list);
+ kfree(match);
+ }
+
out:
if (err && rollover) {
kfree(rollover);
--
2.14.1.690.gbb1197296e-goog
^ permalink raw reply related
* Re: Bug with BPF_ALU64 | BPF_END?
From: Y Song @ 2017-09-14 21:22 UTC (permalink / raw)
To: David Miller; +Cc: Edward Cree, Daniel Borkmann, Alexei Starovoitov, netdev
In-Reply-To: <20170914.111424.934672262035736390.davem@davemloft.net>
On Thu, Sep 14, 2017 at 11:14 AM, David Miller <davem@davemloft.net> wrote:
> From: Edward Cree <ecree@solarflare.com>
> Date: Thu, 14 Sep 2017 18:53:17 +0100
>
>> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
Yes, only BPF_ALU. The below is LLVM bpf swap insn encoding:
...
// bswap16, bswap32, bswap64
class BSWAP ...
...
let op = 0xd; // BPF_END
let BPFSrc = 1; // BPF_TO_BE (TODO: use BPF_TO_LE for big-endian target)
let BPFClass = 4; // BPF_ALU
...
>> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
>> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
>> But the verifier doesn't enforce this; by crafting a program that uses
>> these opcodes I can get a WARN when they're run (without JIT; it looks
>> like the x86 JIT, at least, won't like it either).
>> Proposed patch below the cut; build-tested only.
>
> Good catch.
>
> A really neat test would be a program that uploads random BPF programs
> into the kernel, in a syzkaller'ish way. It might have triggered this
> (eventually).
>
^ permalink raw reply
* Re: feature request for kernel module 8021q
From: Florian Fainelli @ 2017-09-14 22:16 UTC (permalink / raw)
To: Pierre Colombier, netdev
In-Reply-To: <c010d65d-3050-8d88-22aa-cac092dd3904@abeille.com>
On September 14, 2017 12:14:04 PM PDT, Pierre Colombier <pierre@abeille.com> wrote:
>Hello
>
>
>One big problem with vlans on linux is that the native interface always
>
>get all vlans.
>
>This can cause problem with bridges.
What specific problem are you thinking about?
>
>
>One interesting feature would be to have a special vlan number
>
>(let's say 0 or -1 or 4097 ) designed so that it gets all the untagged
>trafic and only the untagged trafic.
4097 is not a valid number nor is -1 since that closely follows what the VLAN ID extracted from the tag would be. VLAN 0 is more or less the untagged VLAN.
>
>what do you think about this ?
Provided the HW supports it, with two netdevice operations: ndo_rx_vlan_add_vid and ndo_rx_vlan_kill_vid you can have your HW filter out the VLAN tags it wishes to receive, respectively not receive anymore. If your device does not support that, then yes if you send frames with a VLAN tag for which you are not interested in processing, you are still receiving it unless there is a switch or any kind of VLAN filtering device in between.
Bridge has also gained a VLAN filtering that can also be used to only the reception/transmission of desired VLAN tags. And it also allows you to define the default VLAN tag (VID1)
--
Florian
^ permalink raw reply
* Re: [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Eric Dumazet @ 2017-09-14 22:53 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, davem, nixiaoming
In-Reply-To: <20170914211441.67326-1-willemb@google.com>
On Thu, 2017-09-14 at 17:14 -0400, Willem de Bruijn wrote:
> Packet socket bind operations must hold the po->bind_lock. This keeps
> po->running consistent with whether the socket is actually on a ptype
> list to receive packets.
>
> fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
> binds the fanout object to receive through packet_rcv_fanout.
>
> Make it hold the po->bind_lock when testing po->running and rebinding.
> Else, it can race with other rebind operations, such as that in
> packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
> can result in a socket being added to a fanout group twice, causing
> use-after-free KASAN bug reports, among others.
>
> Reported independently by both trinity and syzkaller.
> Verified that the syzkaller reproducer passes after this patch.
>
> Reported-by: nixioaming <nixiaoming@huawei.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> net/packet/af_packet.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c26172995511..d288f52c53f7 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
>
> mutex_lock(&fanout_mutex);
>
> - err = -EINVAL;
> - if (!po->running)
> - goto out;
> -
> err = -EALREADY;
> if (po->fanout)
> goto out;
> @@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> list_add(&match->list, &fanout_list);
> }
> err = -EINVAL;
> - if (match->type == type &&
> +
> + spin_lock(&po->bind_lock);
> + if (po->running &&
> + match->type == type &&
> match->prot_hook.type == po->prot_hook.type &&
> match->prot_hook.dev == po->prot_hook.dev) {
> err = -ENOSPC;
> @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> err = 0;
> }
> }
> + spin_unlock(&po->bind_lock);
> +
> + if (err && !refcount_read(&match->sk_ref)) {
It seems sk_ref is always read/changed under
mutex_lock(&fanout_mutex) protection.
Not sure why we use a refcount_t (or an atomic_t in older kernels)
All these atomic/spinlock/mutexes are a maze.
> + list_del(&match->list);
> + kfree(match);
> + }
> +
> out:
> if (err && rollover) {
> kfree(rollover);
^ permalink raw reply
* Re: Bug with BPF_ALU64 | BPF_END?
From: Daniel Borkmann @ 2017-09-14 23:04 UTC (permalink / raw)
To: Edward Cree, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <52c7df55-84d9-f6d2-ed84-51ac90eb6bcc@solarflare.com>
On 09/14/2017 07:53 PM, Edward Cree wrote:
> Is BPF_END supposed to only be used with BPF_ALU, never with BPF_ALU64?
> In kernel/bpf/core.c:___bpf_prog_run(), there are only jump table targets
> for the BPF_ALU case, not for the BPF_ALU64 case (opcodes 0xd7 and 0xdf).
> But the verifier doesn't enforce this; by crafting a program that uses
> these opcodes I can get a WARN when they're run (without JIT; it looks
> like the x86 JIT, at least, won't like it either).
> Proposed patch below the cut; build-tested only.
>
> -Ed
> ---
>
> [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
>
> Neither ___bpf_prog_run nor the JITs accept it.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
Good catch! Can you submit this as an official patch for -net together
with a test case for tools/testing/selftests/bpf/test_verifier.c?
Thanks!
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> kernel/bpf/verifier.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 477b693..799b245 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> }
> } else {
> if (insn->src_reg != BPF_REG_0 || insn->off != 0 ||
> - (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) {
> + (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) ||
> + BPF_CLASS(insn->code) == BPF_ALU64) {
> verbose("BPF_END uses reserved fields\n");
> return -EINVAL;
> }
>
^ permalink raw reply
* RE: [PATCH] e1000e: apply burst mode settings only on default
From: Brown, Aaron F @ 2017-09-14 23:48 UTC (permalink / raw)
To: Willem de Bruijn, Kirsher, Jeffrey T
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
Brandeburg, Jesse, Willem de Bruijn
In-Reply-To: <20170825150626.2843-1-willemdebruijn.kernel@gmail.com>
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Willem de Bruijn
> Sent: Friday, August 25, 2017 8:06 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Brandeburg,
> Jesse <jesse.brandeburg@intel.com>; Willem de Bruijn
> <willemb@google.com>
> Subject: [PATCH] e1000e: apply burst mode settings only on default
>
> From: Willem de Bruijn <willemb@google.com>
>
> Devices that support FLAG2_DMA_BURST have different default values
> for RDTR and RADV. Apply burst mode default settings only when no
> explicit value was passed at module load.
>
> The RDTR default is zero. If the module is loaded for low latency
> operation with RxIntDelay=0, do not override this value with a burst
> default of 32.
>
> Move the decision to apply burst values earlier, where explicitly
> initialized module variables can be distinguished from defaults.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/ethernet/intel/e1000e/e1000.h | 4 ----
> drivers/net/ethernet/intel/e1000e/netdev.c | 8 --------
> drivers/net/ethernet/intel/e1000e/param.c | 16 +++++++++++++++-
> 3 files changed, 15 insertions(+), 13 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH net-next v3] e1000e: Be drop monitor friendly
From: Brown, Aaron F @ 2017-09-14 23:54 UTC (permalink / raw)
To: Florian Fainelli, netdev@vger.kernel.org
Cc: edumazet@gmail.com, open list,
moderated list:INTEL ETHERNET DRIVERS, davem@davemloft.net
In-Reply-To: <20170826011424.27251-1-f.fainelli@gmail.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Florian Fainelli
> Sent: Friday, August 25, 2017 6:14 PM
> To: netdev@vger.kernel.org
> Cc: edumazet@gmail.com; Florian Fainelli <f.fainelli@gmail.com>; open list
> <linux-kernel@vger.kernel.org>; moderated list:INTEL ETHERNET DRIVERS
> <intel-wired-lan@lists.osuosl.org>; davem@davemloft.net
> Subject: [Intel-wired-lan] [PATCH net-next v3] e1000e: Be drop monitor
> friendly
>
> e1000e_put_txbuf() can be called from normal reclamation path as well as
> when a DMA mapping failure, so we need to differentiate these two cases
> when freeing SKBs to be drop monitor friendly. e1000e_tx_hwtstamp_work()
> and e1000_remove() are processing TX timestamped SKBs and those should
> not be accounted as drops either.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
>
> - differentiate normal reclamation from TX DMA fragment mapping errors
> - removed a few invalid dev_kfree_skb() replacements (those are already
> drop monitor friendly)
>
> Changes in v2:
>
> - make it compile
>
> drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 1/5] e1000e: Fix error path in link detection
From: Brown, Aaron F @ 2017-09-15 0:18 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-1-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 1/5] e1000e: Fix error path in link detection
>
> In case of error from e1e_rphy(), the loop will exit early and "success"
> will be set to true erroneously.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/phy.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 3/5] e1000e: Fix return value test
From: Brown, Aaron F @ 2017-09-15 0:20 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-3-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 3/5] e1000e: Fix return value test
>
> All the helpers return -E1000_ERR_PHY.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for link check/link up
From: Brown, Aaron F @ 2017-09-15 0:27 UTC (permalink / raw)
To: Benjamin Poirier
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>
On 7/21/2017 21:36, Benjamin Poirier wrote:
> Lennart reported the following race condition:
>
> \ e1000_watchdog_task
> \ e1000e_has_link
> \ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link
> /* link is up */
> mac->get_link_status = false;
>
> /* interrupt */
> \ e1000_msix_other
> hw->mac.get_link_status = true;
>
> link_active = !hw->mac.get_link_status
> /* link_active is false, wrongly */
>
> This problem arises because the single flag get_link_status is used to
> signal two different states: link status needs checking and link status is
> down.
>
> Avoid the problem by using the return value of .check_for_link to signal
> the link status to e1000e_has_link().
>
> Reported-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
> 2 files changed, 9 insertions(+), 4 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 5/5] e1000e: Avoid receiver overrun interrupt bursts
From: Brown, Aaron F @ 2017-09-15 0:38 UTC (permalink / raw)
To: Benjamin Poirier, Kirsher, Jeffrey T
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org, Lennart Sorensen
In-Reply-To: <20170721183627.13373-5-bpoirier@suse.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf
> Of Benjamin Poirier
> Sent: Friday, July 21, 2017 11:36 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Subject: [Intel-wired-lan] [PATCH 5/5] e1000e: Avoid receiver overrun
> interrupt bursts
>
> When e1000e_poll() is not fast enough to keep up with incoming traffic, the
> adapter (when operating in msix mode) raises the Other interrupt to signal
> Receiver Overrun.
>
> This is a double problem because 1) at the moment e1000_msix_other()
> assumes that it is only called in case of Link Status Change and 2) if the
> condition persists, the interrupt is repeatedly raised again in quick
> succession.
>
> Ideally we would configure the Other interrupt to not be raised in case of
> receiver overrun but this doesn't seem possible on this adapter. Instead,
> we handle the first part of the problem by reverting to the practice of
> reading ICR in the other interrupt handler, like before commit 16ecba59bc33
> ("e1000e: Do not read ICR in Other interrupt"). Thanks to commit
> 0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask") which cleared IAME
> from CTRL_EXT, reading ICR doesn't interfere with RxQ0, TxQ0 interrupts
> anymore. We handle the second part of the problem by not re-enabling the
> Other interrupt right away when there is overrun. Instead, we wait until
> traffic subsides, napi polling mode is exited and interrupts are
> re-enabled.
>
> Reported-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> Fixes: 16ecba59bc33 ("e1000e: Do not read ICR in Other interrupt")
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/intel/e1000e/defines.h | 1 +
> drivers/net/ethernet/intel/e1000e/netdev.c | 33
> +++++++++++++++++++++++------
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
I get an error and a few warnings out of checkpatch from this, but I think the error is false (thinking the reference to a commit in the description is this commit, a fixes commit or something like that) and I'm more concerned with the fix than the warnings, so...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Here is the checkpatch output in case anyone has a different opinion on the severity:
-------------
u1484:[0]/usr/src/kernels/next-queue> git format-patch d81d1e6 -1 --stdout|./scripts/checkpatch.pl -
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask")'
#20:
0a8047ac68e5 ("e1000e: Fix msi-x interrupt automask") which cleared IAME
WARNING: braces {} are not necessary for single statement blocks
#73: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:1931:
+ if (!test_bit(__E1000_DOWN, &adapter->state)) {
+ mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ }
WARNING: braces {} are not necessary for single statement blocks
#83: FILE: drivers/net/ethernet/intel/e1000e/netdev.c:1936:
+ if (enable && !test_bit(__E1000_DOWN, &adapter->state)) {
ew32(IMS, E1000_IMS_OTHER);
}
total: 1 errors, 2 warnings, 0 checks, 59 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Your patch has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
u1484:[0]/usr/src/kernels/next-queue>
^ permalink raw reply
* [PATCH 1/1] forcedeth: replace pci_map_single with dma_map_single functions
From: Zhu Yanjun @ 2017-09-15 3:01 UTC (permalink / raw)
To: jarod, netdev
pci_map_single functions are obsolete. So replace them with
dma_map_single functions.
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 70 +++++++++++++++++----------------
1 file changed, 36 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index e6e0de4..0a7ba3a 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -1812,12 +1812,12 @@ static int nv_alloc_rx(struct net_device *dev)
struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
if (skb) {
np->put_rx_ctx->skb = skb;
- np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
+ np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
skb->data,
skb_tailroom(skb),
- PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(np->pci_dev,
- np->put_rx_ctx->dma)) {
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pci_dev->dev,
+ np->put_rx_ctx->dma)) {
kfree_skb(skb);
goto packet_dropped;
}
@@ -1853,12 +1853,12 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
if (skb) {
np->put_rx_ctx->skb = skb;
- np->put_rx_ctx->dma = pci_map_single(np->pci_dev,
+ np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
skb->data,
skb_tailroom(skb),
- PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(np->pci_dev,
- np->put_rx_ctx->dma)) {
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pci_dev->dev,
+ np->put_rx_ctx->dma)) {
kfree_skb(skb);
goto packet_dropped;
}
@@ -1975,9 +1975,9 @@ static void nv_unmap_txskb(struct fe_priv *np, struct nv_skb_map *tx_skb)
{
if (tx_skb->dma) {
if (tx_skb->dma_single)
- pci_unmap_single(np->pci_dev, tx_skb->dma,
+ dma_unmap_single(&np->pci_dev->dev, tx_skb->dma,
tx_skb->dma_len,
- PCI_DMA_TODEVICE);
+ DMA_TO_DEVICE);
else
pci_unmap_page(np->pci_dev, tx_skb->dma,
tx_skb->dma_len,
@@ -2045,10 +2045,10 @@ static void nv_drain_rx(struct net_device *dev)
}
wmb();
if (np->rx_skb[i].skb) {
- pci_unmap_single(np->pci_dev, np->rx_skb[i].dma,
+ dma_unmap_single(&np->pci_dev->dev, np->rx_skb[i].dma,
(skb_end_pointer(np->rx_skb[i].skb) -
- np->rx_skb[i].skb->data),
- PCI_DMA_FROMDEVICE);
+ np->rx_skb[i].skb->data),
+ DMA_FROM_DEVICE);
dev_kfree_skb(np->rx_skb[i].skb);
np->rx_skb[i].skb = NULL;
}
@@ -2221,10 +2221,11 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
prev_tx = put_tx;
prev_tx_ctx = np->put_tx_ctx;
bcnt = (size > NV_TX2_TSO_MAX_SIZE) ? NV_TX2_TSO_MAX_SIZE : size;
- np->put_tx_ctx->dma = pci_map_single(np->pci_dev, skb->data + offset, bcnt,
- PCI_DMA_TODEVICE);
- if (pci_dma_mapping_error(np->pci_dev,
- np->put_tx_ctx->dma)) {
+ np->put_tx_ctx->dma = dma_map_single(&np->pci_dev->dev,
+ skb->data + offset, bcnt,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(&np->pci_dev->dev,
+ np->put_tx_ctx->dma)) {
/* on DMA mapping error - drop the packet */
dev_kfree_skb_any(skb);
u64_stats_update_begin(&np->swstats_tx_syncp);
@@ -2369,10 +2370,11 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
prev_tx = put_tx;
prev_tx_ctx = np->put_tx_ctx;
bcnt = (size > NV_TX2_TSO_MAX_SIZE) ? NV_TX2_TSO_MAX_SIZE : size;
- np->put_tx_ctx->dma = pci_map_single(np->pci_dev, skb->data + offset, bcnt,
- PCI_DMA_TODEVICE);
- if (pci_dma_mapping_error(np->pci_dev,
- np->put_tx_ctx->dma)) {
+ np->put_tx_ctx->dma = dma_map_single(&np->pci_dev->dev,
+ skb->data + offset, bcnt,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(&np->pci_dev->dev,
+ np->put_tx_ctx->dma)) {
/* on DMA mapping error - drop the packet */
dev_kfree_skb_any(skb);
u64_stats_update_begin(&np->swstats_tx_syncp);
@@ -2805,9 +2807,9 @@ static int nv_rx_process(struct net_device *dev, int limit)
* TODO: check if a prefetch of the first cacheline improves
* the performance.
*/
- pci_unmap_single(np->pci_dev, np->get_rx_ctx->dma,
- np->get_rx_ctx->dma_len,
- PCI_DMA_FROMDEVICE);
+ dma_unmap_single(&np->pci_dev->dev, np->get_rx_ctx->dma,
+ np->get_rx_ctx->dma_len,
+ DMA_FROM_DEVICE);
skb = np->get_rx_ctx->skb;
np->get_rx_ctx->skb = NULL;
@@ -2911,9 +2913,9 @@ static int nv_rx_process_optimized(struct net_device *dev, int limit)
* TODO: check if a prefetch of the first cacheline improves
* the performance.
*/
- pci_unmap_single(np->pci_dev, np->get_rx_ctx->dma,
- np->get_rx_ctx->dma_len,
- PCI_DMA_FROMDEVICE);
+ dma_unmap_single(&np->pci_dev->dev, np->get_rx_ctx->dma,
+ np->get_rx_ctx->dma_len,
+ DMA_FROM_DEVICE);
skb = np->get_rx_ctx->skb;
np->get_rx_ctx->skb = NULL;
@@ -5065,11 +5067,11 @@ static int nv_loopback_test(struct net_device *dev)
ret = 0;
goto out;
}
- test_dma_addr = pci_map_single(np->pci_dev, tx_skb->data,
+ test_dma_addr = dma_map_single(&np->pci_dev->dev, tx_skb->data,
skb_tailroom(tx_skb),
- PCI_DMA_FROMDEVICE);
- if (pci_dma_mapping_error(np->pci_dev,
- test_dma_addr)) {
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pci_dev->dev,
+ test_dma_addr)) {
dev_kfree_skb_any(tx_skb);
goto out;
}
@@ -5124,9 +5126,9 @@ static int nv_loopback_test(struct net_device *dev)
}
}
- pci_unmap_single(np->pci_dev, test_dma_addr,
- (skb_end_pointer(tx_skb) - tx_skb->data),
- PCI_DMA_TODEVICE);
+ dma_unmap_single(&np->pci_dev->dev, test_dma_addr,
+ (skb_end_pointer(tx_skb) - tx_skb->data),
+ DMA_TO_DEVICE);
dev_kfree_skb_any(tx_skb);
out:
/* stop engines */
--
2.7.4
^ permalink raw reply related
* [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: Xin Long @ 2017-09-15 3:02 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, pabeni
Commit 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the
dump") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep
with holding sock and sock lock.
But Paolo noticed that endpoint could be destroyed in sctp_rcv without
sock lock protection. It means the use-after-free issue still could be
triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks
!ep, although it's pretty hard to reproduce.
I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close
and sctp_sock_dump long time.
This patch is to add another param cb_done to sctp_for_each_transport
and dump ep->assocs with holding tsp after jumping out of transport's
traversal in it to avoid this issue.
It can also improve sctp diag dump to make it run faster, as no need
to save sk into cb->args[5] and keep calling sctp_for_each_transport
any more.
This patch is also to use int * instead of int for the pos argument
in sctp_for_each_transport, which could make postion increment only
in sctp_for_each_transport and no need to keep changing cb->args[2]
in sctp_sock_filter and sctp_sock_dump any more.
Fixes: 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the dump")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/sctp.h | 3 ++-
net/sctp/sctp_diag.c | 32 +++++++++-----------------------
net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------
3 files changed, 36 insertions(+), 39 deletions(-)
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 06b4f51..d7d8cba 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -127,7 +127,8 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),
const union sctp_addr *laddr,
const union sctp_addr *paddr, void *p);
int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
- struct net *net, int pos, void *p);
+ int (*cb_done)(struct sctp_transport *, void *),
+ struct net *net, int *pos, void *p);
int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void *p);
int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
struct sctp_info *info);
diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
index e99518e..7008a99 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -279,9 +279,11 @@ static int sctp_tsp_dump_one(struct sctp_transport *tsp, void *p)
return err;
}
-static int sctp_sock_dump(struct sock *sk, void *p)
+static int sctp_sock_dump(struct sctp_transport *tsp, void *p)
{
+ struct sctp_endpoint *ep = tsp->asoc->ep;
struct sctp_comm_param *commp = p;
+ struct sock *sk = ep->base.sk;
struct sk_buff *skb = commp->skb;
struct netlink_callback *cb = commp->cb;
const struct inet_diag_req_v2 *r = commp->r;
@@ -289,9 +291,7 @@ static int sctp_sock_dump(struct sock *sk, void *p)
int err = 0;
lock_sock(sk);
- if (!sctp_sk(sk)->ep)
- goto release;
- list_for_each_entry(assoc, &sctp_sk(sk)->ep->asocs, asocs) {
+ list_for_each_entry(assoc, &ep->asocs, asocs) {
if (cb->args[4] < cb->args[1])
goto next;
@@ -327,40 +327,30 @@ static int sctp_sock_dump(struct sock *sk, void *p)
cb->args[4]++;
}
cb->args[1] = 0;
- cb->args[2]++;
cb->args[3] = 0;
cb->args[4] = 0;
release:
release_sock(sk);
- sock_put(sk);
return err;
}
-static int sctp_get_sock(struct sctp_transport *tsp, void *p)
+static int sctp_sock_filter(struct sctp_transport *tsp, void *p)
{
struct sctp_endpoint *ep = tsp->asoc->ep;
struct sctp_comm_param *commp = p;
struct sock *sk = ep->base.sk;
- struct netlink_callback *cb = commp->cb;
const struct inet_diag_req_v2 *r = commp->r;
struct sctp_association *assoc =
list_entry(ep->asocs.next, struct sctp_association, asocs);
/* find the ep only once through the transports by this condition */
if (tsp->asoc != assoc)
- goto out;
+ return 0;
if (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family)
- goto out;
-
- sock_hold(sk);
- cb->args[5] = (long)sk;
+ return 0;
return 1;
-
-out:
- cb->args[2]++;
- return 0;
}
static int sctp_ep_dump(struct sctp_endpoint *ep, void *p)
@@ -503,12 +493,8 @@ static void sctp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
if (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE)))
goto done;
-next:
- cb->args[5] = 0;
- sctp_for_each_transport(sctp_get_sock, net, cb->args[2], &commp);
-
- if (cb->args[5] && !sctp_sock_dump((struct sock *)cb->args[5], &commp))
- goto next;
+ sctp_for_each_transport(sctp_sock_filter, sctp_sock_dump,
+ net, (int *)&cb->args[2], &commp);
done:
cb->args[1] = cb->args[4];
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 1b00a1e..d4730ad 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4658,29 +4658,39 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),
EXPORT_SYMBOL_GPL(sctp_transport_lookup_process);
int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
- struct net *net, int pos, void *p) {
+ int (*cb_done)(struct sctp_transport *, void *),
+ struct net *net, int *pos, void *p) {
struct rhashtable_iter hti;
- void *obj;
- int err;
-
- err = sctp_transport_walk_start(&hti);
- if (err)
- return err;
+ struct sctp_transport *tsp;
+ int ret;
- obj = sctp_transport_get_idx(net, &hti, pos + 1);
- for (; !IS_ERR_OR_NULL(obj); obj = sctp_transport_get_next(net, &hti)) {
- struct sctp_transport *transport = obj;
+again:
+ ret = sctp_transport_walk_start(&hti);
+ if (ret)
+ return ret;
- if (!sctp_transport_hold(transport))
+ tsp = sctp_transport_get_idx(net, &hti, *pos + 1);
+ for (; !IS_ERR_OR_NULL(tsp); tsp = sctp_transport_get_next(net, &hti)) {
+ if (!sctp_transport_hold(tsp))
continue;
- err = cb(transport, p);
- sctp_transport_put(transport);
- if (err)
+ ret = cb(tsp, p);
+ if (ret)
break;
+ (*pos)++;
+ sctp_transport_put(tsp);
}
sctp_transport_walk_stop(&hti);
- return err;
+ if (ret) {
+ if (cb_done && !cb_done(tsp, p)) {
+ (*pos)++;
+ sctp_transport_put(tsp);
+ goto again;
+ }
+ sctp_transport_put(tsp);
+ }
+
+ return ret;
}
EXPORT_SYMBOL_GPL(sctp_for_each_transport);
--
2.1.0
^ permalink raw reply related
* [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: Xin Long @ 2017-09-15 3:02 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: davem, Marcelo Ricardo Leitner, Neil Horman
sctp_diag would not actually dump out sk/asoc if inet_sctp_diag_fill
returns err, in which case it shouldn't mark sk dumped by setting
cb->args[3] as 1 in sctp_sock_dump().
Otherwise, it could cause some asocs to have no parent's sk dumped
in 'ss --sctp'.
So this patch is to not set cb->args[3] when inet_sctp_diag_fill()
returns err in sctp_sock_dump().
Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/sctp_diag.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
index 7008a99..22ed01a 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -309,7 +309,6 @@ static int sctp_sock_dump(struct sctp_transport *tsp, void *p)
cb->nlh->nlmsg_seq,
NLM_F_MULTI, cb->nlh,
commp->net_admin) < 0) {
- cb->args[3] = 1;
err = 1;
goto release;
}
--
2.1.0
^ permalink raw reply related
* Re: [iproute PATCH v2] ipaddress: Fix segfault in 'addr showdump'
From: Julien Fortin @ 2017-09-15 3:05 UTC (permalink / raw)
To: Phil Sutter; +Cc: Stephen Hemminger, Hangbin Liu, netdev
In-Reply-To: <20170913092034.7002-1-phil@nwl.cc>
v2 looks good to me, thanks for catching this segfault.
On Wed, Sep 13, 2017 at 2:20 AM, Phil Sutter <phil@nwl.cc> wrote:
> Obviously, 'addr showdump' feature wasn't adjusted to json output
> support. As a consequence, calls to print_string() in print_addrinfo()
> tried to dereference a NULL FILE pointer.
>
> Cc: Julien Fortin <julien@cumulusnetworks.com>
> Fixes: d0e720111aad2 ("ip: ipaddress.c: add support for json output")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Acked-by: Julien Fortin <julien@cumulusnetworks.com>
> --
> Changes since v1:
> Align json output with that of 'ip -j addr show':
> - Interface index label is 'ifindex', not 'index' and it doesn't belong
> to 'addr_info' array.
> - Create one 'addr_info' array per dumped address, not one for all.
> ---
> ip/ipaddress.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 9797145023966..4c47809570410 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -1801,17 +1801,33 @@ static int show_handler(const struct sockaddr_nl *nl,
> {
> struct ifaddrmsg *ifa = NLMSG_DATA(n);
>
> - printf("if%d:\n", ifa->ifa_index);
> + open_json_object(NULL);
> + print_int(PRINT_ANY, "ifindex", "if%d:\n", ifa->ifa_index);
> +
> + open_json_array(PRINT_JSON, "addr_info");
> + open_json_object(NULL);
> +
> print_addrinfo(NULL, n, stdout);
> +
> + close_json_object();
> + close_json_array(PRINT_JSON, NULL);
> +
> + close_json_object();
> return 0;
> }
>
> static int ipaddr_showdump(void)
> {
> + int err;
> +
> if (ipadd_dump_check_magic())
> exit(-1);
>
> - exit(rtnl_from_file(stdin, &show_handler, NULL));
> + new_json_obj(json, stdout);
> + err = rtnl_from_file(stdin, &show_handler, NULL);
> + delete_json_obj();
> +
> + exit(err);
> }
>
> static int restore_handler(const struct sockaddr_nl *nl,
> --
> 2.13.1
>
^ permalink raw reply
* Re: Regression in throughput between kvm guests over virtual bridge
From: Matthew Rosato @ 2017-09-15 3:36 UTC (permalink / raw)
To: Jason Wang, netdev; +Cc: davem, mst
In-Reply-To: <50891c14-3fc6-f519-8c03-07bdef3090f4@redhat.com>
> Is the issue gone if you reduce VHOST_RX_BATCH to 1? And it would be
> also helpful to collect perf diff to see if anything interesting.
> (Consider 4.4 shows more obvious regression, please use 4.4).
>
Issue still exists when I force VHOST_RX_BATCH = 1
Collected perf data, with 4.12 as the baseline, 4.13 as delta1 and
4.13+VHOST_RX_BATCH=1 as delta2. All guests running 4.4. Same scenario,
2 uperf client guests, 2 uperf slave guests - I collected perf data
against 1 uperf client process and 1 uperf slave process. Here are the
significant diffs:
uperf client:
75.09% +9.32% +8.52% [kernel.kallsyms] [k] enabled_wait
9.04% -4.11% -3.79% [kernel.kallsyms] [k] __copy_from_user
2.30% -0.79% -0.71% [kernel.kallsyms] [k] arch_free_page
2.17% -0.65% -0.58% [kernel.kallsyms] [k] arch_alloc_page
0.69% -0.25% -0.24% [kernel.kallsyms] [k] get_page_from_freelist
0.56% +0.08% +0.14% [kernel.kallsyms] [k] virtio_ccw_kvm_notify
0.42% -0.11% -0.09% [kernel.kallsyms] [k] tcp_sendmsg
0.31% -0.15% -0.14% [kernel.kallsyms] [k] tcp_write_xmit
uperf slave:
72.44% +8.99% +8.85% [kernel.kallsyms] [k] enabled_wait
8.99% -3.67% -3.51% [kernel.kallsyms] [k] __copy_to_user
2.31% -0.71% -0.67% [kernel.kallsyms] [k] arch_free_page
2.16% -0.67% -0.63% [kernel.kallsyms] [k] arch_alloc_page
0.89% -0.14% -0.11% [kernel.kallsyms] [k] virtio_ccw_kvm_notify
0.71% -0.30% -0.30% [kernel.kallsyms] [k] get_page_from_freelist
0.70% -0.25% -0.29% [kernel.kallsyms] [k] __wake_up_sync_key
0.61% -0.22% -0.22% [kernel.kallsyms] [k] virtqueue_add_inbuf
>
> May worth to try disable zerocopy or do the test form host to guest
> instead of guest to guest to exclude the possible issue of sender.
>
With zerocopy disabled, still seeing the regression. The provided perf
#s have zerocopy enabled.
I replaced 1 uperf guest and instead ran that uperf client as a host
process, pointing at a guest. All traffic still over the virtual
bridge. In this setup, it's still easy to see the regression for the
remaining guest1<->guest2 uperf run, but the host<->guest3 run does NOT
exhibit a reliable regression pattern. The significant perf diffs from
the host uperf process (baseline=4.12, delta=4.13):
59.96% +5.03% [kernel.kallsyms] [k] enabled_wait
6.47% -2.27% [kernel.kallsyms] [k] raw_copy_to_user
5.52% -1.63% [kernel.kallsyms] [k] raw_copy_from_user
0.87% -0.30% [kernel.kallsyms] [k] get_page_from_freelist
0.69% +0.30% [kernel.kallsyms] [k] finish_task_switch
0.66% -0.15% [kernel.kallsyms] [k] swake_up
0.58% -0.00% [vhost] [k] vhost_get_vq_desc
...
0.42% +0.50% [kernel.kallsyms] [k] ckc_irq_pending
I also tried flipping the uperf stream around (a guest uperf client is
communicating to a slave uperf process on the host) and also cannot see
the regression pattern. So it seems to require a guest on both ends of
the connection.
^ permalink raw reply
* [PATCH net] ip6_gre: skb_push ipv6hdr before packing the header in ip6gre_header
From: Xin Long @ 2017-09-15 4:00 UTC (permalink / raw)
To: network dev; +Cc: davem, Dmitry Kozlov
Now in ip6gre_header before packing the ipv6 header, it skb_push t->hlen
which only includes encap_hlen + tun_hlen. It means greh and inner header
would be over written by ipv6 stuff and ipv6h might have no chance to set
up.
Jianlin found this issue when using remote any on ip6_gre, the packets he
captured on gre dev are truncated:
22:50:26.210866 Out ethertype IPv6 (0x86dd), length 120: truncated-ip6 -\
8128 bytes missing!(flowlabel 0x92f40, hlim 0, next-header Options (0) \
payload length: 8192) ::1:2000:0 > ::1:0:86dd: HBH [trunc] ip-proto-128 \
8184
It should also skb_push ipv6hdr so that ipv6h points to the right position
to set ipv6 stuff up.
This patch is to skb_push hlen + sizeof(*ipv6h) and also fix some indents
in ip6gre_header.
Fixes: c12b395a4664 ("gre: Support GRE over IPv6")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv6/ip6_gre.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index b7a72d4..20f66f4 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -940,24 +940,25 @@ static int ip6gre_tunnel_ioctl(struct net_device *dev,
}
static int ip6gre_header(struct sk_buff *skb, struct net_device *dev,
- unsigned short type,
- const void *daddr, const void *saddr, unsigned int len)
+ unsigned short type, const void *daddr,
+ const void *saddr, unsigned int len)
{
struct ip6_tnl *t = netdev_priv(dev);
- struct ipv6hdr *ipv6h = skb_push(skb, t->hlen);
- __be16 *p = (__be16 *)(ipv6h+1);
+ struct ipv6hdr *ipv6h;
+ __be16 *p;
- ip6_flow_hdr(ipv6h, 0,
- ip6_make_flowlabel(dev_net(dev), skb,
- t->fl.u.ip6.flowlabel, true,
- &t->fl.u.ip6));
+ ipv6h = skb_push(skb, t->hlen + sizeof(*ipv6h));
+ ip6_flow_hdr(ipv6h, 0, ip6_make_flowlabel(dev_net(dev), skb,
+ t->fl.u.ip6.flowlabel,
+ true, &t->fl.u.ip6));
ipv6h->hop_limit = t->parms.hop_limit;
ipv6h->nexthdr = NEXTHDR_GRE;
ipv6h->saddr = t->parms.laddr;
ipv6h->daddr = t->parms.raddr;
- p[0] = t->parms.o_flags;
- p[1] = htons(type);
+ p = (__be16 *)(ipv6h + 1);
+ p[0] = t->parms.o_flags;
+ p[1] = htons(type);
/*
* Set the source hardware address.
--
2.1.0
^ permalink raw reply related
* Re: [REGRESSION] Warning in tcp_fastretrans_alert() of net/ipv4/tcp_input.c
From: Oleksandr Natalenko @ 2017-09-15 5:03 UTC (permalink / raw)
To: Neal Cardwell
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Netdev,
Yuchung Cheng
In-Reply-To: <CADVnQy=dzjROPgrOw3vAZvtb9ETRJgTPVGtau8Q3ChteDKnYow@mail.gmail.com>
Hi.
I've applied your test patch but it doesn't fix the issue for me since the
warning is still there.
Were you able to reproduce it?
On pondělí 11. září 2017 1:59:02 CEST Neal Cardwell wrote:
> Thanks for the detailed report!
>
> I suspect this is due to the following commit, which happened between
> 4.10 and 4.11:
>
> 89fe18e44f7e tcp: extend F-RTO to catch more spurious timeouts
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?
> id=89fe18e44f7e
>
> This commit expanded the set of scenarios where we would undo a
> CA_Loss cwnd reduction and return to TCP_CA_Open, but did not include
> a check to see if there were any in-flight retransmissions. I think we
> need a fix like the following:
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 659d1baefb2b..730a2de9d2b0 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -2439,7 +2439,7 @@ static bool tcp_try_undo_loss(struct sock *sk,
> bool frto_undo)
> {
> struct tcp_sock *tp = tcp_sk(sk);
>
> - if (frto_undo || tcp_may_undo(tp)) {
> + if ((frto_undo || tcp_may_undo(tp)) && !tp->retrans_out) {
> tcp_undo_cwnd_reduction(sk, true);
>
> DBGUNDO(sk, "partial loss");
>
> I will try a packetdrill test to see if I can reproduce this issue and
> verify the fix.
>
> thanks,
> neal
^ permalink raw reply
* [PATCH 1/1] ipv6_skip_exthdr: use ipv6_authlen for AH header length computation
From: Xiang Gao @ 2017-09-15 5:04 UTC (permalink / raw)
To: trivial, netdev, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
>From 09cf2e3cf09cf591283785aaa8159baf39ac2e08 Mon Sep 17 00:00:00 2001
From: Xiang Gao <qasdfgtyuiop@gmail.com>
Date: Fri, 15 Sep 2017 00:44:12 -0400
Subject: [PATCH] ipv6_skip_exthdr: use ipv6_authlen for AH hdrlen
In ipv6_skip_exthdr, the lengh of AH header is computed manually
as (hp->hdrlen+2)<<2. However, in include/linux/ipv6.h, a macro
named ipv6_authlen is already defined for exactly the same job. This
commit replaces the manual computation code with the macro.
---
net/ipv6/exthdrs_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
index 305e2ed730bf..115d60919f72 100644
--- a/net/ipv6/exthdrs_core.c
+++ b/net/ipv6/exthdrs_core.c
@@ -99,7 +99,7 @@ int ipv6_skip_exthdr(const struct sk_buff *skb, int
start, u8 *nexthdrp,
break;
hdrlen = 8;
} else if (nexthdr == NEXTHDR_AUTH)
- hdrlen = (hp->hdrlen+2)<<2;
+ hdrlen = ipv6_authlen(hp);
else
hdrlen = ipv6_optlen(hp);
--
2.14.1
Signed-off-by: Xiang Gao <qasdfgtyuiop@gmail.com>
^ permalink raw reply related
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