[PATCH] tools: usb: usbip: adding support for older kernel versions

[PATCH] tools: usb: usbip: adding support for older kernel versions

[David Valleau]

The current usbip tool relies on the behavior of the vhci-hcd driver in
order to work correctly. In instances where a newer version of the tool
is used with an older version of the kernel, there are incompatibilities
that can sometimes result in failure.

This patch adds some fallback capabilities so that the tool will work
with the vhci-hcd driver from kernel version 3.18 onward.

Signed-off-by: David Valleau <valleau@chromium.org>
---

 tools/usb/usbip/libsrc/vhci_driver.c | 246 ++++++++++++++++++++++++---
 tools/usb/usbip/libsrc/vhci_driver.h |   7 +
 2 files changed, 228 insertions(+), 25 deletions(-)

diff --git a/tools/usb/usbip/libsrc/vhci_driver.c b/tools/usb/usbip/libsrc/vhci_driver.c
index ed8c9d360c0f..4e02cf6832a6 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -37,7 +37,136 @@ imported_device_init(struct usbip_imported_device *idev, char *busid)
 	return NULL;
 }
 
-static int parse_status(const char *value)
+static void assign_idev(struct usbip_imported_device *idev, uint8_t port,
+			uint32_t status, uint32_t devid)
+{
+	idev->port	= port;
+	idev->status	= status;
+	idev->devid	= devid;
+	idev->busnum	= (devid >> 16);
+	idev->devnum	= (devid & 0x0000ffff);
+}
+
+static int init_idev(struct usbip_imported_device *idev, const char *lbusid)
+{
+	if (idev->status != VDEV_ST_NULL &&
+	    idev->status != VDEV_ST_NOTASSIGNED) {
+		idev = imported_device_init(idev, lbusid);
+		if (!idev) {
+			dbg("imported_device_init failed");
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static int parse_status_3_18(const char *value)
+{
+	int ret = 0;
+	char *c;
+
+	/* skip a header line */
+	c = strchr(value, '\n');
+	if (!c)
+		return -1;
+	c++;
+
+	while (*c != '\0') {
+		int port, status, speed, devid;
+		unsigned long socket;
+		char lbusid[SYSFS_BUS_ID_SIZE];
+		struct usbip_imported_device *idev;
+
+		ret = sscanf(c, "%d %d %d %x %lx %31s\n",
+				&port, &status, &speed,
+				&devid, &socket, lbusid);
+
+		if (ret < 5) {
+			dbg("sscanf failed: %d", ret);
+			return -1;
+		}
+
+		dbg("port %d status %d speed %d devid %x",
+				port, status, speed, devid);
+		dbg("socket %lx lbusid %s", socket, lbusid);
+
+		/* if a device is connected, look at it */
+		idev = &vhci_driver->idev[port];
+		memset(idev, 0, sizeof(*idev));
+
+		/* assume that the hub is high-speed */
+		idev->hub = HUB_SPEED_HIGH;
+		assign_idev(idev, port, status, devid);
+
+		if (init_idev(idev, lbusid))
+			return -1;
+
+		/* go to the next line */
+		c = strchr(c, '\n');
+		if (!c)
+			break;
+		c++;
+	}
+
+	dbg("exit");
+
+	return 0;
+}
+
+static int parse_status_4_4(const char *value)
+{
+	int ret = 0;
+	char *c;
+
+	/* skip a header line */
+	c = strchr(value, '\n');
+	if (!c)
+		return -1;
+	c++;
+
+	while (*c != '\0') {
+		int port, status, speed, devid;
+		int sockfd;
+		char lbusid[SYSFS_BUS_ID_SIZE];
+		struct usbip_imported_device *idev;
+
+		ret = sscanf(c, "%d %d %d %x %u %31s\n",
+				&port, &status, &speed,
+				&devid, &sockfd, lbusid);
+
+		if (ret < 5) {
+			dbg("sscanf failed: %d", ret);
+			return -1;
+		}
+
+		dbg("port %d status %d speed %d devid %x",
+				port, status, speed, devid);
+		dbg("sockfd %u lbusid %s", sockfd, lbusid);
+
+		/* if a device is connected, look at it */
+		idev = &vhci_driver->idev[port];
+		memset(idev, 0, sizeof(*idev));
+
+		/* assume that the hub is high-speed */
+		idev->hub = HUB_SPEED_HIGH;
+		assign_idev(idev, port, status, devid);
+
+		if (init_idev(idev, lbusid))
+			return -1;
+
+		/* go to the next line */
+		c = strchr(c, '\n');
+		if (!c)
+			break;
+		c++;
+	}
+
+	dbg("exit");
+
+	return 0;
+}
+
+static int parse_status_4_14(const char *value)
 {
 	int ret = 0;
 	char *c;
@@ -61,7 +190,7 @@ static int parse_status(const char *value)
 
 		if (ret < 5) {
 			dbg("sscanf failed: %d", ret);
-			BUG();
+			return -1;
 		}
 
 		dbg("hub %s port %d status %d speed %d devid %x",
@@ -77,22 +206,10 @@ static int parse_status(const char *value)
 		else /* strncmp("ss", hub, 2) == 0 */
 			idev->hub = HUB_SPEED_SUPER;
 
-		idev->port	= port;
-		idev->status	= status;
-
-		idev->devid	= devid;
+		assign_idev(idev, port, status, devid);
 
-		idev->busnum	= (devid >> 16);
-		idev->devnum	= (devid & 0x0000ffff);
-
-		if (idev->status != VDEV_ST_NULL
-		    && idev->status != VDEV_ST_NOTASSIGNED) {
-			idev = imported_device_init(idev, lbusid);
-			if (!idev) {
-				dbg("imported_device_init failed");
-				return -1;
-			}
-		}
+		if (init_idev(idev, lbusid))
+			return -1;
 
 		/* go to the next line */
 		c = strchr(c, '\n');
@@ -106,6 +223,41 @@ static int parse_status(const char *value)
 	return 0;
 }
 
+/*
+ * Parses the sysfs status contained within value to populate the idev fields
+ * within vhci_driver.
+ *
+ * Uses the format of the header line to determine which parsing function to
+ * call.
+ */
+static int parse_status(const char *value)
+{
+	char *c;
+	char header[STATUS_HEADER_MAX] = { 0 };
+	size_t size;
+	int status = -1;
+
+	c = strchr(value, '\n');
+	if (!c)
+		return -1;
+
+	size = c - value;
+	if (size > STATUS_HEADER_MAX)
+		return -1;
+	strncpy(header, value, size);
+
+	if (strncmp(header, V3_18_STATUS_HEADER, size) == 0)
+		return parse_status_3_18(value);
+	if (strncmp(header, V4_4_STATUS_HEADER, size) == 0)
+		return parse_status_4_4(value);
+	if (strncmp(header, V4_14_STATUS_HEADER, size) == 0)
+		return parse_status_4_14(value);
+
+	dbg("unknown header format in status");
+
+	return -1;
+}
+
 #define MAX_STATUS_NAME 16
 
 static int refresh_imported_device_list(void)
@@ -135,14 +287,43 @@ static int refresh_imported_device_list(void)
 	return 0;
 }
 
+static int fallback_get_nports(struct udev_device *hc_device)
+{
+	char *c;
+	int nports = 0;
+	const char *attr_status;
+
+	attr_status = udev_device_get_sysattr_value(hc_device, "status");
+	if (!attr_status) {
+		err("udev_device_get_sysattr_value failed");
+		return -1;
+	}
+
+	/* skip a header line */
+	c = strchr(attr_status, '\n');
+	if (!c)
+		return 0;
+	c++;
+
+	while (*c != '\0') {
+		nports++;
+		/* go to the next line */
+		c = strchr(c, '\n');
+		if (!c)
+			return nports;
+		c++;
+	}
+
+	return nports;
+}
+
 static int get_nports(struct udev_device *hc_device)
 {
 	const char *attr_nports;
 
 	attr_nports = udev_device_get_sysattr_value(hc_device, "nports");
 	if (!attr_nports) {
-		err("udev_device_get_sysattr_value nports failed");
-		return -1;
+		return fallback_get_nports(hc_device);
 	}
 
 	return (int)strtoul(attr_nports, NULL, 10);
@@ -150,7 +331,8 @@ static int get_nports(struct udev_device *hc_device)
 
 static int vhci_hcd_filter(const struct dirent *dirent)
 {
-	return strcmp(dirent->d_name, "vhci_hcd") >= 0;
+	return strncmp(dirent->d_name, USBIP_VHCI_DRV_NAME,
+		       strlen(USBIP_VHCI_DRV_NAME)) == 0;
 }
 
 static int get_ncontrollers(void)
@@ -240,6 +422,24 @@ static int read_record(int rhport, char *host, unsigned long host_len,
 
 /* ---------------------------------------------------------------------- */
 
+/*
+ * Attempt to open the vhci udev device. If the first lookup fails fall back to
+ * looking up the old name in order to support older kernel versions.
+ */
+static struct udev_device *open_hc_device()
+{
+	struct udev_device *device;
+
+	device = udev_device_new_from_subsystem_sysname(
+		udev_context, USBIP_VHCI_BUS_TYPE, USBIP_VHCI_DEVICE_NAME);
+	if (!device) {
+		device = udev_device_new_from_subsystem_sysname(
+			udev_context, USBIP_VHCI_BUS_TYPE,
+			USBIP_VHCI_DEVICE_NAME_OLD);
+	}
+	return device;
+}
+
 int usbip_vhci_driver_open(void)
 {
 	int nports;
@@ -252,10 +452,7 @@ int usbip_vhci_driver_open(void)
 	}
 
 	/* will be freed in usbip_driver_close() */
-	hc_device =
-		udev_device_new_from_subsystem_sysname(udev_context,
-						       USBIP_VHCI_BUS_TYPE,
-						       USBIP_VHCI_DEVICE_NAME);
+	hc_device = open_hc_device();
 	if (!hc_device) {
 		err("udev_device_new_from_subsystem_sysname failed");
 		goto err;
@@ -335,7 +532,6 @@ int usbip_vhci_refresh_device_list(void)
 int usbip_vhci_get_free_port(uint32_t speed)
 {
 	for (int i = 0; i < vhci_driver->nports; i++) {
-
 		switch (speed) {
 		case	USB_SPEED_SUPER:
 			if (vhci_driver->idev[i].hub != HUB_SPEED_SUPER)
diff --git a/tools/usb/usbip/libsrc/vhci_driver.h b/tools/usb/usbip/libsrc/vhci_driver.h
index 6c9aca216705..a18f4d243c7a 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.h
+++ b/tools/usb/usbip/libsrc/vhci_driver.h
@@ -13,6 +13,13 @@
 
 #define USBIP_VHCI_BUS_TYPE "platform"
 #define USBIP_VHCI_DEVICE_NAME "vhci_hcd.0"
+#define USBIP_VHCI_DEVICE_NAME_OLD "vhci_hcd"
+
+#define V3_18_STATUS_HEADER "prt sta spd bus dev socket           local_busid"
+#define V4_4_STATUS_HEADER "prt sta spd dev      sockfd local_busid"
+#define V4_14_STATUS_HEADER "hub port sta spd dev      sockfd local_busid"
+
+#define STATUS_HEADER_MAX 64
 
 enum hub_speed {
 	HUB_SPEED_HIGH = 0,
-- 
2.21.0.352.gf09ad66450-goog

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[shuah]

Hi David,

On 3/6/19 2:47 PM, David Valleau wrote:
> The current usbip tool relies on the behavior of the vhci-hcd driver in
> order to work correctly. In instances where a newer version of the tool
> is used with an older version of the kernel, there are incompatibilities
> that can sometimes result in failure.

usbip tool is tied to the kernel version. This is reason why it is
co-located with the usbip driver in the kernel sources. This is not
a typical tool scenario to be able to use new tool on old kernels.

I would like to understand the reasons for wanting to run new tool on
old kernels.

thanks,
-- Shuah

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

On Sat, Mar 16, 2019 at 4:40 PM shuah <shuah@kernel.org> wrote:
> usbip tool is tied to the kernel version. This is reason why it is
> co-located with the usbip driver in the kernel sources. This is not
> a typical tool scenario to be able to use new tool on old kernels.
>
> I would like to understand the reasons for wanting to run new tool on
> old kernels.

On Chromium OS, we ship more or less the same user space for a variety
of systems, but not all of those run the same kernel. That's not
exactly a novel concept -- many good tools are written such that they
degrade gracefully when running with reduced feature sets (e.g., older
kernels). While we are working on reducing the divergence and number
of kernels we ship, it's currently a fact of life that we have to
support multiple target kernel versions.

Is there a fundamental problem with VHCI such that it doesn't have a
stable ABI that tools can be written against?

If stability is possible but you just don't care, then I guess we can
fork our own version...

Or even worse, we could build N copies of usbip for N kernels. But we
don't do that for any other user space component.

Brian

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[shuah]

On 3/18/19 12:23 PM, Brian Norris wrote:
> On Sat, Mar 16, 2019 at 4:40 PM shuah <shuah@kernel.org> wrote:
>> usbip tool is tied to the kernel version. This is reason why it is
>> co-located with the usbip driver in the kernel sources. This is not
>> a typical tool scenario to be able to use new tool on old kernels.
>>
>> I would like to understand the reasons for wanting to run new tool on
>> old kernels.
> 
> On Chromium OS, we ship more or less the same user space for a variety
> of systems, but not all of those run the same kernel. That's not
> exactly a novel concept -- many good tools are written such that they
> degrade gracefully when running with reduced feature sets (e.g., older
> kernels). While we are working on reducing the divergence and number
> of kernels we ship, it's currently a fact of life that we have to
> support multiple target kernel versions.

Thanks for the context for this change.

> 
> Is there a fundamental problem with VHCI such that it doesn't have a
> stable ABI that tools can be written against?
> 

In general the ABI is stable.

+#define V3_18_STATUS_HEADER "prt sta spd bus dev socket 
local_busid"

What's your 3.18 kernel version? I think you are missing security
fixes that prevent socket address leak in the status file.

+#define V4_4_STATUS_HEADER "prt sta spd dev      sockfd local_busid"
+#define V4_14_STATUS_HEADER "hub port sta spd dev      sockfd local_busid"

The difference here is the high speed support. Let's find a better
way to fix this than hard-coding kernel revisions in the tool.

> If stability is possible but you just don't care, then I guess we can
> fork our own version...
> 
> Or even worse, we could build N copies of usbip for N kernels. But we
> don't do that for any other user space component.
> 

It might be easier to build N versions than maintaining the fork :)

In any case, let's find ways to fix the problem with a constructive
approach.

thanks,
-- Shuah

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

Hi Shuah,

On Mon, Mar 25, 2019 at 8:51 AM shuah <shuah@kernel.org> wrote:
> On 3/18/19 12:23 PM, Brian Norris wrote:
> > On Sat, Mar 16, 2019 at 4:40 PM shuah <shuah@kernel.org> wrote:
> >> I would like to understand the reasons for wanting to run new tool on
> >> old kernels.
> >
> > On Chromium OS, we ship more or less the same user space for a variety
> > of systems, but not all of those run the same kernel. That's not
> > exactly a novel concept -- many good tools are written such that they
> > degrade gracefully when running with reduced feature sets (e.g., older
> > kernels). While we are working on reducing the divergence and number
> > of kernels we ship, it's currently a fact of life that we have to
> > support multiple target kernel versions.
>
> Thanks for the context for this change.

One thing to add: we're currently only using usbip as a test utility,
and we're not yet enabling it on older kernels (because of this
precise problem).

> > Is there a fundamental problem with VHCI such that it doesn't have a
> > stable ABI that tools can be written against?
> >
>
> In general the ABI is stable.

No, it really isn't. This commit was a breaking change:

commit 2f2d0088eb93db5c649d2a5e34a3800a8a935fc5
Author: Shuah Khan <shuahkh@osg.samsung.com>
Date: Thu Dec 7 14:16:49 2017 -0700

  usbip: prevent vhci_hcd driver from leaking a socket pointer address

That one is arguably OK, since nobody was actually using that field
(except for parsing it and throwing it away), and it's a security
leak.

But this one is definitely a break:

commit 1c9de5bf428612458427943b724bea51abde520a
Author: Yuyang Du <yuyang.du@intel.com>
Date: Thu Jun 8 13:04:10 2017 +0800

  usbip: vhci-hcd: Add USB3 SuperSpeed support

You can't just arbitrarily add columns to the beginning of a file like
that and claim that you're not breaking ABI. And I shouldn't need to
remind you that Thou Shalt Not Break User Space.

Now, this whole file has tons of problems:
 * it's not documented at all in Documentation/ABI/ (so hey, you can
argue there's no ABI to break!)
 * it violates the "one piece of information per attribute" rule; this
contributes to the previous point, since it's hard to extend anything
without breaking user space

> +#define V3_18_STATUS_HEADER "prt sta spd bus dev socket
> local_busid"
>
> What's your 3.18 kernel version? I think you are missing security
> fixes that prevent socket address leak in the status file.

We're not using the latest 3.18.y stable series, if that's what you're
asking. So no, we don't have that security patch. But we also don't
currently enable USBIP_CORE and similar. We can backport stuff if we
need (backporting that security fix is on my/our plate), but we'd
probably like to understand the maintenance landscape here first.
Clearly usbip is not maintained with ABI compatibility in mind.

> +#define V4_4_STATUS_HEADER "prt sta spd dev      sockfd local_busid"
> +#define V4_14_STATUS_HEADER "hub port sta spd dev      sockfd local_busid"
>
> The difference here is the high speed support. Let's find a better
> way to fix this than hard-coding kernel revisions in the tool.

Yeah, I'm not in love with this particular patch, but it exposes the
systemic problems you have already, so at least it serves to start the
discussion.

> > If stability is possible but you just don't care, then I guess we can
> > fork our own version...
> >
> > Or even worse, we could build N copies of usbip for N kernels. But we
> > don't do that for any other user space component.
> >
>
> It might be easier to build N versions than maintaining the fork :)
>
> In any case, let's find ways to fix the problem with a constructive
> approach.

I think we should settle the biggest questions first: documenting
(and/or re-implementing) the current ABI, so that we have a stable
target. Going forward, if we are to pretend this is a real kernel
feature, it should not be breaking compatibility arbitrarily like it
has already. If I started using usbip on kernel 3.18 (+ the
pointer-leaking security fix), I should not expect to see my user
space break just because I upgraded to the latest kernel.

If we can agree on that part, *then* we can talk about what (if
anything) to do about patching usbip for compatibility with old
(ABI-broken) kernels. Perhaps we will just throw out old kernels. Or
maybe we'll do some minimal user space patching. Or maybe we patch our
old kernels to match the latest ABI. That's not quite as big as a
problem, as long as we agree on what ABI stability means going
forward.

Brian

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[shuah]

Hi Brian,

On 3/25/19 11:56 AM, Brian Norris wrote:
> Hi Shuah,
> 
> On Mon, Mar 25, 2019 at 8:51 AM shuah <shuah@kernel.org> wrote:
>> On 3/18/19 12:23 PM, Brian Norris wrote:
>>> On Sat, Mar 16, 2019 at 4:40 PM shuah <shuah@kernel.org> wrote:
>>>> I would like to understand the reasons for wanting to run new tool on
>>>> old kernels.
>>>
>>> On Chromium OS, we ship more or less the same user space for a variety
>>> of systems, but not all of those run the same kernel. That's not
>>> exactly a novel concept -- many good tools are written such that they
>>> degrade gracefully when running with reduced feature sets (e.g., older
>>> kernels). While we are working on reducing the divergence and number
>>> of kernels we ship, it's currently a fact of life that we have to
>>> support multiple target kernel versions.
>>
>> Thanks for the context for this change.
> 
> One thing to add: we're currently only using usbip as a test utility,
> and we're not yet enabling it on older kernels (because of this
> precise problem).
> 
>>> Is there a fundamental problem with VHCI such that it doesn't have a
>>> stable ABI that tools can be written against?
>>>
>>
>> In general the ABI is stable.
> 
> No, it really isn't. This commit was a breaking change:
> 
> commit 2f2d0088eb93db5c649d2a5e34a3800a8a935fc5
> Author: Shuah Khan <shuahkh@osg.samsung.com>
> Date: Thu Dec 7 14:16:49 2017 -0700
> 
>    usbip: prevent vhci_hcd driver from leaking a socket pointer address
> 
> That one is arguably OK, since nobody was actually using that field
> (except for parsing it and throwing it away), and it's a security
> leak.
> 

The ABI change and tool change went in the same security fix in my
back-port. I understand you are seeing it because you are using an
older 3.18 kernel. It was a judgment call to break ABI to fix the
leak. Looks like we are on the same page on this one.

> But this one is definitely a break:
> 
> commit 1c9de5bf428612458427943b724bea51abde520a
> Author: Yuyang Du <yuyang.du@intel.com>
> Date: Thu Jun 8 13:04:10 2017 +0800
> 
>    usbip: vhci-hcd: Add USB3 SuperSpeed support
> 
> You can't just arbitrarily add columns to the beginning of a file like
> that and claim that you're not breaking ABI. And I shouldn't need to
> remind you that Thou Shalt Not Break User Space.

USB 3.0 driver and tool support went in, I would say it was oversight to
not make sure the tool continues to work on older kernels.

> 
> Now, this whole file has tons of problems:
>   * it's not documented at all in Documentation/ABI/ (so hey, you can
> argue there's no ABI to break!)
>   * it violates the "one piece of information per attribute" rule; this
> contributes to the previous point, since it's hard to extend anything
> without breaking user space
> 

Agreed. The documentation could use work.

>> +#define V3_18_STATUS_HEADER "prt sta spd bus dev socket
>> local_busid"
>>
>> What's your 3.18 kernel version? I think you are missing security
>> fixes that prevent socket address leak in the status file.
> 
> We're not using the latest 3.18.y stable series, if that's what you're
> asking. So no, we don't have that security patch. But we also don't
> currently enable USBIP_CORE and similar. We can backport stuff if we
> need (backporting that security fix is on my/our plate), but we'd
> probably like to understand the maintenance landscape here first.
> Clearly usbip is not maintained with ABI compatibility in mind.
> 

I wouldn't characterize it is as "Clearly usbip is not maintained
with ABI compatibility in mind." It is not the intent to break ABI.

I back-ported the security to fix to all the stable releases.

> 
> I think we should settle the biggest questions first: documenting
> (and/or re-implementing) the current ABI, so that we have a stable
> target. Going forward, if we are to pretend this is a real kernel
> feature, it should not be breaking compatibility arbitrarily like it
> has already. If I started using usbip on kernel 3.18 (+ the
> pointer-leaking security fix), I should not expect to see my user
> space break just because I upgraded to the latest kernel.
> 

So what's your ability to upgrade to 3.18 latest to get the security
fix. You would want to pick this up anyway.

Now the second issue is "supporting the latest tool on older kernels".
Guess I didn't think about the possibility of tools from 5.1 being run
on 3.18 :)

I am willing to guarantee that going forward the latest usbip tool
will not break on the supported stable releases.

I am going to take a look at fixing the tool to run on older kernels.

thanks,
-- Shuah

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

On Mon, Mar 25, 2019 at 3:07 PM shuah <shuah@kernel.org> wrote:
> On 3/25/19 11:56 AM, Brian Norris wrote:
> > On Mon, Mar 25, 2019 at 8:51 AM shuah <shuah@kernel.org> wrote:
> >> In general the ABI is stable.
> >
> > No, it really isn't. This commit was a breaking change:
...
> > But this one is definitely a break:
> >
> > commit 1c9de5bf428612458427943b724bea51abde520a
> > Author: Yuyang Du <yuyang.du@intel.com>
> > Date: Thu Jun 8 13:04:10 2017 +0800
> >
> >    usbip: vhci-hcd: Add USB3 SuperSpeed support
> >
> > You can't just arbitrarily add columns to the beginning of a file like
> > that and claim that you're not breaking ABI. And I shouldn't need to
> > remind you that Thou Shalt Not Break User Space.
>
> USB 3.0 driver and tool support went in, I would say it was oversight to
> not make sure the tool continues to work on older kernels.

While that's true, you're still not grokking my main point when asking
about ABI stability:

  *old* tools should still work on *new* kernels

The above commit broke that.

> So what's your ability to upgrade to 3.18 latest to get the security
> fix. You would want to pick this up anyway.

We'll take cherry-picks as needed. You don't need to worry about
exactly how we maintain our branches, as that's not really the main
concern of upstream (at least, not in terms of kernel development).
But for the purposes of this conversation, assume that we have that
bugfix.

> Now the second issue is "supporting the latest tool on older kernels".
> Guess I didn't think about the possibility of tools from 5.1 being run
> on 3.18 :)
>
> I am willing to guarantee that going forward the latest usbip tool
> will not break on the supported stable releases.
>
> I am going to take a look at fixing the tool to run on older kernels.

That would be appreciated, but frankly, even that is not really
required: if you want to maintain the tool such that it always assumes
the latest kernel features that's your prerogative -- we don't have to
upgrade every release. At some point, new software tends to require
some minimal dependencies beneath it. So again, it would be nice to
degrade gracefully on older kernels, but it's not an absolute
requirement.

The biggest issue, as noted above, is that I currently cannot run
older usbip (user space) on newer kernels. That's a flagrant violation
of kernel ABI guarantees.

If you did consistently maintain ABI as noted above (old user space;
new kernel), then your users can choose to upgrade user space only
when needed. (And so, for instance, I could solve my problem by simply
downgrading to usbip tools from 3.18.) Today, that's not possible.

Brian

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[shuah]

On 3/25/19 5:02 PM, Brian Norris wrote:
> On Mon, Mar 25, 2019 at 3:07 PM shuah <shuah@kernel.org> wrote:
>> On 3/25/19 11:56 AM, Brian Norris wrote:
>>> On Mon, Mar 25, 2019 at 8:51 AM shuah <shuah@kernel.org> wrote:
>>>> In general the ABI is stable.
>>>
>>> No, it really isn't. This commit was a breaking change:
> ...
>>> But this one is definitely a break:
>>>
>>> commit 1c9de5bf428612458427943b724bea51abde520a
>>> Author: Yuyang Du <yuyang.du@intel.com>
>>> Date: Thu Jun 8 13:04:10 2017 +0800
>>>
>>>     usbip: vhci-hcd: Add USB3 SuperSpeed support
>>>
>>> You can't just arbitrarily add columns to the beginning of a file like
>>> that and claim that you're not breaking ABI. And I shouldn't need to
>>> remind you that Thou Shalt Not Break User Space.
>>
>> USB 3.0 driver and tool support went in, I would say it was oversight to
>> not make sure the tool continues to work on older kernels.
> 
> While that's true, you're still not grokking my main point when asking
> about ABI stability:
> 
>    *old* tools should still work on *new* kernels
> 
> The above commit broke that.
> 

Agreed. Let's move forward with the assumption that this won't happen
in the future.

On a side note, in this specific tool case, the newer version is more
secure than the older version.

thanks,
-- Shuah

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Greg Kroah-Hartman]

On Mon, Mar 25, 2019 at 04:02:30PM -0700, Brian Norris wrote:
> On Mon, Mar 25, 2019 at 3:07 PM shuah <shuah@kernel.org> wrote:
> > On 3/25/19 11:56 AM, Brian Norris wrote:
> > > On Mon, Mar 25, 2019 at 8:51 AM shuah <shuah@kernel.org> wrote:
> > >> In general the ABI is stable.
> > >
> > > No, it really isn't. This commit was a breaking change:
> ...
> > > But this one is definitely a break:
> > >
> > > commit 1c9de5bf428612458427943b724bea51abde520a
> > > Author: Yuyang Du <yuyang.du@intel.com>
> > > Date: Thu Jun 8 13:04:10 2017 +0800
> > >
> > >    usbip: vhci-hcd: Add USB3 SuperSpeed support
> > >
> > > You can't just arbitrarily add columns to the beginning of a file like
> > > that and claim that you're not breaking ABI. And I shouldn't need to
> > > remind you that Thou Shalt Not Break User Space.
> >
> > USB 3.0 driver and tool support went in, I would say it was oversight to
> > not make sure the tool continues to work on older kernels.
> 
> While that's true, you're still not grokking my main point when asking
> about ABI stability:
> 
>   *old* tools should still work on *new* kernels

For some defintion of "old" and "new" :)

We do break lower-level apis like this at times, for when it makes
sense, and for when the "old" kernel is just too old (and when you can
just run the tool that came with that kernel).

3.18 was released at the end of 2014, and as the tool that came with
that kernel still works just fine, you are just pushing the boundry here
really hard :)

Why do you want to update your userspace tool and not update your
kernel?  What is forcing your userspace tool to be updated?

Not to say that this shouldn't be fixed if at all possible, but realize
that this is not the "normal" case of "we do not break userspace" here,
given the tool involved, and the apis being used.

thanks,

greg k-h

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

Hi Greg,

On Mon, Mar 25, 2019 at 5:35 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 3.18 was released at the end of 2014, and as the tool that came with
> that kernel still works just fine, you are just pushing the boundry here
> really hard :)

