* [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
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Jiri Pirko @ 2017-09-15 5:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alexander Duyck, 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: <20170914210132.GC3084@lunn.ch>
Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
>> 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.
I believe that is also big mistake.
Could you put together your requirements so we can work it out to extend
devlink to support them?
Thanks.
^ permalink raw reply
* [PATCH v2] net/ethernet/freescale: fix warning for ucc_geth
From: Valentin Longchamp @ 2017-09-15 5:58 UTC (permalink / raw)
To: leoli, netdev, christophe.leroy; +Cc: linuxppc-dev, Valentin Longchamp
uf_info.regs is resource_size_t i.e. phys_addr_t that can be either u32
or u64 according to CONFIG_PHYS_ADDR_T_64BIT.
The printk format is thus adaptet to u64 and the regs value cast to u64
to take both u32 and u64 into account.
Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---
drivers/net/ethernet/freescale/ucc_geth.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index f77ba9fa257b..a96b838cffce 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3857,8 +3857,9 @@ static int ucc_geth_probe(struct platform_device* ofdev)
}
if (netif_msg_probe(&debug))
- pr_info("UCC%1d at 0x%8x (irq = %d)\n",
- ug_info->uf_info.ucc_num + 1, ug_info->uf_info.regs,
+ pr_info("UCC%1d at 0x%8llx (irq = %d)\n",
+ ug_info->uf_info.ucc_num + 1,
+ (u64)ug_info->uf_info.regs,
ug_info->uf_info.irq);
/* Create an ethernet device instance */
--
2.13.5
^ permalink raw reply related
* [PATCH net] net: phy: Fix mask value write on gmii2rgmii converter speed register
From: Fahad Kunnathadi @ 2017-09-15 6:31 UTC (permalink / raw)
To: f.fainelli
Cc: michal.simek, davem, andrew, appanad, soren.brinkmann, netdev,
linux-kernel, Fahad Kunnathadi
To clear Speed Selection in MDIO control register(0x10),
ie, clear bits 6 and 13 to zero while keeping other bits same.
Before AND operation,The Mask value has to be perform with bitwise NOT
operation (ie, ~ operator)
This patch clears current speed selection before writing the
new speed settings to gmii2rgmii converter
Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
Signed-off-by: Fahad Kunnathadi <fahad.kunnathadi@dexceldesigns.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/phy/xilinx_gmii2rgmii.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index d15dd39..2e5150b 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -44,7 +44,7 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
priv->phy_drv->read_status(phydev);
val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);
- val &= XILINX_GMII2RGMII_SPEED_MASK;
+ val &= ~XILINX_GMII2RGMII_SPEED_MASK;
if (phydev->speed == SPEED_1000)
val |= BMCR_SPEED1000;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] net: phy: Fix mask value write on gmii2rgmii converter speed register.
From: Fahad Kunnathadi @ 2017-09-15 7:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: f fainelli, netdev, Michal Simek, linux-kernel, soren brinkmann,
linux-arm-kernel
In-Reply-To: <20170914143454.GB27601@lunn.ch>
Hi Andrew,
Thank you for review and comments.
I will review this driver code with any review comments before, and update you if I could find any..
Best Regards
Fahad K
----- Original Message -----
From: "Andrew Lunn" <andrew@lunn.ch>
To: "Fahad Kunnathadi" <fahad.kunnathadi@dexceldesigns.com>
Cc: "f fainelli" <f.fainelli@gmail.com>, netdev@vger.kernel.org, "Michal Simek" <michal.simek@xilinx.com>, linux-kernel@vger.kernel.org, "soren brinkmann" <soren.brinkmann@xilinx.com>, linux-arm-kernel@lists.infradead.org
Sent: Thursday, September 14, 2017 8:04:54 PM
Subject: Re: [PATCH] net: phy: Fix mask value write on gmii2rgmii converter speed register.
On Thu, Sep 14, 2017 at 12:46:31PM +0530, Fahad Kunnathadi wrote:
> To clear Speed Selection in MDIO control register(0x10),
> ie, clear bits 6 and 13 to zero while keeping other bits same.
> Before AND operation,The Mask value has to be perform with bitwise NOT
> operation (ie, ~ operator)
>
> This patch clears current speed selection before writing the
> new speed settings to gmii2rgmii converter
Hi Fahad
I expect you will find other issues with this driver. I pointed some
out at the time it is submitted, but the developers went quiet as soon
as it was accepted.
Anyway, please ensure David Miller <davem@davemloft.net> gets a copy.
The subject line should be:
[PATCH net] net: phy: Fix mask value write on gmii2rgmii converter speed register.
and include a fixes tag:
Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: Page allocator bottleneck
From: Jesper Dangaard Brouer @ 2017-09-15 7:28 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Mel Gorman, Eric Dumazet, Alexei Starovoitov,
Saeed Mahameed, Eran Ben Elisha, Linux Kernel Network Developers,
Andrew Morton, Michal Hocko, linux-mm, brouer
In-Reply-To: <cef85936-10b2-5d76-9f97-cb03b418fd94@mellanox.com>
On Thu, 14 Sep 2017 19:49:31 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:
> Hi all,
>
> As part of the efforts to support increasing next-generation NIC speeds,
> I am investigating SW bottlenecks in network stack receive flow.
>
> Here I share some numbers I got for a simple experiment, in which I
> simulate the page allocation rate needed in 200Gpbs NICs.
Thanks for bringing this up again.
> I ran the test below over 3 different (modified) mlx5 driver versions,
> loaded on server side (RX):
> 1) RX page cache disabled, 2 packets per page.
2 packets per page basically reduce the overhead you see from the page
allocator to half.
> 2) RX page cache disabled, one packet per page.
This, should stress the page allocator.
> 3) Huge RX page cache, one packet per page.
A driver level page-cache will look nice, as long as it "works".
Drivers usually have no other option than basing their recycle facility
to be based on the page-refcnt (as there is no destructor callback).
Which implies packets/pages need to be returned quickly enough for it
to work.
> All page allocations are of order 0.
>
> NIC: Connectx-5 100 Gbps.
> CPU: Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz
>
> Test:
> 128 TCP streams (using super_netperf).
> Changing num of RX queues.
> HW LRO OFF, GRO ON, MTU 1500.
With TCP streams and GRO, is actually a good stress test for the page
allocator (or drivers page-recycle cache). As Eric Dumazet have made
some nice optimizations, that (in most situations) cause us to quickly
free/recycle the SKB (coming from driver) and store the pages in 1-SKB.
This cause us to hit the SLUB fastpath for the SKBs, but once the pages
need to be free'ed this stress the page allocator more.
Also be aware that with TCP flows, the packets are likely delivered
into a socket, that is consumed on another CPU. Thus, the pages are
allocated on one CPU and free'ed on another. AFAIK this stress the
order-0 cache PCP (Per-Cpu-Pages).
> Observe: BW as a function of num RX queues.
>
> Results:
>
> Driver #1:
> #rings BW (Mbps)
> 1 23,813
> 2 44,086
> 3 62,128
> 4 78,058
> 6 94,210 (linerate)
> 8 94,205 (linerate)
> 12 94,202 (linerate)
> 16 94,191 (linerate)
>
> Driver #2:
> #rings BW (Mbps)
> 1 18,835
> 2 36,716
> 3 50,521
> 4 61,746
> 6 63,637
> 8 60,299
> 12 51,048
> 16 43,337
>
> Driver #3:
> #rings BW (Mbps)
> 1 19,316
> 2 44,850
> 3 69,549
> 4 87,434
> 6 94,342 (linerate)
> 8 94,350 (linerate)
> 12 94,327 (linerate)
> 16 94,327 (linerate)
>
>
> Insights:
> Major degradation between #1 and #2, not getting any close to linerate!
> Degradation is fixed between #2 and #3.
> This is because page allocator cannot stand the higher allocation rate.
> In #2, we also see that the addition of rings (cores) reduces BW (!!),
> as result of increasing congestion over shared resources.
>
> Congestion in this case is very clear.
> When monitored in perf top:
> 85.58% [kernel] [k] queued_spin_lock_slowpath
Well, we obviously need to know the caller of the spin_lock. In this
case it is likely the page allocator lock. It could also be the TCP
socket locks, but given GRO is enabled, they should be hit much less.
> I think that page allocator issues should be discussed separately:
> 1) Rate: Increase the allocation rate on a single core.
> 2) Scalability: Reduce congestion and sync overhead between cores.
Yes, but this no small task. I is on my TODO-list (emacs org-mode),
but I have other tasks that have higher priority atm. I'll be working
on XDP_REDIRECT for the next many months. Currently trying to convince
people that we do an explicit packet-page return/free callback (which
would avoid many of these issues).
> This is clearly the current bottleneck in the network stack receive
> flow.
>
> I know about some efforts that were made in the past two years.
> For example the ones from Jesper et al.:
>
> - Page-pool (not accepted AFAIK).
The page-pool have many purposes.
1. generic page-cache for drivers,
2. keep pages DMA-mapped
3. facilitate drivers to change RX-ring memory model
From a MM-point-of-view the page pool is just a destructor callback,
that can "steal" the page.
If I can convince XDP_REDIRECT to use an explicit destructor callback,
then I almost get what I need. Except for the generic part, and the
normal network path will not see the benefit. Thus, not helping your
use-case, I guess.
> - Page-allocation bulking.
Notice, that page-allocator bulking, would still be needed by the
page-pool and other page-cache facilities. We should implement it
regardless of the page_pool.
Without a page pool facility to hide the use of page bulking. You
could use page-bulk-alloc in driver RX-ring refill, and find where TCP
free the GRO packets, and do page-bulk-free there.
> - Optimize order-0 allocations in Per-Cpu-Pages.
There is a need to optimize PCP some more for the single-core XDP
performance target (~14Mpps). I guess, the easiest way around this is
implement/integrate a page bulk API into PCP.
The TCP-GRO use-case you are hitting is a different bottleneck.
It is a multi-CPU parallel workload, that exceed the PCP cache size,
and cause you to hit the page buddy allocator.
I wonder if you could "solve"/mitigate the issue if you tune the size
of the PCP cache?
AFAIK it only keeps 128 pages cached per CPU... I know you can see this
via a proc file, but I cannot remember which(?). And I'm not sure how
you tune this(?)
> I am not an mm expert, but wanted to raise the issue again, to combine
> the efforts and hear from you guys about status and possible directions.
Regarding recent changes... if you have you kernel compiled with
CONFIG_NUMA then the page-allocator is slower (due to keeping
numa-stats), except that this was recently optimized and merged(?)
What (exact) kernel git tree did you run these tests on?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Egil Hjelmeland @ 2017-09-15 7:35 UTC (permalink / raw)
To: Jiri Pirko, Andrew Lunn
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, Greg KH, netdev,
linux-kernel@vger.kernel.org, kernel, David S. Miller,
Florian Fainelli, John Crispin, Woojung Huh, Sean Wang,
Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915055107.GA1927@nanopsycho.orion>
On 15. sep. 2017 07:51, Jiri Pirko wrote:
> Thu, Sep 14, 2017 at 11:01:32PM CEST, andrew@lunn.ch wrote:
>>> 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.
>
> I believe that is also big mistake.
>
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
>
> Thanks.
>
$ ack -i devlink Documentation/
$ ack -i dpipe Documentation/
$
How you expect new mechanisms to be taken into use with zero documentation?
To all: Why does reviewers nitpick about undocumented formatting rules,
but not ask about documentation?
Egil
^ permalink raw reply
* Re: [PATCH] net: phy: Fix mask value write on gmii2rgmii converter speed register.
From: Michal Simek @ 2017-09-15 7:49 UTC (permalink / raw)
To: Andrew Lunn, Fahad Kunnathadi
Cc: f.fainelli, netdev, michal.simek, linux-kernel, soren.brinkmann,
linux-arm-kernel
In-Reply-To: <20170914143454.GB27601@lunn.ch>
On 14.9.2017 16:34, Andrew Lunn wrote:
> On Thu, Sep 14, 2017 at 12:46:31PM +0530, Fahad Kunnathadi wrote:
>> To clear Speed Selection in MDIO control register(0x10),
>> ie, clear bits 6 and 13 to zero while keeping other bits same.
>> Before AND operation,The Mask value has to be perform with bitwise NOT
>> operation (ie, ~ operator)
>>
>> This patch clears current speed selection before writing the
>> new speed settings to gmii2rgmii converter
>
> Hi Fahad
>
> I expect you will find other issues with this driver. I pointed some
> out at the time it is submitted, but the developers went quiet as soon
> as it was accepted.
Can you please point me to that email?
I will create ticket about it in our system to get them resolved.
Thanks,
Michal
^ permalink raw reply
* [PATCH net] ip6_tunnel: do not allow loading ip6_tunnel if ipv6 is disabled in cmdline
From: Xin Long @ 2017-09-15 7:58 UTC (permalink / raw)
To: network dev; +Cc: davem, Dmitry Kozlov, jbenc
If ipv6 has been disabled from cmdline since kernel started, it makes
no sense to allow users to create any ip6 tunnel. Otherwise, it could
some potential problem.
Jianlin found a kernel crash caused by this in ip6_gre when he set
ipv6.disable=1 in grub:
[ 209.588865] Unable to handle kernel paging request for data at address 0x00000080
[ 209.588872] Faulting instruction address: 0xc000000000a3aa6c
[ 209.588879] Oops: Kernel access of bad area, sig: 11 [#1]
[ 209.589062] NIP [c000000000a3aa6c] fib_rules_lookup+0x4c/0x260
[ 209.589071] LR [c000000000b9ad90] fib6_rule_lookup+0x50/0xb0
[ 209.589076] Call Trace:
[ 209.589097] fib6_rule_lookup+0x50/0xb0
[ 209.589106] rt6_lookup+0xc4/0x110
[ 209.589116] ip6gre_tnl_link_config+0x214/0x2f0 [ip6_gre]
[ 209.589125] ip6gre_newlink+0x138/0x3a0 [ip6_gre]
[ 209.589134] rtnl_newlink+0x798/0xb80
[ 209.589142] rtnetlink_rcv_msg+0xec/0x390
[ 209.589151] netlink_rcv_skb+0x138/0x150
[ 209.589159] rtnetlink_rcv+0x48/0x70
[ 209.589169] netlink_unicast+0x538/0x640
[ 209.589175] netlink_sendmsg+0x40c/0x480
[ 209.589184] ___sys_sendmsg+0x384/0x4e0
[ 209.589194] SyS_sendmsg+0xd4/0x140
[ 209.589201] SyS_socketcall+0x3e0/0x4f0
[ 209.589209] system_call+0x38/0xe0
This patch is to return -EOPNOTSUPP in ip6_tunnel_init if ipv6 has been
disabled from cmdline.
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv6/ip6_tunnel.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index ae73164..f2f21c2 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -2259,6 +2259,9 @@ static int __init ip6_tunnel_init(void)
{
int err;
+ if (!ipv6_mod_enabled())
+ return -EOPNOTSUPP;
+
err = register_pernet_device(&ip6_tnl_net_ops);
if (err < 0)
goto out_pernet;
--
2.1.0
^ permalink raw reply related
* Job Offer In USA
From: Valero Energy Recruitment @ 2017-09-15 8:12 UTC (permalink / raw)
To: Recipients
Valero Energy Oil & Gas Company USA
Employment Opportunity in Texas, USA.
Oil & Gas Sector ( NEW PLANT RIG )
Salary Range $7,000 USD - $11,700 Per Month
Kindly send your CV/RESUME for more details regarding our Job Program, Employment offers for all Offices.
CC: Forward All Documents/Certificates & Resume to: jobs.valerousa@gmail.com
^ permalink raw reply
* Re: [PATCH 0/3] Fix y2038 issues for security/keys subsystem
From: Baolin Wang @ 2017-09-15 8:38 UTC (permalink / raw)
To: David Howells
Cc: David Miller, James Morris, Serge E. Hallyn, marc.dionne,
Dan Carpenter, Jason A. Donenfeld, Arnd Bergmann, Mark Brown,
keyrings, LKML, linux-security-module, Networking
In-Reply-To: <CAMz4kuL-mriUk3YV6Mz9xgC7geNe5a05vhjfXaxt58x=7BUo0w@mail.gmail.com>
Hi James,
On 21 August 2017 at 20:12, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi David and James,
>
> On 9 August 2017 at 16:28, David Howells <dhowells@redhat.com> wrote:
>> The rxrpc patch isn't part of the security/keys subsystem. I'll push it
>> to the network tree. The other two I'll push to James.
>
> Could you apply this patch serials if there are no other comments? Thanks.
Could you apply patch1 and patch2? I have seen patch3 was merged into
4.14 by David. Thanks.
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: renesas-ravb: Add support for R8A77995 RAVB
From: Simon Horman @ 2017-09-15 8:40 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Yoshihiro Shimoda, davem, robh+dt, mark.rutland, netdev,
linux-renesas-soc, devicetree
In-Reply-To: <de2287b5-f208-1ffd-e8a4-3779a11695f7@cogentembedded.com>
On Thu, Sep 14, 2017 at 12:44:58PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 9/14/2017 3:06 AM, Yoshihiro Shimoda wrote:
>
> >Add a new compatible string for the R8A77995 (R-Car D3) RAVB.
> >
> >Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Usually those are added after sign-off.
>
> >Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>
> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> >---
> > Changes from v1:
> > - Based on r8a77970 patch.
> > https://marc.info/?l=linux-renesas-soc&m=150524655515476
>
> So this is against net-next, right? The net.git repo now has a patch
> making me a reviewer for the Renesas bindings too.
Acked-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* Re: Regression in throughput between kvm guests over virtual bridge
From: Jason Wang @ 2017-09-15 8:55 UTC (permalink / raw)
To: Matthew Rosato, netdev; +Cc: davem, mst
In-Reply-To: <e42083d9-ae7b-7207-e5e6-06483bf6293e@linux.vnet.ibm.com>
On 2017年09月15日 11:36, Matthew Rosato wrote:
>> 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
Interesting, so this looks more like an issue of the changes in
vhost_net instead of batch dequeuing itself. I try this on Intel but
still can't meet it.
>
> 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
It looks like vhost is slowed down for some reason which leads to more
idle time on 4.13+VHOST_RX_BATCH=1. Appreciated if you can collect the
perf.diff on host, one for rx and one for tx.
>
>
>> 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
Another hint to perf vhost threads.
>
> 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.
>
Yes. Will try to get a s390 environment.
Thanks
^ permalink raw reply
* [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
From: Sabrina Dubroca @ 2017-09-15 9:08 UTC (permalink / raw)
To: netdev; +Cc: Sabrina Dubroca, Guillaume Nault, Xin Long
The tunnel is currently removed from the list during destruction. This
can lead to a double-free of the struct sock if we try to delete the tunnel
twice fast enough.
The first delete operation does a lookup (l2tp_tunnel_get), finds the
tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
l2tp_tunnel_del_work.
The second delete operation also finds the tunnel and calls
l2tp_tunnel_delete. If the workqueue has already fired and started
running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
same tunnel a second time, and try to free the socket again.
Add a dead flag and remove tunnel from its list earlier. Then we can
remove the check of queue_work's result that was meant to prevent that
race but doesn't.
Reproducer:
ip l2tp add tunnel tunnel_id 3000 peer_tunnel_id 4000 local 192.168.0.2 remote 192.168.0.1 encap udp udp_sport 5000 udp_dport 6000
ip l2tp add session name l2tp1 tunnel_id 3000 session_id 1000 peer_session_id 2000
ip link set l2tp1 up
ip l2tp del tunnel tunnel_id 3000
ip l2tp del tunnel tunnel_id 3000
Fixes: f8ccac0e4493 ("l2tp: put tunnel socket release on a workqueue")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
net/l2tp/l2tp_core.c | 30 ++++++++++++++++--------------
net/l2tp/l2tp_core.h | 4 +++-
2 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ee485df73ccd..26553d3c4f8f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1245,7 +1245,6 @@ EXPORT_SYMBOL_GPL(l2tp_xmit_skb);
static void l2tp_tunnel_destruct(struct sock *sk)
{
struct l2tp_tunnel *tunnel = l2tp_tunnel(sk);
- struct l2tp_net *pn;
if (tunnel == NULL)
goto end;
@@ -1269,13 +1268,6 @@ static void l2tp_tunnel_destruct(struct sock *sk)
sk->sk_destruct = tunnel->old_sk_destruct;
sk->sk_user_data = NULL;
- /* Remove the tunnel struct from the tunnel list */
- pn = l2tp_pernet(tunnel->l2tp_net);
- spin_lock_bh(&pn->l2tp_tunnel_list_lock);
- list_del_rcu(&tunnel->list);
- spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
- atomic_dec(&l2tp_tunnel_count);
-
l2tp_tunnel_closeall(tunnel);
tunnel->sock = NULL;
@@ -1685,14 +1677,24 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
/* This function is used by the netlink TUNNEL_DELETE command.
*/
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
{
+ struct l2tp_net *pn;
+
+ if (tunnel->dead)
+ return;
+
l2tp_tunnel_inc_refcount(tunnel);
- if (false == queue_work(l2tp_wq, &tunnel->del_work)) {
- l2tp_tunnel_dec_refcount(tunnel);
- return 1;
- }
- return 0;
+
+ /* Remove the tunnel struct from the tunnel list */
+ pn = l2tp_pernet(tunnel->l2tp_net);
+ spin_lock_bh(&pn->l2tp_tunnel_list_lock);
+ tunnel->dead = 1;
+ list_del_rcu(&tunnel->list);
+ spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
+ atomic_dec(&l2tp_tunnel_count);
+
+ queue_work(l2tp_wq, &tunnel->del_work);
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a305e0c5925a..173e68bb8119 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,6 +160,8 @@ struct l2tp_tunnel_cfg {
struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
+ int dead;
+
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
bool acpt_newsess; /* Indicates whether this
@@ -254,7 +256,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
struct l2tp_tunnel **tunnelp);
void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
From: Tom Parkin @ 2017-09-15 9:42 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: netdev, Guillaume Nault, Xin Long
In-Reply-To: <f07e7ba88b9b4651900a888796e3f8816d06a8a8.1505396134.git.sd@queasysnail.net>
[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]
On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> The tunnel is currently removed from the list during destruction. This
> can lead to a double-free of the struct sock if we try to delete the tunnel
> twice fast enough.
>
> The first delete operation does a lookup (l2tp_tunnel_get), finds the
> tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> l2tp_tunnel_del_work.
>
> The second delete operation also finds the tunnel and calls
> l2tp_tunnel_delete. If the workqueue has already fired and started
> running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> same tunnel a second time, and try to free the socket again.
>
> Add a dead flag and remove tunnel from its list earlier. Then we can
> remove the check of queue_work's result that was meant to prevent that
> race but doesn't.
How do we avoid leaving stale information on the tunnel list for
use-cases which don't delete tunnels using netlink? For example the
L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
context on socket destruction. Similarly, userspace may just close
the tunnel socket without first making netlink calls to delete the
tunnel.
By moving the tunnel list removal from l2tp_tunnel_destruct to
l2tp_tunnel_delete I can't see how codepaths which don't involve
l2tp_tunnel_delete don't end up with a corrupted tunnel list.
Tom
--
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: RFC: Audit Kernel Container IDs
From: Richard Guy Briggs @ 2017-09-15 10:19 UTC (permalink / raw)
To: Carlos O'Donell
Cc: mszeredi-H+wXaHxf7aLQT0dZR+AlfA, jlayton-H+wXaHxf7aLQT0dZR+AlfA,
Linux API, Linux Containers, Linux Kernel, Eric Paris, Al Viro,
David Howells, Linux Audit, Andy Lutomirski,
Linux Network Development, Linux FS Devel,
cgroups-u79uwXL29TY76Z2rM5mHXA, Serge E. Hallyn,
trondmy-7I+n7zu2hftEKMMhf/gKZA, Eric W. Biederman
In-Reply-To: <20170914053007.GR3405-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On 2017-09-14 01:30, Richard Guy Briggs wrote:
> On 2017-09-13 14:33, Carlos O'Donell wrote:
> > On 09/13/2017 12:13 PM, Richard Guy Briggs wrote:
> > > Containers are a userspace concept. The kernel knows nothing of them.
> >
> > I am looking at this RFC from a userspace perspective, particularly from
> > the loader's point of view and the unshare syscall and the semantics that
> > arise from the use of it.
> >
> > At a high level what you are doing is providing a way to group, without
> > hierarchy, processes and namespaces. The processes can move between
> > container's if they have CAP_CONTAINER_ADMIN and can open and write to
> > a special proc file.
I should clarify: It wasn't intended that a process can see or modify
its own or a peer's special proc container file to be able to set it or
discover its value. This was only meant for its orchestrator or
delegated agents to do. This can't be left only to CAP_CONTAINER_ADMIN.
This may require a container to have its own mount namespace if the
trigger mechanism is a proc file write. Other methods (additional
namespaces?) may be needed to restrict it for other trigger methods
(syscall?).
> > * With unshare a thread may dissociate part of its execution context and
> > therefore see a distinct mount namespace. When you say "process" in this
> > particular RFC do you exclude the fact that a thread might be in a
> > distinct container from the rest of the threads in the process?
> >
> > > The Linux audit system needs a way to be able to track the container
> > > provenance of events and actions. Audit needs the kernel's help to do
> > > this.
> >
> > * Why does the Linux audit system need to tracker container provenance?
>
> - ability to filter unwanted, irrelevant or unimportant messages before
> they fill queue so important messages don't get lost. This is a
> certification requirement.
>
> - ability to make security claims about containers, require tracking of
> actions within those containers to ensure compliance with established
> security policies.
>
> - ability to route messages from events to relevant audit daemon
> instance or host audit daemon instance or both, as required or
> determined by user-initiated rules
>
> > - How does it help to provide better audit messages?
> >
> > - Is it be enough to list the namespace that a process occupies?
>
> We started with that approach back more than 4 years ago and found it
> helped, but didn't go far enough in terms of quick and inexpensive
> record filtering and left some doubt about provenance of events in the
> case of non-user context events (incoming network packets).
>
> > * Why does it need the kernel's help?
> >
> > - Is there a race condition that is only fixable with kernel support?
>
> This was a concern, but relatively minor compared with the other benefits.
>
> > - Or is it easier with kernel help but not required?
>
> It is much easier and much less expensive.
>
> > Providing background on these questions would help clarify the
> > design requirements.
>
> Here are some references that should help provide some background:
> https://github.com/linux-audit/audit-kernel/issues/32
> RFE: add namespace IDs to audit records
>
> https://github.com/linux-audit/audit-documentation/wiki/SPEC-Virtualization-Manager-Guest-Lifecycle-Events
> SPEC Virtualization Manager Guest Lifecycle Events
>
> https://lwn.net/Articles/699819/
> Audit, namespaces, and containers
>
> https://lwn.net/Articles/723561/
> Containers as kernel objects
> (my reply, with references: https://lkml.org/lkml/2017/8/14/15 )
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1045666
> audit: add namespace IDs to log records
>
> > > Since the concept of a container is entirely a userspace concept, a
> > > trigger signal from the userspace container orchestration system
> > > initiates this. This will define a point in time and a set of resources
> > > associated with a particular container with an audit container ID.
> >
> > Please don't use the word 'signal', I suggest 'register' since you are
> > writing to a filesystem.
>
> Ok, that's a very reasonable request. 'signal' has a previous meaning.
>
> > > The trigger is a pseudo filesystem (proc, since PID tree already exists)
> > > write of a u64 representing the container ID to a file representing a
> > > process that will become the first process in a new container.
> > > This might place restrictions on mount namespaces required to define a
> > > container, or at least careful checking of namespaces in the kernel to
> > > verify permissions of the orchestrator so it can't change its own
> > > container ID.
> > > A bind mount of nsfs may be necessary in the container orchestrator's
> > > mntNS.
> > >
> > > Require a new CAP_CONTAINER_ADMIN to be able to write to the pseudo
> > > filesystem to have this action permitted. At that time, record the
> > > child container's user-supplied 64-bit container identifier along with
> >
> > What is a "child container?" Containers don't have any hierarchy.
>
> Maybe some don't, but that's not likely to last long given the
> abstraction and nesting of orchestration tools. This must be nestable.
This is why we can't rely only on CAP_CONTAINER_ADMIN to restrict the
ability for self-modification or self-discovery.
> > I assume that if you don't have CAP_CONTAINER_ADMIN, that nothing prevents
> > your continued operation as we have today?
>
> Correct. It won't prevent processes that otherwise have permissions
> today from creating all the namespaces it wishes.
>
> > > the child container's first process (which may become the container's
> > > "init" process) process ID (referenced from the initial PID namespace),
> > > all namespace IDs (in the form of a nsfs device number and inode number
> > > tuple) in a new auxilliary record AUDIT_CONTAINER with a qualifying
> > > op=$action field.
> >
> > What kind of requirement is there on the first tid/pid registering
> > the container ID? What if the 8th tid/pid does the registration?
> > Would that mean that the first process of the container did not
> > register? It seems like you are suggesting that the registration
> > by the 8th tid/pid causes a cascading registration progress,
> > registering all tid/pids in the same grouping? Is that true?
>
> Ah, good question, I forgot to address that fact. The intent is that
> either threaded processes after initiating threading will not have
> permission to execute this, or all the processes in the thread group
> will be forced into the same container. I don't have a strong opinion
> on whether or not it must be the lead thread process that must be the
> one to receive that registration, but I suspect that would be wise.
>
> > > Issue a new auxilliary record AUDIT_CONTAINER_INFO for each valid
> > > container ID present on an auditable action or event.
> > >
> > > Forked and cloned processes inherit their parent's container ID,
> > > referenced in the process' audit_context struct.
> >
> > So a cloned process with CLONE_NEWNS has the came container ID
> > as the parent process that called clone, at least until the clone
> > has time to change to a new container ID?
>
> Yes.
And as pointed to above, it isn't the process itself that is able to
change to a new container, but its orchestrator to move/assign it.
> > Do you forsee any case where someone might need a semantic that is
> > slightly different? For example wanting to set the container ID on
> > clone?
>
> I could envision that situation and I think that might be workable but
> for the synchronicity of having one initiated by a specific syscall and
> the other initiated by a /proc write.
The ability to clone while providing a containerID would work really
well, but I'm hesitant to extend or duplicate the clone call. This
actually sounds like a potentially sane way of approaching it.
> > > Log the creation of every namespace, inheriting/adding its spawning
> > > process' containerID(s), if applicable. Include the spawning and
> > > spawned namespace IDs (device and inode number tuples).
> > > [AUDIT_NS_CREATE, AUDIT_NS_DESTROY] [clone(2), unshare(2), setns(2)]
> > > Note: At this point it appears only network namespaces may need to track
> > > container IDs apart from processes since incoming packets may cause an
> > > auditable event before being associated with a process.
> >
> > OK.
> >
> > > Log the destruction of every namespace when it is no longer used by any
> > > process, include the namespace IDs (device and inode number tuples).
> > > [AUDIT_NS_DESTROY] [process exit, unshare(2), setns(2)]
> > >
> > > Issue a new auxilliary record AUDIT_NS_CHANGE listing (opt: op=$action)
> > > the parent and child namespace IDs for any changes to a process'
> > > namespaces. [setns(2)]
> > > Note: It may be possible to combine AUDIT_NS_* record formats and
> > > distinguish them with an op=$action field depending on the fields
> > > required for each message type.
> > >
> > > A process can be moved from one container to another by using the
> > > container assignment method outlined above a second time.
> >
> > OK.
> >
> > > When a container ceases to exist because the last process in that
> > > container has exited and hence the last namespace has been destroyed and
> > > its refcount dropping to zero, log the fact.
> > > (This latter is likely needed for certification accountability.) A
> > > container object may need a list of processes and/or namespaces.
> >
> > OK.
> >
> > > A namespace cannot directly migrate from one container to another but
> > > could be assigned to a newly spawned container. A namespace can be
> > > moved from one container to another indirectly by having that namespace
> > > used in a second process in another container and then ending all the
> > > processes in the first container.
> >
> > OK.
> >
> > > Feedback please.
>
> Thank you sir!
>
> > Carlos.
>
> - RGB
>
> --
> Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: Page allocator bottleneck
From: Mel Gorman @ 2017-09-15 10:23 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Jesper Dangaard Brouer, 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>
On Thu, Sep 14, 2017 at 07:49:31PM +0300, Tariq Toukan wrote:
> Insights:
> Major degradation between #1 and #2, not getting any close to linerate!
> Degradation is fixed between #2 and #3.
> This is because page allocator cannot stand the higher allocation rate.
> In #2, we also see that the addition of rings (cores) reduces BW (!!), as
> result of increasing congestion over shared resources.
>
Unfortunately, no surprises there.
> Congestion in this case is very clear.
> When monitored in perf top:
> 85.58% [kernel] [k] queued_spin_lock_slowpath
>
While it's not proven, the most likely candidate is the zone lock and
that should be confirmed using a call-graph profile. If so, then the
suggestion to tune to the size of the per-cpu allocator would mitigate
the problem.
> I think that page allocator issues should be discussed separately:
> 1) Rate: Increase the allocation rate on a single core.
> 2) Scalability: Reduce congestion and sync overhead between cores.
>
> This is clearly the current bottleneck in the network stack receive flow.
>
> I know about some efforts that were made in the past two years.
> For example the ones from Jesper et al.:
> - Page-pool (not accepted AFAIK).
Indeed not and it would also need driver conversion.
> - Page-allocation bulking.
Prototypes exist but it's pointless without the pool or driver
conversion so it's in the back burner for the moment.
> - Optimize order-0 allocations in Per-Cpu-Pages.
>
This had a prototype that was reverted as it must be able to cope with
both irq and noirq contexts. Unfortunately I never found the time to
revisit it but a split there to handle both would mitigate the problem.
Probably not enough to actually reach line speed though so tuning of the
per-cpu allocator sizes would still be needed. I don't know when I'll
get the chance to revisit it. I'm travelling all next week and am mostly
occupied with other work at the moment that is consuming all my
concentration.
> I am not an mm expert, but wanted to raise the issue again, to combine the
> efforts and hear from you guys about status and possible directions.
The recent effort to reduce overhead from stats will help mitigate the
problem. Finishing the page pool, the bulk allocator and converting drivers
would be the most likely successful path forward but it's currently stalled
as everyone that was previously involved is too busy.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: Marcelo Ricardo Leitner @ 2017-09-15 12:55 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, pabeni
In-Reply-To: <9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:
> 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>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: Marcelo Ricardo Leitner @ 2017-09-15 12:55 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <c5398945d4e80ed8b8497675dd0449763fec4eec.1505444568.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:48AM +0800, Xin Long wrote:
> 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>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: Neil Horman @ 2017-09-15 12:57 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, pabeni
In-Reply-To: <9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:
> 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(-)
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: Neil Horman @ 2017-09-15 12:59 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <c5398945d4e80ed8b8497675dd0449763fec4eec.1505444568.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:48AM +0800, Xin Long wrote:
> 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(-)
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: feature request for kernel module 8021q
From: Pierre Colombier @ 2017-09-15 13:02 UTC (permalink / raw)
To: Florian Fainelli, netdev
In-Reply-To: <A4D84B7F-5D4A-44C8-875C-C113D7C5C688@gmail.com>
>> 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.
I know -1 or 4097 are invalid numbers
This is precicely why i suggested them as "special"
I wasn't aware of the existing Vlan 0 feature
I just tried again and it seems to be exactly what I was requesting.
What do you mean by "more or less" ?
^ permalink raw reply
* [patch net] mlxsw: spectrum_router: Only handle IPv4 and IPv6 events
From: Jiri Pirko @ 2017-09-15 13:31 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, flokli, andreas, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
The driver doesn't support events from address families other than IPv4
and IPv6, so ignore them. Otherwise, we risk queueing a work item before
it's initialized.
This can happen in case a VRF is configured when MROUTE_MULTIPLE_TABLES
is enabled, as the VRF driver will try to add an l3mdev rule for the
IPMR family.
Fixes: 65e65ec137f4 ("mlxsw: spectrum_router: Don't ignore IPv6 notifications")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Andreas Rammhold <andreas@rammhold.de>
Reported-by: Florian Klink <flokli@flokli.de>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index f0fb898..2cfb3f5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -4868,7 +4868,8 @@ static int mlxsw_sp_router_fib_event(struct notifier_block *nb,
struct fib_notifier_info *info = ptr;
struct mlxsw_sp_router *router;
- if (!net_eq(info->net, &init_net))
+ if (!net_eq(info->net, &init_net) ||
+ (info->family != AF_INET && info->family != AF_INET6))
return NOTIFY_DONE;
fib_work = kzalloc(sizeof(*fib_work), GFP_ATOMIC);
--
2.9.3
^ permalink raw reply related
* [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: Edward Cree @ 2017-09-15 13:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Daniel Borkmann
Neither ___bpf_prog_run nor the JITs accept it.
Also adds a new test case.
Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
kernel/bpf/verifier.c | 3 ++-
tools/testing/selftests/bpf/test_verifier.c | 16 ++++++++++++++++
2 files changed, 18 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;
}
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 8eb0995..26f3250 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -6629,6 +6629,22 @@ static struct bpf_test tests[] = {
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
+ {
+ "invalid 64-bit BPF_END",
+ .insns = {
+ BPF_MOV32_IMM(BPF_REG_0, 0),
+ {
+ .code = BPF_ALU64 | BPF_END | BPF_TO_LE,
+ .dst_reg = BPF_REG_0,
+ .src_reg = 0,
+ .off = 0,
+ .imm = 32,
+ },
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "BPF_END uses reserved fields",
+ .result = REJECT,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
^ permalink raw reply related
* Re: [REGRESSION] Warning in tcp_fastretrans_alert() of net/ipv4/tcp_input.c
From: Neal Cardwell @ 2017-09-15 14:03 UTC (permalink / raw)
To: Oleksandr Natalenko
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Netdev,
Yuchung Cheng
In-Reply-To: <1760119.uO98D2ft6f@natalenko.name>
On Fri, Sep 15, 2017 at 1:03 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> 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?
Hi,
Thanks for testing that. That is a very useful data point.
I was able to cook up a packetdrill test that could put the connection
in CA_Disorder with retransmitted packets out, but not in CA_Open. So
we do not yet have a test case to reproduce this.
We do not see this warning on our fleet at Google. One significant
difference I see between our environment and yours is that it seems
you run with FACK enabled:
net.ipv4.tcp_fack = 1
Note that FACK was disabled by default (since it was replaced by RACK)
between kernel v4.10 and v4.11. And this is exactly the time when this
bug started manifesting itself for you and some others, but not our
fleet. So my new working hypothesis would be that this warning is due
to a behavior that only shows up in kernels >=4.11 when FACK is
enabled.
Would you be able to disable FACK ("sysctl net.ipv4.tcp_fack=0" at
boot, or net.ipv4.tcp_fack=0 in /etc/sysctl.conf, or equivalent),
reboot, and test the kernel for a few days to see if the warning still
pops up?
thanks,
neal
[ps: apologies for the previous, mis-formatted post...]
^ 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