Netdev List
 help / color / mirror / Atom feed
* [PATCHv3 0/2] capability controlled user-namespaces
From: Mahesh Bandewar @ 2017-12-05 22:30 UTC (permalink / raw)
  To: LKML, Netdev
  Cc: Kernel-hardening, Linux API, Kees Cook, Serge Hallyn,
	Eric W . Biederman, Eric Dumazet, David Miller, Mahesh Bandewar,
	Mahesh Bandewar

From: Mahesh Bandewar <maheshb@google.com>

TL;DR version
-------------
Creating a sandbox environment with namespaces is challenging
considering what these sandboxed processes can engage into. e.g.
CVE-2017-6074, CVE-2017-7184, CVE-2017-7308 etc. just to name few.
Current form of user-namespaces, however, if changed a bit can allow
us to create a sandbox environment without locking down user-
namespaces.

Detailed version
----------------

Problem
-------
User-namespaces in the current form have increased the attack surface as
any process can acquire capabilities which are not available to them (by
default) by performing combination of clone()/unshare()/setns() syscalls.

    #define _GNU_SOURCE
    #include <stdio.h>
    #include <sched.h>
    #include <netinet/in.h>

    int main(int ac, char **av)
    {
        int sock = -1;

        printf("Attempting to open RAW socket before unshare()...\n");
        sock = socket(AF_INET6, SOCK_RAW, IPPROTO_RAW);
        if (sock < 0) {
            perror("socket() SOCK_RAW failed: ");
        } else {
            printf("Successfully opened RAW-Sock before unshare().\n");
            close(sock);
            sock = -1;
        }

        if (unshare(CLONE_NEWUSER | CLONE_NEWNET) < 0) {
            perror("unshare() failed: ");
            return 1;
        }

        printf("Attempting to open RAW socket after unshare()...\n");
        sock = socket(AF_INET6, SOCK_RAW, IPPROTO_RAW);
        if (sock < 0) {
            perror("socket() SOCK_RAW failed: ");
        } else {
            printf("Successfully opened RAW-Sock after unshare().\n");
            close(sock);
            sock = -1;
        }

        return 0;
    }

The above example shows how easy it is to acquire NET_RAW capabilities
and once acquired, these processes could take benefit of above mentioned
or similar issues discovered/undiscovered with malicious intent. Note
that this is just an example and the problem/solution is not limited
to NET_RAW capability *only*. 

The easiest fix one can apply here is to lock-down user-namespaces which
many of the distros do (i.e. don't allow users to create user namespaces),
but unfortunately that prevents everyone from using them.

Approach
--------
Introduce a notion of 'controlled' user-namespaces. Every process on
the host is allowed to create user-namespaces (governed by the limit
imposed by per-ns sysctl) however, mark user-namespaces created by
sandboxed processes as 'controlled'. Use this 'mark' at the time of
capability check in conjunction with a global capability whitelist.
If the capability is not whitelisted, processes that belong to 
controlled user-namespaces will not be allowed.

Once a user-ns is marked as 'controlled'; all its child user-
namespaces are marked as 'controlled' too.

A global whitelist is list of capabilities governed by the
sysctl which is available to (privileged) user in init-ns to modify
while it's applicable to all controlled user-namespaces on the host.

Marking user-namespaces controlled without modifying the whitelist is
equivalent of the current behavior. The default value of whitelist includes
all capabilities so that the compatibility is maintained. However it gives
admins fine-grained ability to control various capabilities system wide
without locking down user-namespaces.

Please see individual patches in this series.

Mahesh Bandewar (2):
  capability: introduce sysctl for controlled user-ns capability whitelist
  userns: control capabilities of some user namespaces

 Documentation/sysctl/kernel.txt | 21 +++++++++++++++++
 include/linux/capability.h      |  7 ++++++
 include/linux/user_namespace.h  | 25 ++++++++++++++++++++
 kernel/capability.c             | 52 +++++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c                 |  5 ++++
 kernel/user_namespace.c         |  4 ++++
 security/commoncap.c            |  8 +++++++
 7 files changed, 122 insertions(+)

-- 
2.15.0.531.g2ccb3012c9-goog

^ permalink raw reply

* [PATCHv3 1/2] capability: introduce sysctl for controlled user-ns capability whitelist
From: Mahesh Bandewar @ 2017-12-05 22:31 UTC (permalink / raw)
  To: LKML, Netdev
  Cc: Kernel-hardening, Linux API, Kees Cook, Serge Hallyn,
	Eric W . Biederman, Eric Dumazet, David Miller, Mahesh Bandewar,
	Mahesh Bandewar

From: Mahesh Bandewar <maheshb@google.com>

Add a sysctl variable kernel.controlled_userns_caps_whitelist. This
takes input as capability mask expressed as two comma separated hex
u32 words. The mask, however, is stored in kernel as kernel_cap_t type.

Any capabilities that are not part of this mask will be controlled and
will not be allowed to processes in controlled user-ns.

Acked-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v3:
  Added couple of comments as requested by Serge Hallyn
v2:
  Rebase
v1:
  Initial submission

 Documentation/sysctl/kernel.txt | 21 ++++++++++++++++++
 include/linux/capability.h      |  3 +++
 kernel/capability.c             | 47 +++++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c                 |  5 +++++
 4 files changed, 76 insertions(+)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 694968c7523c..a1d39dbae847 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -25,6 +25,7 @@ show up in /proc/sys/kernel:
 - bootloader_version	     [ X86 only ]
 - callhome		     [ S390 only ]
 - cap_last_cap
+- controlled_userns_caps_whitelist
 - core_pattern
 - core_pipe_limit
 - core_uses_pid
@@ -187,6 +188,26 @@ CAP_LAST_CAP from the kernel.
 
 ==============================================================
 
+controlled_userns_caps_whitelist
+
+Capability mask that is whitelisted for "controlled" user namespaces.
+Any capability that is missing from this mask will not be allowed to
+any process that is attached to a controlled-userns. e.g. if CAP_NET_RAW
+is not part of this mask, then processes running inside any controlled
+userns's will not be allowed to perform action that needs CAP_NET_RAW
+capability. However, processes that are attached to a parent user-ns
+hierarchy that is *not* controlled and has CAP_NET_RAW can continue
+performing those actions. User-namespaces are marked "controlled" at
+the time of their creation based on the capabilities of the creator.
+A process that does not have CAP_SYS_ADMIN will create user-namespaces
+that are controlled.
+
+The value is expressed as two comma separated hex words (u32). This
+sysctl is avaialble in init-ns and users with CAP_SYS_ADMIN in init-ns
+are allowed to make changes.
+
+==============================================================
+
 core_pattern:
 
 core_pattern is used to specify a core dumpfile pattern name.
diff --git a/include/linux/capability.h b/include/linux/capability.h
index f640dcbc880c..7d79a4689625 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -14,6 +14,7 @@
 #define _LINUX_CAPABILITY_H
 
 #include <uapi/linux/capability.h>
+#include <linux/sysctl.h>
 
 
 #define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
@@ -248,6 +249,8 @@ extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
 
 /* audit system wants to get cap info from files as well */
 extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
+int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
+				 void __user *buff, size_t *lenp, loff_t *ppos);
 
 extern int cap_convert_nscap(struct dentry *dentry, void **ivalue, size_t size);
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 1e1c0236f55b..4a859b7d4902 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -29,6 +29,8 @@ EXPORT_SYMBOL(__cap_empty_set);
 
 int file_caps_enabled = 1;
 
+kernel_cap_t controlled_userns_caps_whitelist = CAP_FULL_SET;
+
 static int __init file_caps_disable(char *str)
 {
 	file_caps_enabled = 0;
@@ -507,3 +509,48 @@ bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns)
 	rcu_read_unlock();
 	return (ret == 0);
 }
