* [PATCH iproute2 v2 1/3] ip address: do not set nodad option for IPv4 addresses
From: Andrea Claudi @ 2019-06-25 10:29 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern
In-Reply-To: <cover.1561457597.git.aclaudi@redhat.com>
Duplicate Address Detection (RFC 4862) is available only for IPv6
addresses. As a consequence, 'nodad' option, turning it off, should
be available only for IPv6, and is defined like that in the man page.
However it is possible to set nodad on IPv4 addresses, too:
$ ip link add dummy0 type dummy
$ ip -4 addr add 192.168.1.1 dev dummy0 nodad
$ ip a
1: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 1a:6d:c6:96:ca:f8 brd ff:ff:ff:ff:ff:ff
inet 192.168.1.1/32 scope global nodad dummy0
valid_lft forever preferred_lft forever
Fix this adding a check on the protocol family before setting
IFA_F_NODAD flag.
Fixes: bac735c53a36d ("enabled to manipulate the flags of IFA_F_HOMEADDRESS or IFA_F_NODAD from ip.")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
ip/ipaddress.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 47e5be7462fe7..d157f72784a21 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2250,7 +2250,10 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
} else if (strcmp(*argv, "home") == 0) {
ifa_flags |= IFA_F_HOMEADDRESS;
} else if (strcmp(*argv, "nodad") == 0) {
- ifa_flags |= IFA_F_NODAD;
+ if (req.ifa.ifa_family == AF_INET6)
+ ifa_flags |= IFA_F_NODAD;
+ else
+ fprintf(stderr, "Warning: nodad option can be set only for IPv6 addresses\n");
} else if (strcmp(*argv, "mngtmpaddr") == 0) {
ifa_flags |= IFA_F_MANAGETEMPADDR;
} else if (strcmp(*argv, "noprefixroute") == 0) {
--
2.20.1
^ permalink raw reply related
* [PATCH iproute2 v2 2/3] ip address: do not set home option for IPv4 addresses
From: Andrea Claudi @ 2019-06-25 10:29 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern
In-Reply-To: <cover.1561457597.git.aclaudi@redhat.com>
'home' option designates a IPv6 address as "home address" as
defined in RFC 6275. This option should be available only for
IPv6 addresses, as correctly stated in the manpage.
However it is possible to set home on IPv4 addresses, too:
$ ip link add dummy0 type dummy
$ ip -4 addr add 192.168.1.1 dev dummy0 home
$ ip a
1: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 1a:6d:c6:96:ca:f8 brd ff:ff:ff:ff:ff:ff
inet 192.168.1.1/32 scope global home dummy0
valid_lft forever preferred_lft forever
Fix this adding a check on the protocol family before setting
IFA_F_HOMEADDRESS flag.
Fixes: bac735c53a36d ("enabled to manipulate the flags of IFA_F_HOMEADDRESS or IFA_F_NODAD from ip.")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
ip/ipaddress.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index d157f72784a21..e1b0e2224a768 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2248,7 +2248,10 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
if (set_lifetime(&preferred_lft, *argv))
invarg("preferred_lft value", *argv);
} else if (strcmp(*argv, "home") == 0) {
- ifa_flags |= IFA_F_HOMEADDRESS;
+ if (req.ifa.ifa_family == AF_INET6)
+ ifa_flags |= IFA_F_HOMEADDRESS;
+ else
+ fprintf(stderr, "Warning: home option can be set only for IPv6 addresses\n");
} else if (strcmp(*argv, "nodad") == 0) {
if (req.ifa.ifa_family == AF_INET6)
ifa_flags |= IFA_F_NODAD;
--
2.20.1
^ permalink raw reply related
* [PATCH iproute2 v2 3/3] ip address: do not set mngtmpaddr option for IPv4 addresses
From: Andrea Claudi @ 2019-06-25 10:29 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern
In-Reply-To: <cover.1561457597.git.aclaudi@redhat.com>
'mngtmpaddr' option make the kernel manage temporary addresses
created from the specified one as template on behalf of Privacy
Extensions (RFC3041). This option should be available only for
IPv6 addresses, as correctly stated in the manpage.
However it is possible to set mngtmpaddr on IPv4 addresses, too:
$ ip link add dummy0 type dummy
$ ip -4 addr add 192.168.1.1 dev dummy0 mngtmpaddr
$ ip a
1: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 1a:6d:c6:96:ca:f8 brd ff:ff:ff:ff:ff:ff
inet 192.168.1.1/32 scope global mngtmpaddr dummy0
valid_lft forever preferred_lft forever
Fix this adding a check on the protocol family before setting
IFA_F_MANAGETEMPADDR flag.
Fixes: 5b7e21c417bea ("add support for IFA_F_MANAGETEMPADDR")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
ip/ipaddress.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index e1b0e2224a768..8d5f4f9e51ffc 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2258,7 +2258,10 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
else
fprintf(stderr, "Warning: nodad option can be set only for IPv6 addresses\n");
} else if (strcmp(*argv, "mngtmpaddr") == 0) {
- ifa_flags |= IFA_F_MANAGETEMPADDR;
+ if (req.ifa.ifa_family == AF_INET6)
+ ifa_flags |= IFA_F_MANAGETEMPADDR;
+ else
+ fprintf(stderr, "Warning: mngtmpaddr option can be set only for IPv6 addresses\n");
} else if (strcmp(*argv, "noprefixroute") == 0) {
ifa_flags |= IFA_F_NOPREFIXROUTE;
} else if (strcmp(*argv, "autojoin") == 0) {
--
2.20.1
^ permalink raw reply related
* [PATCH iproute2 v2 0/3] do not set IPv6-only options on IPv4 addresses
From: Andrea Claudi @ 2019-06-25 10:29 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern
'home', 'nodad' and 'mngtmpaddr' options are IPv6-only, but
it is possible to set them on IPv4 addresses, too. This should
not be possible.
Fix this adding a check on the protocol family before setting
the flags, and print warning messages on error to not break
existing scripted setups.
Andrea Claudi (3):
ip address: do not set nodad option for IPv4 addresses
ip address: do not set home option for IPv4 addresses
ip address: do not set mngtmpaddr option for IPv4 addresses
ip/ipaddress.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/4] b43legacy: remove b43legacy_dma_set_mask
From: Christoph Hellwig @ 2019-06-25 10:29 UTC (permalink / raw)
To: Larry Finger, Kalle Valo; +Cc: b43-dev, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190625102932.32257-1-hch@lst.de>
These days drivers are not required to fallback to smaller DMA masks,
but can just set the largest mask they support, removing the need for
this trial and error logic.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/net/wireless/broadcom/b43legacy/dma.c | 39 +------------------
1 file changed, 1 insertion(+), 38 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
index 2ce1537d983c..0c2de20622e3 100644
--- a/drivers/net/wireless/broadcom/b43legacy/dma.c
+++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
@@ -797,43 +797,6 @@ void b43legacy_dma_free(struct b43legacy_wldev *dev)
dma->tx_ring0 = NULL;
}
-static int b43legacy_dma_set_mask(struct b43legacy_wldev *dev, u64 mask)
-{
- u64 orig_mask = mask;
- bool fallback = false;
- int err;
-
- /* Try to set the DMA mask. If it fails, try falling back to a
- * lower mask, as we can always also support a lower one. */
- while (1) {
- err = dma_set_mask_and_coherent(dev->dev->dma_dev, mask);
- if (!err)
- break;
- if (mask == DMA_BIT_MASK(64)) {
- mask = DMA_BIT_MASK(32);
- fallback = true;
- continue;
- }
- if (mask == DMA_BIT_MASK(32)) {
- mask = DMA_BIT_MASK(30);
- fallback = true;
- continue;
- }
- b43legacyerr(dev->wl, "The machine/kernel does not support "
- "the required %u-bit DMA mask\n",
- (unsigned int)dma_mask_to_engine_type(orig_mask));
- return -EOPNOTSUPP;
- }
- if (fallback) {
- b43legacyinfo(dev->wl, "DMA mask fallback from %u-bit to %u-"
- "bit\n",
- (unsigned int)dma_mask_to_engine_type(orig_mask),
- (unsigned int)dma_mask_to_engine_type(mask));
- }
-
- return 0;
-}
-
int b43legacy_dma_init(struct b43legacy_wldev *dev)
{
struct b43legacy_dma *dma = &dev->dma;
@@ -844,7 +807,7 @@ int b43legacy_dma_init(struct b43legacy_wldev *dev)
dmamask = supported_dma_mask(dev);
type = dma_mask_to_engine_type(dmamask);
- err = b43legacy_dma_set_mask(dev, dmamask);
+ err = dma_set_mask_and_coherent(dev->dev->dma_dev, dmamask);
if (err) {
#ifdef CONFIG_B43LEGACY_PIO
b43legacywarn(dev->wl, "DMA for this device not supported. "
--
2.20.1
^ permalink raw reply related
* [PATCH 2/4] b43legacy: simplify engine type / DMA mask selection
From: Christoph Hellwig @ 2019-06-25 10:29 UTC (permalink / raw)
To: Larry Finger, Kalle Valo; +Cc: b43-dev, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190625102932.32257-1-hch@lst.de>
Return the engine type from the function looking at the registers, and
just derive the DMA mask from that in the one place we care.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/net/wireless/broadcom/b43legacy/dma.c | 20 +++----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
index 0c2de20622e3..e66d05ae2466 100644
--- a/drivers/net/wireless/broadcom/b43legacy/dma.c
+++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
@@ -616,7 +616,7 @@ static void free_all_descbuffers(struct b43legacy_dmaring *ring)
}
}
-static u64 supported_dma_mask(struct b43legacy_wldev *dev)
+static enum b43legacy_dmatype b43legacy_engine_type(struct b43legacy_wldev *dev)
{
u32 tmp;
u16 mmio_base;
@@ -628,18 +628,7 @@ static u64 supported_dma_mask(struct b43legacy_wldev *dev)
tmp = b43legacy_read32(dev, mmio_base +
B43legacy_DMA32_TXCTL);
if (tmp & B43legacy_DMA32_TXADDREXT_MASK)
- return DMA_BIT_MASK(32);
-
- return DMA_BIT_MASK(30);
-}
-
-static enum b43legacy_dmatype dma_mask_to_engine_type(u64 dmamask)
-{
- if (dmamask == DMA_BIT_MASK(30))
- return B43legacy_DMA_30BIT;
- if (dmamask == DMA_BIT_MASK(32))
return B43legacy_DMA_32BIT;
- B43legacy_WARN_ON(1);
return B43legacy_DMA_30BIT;
}
@@ -801,13 +790,10 @@ int b43legacy_dma_init(struct b43legacy_wldev *dev)
{
struct b43legacy_dma *dma = &dev->dma;
struct b43legacy_dmaring *ring;
+ enum b43legacy_dmatype type = b43legacy_engine_type(dev);
int err;
- u64 dmamask;
- enum b43legacy_dmatype type;
- dmamask = supported_dma_mask(dev);
- type = dma_mask_to_engine_type(dmamask);
- err = dma_set_mask_and_coherent(dev->dev->dma_dev, dmamask);
+ err = dma_set_mask_and_coherent(dev->dev->dma_dev, DMA_BIT_MASK(type));
if (err) {
#ifdef CONFIG_B43LEGACY_PIO
b43legacywarn(dev->wl, "DMA for this device not supported. "
--
2.20.1
^ permalink raw reply related
* b43 / b43 legacy dma mask cleanups
From: Christoph Hellwig @ 2019-06-25 10:29 UTC (permalink / raw)
To: Larry Finger, Kalle Valo; +Cc: b43-dev, linux-wireless, netdev, linux-kernel
Hi all,
below are a few cleanups for the DMA mask handling. I came up with these
untested patches after looking through the code to debug the 32-bit pmac
30-bit dma issue involving b43legacy.
^ permalink raw reply
* Re: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface
From: Russell King - ARM Linux admin @ 2019-06-25 10:34 UTC (permalink / raw)
To: Parshuram Raju Thombare
Cc: andrew@lunn.ch, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <SN2PR07MB24800C63DCBC143B3A802A6EC1E30@SN2PR07MB2480.namprd07.prod.outlook.com>
On Tue, Jun 25, 2019 at 09:38:37AM +0000, Parshuram Raju Thombare wrote:
>
> >> >In which case, gem_phylink_validate() must clear the support mask when
> >> >SGMII mode is requested to indicate that the interface mode is not
> >> >supported.
> >> >The same goes for _all_ other PHY link modes that the hardware does not
> >> >actually support, such as PHY_INTERFACE_MODE_10GKR...
> >> If interface is not supported by hardware probe returns with error, so we don't
> >> net interface is not registered at all.
> >That does not negate my comment above.
> Sorry if I misunderstood your question, but hardware supports interfaces and based
> on that link modes are supported. So if interface is not supported by hardware,
> net device is not registered and there will be no phylink_validate call.
> If hardware support particular interface, link modes supported by interface
> are added to (not cleared from) supported mask, provided configs is not trying to limit
> data rate using GIGABIT_ENABLE* macro.
Why do you want to use phylink?
If you are only interested in supporting 10G PHYs, you don't need
phylink for that.
If you are interested in supporting SFPs as well, then using phylink
makes sense, but you need to implement your phylink conversion properly,
and that means supporting dynamic switching of the PHY interface mode,
and allowing phylink to determine whether a PHY interface mode is
supported or not.
However, with what you've indicated through our discussion, your MAC
does not support BASE-X modes, nor does it support 10GBASE-R, both of
which are required for direct connection of SFP or SFP+ modules.
The only phy link mode that you support which SFPs can make use of is
SGMII, and that will only be useful for copper SFPs configured for
SGMII mode. It basically means you can not use any fiber SFPs.
The only other way SFPs can be supported is via an intermediary PHY to
convert the MAC interface to BASE-X, SGMII or 10GBASE-R, and we don't
yet have support for that in mainline.
So, given that you seem unwilling to take on board my comments, and
your hardware does not appear support SFPs, I'm wondering what the
point of this conversion actually is.
As a result of our reviews, I've been improving the documentation for
phylink, so there has been some positives coming out of this - which
will hopefully help others to avoid the same mistakes.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH] let proc net directory inodes reflect to active net namespace
From: Hallsmark, Per @ 2019-06-25 10:36 UTC (permalink / raw)
To: Alexey Dobriyan, David S. Miller
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Hallsmark, Per
[-- Attachment #1: Type: text/plain, Size: 6364 bytes --]
Hi,
Linux kernel recently got a bugfix 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)"),
but unfortunately it only solves the issue for procfs net file inodes so they get correct
content after a process change namespace.
Checking on a v5.2-rc6 kernel :
sh-4.4# sh netns_procfs_test.sh
[ 16.451640] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
eth0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
sit0: 0 0 0 0 0 0 0 0 0 0
if_other: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
This kernel is fixed for file inode bug but suffers dir inode bug
sh-4.4#
As can be seen above, after the namespace change we see new content in procfs net/dev
but the listing of procfs net/dev_snmp6 still shows entries from previous namespace.
We need to apply similar bugfix for directory creation in procfs net as the mentioned
commit do for files.
Checking on a v5.2-rc6 kernel with bugfixes :
sh-4.4# sh netns_procfs_test.sh
[ 745.993882] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
lo: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
eth0: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
if_other: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
sit0
if_other
This kernel is fixed for both file and dir inode bug
sh-4.4#
Here we see that the directory procfs net/dev_snmp6 is updated according to the namespace
change.
The fix is two commits, first updates proc_net_mkdir() entries similar to mentioned patch
and second one is changing net/ipv6/proc.c to use proc_net_mkdir() instead.
Speaking about proc_net_mkdir()...
[phallsma@arn-phallsma-l3 linux]$ git grep proc_mkdir | grep proc_net
drivers/isdn/divert/divert_procfs.c: isdn_proc_entry = proc_mkdir("isdn", init_net.proc_net);
drivers/isdn/hysdn/hysdn_procconf.c: hysdn_proc_entry = proc_mkdir(PROC_SUBDIR_NAME, init_net.proc_net);
drivers/net/bonding/bond_procfs.c: bn->proc_dir = proc_mkdir(DRV_NAME, bn->net->proc_net);
drivers/net/wireless/intel/ipw2x00/libipw_module.c: libipw_proc = proc_mkdir(DRV_PROCNAME, init_net.proc_net);
drivers/net/wireless/intersil/hostap/hostap_main.c: hostap_proc = proc_mkdir("hostap", init_net.proc_net);
drivers/staging/rtl8192u/ieee80211/ieee80211_module.c: ieee80211_proc = proc_mkdir(DRV_NAME, init_net.proc_net);
drivers/staging/rtl8192u/r8192U_core.c: rtl8192_proc = proc_mkdir(RTL819XU_MODULE_NAME, init_net.proc_net);
net/appletalk/atalk_proc.c: if (!proc_mkdir("atalk", init_net.proc_net))
net/core/pktgen.c: pn->proc_dir = proc_mkdir(PG_PROC_DIR, pn->net->proc_net);
net/ipv4/netfilter/ipt_CLUSTERIP.c: cn->procdir = proc_mkdir("ipt_CLUSTERIP", net->proc_net);
net/ipv6/proc.c: net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
net/llc/llc_proc.c: llc_proc_dir = proc_mkdir("llc", init_net.proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ipt_hashlimit = proc_mkdir("ipt_hashlimit", net->proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ip6t_hashlimit = proc_mkdir("ip6t_hashlimit", net->proc_net);
net/netfilter/xt_recent.c: recent_net->xt_recent = proc_mkdir("xt_recent", net->proc_net);
net/sunrpc/cache.c: cd->procfs = proc_mkdir(cd->name, sn->proc_net_rpc);
net/sunrpc/stats.c: sn->proc_net_rpc = proc_mkdir("rpc", net->proc_net);
net/x25/x25_proc.c: if (!proc_mkdir("x25", init_net.proc_net))
[phallsma@arn-phallsma-l3 linux]$
IMHO all code should use proc_net_mkdir() instead of proc_mkdir() for procfs net entries,
or am I missing something here? If not possible to changeover to proc_net_mkdir() there
is a need for duplicating my first commit at those places. I'm fixing the one for dev_snmp6()
which is what I've tested as well.
Also wonder if it all is optimal. Wouldn't it be better to re-enable dcache for these (files as well as directories)
and in addition have kernel drop dcache in case of a namespace change?
Attaching patches and app/script for verifying.
I'm not on the mailing lists so please keep me on CC in case of responding.
Best regards,
Per
--
Per Hallsmark per.hallsmark@windriver.com
Senior Member Technical Staff Wind River AB
Mobile: +46733249340 Office: +46859461127
Torshamnsgatan 27 164 40 Kista
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch --]
[-- Type: text/x-patch; name="0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch", Size: 1895 bytes --]
From 66769af1f931124da1aa04b34350b3623a9003e3 Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Wed, 19 Jun 2019 15:46:39 +0200
Subject: [PATCH 1/2] Make directory inodes in /proc/net adhere to net
namespace
This patch fixes /proc/net directory inodes in similar way as
commit 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)")
fixes file inodes.
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
fs/proc/proc_net.c | 12 ++++++++++++
include/linux/proc_fs.h | 7 ++-----
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 76ae278df1c4..1c4231a60c9e 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -55,6 +55,18 @@ static void pde_force_lookup(struct proc_dir_entry *pde)
pde->proc_dops = &proc_net_dentry_ops;
}
+struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
+ struct proc_dir_entry *parent)
+{
+ struct proc_dir_entry *pde;
+
+ pde = proc_mkdir_data(name, 0, parent, net);
+ pde->proc_dops = &proc_net_dentry_ops;
+
+ return pde;
+}
+EXPORT_SYMBOL(proc_net_mkdir);
+
static int seq_open_net(struct inode *inode, struct file *file)
{
unsigned int state_size = PDE(inode)->state_size;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 52a283ba0465..608b8a10e338 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -124,11 +124,8 @@ static inline struct pid *tgid_pidfd_to_pid(const struct file *file)
struct net;
-static inline struct proc_dir_entry *proc_net_mkdir(
- struct net *net, const char *name, struct proc_dir_entry *parent)
-{
- return proc_mkdir_data(name, 0, parent, net);
-}
+extern struct proc_dir_entry *proc_net_mkdir(
+ struct net *net, const char *name, struct proc_dir_entry *parent);
struct ns_common;
int open_related_ns(struct ns_common *ns,
--
2.20.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-net-Directories-created-in-proc-net-should-be-done-v.patch --]
[-- Type: text/x-patch; name="0002-net-Directories-created-in-proc-net-should-be-done-v.patch", Size: 976 bytes --]
From 6925a5408ffce37dc71871c2ee05db96e60cae0d Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Thu, 20 Jun 2019 10:21:31 +0200
Subject: [PATCH 2/2] net: Directories created in /proc/net should be done via
proc_net_mkdir()
Directories created in /proc/net should be done via proc_net_mkdir()
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
net/ipv6/proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 4a8da679866e..3728c57e93dc 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -282,7 +282,8 @@ static int __net_init ipv6_proc_init_net(struct net *net)
snmp6_seq_show, NULL))
goto proc_snmp6_fail;
- net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
+ net->mib.proc_net_devsnmp6 = proc_net_mkdir(net,
+ "dev_snmp6", net->proc_net);
if (!net->mib.proc_net_devsnmp6)
goto proc_dev_snmp6_fail;
return 0;
--
2.20.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: netns_procfs_test.c --]
[-- Type: text/x-csrc; name="netns_procfs_test.c", Size: 3920 bytes --]
/*
Verifier of /proc/net and net namespace consistency
Author : Per Hallsmark <per.hallsmark@windriver.com>
First setup a net namespace and add a veth so we get something to test with
ip netns add netns_other
ip link add if_default type veth peer name if_other
ip link set if_other netns netns_other
Run test app without first reading some info from /proc/net in default namespace
./netns_procfs_test
Run test app again, this time reading some info from /proc/net in default namespace
before changing net namespace
./netns_procfs_test cached
On a bugfree kernel, the output after "After net namespace change" should be same as
in first test run, meaning we should see if_other in the /proc/net/dev dump and we
should see if_other in the directory.
The issue is not visible if it is different processes doing the readings since then
the inode's aren't cached.
*/
#define _GNU_SOURCE
#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include <fcntl.h>
#include <errno.h>
#include <limits.h>
#include <sys/types.h>
#include <sys/mount.h>
#include <errno.h>
#include <dirent.h>
#define NETNS_RUN_DIR "/var/run/netns"
static int get_nsfd(const char *ns)
{
char path[strlen(NETNS_RUN_DIR) + strlen(ns) + 2];
int fd;
snprintf(path, sizeof(path), "%s/%s", NETNS_RUN_DIR, ns);
fd = open(path, O_RDONLY, 0);
return fd;
}
static int dump_file(const char *fn, const int checkforbug)
{
int fd;
char buf[512];
ssize_t len;
char *wholebuf = NULL;
ssize_t wholelen = 0;
int status = 0;
printf("==== %s ====\n", fn);
fd = open(fn, O_RDONLY);
if (fd == -1) {
printf("Couldn't open %s (%s)\n", fn, strerror(errno));
return -1;
}
do {
len = read(fd, buf, sizeof(buf)-1);
buf[len] = 0;
wholebuf = realloc(wholebuf, wholelen + len + 1);
sprintf(&wholebuf[wholelen], "%s", buf);
wholelen += len;
} while (len > 0);
printf("%s\n", wholebuf);
if (checkforbug && strstr(wholebuf, "if_other")) {
status = 1;
}
free(wholebuf);
close(fd);
return status;
}
static int dump_dir(const char *dn, const int checkforbug)
{
DIR *dir;
struct dirent *dir_entry;
int status = 0;
printf("==== files in %s ====\n", dn);
dir = opendir(dn);
if (!dir) {
printf("Couldn't open %s (%s)\n", dn, strerror(errno));
return -1;
}
while ((dir_entry = readdir(dir)) != NULL) {
printf(" %s", dir_entry->d_name);
if (checkforbug && !strcmp(dir_entry->d_name, "if_other")) {
status = 2;
}
printf("\n");
}
closedir(dir);
return status;
}
static int switch_ns(const char *ns)
{
int nsfd = get_nsfd(ns);
if (setns(nsfd, CLONE_NEWNET) < 0) {
fprintf(stderr, "Unable to switch to namespace(fd=%d): %s.\n",
nsfd, strerror(errno));
exit(-1);
}
return 0;
}
int main (int argc, char **argv)
{
int bugcheck = 0;
int checkforbug = 1;
if (argc == 2 && !strcmp(argv[1], "cached")) {
// access /proc/net a bit so we dcache file and
// directory inodes
printf("Before net namespace change :\n");
dump_file("/proc/net/dev", 0);
dump_dir("/proc/net/dev_snmp6", 0);
} else {
// we cannot check for bug if not run with cached arg
checkforbug = 0;
}
// change namespace
switch_ns("netns_other");
// access /proc/net again, if all works we should see
// new info because of net namespace change
printf("\n\nAfter net namespace change :\n");
bugcheck += dump_file("/proc/net/dev", 1);
bugcheck += dump_dir("/proc/net/dev_snmp6", 1);
if (!checkforbug)
return 0;
switch (bugcheck) {
case 1 :
printf("This kernel is fixed for file inode bug but suffers dir inode bug\n");
break;
case 3 :
printf("This kernel is fixed for both file and dir inode bug\n");
break;
default :
printf("This kernel suffers both file and dir inode bug\n");
}
return bugcheck != 3;
}
[-- Attachment #5: netns_procfs_test.sh --]
[-- Type: application/x-shellscript, Size: 144 bytes --]
^ permalink raw reply related
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Ivan Khoronzhuk @ 2019-06-25 10:50 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce
In-Reply-To: <156086314789.27760.6549333469314693352.stgit@firesoul>
Hi Jesper,
Could you please clarify one question.
On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
>This patch is needed before we can allow drivers to use page_pool for
>DMA-mappings. Today with page_pool and XDP return API, it is possible to
>remove the page_pool object (from rhashtable), while there are still
>in-flight packet-pages. This is safely handled via RCU and failed lookups in
>__xdp_return() fallback to call put_page(), when page_pool object is gone.
>In-case page is still DMA mapped, this will result in page note getting
>correctly DMA unmapped.
>
>To solve this, the page_pool is extended with tracking in-flight pages. And
>XDP disconnect system queries page_pool and waits, via workqueue, for all
>in-flight pages to be returned.
>
>To avoid killing performance when tracking in-flight pages, the implement
>use two (unsigned) counters, that in placed on different cache-lines, and
>can be used to deduct in-flight packets. This is done by mapping the
>unsigned "sequence" counters onto signed Two's complement arithmetic
>operations. This is e.g. used by kernel's time_after macros, described in
>kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
>
>The trick is these two incrementing counters only need to be read and
>compared, when checking if it's safe to free the page_pool structure. Which
>will only happen when driver have disconnected RX/alloc side. Thus, on a
>non-fast-path.
>
>It is chosen that page_pool tracking is also enabled for the non-DMA
>use-case, as this can be used for statistics later.
>
>After this patch, using page_pool requires more strict resource "release",
>e.g. via page_pool_release_page() that was introduced in this patchset, and
>previous patches implement/fix this more strict requirement.
>
>Drivers no-longer call page_pool_destroy(). Drivers already call
>xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
>attempt to disconnect the mem id, and if attempt fails schedule the
>disconnect for later via delayed workqueue.
>
>Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>---
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
> include/net/page_pool.h | 41 ++++++++++---
> net/core/page_pool.c | 62 +++++++++++++++-----
> net/core/xdp.c | 65 +++++++++++++++++++--
> 4 files changed, 136 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index 2f647be292b6..6c9d4d7defbc 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> }
>
> xdp_rxq_info_unreg(&rq->xdp_rxq);
>- if (rq->page_pool)
>- page_pool_destroy(rq->page_pool);
>-
> mlx5_wq_destroy(&rq->wq_ctrl);
> }
>
>diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>index 754d980700df..f09b3f1994e6 100644
>--- a/include/net/page_pool.h
>+++ b/include/net/page_pool.h
>@@ -16,14 +16,16 @@
> * page_pool_alloc_pages() call. Drivers should likely use
> * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
> *
>- * If page_pool handles DMA mapping (use page->private), then API user
>- * is responsible for invoking page_pool_put_page() once. In-case of
>- * elevated refcnt, the DMA state is released, assuming other users of
>- * the page will eventually call put_page().
>+ * API keeps track of in-flight pages, in-order to let API user know
>+ * when it is safe to dealloactor page_pool object. Thus, API users
>+ * must make sure to call page_pool_release_page() when a page is
>+ * "leaving" the page_pool. Or call page_pool_put_page() where
>+ * appropiate. For maintaining correct accounting.
> *
>- * If no DMA mapping is done, then it can act as shim-layer that
>- * fall-through to alloc_page. As no state is kept on the page, the
>- * regular put_page() call is sufficient.
>+ * API user must only call page_pool_put_page() once on a page, as it
>+ * will either recycle the page, or in case of elevated refcnt, it
>+ * will release the DMA mapping and in-flight state accounting. We
>+ * hope to lift this requirement in the future.
> */
> #ifndef _NET_PAGE_POOL_H
> #define _NET_PAGE_POOL_H
>@@ -66,9 +68,10 @@ struct page_pool_params {
> };
>
> struct page_pool {
>- struct rcu_head rcu;
> struct page_pool_params p;
>
>+ u32 pages_state_hold_cnt;
>+
> /*
> * Data structure for allocation side
> *
>@@ -96,6 +99,8 @@ struct page_pool {
> * TODO: Implement bulk return pages into this structure.
> */
> struct ptr_ring ring;
>+
>+ atomic_t pages_state_release_cnt;
> };
>
> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>
> struct page_pool *page_pool_create(const struct page_pool_params *params);
>
>-void page_pool_destroy(struct page_pool *pool);
>-
> void __page_pool_free(struct page_pool *pool);
> static inline void page_pool_free(struct page_pool *pool)
> {
>@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> __page_pool_put_page(pool, page, true);
> }
>
>+/* API user MUST have disconnected alloc-side (not allowed to call
>+ * page_pool_alloc_pages()) before calling this. The free-side can
>+ * still run concurrently, to handle in-flight packet-pages.
>+ *
>+ * A request to shutdown can fail (with false) if there are still
>+ * in-flight packet-pages.
>+ */
>+bool __page_pool_request_shutdown(struct page_pool *pool);
>+static inline bool page_pool_request_shutdown(struct page_pool *pool)
>+{
>+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
>+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>+ */
>+#ifdef CONFIG_PAGE_POOL
>+ return __page_pool_request_shutdown(pool);
>+#endif
>+}
The free side can ran in softirq, that means fast cache recycle is accessed.
And it increments not atomic pool->alloc.count.
For instance While redirect, for remote interface, while .ndo_xdp_xmit the
xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
In the same time, simultaneously, the work queue can try one more time to clear
cash, calling __page_pool_request_shutdown()....
Question, what prevents pool->alloc.count to be corrupted by race,
causing to wrong array num and as result wrong page to be unmapped/put ....or
even page leak. alloc.count usage is not protected,
__page_pool_request_shutdown() is called not from same rx NAPI, even not from
NAPI.
Here, while alloc cache empty procedure in __page_pool_request_shutdown():
while (pool->alloc.count) {
page = pool->alloc.cache[--pool->alloc.count];
__page_pool_return_page(pool, page);
}
For me seems all works fine, but I can't find what have I missed?
...
Same question about how xdp frame should be returned for drivers running
tx napi exclusively, it can be still softirq but another CPU? What API
should be used to return xdp frame.
[...]
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v4 4/5] net: macb: add support for high speed interface
From: Russell King - ARM Linux admin @ 2019-06-25 10:51 UTC (permalink / raw)
To: Parshuram Raju Thombare
Cc: Andrew Lunn, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <SN2PR07MB2480DBE64F2550C0135335EEC1E30@SN2PR07MB2480.namprd07.prod.outlook.com>
On Tue, Jun 25, 2019 at 08:26:29AM +0000, Parshuram Raju Thombare wrote:
> Hi Andrew,
>
> >What i'm saying is that the USXGMII rate is fixed. So why do you need a device
> >tree property for the SERDES rate?
> This is based on Cisco USXGMII specification, it specify USXGMII 5G and USXGMII 10G.
> Sorry I can't share that document here.
The closed nature of the USXGMII spec makes it very hard for us to know
whether your implementation is correct or not.
I have some documentation which suggests that USVGMII is a USXGMII link
running at "5GR" rate as opposed to USXGMII running at "10GR" rate.
So, I think 5G mode should be left out until it becomes clear that (a)
we should specify it as USXGMII with a 5G rate, or as USVGMII.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Sergei Shtylyov @ 2019-06-25 10:51 UTC (permalink / raw)
To: Jose Abreu, linux-kernel, netdev
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <895a67c1-3b83-d7be-b64e-61cff86d057d@cogentembedded.com>
On 25.06.2019 13:29, Sergei Shtylyov wrote:
>> Some DT bindings do not have the PHY handle. Let's fallback to manually
>> discovery in case phylink_of_phy_connect() fails.
>>
>> Reported-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>> Fixes: 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic")
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> Hello Katsuhiro,
>>
>> Can you please test this patch ?
>> ---
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index a48751989fa6..f4593d2d9d20 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -950,9 +950,12 @@ static int stmmac_init_phy(struct net_device *dev)
>> node = priv->plat->phylink_node;
>> - if (node) {
>> + if (node)
>> ret = phylink_of_phy_connect(priv->phylink, node, 0);
>> - } else {
>> +
>> + /* Some DT bindings do not set-up the PHY handle. Let's try to
>> + * manually parse it */
>
> The multi-line comments inb the networking code should be formatted like
> below:
>
> /*
> * bla
Oops, that was the general comment format, networking code starts without
the leading empty line:\
/* bla
> * bla
> */
[...]
MBR, Sergei
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Jamal Hadi Salim @ 2019-06-25 10:55 UTC (permalink / raw)
To: Eric Dumazet, Joe Stringer, Florian Westphal
Cc: netdev, john fastabend, Daniel Borkmann, Lorenz Bauer,
Jakub Sitnicki, Paolo Abeni
In-Reply-To: <d2692f14-6ac7-1335-3359-d397fbe1676f@gmail.com>
On 2019-06-24 12:49 p.m., Eric Dumazet wrote:
>
>
> Well, I would simply remove the skb_orphan() call completely.
My experience: You still need to call skb_orphan() from the lower level
(and set the skb destructor etc).
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-25 11:02 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-L2dgypSCTDwdEXa7EUYyWTcD_hLwW_kuUkk0tA_iggDw@mail.gmail.com>
On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > + po->wait_on_complete = 1;
> > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > >
> > > This resets timeout on every loop. should only set above the loop once.
> > >
> > I explained exactly why I did that in the change log. Its because I reuse the
> > timeout variable to get the return value of the wait_for_complete call.
> > Otherwise I need to add additional data to the stack, which I don't want to do.
> > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > path, so I see no reason not to reuse the variable.
>
> The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> each time. Whereas wait_for_common and its variants return the
> number of jiffies left in case the loop needs to sleep again later.
>
> Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> common pattern across the stack.
>
But those patterns are unique to those situations. For instance, in
tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
wait for the entire message send operation to complete, which consists of
potentially several blocking operations (waiting for the tcp connection to be
established, waiting for socket memory, etc). In that situation we want to wait
for all of those operations to complete to send a single message, and fail if
they exceed the timeout in aggregate. The semantics are different with
AF_PACKET. In this use case, the message is in effect empty, and just used to
pass some control information. tpacket_snd, sends as many frames from the
memory mapped buffer as possible, and on each iteration we want to wait for the
specified timeout for those frames to complete sending. I think resetting the
timeout on each wait instance is the right way to go here.
> > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > err = net_xmit_errno(err);
> > > > if (err && __packet_get_status(po, ph) ==
> > > > TP_STATUS_AVAILABLE) {
> > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > + */
> > > > + po->wait_on_complete = 0;
> > >
> > > If setting where sleeping, no need for resetting if a failure happens
> > > between those blocks.
> > >
> > > > + init_completion(&po->skb_completion);
> > >
> > > no need to reinit between each use?
> > >
> > I explained exactly why I did this in the comment above. We have to set
> > wait_for_complete prior to calling transmit, so as to ensure that we call
> > wait_for_completion before we exit the loop. However, in this error case, we
> > exit the loop prior to calling wait_for_complete, so we need to reset the
> > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > case where, on the next entrace to this loop we will have a completion variable
> > with completion->done > 0, meaning the next wait will be a fall through case,
> > which we don't want.
>
> By moving back to the point where schedule() is called, hopefully this
> complexity automatically goes away. Same as my comment to the line
> immediately above.
>
Its going to change what the complexity is, actually. I was looking at this
last night, and I realized that your assertion that we could remove
packet_next_frame came at a cost. This is because we need to determine if we
have to wait before we call po->xmit, but we need to actually do the wait after
po->xmit (to ensure that wait_on_complete is set properly when the desructor is
called). By moving the wait to the top of the loop and getting rid of
packet_next_frame we now have a race condition in which we might call
tpacket_destruct_skb with wait_on_complete set to false, causing us to wait for
the maximum timeout erroneously. So I'm going to have to find a new way to
signal the need to call complete, which I think will introduce a different level
of complexity.
> > > > @@ -2740,6 +2772,20 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > }
> > > > packet_increment_head(&po->tx_ring);
> > > > len_sum += tp_len;
> > > > +
> > > > + if (po->wait_on_complete) {
> > > > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> > > > + po->wait_on_complete = 0;
> > >
> > > I was going to argue for clearing in tpacket_destruct_skb. But then we
> > > would have to separate clear on timeout instead of signal, too.
> > >
> > > po->wait_on_complete = 1;
> > > timeo = wait_for_completion...
> > > po->wait_on_complete = 0;
> > >
> > Also, we would have a race condition, since the destructor may be called from
> > softirq context (the first cause of the bug I'm fixing here), and so if the
> > packet is freed prior to us checking wait_for_complete in tpacket_snd, we will
> > be in the above situation again, exiting the loop with a completion variable in
> > an improper state.
>
> Good point.
>
> The common pattern is to clear in tpacket_destruct_skb. Then
> we do need to handle the case where the wait is interrupted or
> times out and reset it in those cases.
>
As noted above, if we restore the original control flow, the wait_on_complete
flag is not useable, as its state needs to be determined prior to actually
sending the frame to the driver via po->xmit.
I'm going to try some logic in which both tpacket_snd and tpacket_destruct_skb
key off of packet_read_pending. I think this will require a re-initalization of
the completion queue on each entry to tpacket_snd, but perhaps thats more
pallatable.
> > > This is basically replacing a busy-wait with schedule() with sleeping
> > > using wait_for_completion_interruptible_timeout. My main question is
> > > does this really need to move control flow around and add
> > > packet_next_frame? If not, especially for net, the shortest, simplest
> > > change is preferable.
> > >
> > Its not replacing a busy wait at all, its replacing a non-blocking schedule with
> > a blocking schedule (via completion queues). As for control flow, Im not sure I
> > why you are bound to the existing control flow, and given that we already have
> > packet_previous_frame, I didn't see anything egregious about adding
> > packet_next_frame as well, but since you've seen a way to eliminate it, I'm ok
> > with it.
>
> The benefit of keeping to the existing control flow is that that is a
> smaller change, so easier to verify.
>
I don't think it will really, because its going to introduce different
complexities, but we'll see for certain when I finish getting it rewritten
again.
> I understand the benefit of moving the wait outside the loop. Before
> this report, I was not even aware of that behavior on !MSG_DONTWAIT,
> because it is so co-located.
>
> But moving it elsewhere in the loop does not have the same benefit,
> imho. Either way, I think we better leave any such code improvements
> to net-next and focus on the minimal , least risky, patch for net.
>
Ok.
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Jamal Hadi Salim @ 2019-06-25 11:06 UTC (permalink / raw)
To: Joe Stringer
Cc: Eric Dumazet, Florian Westphal, netdev, john fastabend,
Daniel Borkmann, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni
In-Reply-To: <CAOftzPj_+6hfrb-FwU+E2P83RLLp6dtv0nJizSG1Fw7+vCgYwA@mail.gmail.com>
On 2019-06-24 11:26 p.m., Joe Stringer wrote:
[..]
>
> I haven't got as far as UDP yet, but I didn't see any need for a
> dependency on netfilter.
I'd be curious to see what you did. My experience, even for TCP is
the socket(transparent/tproxy) lookup code (to set skb->sk either
listening or established) is entangled in
CONFIG_NETFILTER_SOMETHING_OR_OTHER. You have to rip it out of
there (in the tproxy tc action into that code). Only then can you
compile out netfilter.
I didnt bother to rip out code for udp case.
i.e if you needed udp to work with the tc action,
youd have to turn on NF. But that was because we had
no need for udp transparent proxying.
IOW:
There is really no reason, afaik, for tproxy code to only be
accessed if netfilter is compiled in. Not sure i made sense.
cheers,
jamal
^ permalink raw reply
* memory leak in genl_register_family
From: syzbot @ 2019-06-25 11:07 UTC (permalink / raw)
To: davem, dsahern, johannes.berg, linux-kernel, marcel, mkubecek,
netdev, syzkaller-bugs, yuehaibing
Hello,
syzbot found the following crash on:
HEAD commit: 4b972a01 Linux 5.2-rc6
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1305b385a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=1db8bd6825f9661c
dashboard link: https://syzkaller.appspot.com/bug?extid=fc577f12f25f2ac3b211
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15bd1385a00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+fc577f12f25f2ac3b211@syzkaller.appspotmail.com
BUG: memory leak
unreferenced object 0xffff88812a7f0c80 (size 64):
comm "swapper/0", pid 1, jiffies 4294937561 (age 881.930s)
hex dump (first 32 bytes):
2f 64 65 76 69 63 65 73 2f 76 69 72 74 75 61 6c /devices/virtual
2f 6e 65 74 2f 6c 6f 2f 71 75 65 75 65 73 2f 74 /net/lo/queues/t
backtrace:
[<00000000d629f5fa>] kmemleak_alloc_recursive
include/linux/kmemleak.h:43 [inline]
[<00000000d629f5fa>] slab_post_alloc_hook mm/slab.h:439 [inline]
[<00000000d629f5fa>] slab_alloc mm/slab.c:3326 [inline]
[<00000000d629f5fa>] __do_kmalloc mm/slab.c:3658 [inline]
[<00000000d629f5fa>] __kmalloc+0x161/0x2c0 mm/slab.c:3669
[<000000003291d450>] kmalloc_array include/linux/slab.h:670 [inline]
[<000000003291d450>] genl_register_family net/netlink/genetlink.c:355
[inline]
[<000000003291d450>] genl_register_family+0x5e1/0x7e0
net/netlink/genetlink.c:322
[<00000000f8e2dd0d>] netlbl_unlabel_genl_init+0x15/0x17
net/netlabel/netlabel_unlabeled.c:1387
[<00000000189b4a0c>] netlbl_netlink_init+0x39/0x46
net/netlabel/netlabel_user.c:65
[<0000000083adc8f0>] netlbl_init+0x65/0xa8
net/netlabel/netlabel_kapi.c:1502
[<000000003a053024>] do_one_initcall+0x5c/0x2ca init/main.c:915
[<00000000d70991fc>] do_initcall_level init/main.c:983 [inline]
[<00000000d70991fc>] do_initcalls init/main.c:991 [inline]
[<00000000d70991fc>] do_basic_setup init/main.c:1009 [inline]
[<00000000d70991fc>] kernel_init_freeable+0x1af/0x26c init/main.c:1169
[<00000000232b80c4>] kernel_init+0x10/0x155 init/main.c:1087
[<000000006b3bb174>] ret_from_fork+0x1f/0x30
arch/x86/entry/entry_64.S:352
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Baruch Siach @ 2019-06-25 11:04 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf,
Dmitry V . Levin, Arnd Bergmann, linux-arch, Baruch Siach,
Jiri Olsa, Geert Uytterhoeven, Linus Torvalds
Merge commit 1c8c5a9d38f60 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
applications") by taking the gpl_compatible 1-bit field definition from
commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
bpf_prog_info") as is. That breaks architectures with 16-bit alignment
like m68k. Embed gpl_compatible into an anonymous union with 32-bit pad
member to restore alignment of following fields.
Thanks to Dmitry V. Levin his analysis of this bug history.
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2:
Use anonymous union with pad to make it less likely to break again in
the future.
---
include/uapi/linux/bpf.h | 5 ++++-
tools/include/uapi/linux/bpf.h | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a8b823c30b43..766eae02d7ae 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3142,7 +3142,10 @@ struct bpf_prog_info {
__aligned_u64 map_ids;
char name[BPF_OBJ_NAME_LEN];
__u32 ifindex;
- __u32 gpl_compatible:1;
+ union {
+ __u32 gpl_compatible:1;
+ __u32 pad;
+ };
__u64 netns_dev;
__u64 netns_ino;
__u32 nr_jited_ksyms;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a8b823c30b43..766eae02d7ae 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3142,7 +3142,10 @@ struct bpf_prog_info {
__aligned_u64 map_ids;
char name[BPF_OBJ_NAME_LEN];
__u32 ifindex;
- __u32 gpl_compatible:1;
+ union {
+ __u32 gpl_compatible:1;
+ __u32 pad;
+ };
__u64 netns_dev;
__u64 netns_ino;
__u32 nr_jited_ksyms;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next 3/3] net: stmmac: Convert to phylink and remove phylib logic
From: Jon Hunter @ 2019-06-25 11:10 UTC (permalink / raw)
To: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Russell King, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, linux-tegra
In-Reply-To: <78EB27739596EE489E55E81C33FEC33A0B9D7024@DE02WEMBXB.internal.synopsys.com>
On 25/06/2019 08:37, Jose Abreu wrote:
> From: Jon Hunter <jonathanh@nvidia.com>
>
>> Any further feedback? I am still seeing this issue on today's -next.
>
> Apologies but I was in FTO.
>
> Is there any possibility you can just disable the ethX configuration in
> the rootfs mount and manually configure it after rootfs is done ?
>
> I just want to make sure in which conditions this is happening (if in
> ifdown or ifup).
I have been looking at this a bit closer and I can see the problem. What
happens is that ...
1. stmmac_mac_link_up() is called and priv->eee_active is set to false
2. stmmac_eee_init() is called but because priv->eee_active is false,
timer_setup() for eee_ctrl_timer is never called.
3. stmmac_eee_init() returns true and so then priv->eee_enabled is set
to true.
4. When stmmac_tx_clean() is called because priv->eee_enabled is set to
true, mod_timer() is called for the eee_ctrl_timer, but because
timer_setup() was never called, we hit the BUG defined at
kernel/time/timer.c:952, because no function is defined for the
timer.
The following fixes it for me ...
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -399,10 +399,13 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
mutex_lock(&priv->lock);
/* Check if it needs to be deactivated */
- if (!priv->eee_active && priv->eee_enabled) {
- netdev_dbg(priv->dev, "disable EEE\n");
- del_timer_sync(&priv->eee_ctrl_timer);
- stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
+ if (!priv->eee_active) {
+ if (priv->eee_enabled) {
+ netdev_dbg(priv->dev, "disable EEE\n");
+ del_timer_sync(&priv->eee_ctrl_timer);
+ stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
+ }
+ mutex_unlock(&priv->lock);
return false;
}
It also looks like you have a potention deadlock in the current code
because in the case of if (!priv->eee_active && priv->eee_enabled)
you don't unlock the mutex. The above fixes this as well. I can send a
formal patch if this looks correct.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH net] sctp: change to hold sk after auth shkey is created successfully
From: Neil Horman @ 2019-06-25 11:18 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner,
syzkaller-bugs
In-Reply-To: <14de0d292dc2fe01ecadaba00feb925b337b558f.1561393305.git.lucien.xin@gmail.com>
On Tue, Jun 25, 2019 at 12:21:45AM +0800, Xin Long wrote:
> Now in sctp_endpoint_init(), it holds the sk then creates auth
> shkey. But when the creation fails, it doesn't release the sk,
> which causes a sk defcnf leak,
>
> Here to fix it by only holding the sk when auth shkey is created
> successfully.
>
> Fixes: a29a5bd4f5c3 ("[SCTP]: Implement SCTP-AUTH initializations.")
> Reported-by: syzbot+afabda3890cc2f765041@syzkaller.appspotmail.com
> Reported-by: syzbot+276ca1c77a19977c0130@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/endpointola.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index e358437..69cebb2 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -118,10 +118,6 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
> /* Initialize the bind addr area */
> sctp_bind_addr_init(&ep->base.bind_addr, 0);
>
> - /* Remember who we are attached to. */
> - ep->base.sk = sk;
> - sock_hold(ep->base.sk);
> -
> /* Create the lists of associations. */
> INIT_LIST_HEAD(&ep->asocs);
>
> @@ -154,6 +150,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
> ep->prsctp_enable = net->sctp.prsctp_enable;
> ep->reconf_enable = net->sctp.reconf_enable;
>
> + /* Remember who we are attached to. */
> + ep->base.sk = sk;
> + sock_hold(ep->base.sk);
> +
> return ep;
>
> nomem_shkey:
> --
> 2.1.0
>
>
Acked-by: Neil Horman <nhorman@redhat.com>
^ permalink raw reply
* Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred
From: Jamal Hadi Salim @ 2019-06-25 11:18 UTC (permalink / raw)
To: John Hurley, netdev; +Cc: davem, fw, simon.horman, jakub.kicinski, oss-drivers
In-Reply-To: <1561414416-29732-1-git-send-email-john.hurley@netronome.com>
On 2019-06-24 6:13 p.m., John Hurley wrote:
> These patches aim to prevent act_mirred causing stack overflow events from
> recursively calling packet xmit or receive functions. Such events can
> occur with poor TC configuration that causes packets to travel in loops
> within the system.
>
> Florian Westphal advises that a recursion crash and packets looping are
> separate issues and should be treated as such. David Miller futher points
> out that pcpu counters cannot track the precise skb context required to
> detect loops. Hence these patches are not aimed at detecting packet loops,
> rather, preventing stack flows arising from such loops.
Sigh. So we are still trying to save 2 bits?
John, you said ovs has introduced a similar loop handling code;
Is their approach similar to this? Bigger question: Is this approach
"good enough"?
Not to put more work for you, but one suggestion is to use skb metadata
approach which is performance unfriendly (could be argued to more
correct).
Also: Please consider submitting a test case or two for tdc.
cheers,
jamal
^ permalink raw reply
* RE: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface
From: Parshuram Raju Thombare @ 2019-06-25 11:22 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: andrew@lunn.ch, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <20190625103408.5rh2slqobruavyju@shell.armlinux.org.uk>
>If you are interested in supporting SFPs as well, then using phylink
>makes sense, but you need to implement your phylink conversion properly,
>and that means supporting dynamic switching of the PHY interface mode,
>and allowing phylink to determine whether a PHY interface mode is
>supported or not.
Yes, we want to support SFP+.
10G is tested in fixed mode on SFP+.
Based on your suggestions, dynamic switching of PHY interface is handled.
And check in probe is for hardware support for user selected interface,
which can be moved to validate callback but then supported mask will be empty.
>However, with what you've indicated through our discussion, your MAC
>does not support BASE-X modes, nor does it support 10GBASE-R, both of
>which are required for direct connection of SFP or SFP+ modules.
1000Base-X and 10GBASE-R supported by separate PCS.
Regards,
Parshuram Thombare
^ permalink raw reply
* RE: [PATCH v4 4/5] net: macb: add support for high speed interface
From: Parshuram Raju Thombare @ 2019-06-25 11:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <20190625105101.xvcwgt3jh5pk7p2x@shell.armlinux.org.uk>
>The closed nature of the USXGMII spec makes it very hard for us to know
>whether your implementation is correct or not.
>
>I have some documentation which suggests that USVGMII is a USXGMII link
>running at "5GR" rate as opposed to USXGMII running at "10GR" rate.
>
>So, I think 5G mode should be left out until it becomes clear that (a)
>we should specify it as USXGMII with a 5G rate, or as USVGMII.
>
Ok, I will remove 5G rate for USXGMII.
Regards,
Parshuram Thombare
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred
From: Jamal Hadi Salim @ 2019-06-25 11:24 UTC (permalink / raw)
To: John Hurley, Eyal Birger
Cc: Linux Netdev List, David Miller, Florian Westphal, Simon Horman,
Jakub Kicinski, oss-drivers, shmulik
In-Reply-To: <CAK+XE=mOjtp16tdz83RZ-x_jEp3nPRY3smxbG=OfCmGi9_DnXg@mail.gmail.com>
On 2019-06-25 5:06 a.m., John Hurley wrote:
> On Tue, Jun 25, 2019 at 9:30 AM Eyal Birger <eyal.birger@gmail.com> wrote:
> I'm not sure on the history of why a value of 4 was selected here but
> it seems to fall into line with my findings.
Back then we could only loop in one direction (as opposed to two right
now) - so seeing something twice would have been suspect enough,
so 4 seems to be a good number. I still think 4 is a good number.
> Is there a hard requirement for >4 recursive calls here?
I think this is where testcases help (which then get permanently
added in tdc repository). Eyal - if you have a test scenario where
this could be demonstrated it would help.
cheers,
jamal
^ permalink raw reply
* RE: [PATCH net-next 3/3] net: stmmac: Convert to phylink and remove phylib logic
From: Jose Abreu @ 2019-06-25 11:25 UTC (permalink / raw)
To: Jon Hunter, Jose Abreu, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Russell King, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, linux-tegra
In-Reply-To: <113f37a2-c37f-cdb5-5194-4361d949258a@nvidia.com>
From: Jon Hunter <jonathanh@nvidia.com>
> I have been looking at this a bit closer and I can see the problem. What
> happens is that ...
>
> 1. stmmac_mac_link_up() is called and priv->eee_active is set to false
> 2. stmmac_eee_init() is called but because priv->eee_active is false,
> timer_setup() for eee_ctrl_timer is never called.
> 3. stmmac_eee_init() returns true and so then priv->eee_enabled is set
> to true.
> 4. When stmmac_tx_clean() is called because priv->eee_enabled is set to
> true, mod_timer() is called for the eee_ctrl_timer, but because
> timer_setup() was never called, we hit the BUG defined at
> kernel/time/timer.c:952, because no function is defined for the
> timer.
>
> The following fixes it for me ...
>
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -399,10 +399,13 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
> mutex_lock(&priv->lock);
>
> /* Check if it needs to be deactivated */
> - if (!priv->eee_active && priv->eee_enabled) {
> - netdev_dbg(priv->dev, "disable EEE\n");
> - del_timer_sync(&priv->eee_ctrl_timer);
> - stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
> + if (!priv->eee_active) {
> + if (priv->eee_enabled) {
> + netdev_dbg(priv->dev, "disable EEE\n");
> + del_timer_sync(&priv->eee_ctrl_timer);
> + stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
> + }
> + mutex_unlock(&priv->lock);
> return false;
> }
>
> It also looks like you have a potention deadlock in the current code
> because in the case of if (!priv->eee_active && priv->eee_enabled)
> you don't unlock the mutex. The above fixes this as well. I can send a
> formal patch if this looks correct.
Thanks for looking into this! The fix looks correct so if you could
submit a patch it would be great!
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Jesper Dangaard Brouer @ 2019-06-25 11:27 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce, brouer
In-Reply-To: <20190625105013.GA6485@khorivan>
On Tue, 25 Jun 2019 13:50:14 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> Hi Jesper,
>
> Could you please clarify one question.
>
> On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
> >This patch is needed before we can allow drivers to use page_pool for
> >DMA-mappings. Today with page_pool and XDP return API, it is possible to
> >remove the page_pool object (from rhashtable), while there are still
> >in-flight packet-pages. This is safely handled via RCU and failed lookups in
> >__xdp_return() fallback to call put_page(), when page_pool object is gone.
> >In-case page is still DMA mapped, this will result in page note getting
> >correctly DMA unmapped.
> >
> >To solve this, the page_pool is extended with tracking in-flight pages. And
> >XDP disconnect system queries page_pool and waits, via workqueue, for all
> >in-flight pages to be returned.
> >
> >To avoid killing performance when tracking in-flight pages, the implement
> >use two (unsigned) counters, that in placed on different cache-lines, and
> >can be used to deduct in-flight packets. This is done by mapping the
> >unsigned "sequence" counters onto signed Two's complement arithmetic
> >operations. This is e.g. used by kernel's time_after macros, described in
> >kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
> >
> >The trick is these two incrementing counters only need to be read and
> >compared, when checking if it's safe to free the page_pool structure. Which
> >will only happen when driver have disconnected RX/alloc side. Thus, on a
> >non-fast-path.
> >
> >It is chosen that page_pool tracking is also enabled for the non-DMA
> >use-case, as this can be used for statistics later.
> >
> >After this patch, using page_pool requires more strict resource "release",
> >e.g. via page_pool_release_page() that was introduced in this patchset, and
> >previous patches implement/fix this more strict requirement.
> >
> >Drivers no-longer call page_pool_destroy(). Drivers already call
> >xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
> >attempt to disconnect the mem id, and if attempt fails schedule the
> >disconnect for later via delayed workqueue.
> >
> >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >---
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
> > include/net/page_pool.h | 41 ++++++++++---
> > net/core/page_pool.c | 62 +++++++++++++++-----
> > net/core/xdp.c | 65 +++++++++++++++++++--
> > 4 files changed, 136 insertions(+), 35 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >index 2f647be292b6..6c9d4d7defbc 100644
> >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> > }
> >
> > xdp_rxq_info_unreg(&rq->xdp_rxq);
> >- if (rq->page_pool)
> >- page_pool_destroy(rq->page_pool);
> >-
> > mlx5_wq_destroy(&rq->wq_ctrl);
> > }
> >
> >diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> >index 754d980700df..f09b3f1994e6 100644
> >--- a/include/net/page_pool.h
> >+++ b/include/net/page_pool.h
> >@@ -16,14 +16,16 @@
> > * page_pool_alloc_pages() call. Drivers should likely use
> > * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
> > *
> >- * If page_pool handles DMA mapping (use page->private), then API user
> >- * is responsible for invoking page_pool_put_page() once. In-case of
> >- * elevated refcnt, the DMA state is released, assuming other users of
> >- * the page will eventually call put_page().
> >+ * API keeps track of in-flight pages, in-order to let API user know
> >+ * when it is safe to dealloactor page_pool object. Thus, API users
> >+ * must make sure to call page_pool_release_page() when a page is
> >+ * "leaving" the page_pool. Or call page_pool_put_page() where
> >+ * appropiate. For maintaining correct accounting.
> > *
> >- * If no DMA mapping is done, then it can act as shim-layer that
> >- * fall-through to alloc_page. As no state is kept on the page, the
> >- * regular put_page() call is sufficient.
> >+ * API user must only call page_pool_put_page() once on a page, as it
> >+ * will either recycle the page, or in case of elevated refcnt, it
> >+ * will release the DMA mapping and in-flight state accounting. We
> >+ * hope to lift this requirement in the future.
> > */
> > #ifndef _NET_PAGE_POOL_H
> > #define _NET_PAGE_POOL_H
> >@@ -66,9 +68,10 @@ struct page_pool_params {
> > };
> >
> > struct page_pool {
> >- struct rcu_head rcu;
> > struct page_pool_params p;
> >
> >+ u32 pages_state_hold_cnt;
> >+
> > /*
> > * Data structure for allocation side
> > *
> >@@ -96,6 +99,8 @@ struct page_pool {
> > * TODO: Implement bulk return pages into this structure.
> > */
> > struct ptr_ring ring;
> >+
> >+ atomic_t pages_state_release_cnt;
> > };
> >
> > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> >@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
> >
> > struct page_pool *page_pool_create(const struct page_pool_params *params);
> >
> >-void page_pool_destroy(struct page_pool *pool);
> >-
> > void __page_pool_free(struct page_pool *pool);
> > static inline void page_pool_free(struct page_pool *pool)
> > {
> >@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> > __page_pool_put_page(pool, page, true);
> > }
> >
> >+/* API user MUST have disconnected alloc-side (not allowed to call
> >+ * page_pool_alloc_pages()) before calling this. The free-side can
> >+ * still run concurrently, to handle in-flight packet-pages.
> >+ *
> >+ * A request to shutdown can fail (with false) if there are still
> >+ * in-flight packet-pages.
> >+ */
> >+bool __page_pool_request_shutdown(struct page_pool *pool);
> >+static inline bool page_pool_request_shutdown(struct page_pool *pool)
> >+{
> >+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
> >+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
> >+ */
> >+#ifdef CONFIG_PAGE_POOL
> >+ return __page_pool_request_shutdown(pool);
> >+#endif
> >+}
>
> The free side can ran in softirq, that means fast cache recycle is accessed.
> And it increments not atomic pool->alloc.count.
>
> For instance While redirect, for remote interface, while .ndo_xdp_xmit the
> xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
>
> In the same time, simultaneously, the work queue can try one more
> time to clear cash, calling __page_pool_request_shutdown()....
>
> Question, what prevents pool->alloc.count to be corrupted by race,
> causing to wrong array num and as result wrong page to be unmapped/put ....or
> even page leak. alloc.count usage is not protected,
> __page_pool_request_shutdown() is called not from same rx NAPI, even not from
> NAPI.
>
> Here, while alloc cache empty procedure in __page_pool_request_shutdown():
You forgot to copy this comment, which explains:
/* Empty alloc cache, assume caller made sure this is
* no-longer in use, and page_pool_alloc_pages() cannot be
* call concurrently.
*/
> while (pool->alloc.count) {
> page = pool->alloc.cache[--pool->alloc.count];
> __page_pool_return_page(pool, page);
> }
>
> For me seems all works fine, but I can't find what have I missed?
You have missed that, it is the drivers responsibility to "disconnect"
the xdp_rxq_info before calling shutdown. Which means that it is not
allowed to be used for RX, while the driver is shutting down a
RX-queue. For drivers this is very natural, else other things will
break.
> ...
>
> Same question about how xdp frame should be returned for drivers running
> tx napi exclusively, it can be still softirq but another CPU? What API
> should be used to return xdp frame.
You have to use the normal xdp_return_frame() which doesn't do "direct"
return.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ 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