For some reason, we're focusing on 3.18, when the last breakage we're
currently aware of was as recent as 4.13. So we (Chrom{ium,e} OS) also
dealing with 4.4, which is still getting active LTS releases (thanks!)
and is in heavy use.

> Why do you want to update your userspace tool and not update your
> kernel?  What is forcing your userspace tool to be updated?

I tried to downplay that part: we're primarily interested in finding
some way to utilize the *same* tool on all kernels in question. It
doesn't have to be the latest. But it's currently impossible, mostly
due to commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
support").

We also certainly have flexibility to hack things on our own and don't
need upstream to care about everything we care about (e.g.,
sufficiently old kernels), but I did want to have this conversation at
least, so that people are aware of the breakages they are making, and
so we can understand what to expect.

> Not to say that this shouldn't be fixed if at all possible, but realize
> that this is not the "normal" case of "we do not break userspace" here,
> given the tool involved, and the apis being used.

I think I sort of understand what you're going for here, but can you
elaborate so I don't have to assume?

Thanks,
Brian

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Greg Kroah-Hartman]

On Mon, Mar 25, 2019 at 05:49:07PM -0700, Brian Norris wrote:
> > Not to say that this shouldn't be fixed if at all possible, but realize
> > that this is not the "normal" case of "we do not break userspace" here,
> > given the tool involved, and the apis being used.
> 
> I think I sort of understand what you're going for here, but can you
> elaborate so I don't have to assume?