+
+/* Controlled-userns capabilities routines */
+#ifdef CONFIG_SYSCTL
+int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
+				 void __user *buff, size_t *lenp, loff_t *ppos)
+{
+	DECLARE_BITMAP(caps_bitmap, CAP_LAST_CAP);
+	struct ctl_table caps_table;
+	char tbuf[NAME_MAX];
+	int ret;
+
+	ret = bitmap_from_u32array(caps_bitmap, CAP_LAST_CAP,
+				   controlled_userns_caps_whitelist.cap,
+				   _KERNEL_CAPABILITY_U32S);
+	if (ret != CAP_LAST_CAP)
+		return -1;
+
+	scnprintf(tbuf, NAME_MAX, "%*pb", CAP_LAST_CAP, caps_bitmap);
+
+	caps_table.data = tbuf;
+	caps_table.maxlen = NAME_MAX;
+	caps_table.mode = table->mode;
+	ret = proc_dostring(&caps_table, write, buff, lenp, ppos);
+	if (ret)
+		return ret;
+	if (write) {
+		kernel_cap_t tmp;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		ret = bitmap_parse_user(buff, *lenp, caps_bitmap, CAP_LAST_CAP);
+		if (ret)
+			return ret;
+
+		ret = bitmap_to_u32array(tmp.cap, _KERNEL_CAPABILITY_U32S,
+					 caps_bitmap, CAP_LAST_CAP);
+		if (ret != CAP_LAST_CAP)
+			return -1;
+
+		controlled_userns_caps_whitelist = tmp;
+	}
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 557d46728577..759b6c286806 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1217,6 +1217,11 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &one,
 	},
 #endif
+	{
+		.procname	= "controlled_userns_caps_whitelist",
+		.mode		= 0644,
+		.proc_handler	= proc_douserns_caps_whitelist,
+	},
 	{ }
 };
 
-- 
2.15.0.531.g2ccb3012c9-goog

^ permalink raw reply related

* Re: [RfC net-next 0/3] RTL8211F Ethernet PHY "documentation"
From: Andrew Lunn @ 2017-12-05 22:30 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: netdev, f.fainelli, linux-amlogic, hkallweit1, Shengzhou.Liu,
	jaswinder.singh
In-Reply-To: <20171202220650.23391-1-martin.blumenstingl@googlemail.com>

> I do not expect that this series is applied. if someone is interested
> in testing this: it applies on top of my other series:
> "Realtek Ethernet PHY driver improvements" [1]

Hi Martin

Thanks for the patches. Documentation like this is often useful.

       Andrew

^ permalink raw reply

* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jakub Kicinski @ 2017-12-05 22:29 UTC (permalink / raw)
  To: achiad shochat
  Cc: Alexander Duyck, Stephen Hemminger, Michael S. Tsirkin,
	Hannes Frederic Sowa, Sridhar Samudrala, netdev, virtualization,
	Achiad, Peter Waskiewicz Jr, Singhai, Anjali, Shannon Nelson,
	Andy Gospodarek, Or Gerlitz
In-Reply-To: <CAEHy93Kw-GePsraaAkVZNbOwt=rDPMVjmwuU22004LHCAoSSFA@mail.gmail.com>

On Tue, 5 Dec 2017 11:59:17 +0200, achiad shochat wrote:
> >>>> I second Jacob - having a netdev of one device driver enslave a netdev
> >>>> of another device driver is an awkward a-symmetric model.
> >>>> Regardless of whether they share the same backend device.
> >>>> Only I am not sure the Linux Bond is the right choice.
> >>>> e.g one may well want to use the virtio device also when the
> >>>> pass-through device is available, e.g for multicasts, east-west
> >>>> traffic, etc.
> >>>> I'm not sure the Linux Bond fits that functionality.
> >>>> And, as I hear in this thread, it is hard to make it work out of the box.
> >>>> So I think the right thing would be to write a new dedicated module
> >>>> for this purpose.  
> >
> > This part I can sort of agree with. What if we were to look at
> > providing a way to somehow advertise that the two devices were meant
> > to be boded for virtualization purposes? For now lets call it a
> > "virt-bond". Basically we could look at providing a means for virtio
> > and VF drivers to advertise that they want this sort of bond. Then it
> > would just be a matter of providing some sort of side channel to
> > indicate where you want things like multicast/broadcast/east-west
> > traffic to go.
> 
> I like this approach.

+1 on a separate driver, just enslaving devices to virtio may break
existing setups.  If people are bonding from user space today, if they
update their kernel it may surprise them how things get auto-mangled.

Is what Alex is suggesting a separate PV device that says "I would
like to be a bond of those two interfaces"?  That would make the HV
intent explicit and kernel decisions more understandable.

^ permalink raw reply

* Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command
From: Michal Kubecek @ 2017-12-05 22:29 UTC (permalink / raw)
  To: Scott Branden
  Cc: John W. Linville, BCM Kernel Feedback, Steve Lin, Michael Chan,
	netdev, Paul Greenwalt, Stephen Hemminger
In-Reply-To: <42b76996-b351-4dd4-741d-879e156d1c5c@broadcom.com>

On Tue, Dec 05, 2017 at 02:06:09PM -0800, Scott Branden wrote:
> On 17-12-05 01:30 PM, Michal Kubecek wrote:
> > On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> > > Add ETHTOOL_RESET support via --reset command.
> > > 
> > > ie.  ethtool --reset DEVNAME <flagname(s)>
> > > 
> > > flagnames currently match the ETH_RESET_xxx names:
> > > mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
> > > 
> > > Alternatively, you can specific component bitfield directly using
> > > ethtool --reset DEVNAME flags %x
> > IMHO it would be more consistent with e.g. msglvl without the keyword
> > "flags".
> I don't see the consistency in ethtool of specifying a number without a
> keyword in front of it.
> I can only find --set-dump specify a number?
> Others have keyword and number.  msglvl is the keyword after specifying -s -
> same as flags is the keyword I use after specifying --reset.

What I meant is that you can write

    ethtool -s eth0 msglvl drv on probe off
    ethtool -s eth0 msglvl 0x7

i.e. either number or names (with on/off in this case) while your patch
has

    ethtool --reset eth0 mgmg,irq
    ethtool --reset eth0 flags 0x3

i.e. an extra keyword if a number is used.

But it's not really important, it doesn't seem I would be able to share
a parser for this with any other subcommand or parameter anyway.

> >   It would be also nice to provide a symbolic way to specify the
> > shared flags.
> 
> I'll change to allow -shared to be added to the end of each component
> specified to use the shared bit.
>  IE. mgmt-shared, irq-shared, dma-shared ?

Sounds good to me.

> > > +	resetinfo.cmd = ETHTOOL_RESET;
> > > +
> > > +	if (send_ioctl(ctx, &resetinfo)) {
> > > +		perror("Cannot issue RESET");
> > > +		return 1;
> > > +	}
> > > +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
> > 
> > According to documentation, driver is supposed to clear the flags
> > corresponding to components which were reset so that what is left are
> > those which were _not_ reset.
> 
> I'll move the print above the send_ioctl.

It might be even more useful if ethtool informed user what actually
happened, i.e. either change the message to saying these are bits for
components not reset (if resetinfo.data is not zero) or save the
original value of resetinfo.data and show  saved_data & ~resetinfo.data

Michal Kubecek

^ permalink raw reply

* Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command
From: Andrew Lunn @ 2017-12-05 22:26 UTC (permalink / raw)
  To: Scott Branden
  Cc: John W. Linville, BCM Kernel Feedback, Steve Lin, Michael Chan,
	netdev, Paul Greenwalt, Stephen Hemminger
In-Reply-To: <1512507203-17693-4-git-send-email-scott.branden@broadcom.com>

On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> Add ETHTOOL_RESET support via --reset command.
> 
> ie.  ethtool --reset DEVNAME <flagname(s)>
> 
> flagnames currently match the ETH_RESET_xxx names:
> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all

[Snip]

> +.B ethtool \-\-reset
> +.I devname
> +.BN flags
> +.RB [ mgmt ]
> +.RB [ irq ]
> +.RB [ dma ]
> +.RB [ filter ]
> +.RB [ offload ]
> +.RB [ mac ]
> +.RB [ phy ]
> +.RB [ ram ]
> +.RB [ ap ]
> +.RB [ dedicated ]
> +.RB [ all ]

Hi Scott

Just a nick pick. You don't list ap above, which is kind of why you
are doing this, if i remember correctly.

    Andrew

^ permalink raw reply

* Re: [PATCH net-next 1/1] net: dsa: microchip: Add Microchip KSZ8895 DSA driver
From: Pavel Machek @ 2017-12-05 22:23 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: andrew, f.fainelli, muvarov, nathan.leigh.conrad, vivien.didelot,
	UNGLinuxDriver, netdev
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD4113DCE0@CHN-SV-EXMX02.mchp-main.com>

[-- Attachment #1: Type: text/plain, Size: 4168 bytes --]

On Tue 2017-12-05 22:16:45, Tristram.Ha@microchip.com wrote:
> > Thanks for patches. I installed whole series on top of net-next.
> > 
> > Hardware is:
> > 
> > root@miro:~# cat /proc/cpuinfo
> > model name   : ARM926EJ-S rev 5 (v5l)
> > Hardware	  : Freescale MXS (Device Tree)
> > 
> > I added devicetree chunks, and enabled DSA in the config. It seems
> > switch is detected:
> > 
> > [    4.775934] Micrel KSZ8051 dsa-0.0:00: attached PHY driver [Micrel
> > KSZ8051] (mii_bus:phy_addr=dsa-0.0:00, irq=POLL)
> > [    4.885952] Micrel KSZ8051 dsa-0.0:01: attached PHY driver [Micrel
> > KSZ8051] (mii_bus:phy_addr=dsa-0.0:01, irq=POLL)
> > [    4.995934] Micrel KSZ8051 dsa-0.0:02: attached PHY driver [Micrel
> > KSZ8051] (mii_bus:phy_addr=dsa-0.0:02, irq=POLL)
> > [    5.011484] DSA: tree 0 setup
> > 
> > root@miro:~# ifconfig lan3 192.168.20.103 netmask 255.255.0.0 up
> > [  131.196667] IPv6: ADDRCONF(NETDEV_UP): lan3: link is not ready
> > root@miro:~# [  132.225863] ksz8895-switch spi2.0 lan3: Link is Up -
> > 100Mbps/Full - flow control rx/tx
> > [  132.233939] IPv6: ADDRCONF(NETDEV_CHANGE): lan3: link becomes ready
> > 
> > root@miro:~# ping 192.168.1.1
> > PING 192.168.1.1 (192.168.1.1): 56 data bytes
> > ^C
> > --- 192.168.1.1 ping statistics ---
> > 7 packets transmitted, 0 packets received, 100% packet loss
> > root@miro:~# ifconfig [  149.904234] random: crng init done
> > 
> > But packets do not go through, and there is nothing helpful in
> > dmesg. Dts part is:
> > 
> >                 spi@0 {
> >                        	compatible = "microchip,ksz8895";
> >                         spi-max-frequency = <25000000>;
> >                         reg = <0>;
> > 			// reset-gpios = <&gpio2 8 0>;
> >                         status = "okay";
> > 
> >                         spi-cpha;
> > 			spi-cpol;
> >                        ports {
> >                              #address-cells = <1>;
> >                              #size-cells = <0>;
> >                              port@0 {
> >                                     reg = <0>;
> >                                     label = "lan1";
> >                              };
> >                              port@1 {
> >                                     reg = <1>;
> >                                     label = "lan2";
> >                              };
> >                              port@2 {
> >                                     reg = <2>;
> >                                     label = "lan3";
> >                              };
> >                              port@4 {
> >                                     reg = <4>;
> >                                     label = "cpu";
> >                                     ethernet = <&mac0>;
> >                                     fixed-link {
> >                                                speed = <100>;
> >                                                full-duplex;
> >                                     };
> >                              };
> >                        };
> > 		};
> > 
> > I went back to my version of dsa patches, and test above works as
> > expected.
> 
> Sorry to be this late for the reply.  I finally got hold of a KSZ8895 board that
> works with my SoC board to confirm the network communication.
> 
> As expected the KSZ8895 board works correctly as the chip uses the same
> tail tagging feature in KSZ8795, and I did verify that board is working.
> 
> One thing to debug this problem is to dump the MIB counters.  Use the ethtool
> utility to show MIB counters of both ports:
> 
> ethtool -S lan3
> ethtool -S eth0
> 
> Assuming eth0 is the MAC controller that drives the switch, the receive counters of
> the host port of the switch should match the transmit counters of
> lan3, and vice versa.

Thanks for reply. I'll get to the tests shortly. Could I get .dts
snippet that works for you and commands you are using for testing?

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* RE: [PATCH net-next 1/1] net: dsa: microchip: Add Microchip KSZ8895 DSA driver
From: Tristram.Ha @ 2017-12-05 22:16 UTC (permalink / raw)
  To: pavel
  Cc: andrew, f.fainelli, muvarov, nathan.leigh.conrad, vivien.didelot,
	UNGLinuxDriver, netdev
In-Reply-To: <20171120105609.GA4460@amd>

> Thanks for patches. I installed whole series on top of net-next.
> 
> Hardware is:
> 
> root@miro:~# cat /proc/cpuinfo
> model name   : ARM926EJ-S rev 5 (v5l)
> Hardware	  : Freescale MXS (Device Tree)
> 
> I added devicetree chunks, and enabled DSA in the config. It seems
> switch is detected:
> 
> [    4.775934] Micrel KSZ8051 dsa-0.0:00: attached PHY driver [Micrel
> KSZ8051] (mii_bus:phy_addr=dsa-0.0:00, irq=POLL)
> [    4.885952] Micrel KSZ8051 dsa-0.0:01: attached PHY driver [Micrel
> KSZ8051] (mii_bus:phy_addr=dsa-0.0:01, irq=POLL)
> [    4.995934] Micrel KSZ8051 dsa-0.0:02: attached PHY driver [Micrel
> KSZ8051] (mii_bus:phy_addr=dsa-0.0:02, irq=POLL)
> [    5.011484] DSA: tree 0 setup
> 
> root@miro:~# ifconfig lan3 192.168.20.103 netmask 255.255.0.0 up
> [  131.196667] IPv6: ADDRCONF(NETDEV_UP): lan3: link is not ready
> root@miro:~# [  132.225863] ksz8895-switch spi2.0 lan3: Link is Up -
> 100Mbps/Full - flow control rx/tx
> [  132.233939] IPv6: ADDRCONF(NETDEV_CHANGE): lan3: link becomes ready
> 
> root@miro:~# ping 192.168.1.1
> PING 192.168.1.1 (192.168.1.1): 56 data bytes
> ^C
> --- 192.168.1.1 ping statistics ---
> 7 packets transmitted, 0 packets received, 100% packet loss
> root@miro:~# ifconfig [  149.904234] random: crng init done
> 
> But packets do not go through, and there is nothing helpful in
> dmesg. Dts part is:
> 
>                 spi@0 {
>                        	compatible = "microchip,ksz8895";
>                         spi-max-frequency = <25000000>;
>                         reg = <0>;
> 			// reset-gpios = <&gpio2 8 0>;
>                         status = "okay";
> 
>                         spi-cpha;
> 			spi-cpol;
>                        ports {
>                              #address-cells = <1>;
>                              #size-cells = <0>;
>                              port@0 {
>                                     reg = <0>;
>                                     label = "lan1";
>                              };
>                              port@1 {
>                                     reg = <1>;
>                                     label = "lan2";
>                              };
>                              port@2 {
>                                     reg = <2>;
>                                     label = "lan3";
>                              };
>                              port@4 {
>                                     reg = <4>;
>                                     label = "cpu";
>                                     ethernet = <&mac0>;
>                                     fixed-link {
>                                                speed = <100>;
>                                                full-duplex;
>                                     };
>                              };
>                        };
> 		};
> 
> I went back to my version of dsa patches, and test above works as
> expected.

Sorry to be this late for the reply.  I finally got hold of a KSZ8895 board that
works with my SoC board to confirm the network communication.

As expected the KSZ8895 board works correctly as the chip uses the same
tail tagging feature in KSZ8795, and I did verify that board is working.

One thing to debug this problem is to dump the MIB counters.  Use the ethtool
utility to show MIB counters of both ports:

ethtool -S lan3
ethtool -S eth0

Assuming eth0 is the MAC controller that drives the switch, the receive counters of
the host port of the switch should match the transmit counters of lan3, and vice versa.

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 22:09 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Paul E. McKenney, linux-kernel, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205215700.GV3165@worktop.lehotels.local>

On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
> 
> Yeah, so?

Oh my point was I can't just look for READ_ONCE and go
*that's the pair*. there are too many of these.
At Paul's suggestion I will document the pairing *this read once has a
barrier that is paired with that barrier*.

> Complain to the compiler people for forcing us into that.

In some cases when you end up with all accesses
going through read/write once volatile just might better.

> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
> 
> No, the whole point of the exercise was to get away from the fact that
> dependent loads are special.

It's a pity that dependent stores are still special.

-- 
MST

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 22:09 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Michael S. Tsirkin, linux-kernel, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205213644.GU7829@linux.vnet.ibm.com>

On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located.  Would that work for you?

I would say that that is mandatory for any memory ordering code ;-)