To be specific, tools at the "very low level" that are used to configure
the kernel for userspace, or to interact with the kernel such that other
programs work on top of things properly (like udev), we have broken apis
such that we can fix issues with old mistakes and move on to more secure
or "correct" apis.

So we do change things at this layer at times, and normally no one
notices as they have moved on to newer userspace programs in the past 3
years.  Sometimes we do keep kernel support for really old userspace
systems, until they die out, and then we can finally drop the kernel
code.  Again, udev has done this over the years as we worked to figure
out a sane way to handle stuff.  We have rolled back kernel changes
until people finally killed their old Fedora 3 ppc32 systems for
example :)

To be honest, with the USB3 support added to usbip, no one noticed that
things broke, and the fact that it took 4 years to notice implies that
maybe it wasn't that big of a deal as no one used this.  But, as you
show, that assumption was not correct, so if we can fix things, we
should.

Did that help?

thanks,

greg k-h

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

On Mon, Mar 25, 2019 at 5:55 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> To be honest, with the USB3 support added to usbip, no one noticed that
> things broke, and the fact that it took 4 years to notice implies that
> maybe it wasn't that big of a deal as no one used this.

Again, the last breaking change (USB3 support) was in v4.13 (2017 Sept
release date). One of the other changes was a security fix and went to
-stable, so I could imagine fewer people being upset.