^ permalink raw reply

* Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command
From: Scott Branden @ 2017-12-05 22:06 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: John W. Linville, BCM Kernel Feedback, Steve Lin, Michael Chan,
	netdev, Paul Greenwalt, Stephen Hemminger
In-Reply-To: <20171205213012.i2me64tpnump362c@unicorn.suse.cz>

Hi Michal,

thanks for review - see comments

On 17-12-05 01:30 PM, Michal Kubecek wrote:
> On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
>> Add ETHTOOL_RESET support via --reset command.
>>
>> ie.  ethtool --reset DEVNAME <flagname(s)>
>>
>> flagnames currently match the ETH_RESET_xxx names:
>> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
>>
>> Alternatively, you can specific component bitfield directly using
>> ethtool --reset DEVNAME flags %x
> IMHO it would be more consistent with e.g. msglvl without the keyword
> "flags".
I don't see the consistency in ethtool of specifying a number without a 
keyword in front of it.
I can only find --set-dump specify a number?
Others have keyword and number.  msglvl is the keyword after specifying 
-s - same as flags is the keyword I use after specifying --reset.
Whichever convention is decided for the --reset command I will change 
the patch to though.

>   It would be also nice to provide a symbolic way to specify the
> shared flags.
I'll change to allow -shared to be added to the end of each component 
specified to use the shared bit.
  IE. mgmt-shared, irq-shared, dma-shared ?
>
>> +static int do_reset(struct cmd_context *ctx)
>> +{
>> +	struct ethtool_value resetinfo;
>> +	int argc = ctx->argc;
>> +	char **argp = ctx->argp;
>> +	int i;
>> +
>> +	if (argc == 0)
>> +		exit_bad_args();
>> +
>> +	resetinfo.data = 0;
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		if (!strcmp(argp[i], "flags")) {
>> +			__u32 flags;
>> +
>> +			i++;
>> +			if (i >= argc)
>> +				exit_bad_args();
>> +			flags = strtoul(argp[i], NULL, 0);
>> +			if (flags == 0)
>> +				exit_bad_args();
>> +			else
>> +				resetinfo.data |= flags;
>> +		} else if (!strcmp(argp[i], "mgmt")) {
>> +			resetinfo.data |= ETH_RESET_MGMT;
>> +		} else if (!strcmp(argp[i], "irq")) {
>> +			resetinfo.data |= ETH_RESET_IRQ;
>> +		} else if (!strcmp(argp[i], "dma")) {
>> +			resetinfo.data |= ETH_RESET_DMA;
>> +		} else if (!strcmp(argp[i], "filter")) {
>> +			resetinfo.data |= ETH_RESET_FILTER;
>> +		} else if (!strcmp(argp[i], "offload")) {
>> +			resetinfo.data |= ETH_RESET_OFFLOAD;
>> +		} else if (!strcmp(argp[i], "mac")) {
>> +			resetinfo.data |= ETH_RESET_MAC;
>> +		} else if (!strcmp(argp[i], "phy")) {
>> +			resetinfo.data |= ETH_RESET_PHY;
>> +		} else if (!strcmp(argp[i], "ram")) {
>> +			resetinfo.data |= ETH_RESET_RAM;
>> +		} else if (!strcmp(argp[i], "ap")) {
>> +			resetinfo.data |= ETH_RESET_AP;
>> +		} else if (!strcmp(argp[i], "dedicated")) {
>> +			resetinfo.data |= ETH_RESET_DEDICATED;
>> +		} else if (!strcmp(argp[i], "all")) {
>> +			resetinfo.data |= ETH_RESET_ALL;
>> +		} else {
>> +			exit_bad_args();
>> +		}
>> +	}
>> +
>> +	resetinfo.cmd = ETHTOOL_RESET;
>> +
>> +	if (send_ioctl(ctx, &resetinfo)) {
>> +		perror("Cannot issue RESET");
>> +		return 1;
>> +	}
>> +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
> According to documentation, driver is supposed to clear the flags
> corresponding to components which were reset so that what is left are
> those which were _not_ reset.
I'll move the print above the send_ioctl.
> Michal Kubecek

^ permalink raw reply

* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-05 22:05 UTC (permalink / raw)
  To: Jesse Brandeburg
  Cc: achiad shochat, Jakub Kicinski, Hannes Frederic Sowa,
	Sridhar Samudrala, Alexander Duyck, virtualization,
	Shannon Nelson, Achiad, Peter Waskiewicz Jr, netdev,
	Anjali Singhai Jain, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171205135226.00002b68@intel.com>

On Tue, Dec 05, 2017 at 01:52:26PM -0800, Jesse Brandeburg wrote:
> On Tue, 5 Dec 2017 21:20:07 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
> > > Then we'll have a single solution for both netvsc and virtio (and any
> > > other PV device).
> > > And we could handle the VF DMA dirt issue agnostically.  
> > 
> > For the record, I won't block patches adding this kist to virtio
> > on the basis that they must be generic. It's not a lot
> > of code, implementation can come first, prettify later.
> 
> Thanks, based on this discussion we're going to work on improving
> virtio-net first, but some of Achiad's points are good.  I don't believe
> it should block the virtio work however.
> 
> In particular I'm really interested in figuring out how we can get to
> the point that virtio is able to make or implement some smart decisions
> about which NIC to pick for traffic delivery (it's own paravirt path or
> the passthorugh device path), if Achiad wants to develop the idea into
> some code, I'd be interested to review it.
> 
> > But we do need to have a discussion about how devices are paired.
> > I am not sure using just MAC works. E.g. some passthrough
> > devices don't give host ability to set the MAC.
> > Are these worth worrying about?
> 
> I personally don't think that will be much of a problem, if a
> certain device has that issue, can't we just have the virtio-net device
> pick up the MAC address of the passthrough device?

Then what do you do after you have migrated to another box?
The PT device there likely has a different MAC.

> As long as they match
> things should work OK. It at least is an initial way to do the
> configuration that has at least some traction as workable, as proved by
> the Microsoft design.

Yes - that design just implements what people have been doing for years
using bond so of course it's workable.

> FWIW, the Intel SR-IOV devices all accept a hypervisor/host provided
> MAC address.

For VFs you often can program the MAC through the PF, but you typically
can't do this for PFs. Or as another example consider nested virt with a
VF passed through.  PF isn't there within L1 guest so can't be used to
program the mac of the VF.

Still, we can always start small and require same mac, add other ways
to address issues later as we come up with them.

-- 
MST

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 22:02 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Peter Zijlstra, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
	mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205234308-mutt-send-email-mst@kernel.org>

On Tue, Dec 05, 2017 at 11:43:41PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > > and this barrier is no longer paired with anything until
> > > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > > > 
> > > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > > maybe there are other, better tools now.
> > > > > > 
> > > > > > There are quite a few people who say that smp_store_release() is
> > > > > > easier for the tools to analyze than is smp_wmb().  My experience with
> > > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > > that they are correct.
> > > > > 
> > > > > OK, but smp_store_release is still not paired with anything since we
> > > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > > > 
> > > > Why wouldn't you consider the smp_store_release() to be paired with
> > > > the new improved READ_ONCE()?
> > > 
> > > READ_ONCE is really all over the place (some code literally replaced all
> > > memory accesses with READ/WRITE ONCE).
> > > 
> > > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> > > 
> > > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > > and READ_POINTER for symmetry?
> > 
> > What we do in some code is to comment the pairings, allowing the other
> > side of the pairing to be easily located.  Would that work for you?
> 
> Yes, that's exactly what I did for now.

Very good, thank you!

							Thanx, Paul

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 21:57 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Paul E. McKenney, linux-kernel, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205222857-mutt-send-email-mst@kernel.org>

On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).

Yeah, so? Complain to the compiler people for forcing us into that.

> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?

No, the whole point of the exercise was to get away from the fact that
dependent loads are special.

^ permalink raw reply

* Re: [PATCHv2 2/2] userns: control capabilities of some user namespaces
From: Mahesh Bandewar (महेश बंडेवार) @ 2017-12-05 21:55 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Mahesh Bandewar, LKML, Netdev, Kernel-hardening, Linux API,
	Kees Cook, Eric W . Biederman, Eric Dumazet, David Miller
In-Reply-To: <20171129175712.GB14545@mail.hallyn.com>

On Wed, Nov 29, 2017 at 9:57 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Mahesh Bandewar (महेश बंडेवार) (maheshb@google.com):
>> On Tue, Nov 28, 2017 at 3:04 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Quoting Mahesh Bandewar (महेश बंडेवार) (maheshb@google.com):
>> > ...
>> >> >> diff --git a/security/commoncap.c b/security/commoncap.c
>> >> >> index fc46f5b85251..89103f16ac37 100644
>> >> >> --- a/security/commoncap.c
>> >> >> +++ b/security/commoncap.c
>> >> >> @@ -73,6 +73,14 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
>> >> >>  {
>> >> >>       struct user_namespace *ns = targ_ns;
>> >> >>
>> >> >> +     /* If the capability is controlled and user-ns that process
>> >> >> +      * belongs-to is 'controlled' then return EPERM and no need
>> >> >> +      * to check the user-ns hierarchy.
>> >> >> +      */
>> >> >> +     if (is_user_ns_controlled(cred->user_ns) &&
>> >> >> +         is_capability_controlled(cap))
>> >> >> +             return -EPERM;
>> >> >
>> >> > I'd be curious to see the performance impact on this on a regular
>> >> > workload (kernel build?) in a controlled ns.
>> >> >
>> >> Should it affect? If at all, it should be +ve since, the recursive
>> >> user-ns hierarchy lookup is avoided with the above check if the
>> >> capability is controlled.
>> >
>> > Yes but I expect that to be the rare case for normal lxc installs
>> > (which are of course what I am interested in)
>> >
>> >>  The additional cost otherwise is this check
>> >> per cap_capable() call.
>> >
>> > And pipeline refetching?
>> >
>> > Capability calls also shouldn't be all that frequent, but still I'm
>> > left wondering...
>>
>> Correct, and capability checks are part of the control-path and not
>> the data-path so shouldn't matter but I guess it doesn't hurt to
>> find-out the number. Do you have any workload in mind, that we can use
>> for this test/benchmark?
>
> I suppose if you did both (a) a kernel build and (b) a webserve
> like https://github.com/m3ng9i/ran , being hit for a minute by a
> heavy load of requests, those two together would be re-assuring.
>
Well, I did (a) and (b). Here are the results.

(a0) I used the ubuntu-artful (17.10) vm instance with standard kernel
to compile the kernel

mahesh@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s clean
mahesh@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s
real 6m47.525s
user 22m37.424s
sys 2m44.745s

(b0) Now in an user-namespce create by an user that does not have
SYS_ADMIN (just for apples-to-apples comparison)
mahesh@mahesh-vm0-artful:~$ sysctl -q kernel.controlled_userns_caps_whitelist
sysctl: cannot stat /proc/sys/kernel/controlled_userns_caps_whitelist:
No such file or directory
mahesh@mahesh-vm0-artful:~$ id
uid=1000(mahesh) gid=1000(mahesh)
groups=1000(mahesh),4(adm),24(cdrom),27(sudo),30(dip),46(plugdev),118(lpadmin),128(sambashare)
mahesh@mahesh-vm0-artful:~/Work/Linux$ unshare -Uf -- bash
nobody@mahesh-vm0-artful:~/Work/Linux$ id
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)
nobody@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s clean
nobody@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s
real 9m10.115s
user 25m20.984s
sys 2m48.129s


(a1) Now patched the same kernel and built and booted with this new kernel -

mahesh@mahesh-vm0-artful:~$ sysctl -q kernel.controlled_userns_caps_whitelist
kernel.controlled_userns_caps_whitelist = 1f,ffffffff
mahesh@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s clean
mahesh@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s
real 6m39.964s
user 22m23.538s
sys 2m34.258s

(b1) Now in an user-namespace created by an user that does not have SYS_ADMIN

mahesh@mahesh-vm0-artful:~$ id
uid=1000(mahesh) gid=1000(mahesh)
groups=1000(mahesh),4(adm),24(cdrom),27(sudo),30(dip),46(plugdev),118(lpadmin),128(sambashare)
mahesh@mahesh-vm0-artful:~/Work/Linux$ unshare -Uf -- bash
nobody@mahesh-vm0-artful:~/Work/Linux$ id
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)
nobody@mahesh-vm0-artful:~/Work/Linux$ make -s clean
nobody@mahesh-vm0-artful:~/Work/Linux$ time make -j4 -s
real 6m54.725s
user 23m18.833s
sys 2m38.996s

---

For the http-get test, I used the same 'ran' utility you have proposed
and wrapped inside a script like -
mahesh@mahesh-vm0-artful:~/Work/Scripts$ cat RanLauncher1m.sh
#!/bin/bash
set -v
(sleep 60; killall ran) &
time (cd ~/go/bin; ./ran -i index.html >& /dev/null)

and another script that constantly performs wget -
mahesh@mahesh-vm0-artful:~/Work/Scripts$ cat WgetLoop.sh#!/bin/bash
#set -v
while true; do
wget http://127.0.0.1:8080 >& /dev/null