> But, as you
> show, that assumption was not correct, so if we can fix things, we
> should.

Well, we "can" -- it's not too hard to parse the header to figure out
which version is in use -- but it's a bit ugly (see $subject patch). I
wonder if some of that should just be discarded and we give up on
sufficiently old kernels, since it's not immediately clear to me
whether David's patching to account for commit 0775a9cbc694 ("usbip:
vhci extension: modifications to vhci driver") is correct. (I'm not
that intimately familiar with usbip.)

> Did that help?

Yes, thanks. I was mostly hoping you weren't trying to say "vhci /
usbip are toys and shouldn't be treated seriously."

I think I mostly agree with what you're saying, and I don't think we
should place a lot of undue burden on maintaining old compatibility
for too many years, especially on stuff that wasn't very well thought
out in the first place. But I think it's a valid forward-looking goal
to avoid doing this sort of stuff again with no good reason.

Brian

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

Hi Shuah,

On Mon, Mar 25, 2019 at 5:04 PM shuah <shuah@kernel.org> wrote:
> Agreed. Let's move forward with the assumption that this won't happen
> in the future.

Great! Are you going to propose a Documentation/ABI/ patch to help
with that? e.g., Documentation/ABI/testing/sysfs-platform-vhci-hcd?
I'm frankly not that familiar with usbip (I'm just trying to help
David out) and might not be the best person for describing the details
exposed here.