... here are the results -
(A0) Kernel that is unpatched and comes with ubuntu-artful

mahesh@mahesh-vm0-artful:~/Work/Scripts$ ./RanLauncher1m.sh
(sleep 60; killall ran) &
time (cd ~/go/bin; ./ran -i index.html >& /dev/null)
real 1m0.009s
user 0m2.885s
sys 0m2.774s

(B0) Now in an user-ns created by an user that does not have SYS_ADMIN

mahesh@mahesh-vm0-artful:~/Work/Scripts$ unshare -Uf -- bash
nobody@mahesh-vm0-artful:~/Work/Scripts$ ./RanLauncher1m.sh
(sleep 60; killall ran) &
time (cd ~/go/bin; ./ran -i index.html >& /dev/null)
real 1m0.004s
user 0m3.003s
sys 0m2.737s

(A1) With the patched kernel

mahesh@mahesh-vm0-artful:~/Work/Scripts$ ./RanLauncher1m.sh
(sleep 60; killall ran) &
time (cd ~/go/bin; ./ran -i index.html >& /dev/null)

real 1m0.005s
user 0m1.941s
sys 0m1.507s


(B1) With patched kernel and inside user-ns

mahesh@mahesh-vm0-artful:~/Work/Scripts$ unshare -Uf -- bash
nobody@mahesh-vm0-artful:~/Work/Scripts$ ./RanLauncher1m.sh
(sleep 60; killall ran) &
time (cd ~/go/bin; ./ran -i index.html >& /dev/null)
real 1m0.004s
user 0m1.513s
sys 0m1.254s

> thanks,
> -serge

^ permalink raw reply

* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jesse Brandeburg @ 2017-12-05 21:52 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: achiad shochat, Jakub Kicinski, Hannes Frederic Sowa,
	Sridhar Samudrala, Alexander Duyck, virtualization,
	Shannon Nelson, Achiad, Peter Waskiewicz Jr, netdev,
	Anjali Singhai Jain, Andy Gospodarek, Or Gerlitz,
	jesse.brandeburg
In-Reply-To: <20171205211603-mutt-send-email-mst@kernel.org>

On Tue, 5 Dec 2017 21:20:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
> > Then we'll have a single solution for both netvsc and virtio (and any
> > other PV device).
> > And we could handle the VF DMA dirt issue agnostically.  
> 
> For the record, I won't block patches adding this kist to virtio
> on the basis that they must be generic. It's not a lot
> of code, implementation can come first, prettify later.

Thanks, based on this discussion we're going to work on improving
virtio-net first, but some of Achiad's points are good.  I don't believe
it should block the virtio work however.

In particular I'm really interested in figuring out how we can get to
the point that virtio is able to make or implement some smart decisions
about which NIC to pick for traffic delivery (it's own paravirt path or
the passthorugh device path), if Achiad wants to develop the idea into
some code, I'd be interested to review it.

> But we do need to have a discussion about how devices are paired.
> I am not sure using just MAC works. E.g. some passthrough
> devices don't give host ability to set the MAC.
> Are these worth worrying about?

I personally don't think that will be much of a problem, if a
certain device has that issue, can't we just have the virtio-net device
pick up the MAC address of the passthrough device? As long as they match
things should work OK. It at least is an initial way to do the
configuration that has at least some traction as workable, as proved by
the Microsoft design.

FWIW, the Intel SR-IOV devices all accept a hypervisor/host provided
MAC address.

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:43 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Peter Zijlstra, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
	mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205213644.GU7829@linux.vnet.ibm.com>

On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > > 
> > > [ . . . ]
> > > 
> > > > > > and this barrier is no longer paired with anything until
> > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > > 
> > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > maybe there are other, better tools now.
> > > > > 
> > > > > There are quite a few people who say that smp_store_release() is
> > > > > easier for the tools to analyze than is smp_wmb().  My experience with
> > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > that they are correct.
> > > > 
> > > > OK, but smp_store_release is still not paired with anything since we
> > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > > 
> > > Why wouldn't you consider the smp_store_release() to be paired with
> > > the new improved READ_ONCE()?
> > 
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
> > 
> > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> > 
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
> 
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located.  Would that work for you?
> 
> 							Thanx, Paul

Yes, that's exactly what I did for now.

Thanks!

-- 
MST

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:42 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Paul E. McKenney, linux-kernel, mingo, jiangshanlai, dipankar,
	akpm, mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205211735.GT3165@worktop.lehotels.local>

On Tue, Dec 05, 2017 at 10:17:35PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > > WRITE_ONCE(obj->val, 1);
> > > > > > smp_wmb();
> > > > > > WRITE_ONCE(*foo, obj);
> > > > > 
> > > > > I believe Peter was instead suggesting:
> > > > > 
> > > > > WRITE_ONCE(obj->val, 1);
> > > > > smp_store_release(foo, obj);
> > > > 
> > > > Isn't that more expensive though?
> > > 
> > > Depends on the architecture. The only architecture where it is more
> > > expensive and people actually still care about is ARM I think.
> > 
> > Right. Why should I use the more expensive smp_store_release then?
> 
> Because it makes more sense. Memory ordering is hard enough, don't make
> it harder still if you don't have to.

I suspect I have to -  ptr_ring is a very low level construct used by
netowrking on data path so making it a bit more complicated for
a bit of performance is probably justified.

-- 
MST

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 21:36 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Peter Zijlstra, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
	mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205222857-mutt-send-email-mst@kernel.org>

On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > 
> > [ . . . ]
> > 
> > > > > and this barrier is no longer paired with anything until
> > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > 
> > > > > Barrier pairing was a useful tool to check code validity,
> > > > > maybe there are other, better tools now.
> > > > 
> > > > There are quite a few people who say that smp_store_release() is
> > > > easier for the tools to analyze than is smp_wmb().  My experience with
> > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > that they are correct.
> > > 
> > > OK, but smp_store_release is still not paired with anything since we
> > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > 
> > Why wouldn't you consider the smp_store_release() to be paired with
> > the new improved READ_ONCE()?
> 
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
> 
> And I also prefer smp_wmb as it seems to be cheaper on ARM.
> 
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?

What we do in some code is to comment the pairings, allowing the other
side of the pairing to be easily located.  Would that work for you?

							Thanx, Paul

^ permalink raw reply

* Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command
From: Michal Kubecek @ 2017-12-05 21:30 UTC (permalink / raw)
  To: Scott Branden
  Cc: John W. Linville, BCM Kernel Feedback, Steve Lin, Michael Chan,
	netdev, Paul Greenwalt, Stephen Hemminger
In-Reply-To: <1512507203-17693-4-git-send-email-scott.branden@broadcom.com>

On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> Add ETHTOOL_RESET support via --reset command.
> 
> ie.  ethtool --reset DEVNAME <flagname(s)>
> 
> flagnames currently match the ETH_RESET_xxx names:
> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
> 
> Alternatively, you can specific component bitfield directly using
> ethtool --reset DEVNAME flags %x

IMHO it would be more consistent with e.g. msglvl without the keyword
"flags". It would be also nice to provide a symbolic way to specify the
shared flags.