And are you planning to try something else to maintain compatibility
pre-[1]? I'm not saying you have to, but you mentioned it and I'd like
to know if we're doing any duplicate work.

Regards,
Brian

[1] 1c9de5bf4286 usbip: vhci-hcd: Add USB3 SuperSpeed support

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[shuah]

On 3/25/19 7:29 PM, Brian Norris wrote:
> Hi Shuah,
> 
> On Mon, Mar 25, 2019 at 5:04 PM shuah <shuah@kernel.org> wrote:
>> Agreed. Let's move forward with the assumption that this won't happen
>> in the future.
> 

I would amend that to say, unless absolutely necessary. The user
exported information is stable after fixing the kernel address
pointer leaks. If something pops up that requires changes, I will
have to make a call on that.

> Great! Are you going to propose a Documentation/ABI/ patch to help
> with that? e.g., Documentation/ABI/testing/sysfs-platform-vhci-hcd?
> I'm frankly not that familiar with usbip (I'm just trying to help
> David out) and might not be the best person for describing the details
> exposed here.
> 

ABI will need to documented at some point. :) This driver moved from
staging to mainline around 3.18 time frame. So what you are seeing is
adaption and enhancements. Some background on this driver might help
understand some of the growing pains. :) This is one of the reasons,
I strongly recommend not using old tool on new kernels and make sure
you are on the latest stable releases. For more details on the nature
if security fixes:

https://events.linuxfoundation.org/wp-content/uploads/2017/11/One-Small-Step-to-Harden-USB-Over-IP-on-Linux-Shuah-Khan-Samsung-OSG.pdf

> And are you planning to try something else to maintain compatibility
> pre-[1]? I'm not saying you have to, but you mentioned it and I'd like
> to know if we're doing any duplicate work.
> 

1. I plan to fix the tool to make it run on older kernels (pre-USB 3.0).
2. I don't plan to support old tool running on new kernels. It doesn't
make sense considering the security fixes. You can use the latest on
older kernels (pre-USB 3.0).
3. I am going to assume you are *using* the latest stable kernels that
have the security fixes.

thanks,
-- Shuah

Re: [PATCH] tools: usb: usbip: adding support for older kernel versions

[Brian Norris]

On Mon, Mar 25, 2019 at 6:59 PM shuah <shuah@kernel.org> wrote:
> 1. I plan to fix the tool to make it run on older kernels (pre-USB 3.0).

Great, thanks! We will probably do something downstream anyway, based
off the work David has done. But it would be great if upstream
supported this too.

> 2. I don't plan to support old tool running on new kernels. It doesn't
> make sense considering the security fixes. You can use the latest on
> older kernels (pre-USB 3.0).

Agreed for sufficiently-old tools. (Say, for instance, before this
year, where it seems we now agree that the interface shouldn't change
unnecessarily.) But eventually, this is an essential part of any
mature interface: that today's tools don't break on tomorrow's kernel.
(And yes, Greg noted that old things must die eventually, esp. for
such low-level stuff, so that might not be true for sufficiently long
values of "tomorrow." But it shouldn't be taken lightly.)

I think we already more or less agreed on that, but in case we didn't,
I want to make the disagreement clear.

> 3. I am going to assume you are *using* the latest stable kernels that
> have the security fixes.

That's a reasonable assumption. It's not necessarily a true one (prior
to 4.4, where we started taking that seriously), but where it's not
true, it's our problem not yours.

Thanks,
Brian