> +static int do_reset(struct cmd_context *ctx)
> +{
> +	struct ethtool_value resetinfo;
> +	int argc = ctx->argc;
> +	char **argp = ctx->argp;
> +	int i;
> +
> +	if (argc == 0)
> +		exit_bad_args();
> +
> +	resetinfo.data = 0;
> +
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(argp[i], "flags")) {
> +			__u32 flags;
> +
> +			i++;
> +			if (i >= argc)
> +				exit_bad_args();
> +			flags = strtoul(argp[i], NULL, 0);
> +			if (flags == 0)
> +				exit_bad_args();
> +			else
> +				resetinfo.data |= flags;
> +		} else if (!strcmp(argp[i], "mgmt")) {
> +			resetinfo.data |= ETH_RESET_MGMT;
> +		} else if (!strcmp(argp[i], "irq")) {
> +			resetinfo.data |= ETH_RESET_IRQ;
> +		} else if (!strcmp(argp[i], "dma")) {
> +			resetinfo.data |= ETH_RESET_DMA;
> +		} else if (!strcmp(argp[i], "filter")) {
> +			resetinfo.data |= ETH_RESET_FILTER;
> +		} else if (!strcmp(argp[i], "offload")) {
> +			resetinfo.data |= ETH_RESET_OFFLOAD;
> +		} else if (!strcmp(argp[i], "mac")) {
> +			resetinfo.data |= ETH_RESET_MAC;
> +		} else if (!strcmp(argp[i], "phy")) {
> +			resetinfo.data |= ETH_RESET_PHY;
> +		} else if (!strcmp(argp[i], "ram")) {
> +			resetinfo.data |= ETH_RESET_RAM;
> +		} else if (!strcmp(argp[i], "ap")) {
> +			resetinfo.data |= ETH_RESET_AP;
> +		} else if (!strcmp(argp[i], "dedicated")) {
> +			resetinfo.data |= ETH_RESET_DEDICATED;
> +		} else if (!strcmp(argp[i], "all")) {
> +			resetinfo.data |= ETH_RESET_ALL;
> +		} else {
> +			exit_bad_args();
> +		}
> +	}
> +
> +	resetinfo.cmd = ETHTOOL_RESET;
> +
> +	if (send_ioctl(ctx, &resetinfo)) {
> +		perror("Cannot issue RESET");
> +		return 1;
> +	}
> +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);

According to documentation, driver is supposed to clear the flags
corresponding to components which were reset so that what is left are
those which were _not_ reset.

Michal Kubecek

^ permalink raw reply

* Re: [PATCH 2/2] net: macb: kill useless use of list_empty()
From: Nicolas Ferre @ 2017-12-05 21:28 UTC (permalink / raw)
  To: Julia Cartwright, Julia Lawall
  Cc: Rafal Ozieblo, netdev, linux-kernel, kbuild-all
In-Reply-To: <20171205201830.GB18445@jcartwri.amer.corp.natinst.com>

On 05/12/2017 at 21:18, Julia Cartwright wrote:
> The list_for_each_entry() macro already handles the case where the list
> is empty (by not executing the loop body).  It's not necessary to handle
> this case specially, so stop doing so.
> 
> Cc: Rafal Ozieblo <rafalo@cadence.com>
> Signed-off-by: Julia Cartwright <julia@ni.com>
> ---
> This is an additional cleanup patch found when looking at this code.
> 
>    Julia

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks

> 
>  drivers/net/ethernet/cadence/macb_main.c | 34 ++++++++++++--------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e7ef104a077d..3643c6ad2322 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2815,25 +2815,22 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  	spin_lock_irqsave(&bp->rx_fs_lock, flags);
>  
>  	/* find correct place to add in list */
> -	if (list_empty(&bp->rx_fs_list.list))
> -		list_add(&newfs->list, &bp->rx_fs_list.list);
> -	else {
> -		list_for_each_entry(item, &bp->rx_fs_list.list, list) {
> -			if (item->fs.location > newfs->fs.location) {
> -				list_add_tail(&newfs->list, &item->list);
> -				added = true;
> -				break;
> -			} else if (item->fs.location == fs->location) {
> -				netdev_err(netdev, "Rule not added: location %d not free!\n",
> -						fs->location);
> -				ret = -EBUSY;
> -				goto err;
> -			}
> +	list_for_each_entry(item, &bp->rx_fs_list.list, list) {
> +		if (item->fs.location > newfs->fs.location) {
> +			list_add_tail(&newfs->list, &item->list);
> +			added = true;
> +			break;
> +		} else if (item->fs.location == fs->location) {
> +			netdev_err(netdev, "Rule not added: location %d not free!\n",
> +					fs->location);
> +			ret = -EBUSY;
> +			goto err;
>  		}
> -		if (!added)
> -			list_add_tail(&newfs->list, &bp->rx_fs_list.list);
>  	}
>  
> +	if (!added)
> +		list_add_tail(&newfs->list, &bp->rx_fs_list.list);
> +
>  	gem_prog_cmp_regs(bp, fs);
>  	bp->rx_fs_list.count++;
>  	/* enable filtering if NTUPLE on */
> @@ -2859,11 +2856,6 @@ static int gem_del_flow_filter(struct net_device *netdev,
>  
>  	spin_lock_irqsave(&bp->rx_fs_lock, flags);
>  
> -	if (list_empty(&bp->rx_fs_list.list)) {
> -		spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
> -		return -EINVAL;
> -	}
> -
>  	list_for_each_entry(item, &bp->rx_fs_list.list, list) {
>  		if (item->fs.location == cmd->fs.location) {
>  			/* disable screener regs for the flow entry */
> 


-- 
Nicolas Ferre

^ permalink raw reply

* Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions
From: Nicolas Ferre @ 2017-12-05 21:26 UTC (permalink / raw)
  To: Julia Cartwright, Julia Lawall
  Cc: Rafal Ozieblo, netdev, linux-kernel, kbuild-all
In-Reply-To: <20171205201711.GA18445@jcartwri.amer.corp.natinst.com>

On 05/12/2017 at 21:17, Julia Cartwright wrote:
> Commit ae8223de3df5 ("net: macb: Added support for RX filtering")
> introduces a lock, rx_fs_lock which is intended to protect the list of
> rx_flow items and synchronize access to the hardware rx filtering
> registers.
> 
> However, the region protected by this lock is overscoped, unnecessarily
> including things like slab allocation.  Reduce this lock scope to only
> include operations which must be performed atomically: list traversal,
> addition, and removal, and hitting the macb filtering registers.
> 
> This fixes the use of kmalloc w/ GFP_KERNEL in atomic context.
> 
> Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering")
> Cc: Rafal Ozieblo <rafalo@cadence.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Julia Cartwright <julia@ni.com>
> ---
> While Julia Lawall's cocci-generated patch fixes the problem, the right
> solution is to obviate the problem altogether.
> 
> Thanks,
>    The Other Julia

Julia,

Thanks for your patch, it seems good indeed. Here is my:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

As the patch by Julia L. is already in net-next, I suspect that you
would need to add a kind of revert patch if we want to come back to a
more usual GFP_KERNEL for the kmalloc.

Best regards,
  Nicolas

>  drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c5fa87cdc6c4..e7ef104a077d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2796,6 +2796,7 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  	struct macb *bp = netdev_priv(netdev);
>  	struct ethtool_rx_flow_spec *fs = &cmd->fs;
>  	struct ethtool_rx_fs_item *item, *newfs;
> +	unsigned long flags;
>  	int ret = -EINVAL;
>  	bool added = false;
>  
> @@ -2811,6 +2812,8 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  			htonl(fs->h_u.tcp_ip4_spec.ip4dst),
>  			htons(fs->h_u.tcp_ip4_spec.psrc), htons(fs->h_u.tcp_ip4_spec.pdst));
>  
> +	spin_lock_irqsave(&bp->rx_fs_lock, flags);
> +
>  	/* find correct place to add in list */
>  	if (list_empty(&bp->rx_fs_list.list))
>  		list_add(&newfs->list, &bp->rx_fs_list.list);
> @@ -2837,9 +2840,11 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  	if (netdev->features & NETIF_F_NTUPLE)
>  		gem_enable_flow_filters(bp, 1);
>  
> +	spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
>  	return 0;
>  
>  err:
> +	spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
>  	kfree(newfs);
>  	return ret;
>  }
> @@ -2850,9 +2855,14 @@ static int gem_del_flow_filter(struct net_device *netdev,
>  	struct macb *bp = netdev_priv(netdev);
>  	struct ethtool_rx_fs_item *item;
>  	struct ethtool_rx_flow_spec *fs;
> +	unsigned long flags;
>  
> -	if (list_empty(&bp->rx_fs_list.list))
> +	spin_lock_irqsave(&bp->rx_fs_lock, flags);
> +
> +	if (list_empty(&bp->rx_fs_list.list)) {
> +		spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
>  		return -EINVAL;
> +	}
>  
>  	list_for_each_entry(item, &bp->rx_fs_list.list, list) {
>  		if (item->fs.location == cmd->fs.location) {
> @@ -2869,12 +2879,14 @@ static int gem_del_flow_filter(struct net_device *netdev,
>  			gem_writel_n(bp, SCRT2, fs->location, 0);
>  
>  			list_del(&item->list);
> -			kfree(item);
>  			bp->rx_fs_list.count--;
> +			spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
> +			kfree(item);
>  			return 0;
>  		}
>  	}
>  
> +	spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
>  	return -EINVAL;
>  }
>  
> @@ -2943,11 +2955,8 @@ static int gem_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
>  static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
>  {
>  	struct macb *bp = netdev_priv(netdev);
> -	unsigned long flags;
>  	int ret;
>  
> -	spin_lock_irqsave(&bp->rx_fs_lock, flags);
> -
>  	switch (cmd->cmd) {
>  	case ETHTOOL_SRXCLSRLINS:
>  		if ((cmd->fs.location >= bp->max_tuples)
> @@ -2966,7 +2975,6 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
>  		ret = -EOPNOTSUPP;
>  	}
>  
> -	spin_unlock_irqrestore(&bp->rx_fs_lock, flags);
>  	return ret;
>  }
>  
> 


-- 
Nicolas Ferre

^ permalink raw reply

* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:24 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Peter Zijlstra, linux-kernel, mingo, jiangshanlai, dipankar, akpm,
	mathieu.desnoyers, josh, tglx, rostedt, dhowells, edumazet,
	fweisbec, oleg, Jason Wang, kvm, virtualization, netdev
In-Reply-To: <20171205200801.GR7829@linux.vnet.ibm.com>

On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> 
> [ . . . ]
> 
> > > > and this barrier is no longer paired with anything until
> > > > you realize there's a dependency barrier within READ_ONCE.
> > > > 
> > > > Barrier pairing was a useful tool to check code validity,
> > > > maybe there are other, better tools now.
> > > 
> > > There are quite a few people who say that smp_store_release() is
> > > easier for the tools to analyze than is smp_wmb().  My experience with
> > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > that they are correct.
> > 
> > OK, but smp_store_release is still not paired with anything since we
> > rely on READ_ONCE to include the implicit dpendendency barrier.
> 
> Why wouldn't you consider the smp_store_release() to be paired with
> the new improved READ_ONCE()?
> 
> 							Thanx, Paul

READ_ONCE is really all over the place (some code literally replaced all
memory accesses with READ/WRITE ONCE).

And I also prefer smp_wmb as it seems to be cheaper on ARM.

Would an API like WRITE_POINTER()/smp_store_pointer make sense,
and READ_POINTER for symmetry?

-- 
MST

^ permalink raw reply

* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
From: Linus Torvalds @ 2017-12-05 21:22 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: David Laight, Kees Cook, Tobin C. Harding,
	kernel-hardening@lists.openwall.com, Jason A. Donenfeld,
	Theodore Ts'o, Paolo Bonzini, Tycho Andersen,
	Roberts, William C, Tejun Heo, Jordan Glover, Greg KH,
	Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky,
	Catalin Marinas, Will Deacon, Steven Rostedt
In-Reply-To: <596c917e-3183-d80a-89d4-97e0eb5dc077@infradead.org>

On Tue, Dec 5, 2017 at 1:08 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
>
> This kind of option (with default hashed) is what I was just thinking of
> after having seen a few unhelpful traces.  But then the knob might not be
> changed in time for the traces either. :(

.. I really dislike the idea of such a knob.

First off, the traces I've seen that had the new %p behavior, the
hashing didn't actually matter AT ALL. The only values that were
hashed were values that weren't actually useful for debugging the
oops.

Secondly, the notion that "we want a unhashed knob for debugging" is
exactly the wrong kind of mentality. 99% of all bug reports happen in
the wild - not on developer boxes. So by default, those bug reports
had better happen with hashing enabled, or it's all entirely
pointless.

If you have an oops that happens on your own box due to code that
you're writing yourself (and expect to debug yourself), then honestly,
the hashing is going to be the least of your issues. If you can't find
out the bug under those circumstances, and you're confused by the tiny
detail of hashing, you're doing something wrong.

So the case that matters is when an oops comes from some outside
source that won't have turned the knob off anyway.

So no. We're not adding a knob. It is fundamentally pointless.

It's not like those hex numbers were really helping people anyway.
We've turned off most of them on x86 oops reports long ago (and
entirely independently of the pointer hashing). Having stared at a lot
of oopses in my time, the only hex numbers that tend to be really
relevant are (a) the register contents (which aren't %p anyway), and
things like the faulting address (which is not, and never has been, %p
on x86, but might be on some other architecture).

Honestly, the next time anybody says "hashing makes debugging harder",
I'm going to require some actual proof of an actual oops where it
mattered that a particular value was hashed.

Not hand-waving.

Not "it surprised and confused me" because it looked different. You'll
get used to it.

So an actual "this was critical information that mattered for this
particular bug, and it was missing due to the hashing of this
particular value and debugging was harder in actual reality due to
that".

Because the actual example I have seen so far, not only didn't the
hashing matter AT ALL, most of the _unhashed_ values shouldn't have
been there either, and were due to arm still printing stuff that
shouldn't have been printed at all and just made the oops more complex
and harder to read and report.

            Linus

^ permalink raw reply

* Re: [PATCH RFC V1 net-next 0/6] Time based packet transmission
From: Vinicius Costa Gomes @ 2017-12-05 21:22 UTC (permalink / raw)
  To: David Miller, rcochran
  Cc: netdev, linux-kernel, intel-wired-lan, andre.guedes, anna-maria,
	henrik, jesus.sanchez-palencia, john.stultz, tglx
In-Reply-To: <20170918.093436.1069699729581966538.davem@davemloft.net>

Hi David,

David Miller <davem@davemloft.net> writes:

> From: Richard Cochran <rcochran@linutronix.de>
> Date: Mon, 18 Sep 2017 09:41:15 +0200
>
>>   - The driver does not handle out of order packets.  If user space
>>     sends a packet with an earlier Tx time, then the code should stop
>>     the queue, reshuffle the descriptors accordingly, and then
>>     restart the queue.
>
> The user should simply be not allowed to do this.
>
> Once the packet is in the device queue, that's it.  You cannot insert
> a new packet to be transmitted before an already hw queued packet,
> period.
>
> Any out of order request should be rejected with an error.

Just to clarify, I agree that after after the packet is enqueued to the
HW, there's no going back, in another words, we must never enqueue
anything to the HW with a timestamp earlier than the last enqueued
packet.

But re-ordering packets at the Qdisc level is, I think, necessary: two
applications (one (A) with period of 50us and the other (B) of 100us),
if it happens that (B) enqueues its packet before (A), I think, we would
have a problem.

The problem is deciding for how long we should keep packets in the Qdisc
queue. In the implementation we are working on, this is left for the
user to decide.

Or do you have a reason for not doing *any* kind of re-ordering?

>
> I'd say the same is true for requests to send packets timed
> in the past.

+1


Cheers,
--
Vinicius

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox