From: Brian Norris <briannorris@chromium.org> To: shuah <shuah@kernel.org> Cc: David Valleau <valleau@chromium.org>, LKML <linux-kernel@vger.kernel.org>, Linux USB Mailing List <linux-usb@vger.kernel.org>, Michael Grzeschik <m.grzeschik@pengutronix.de>, Valentina Manea <valentina.manea.m@gmail.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Sasha Levin <alexander.levin@microsoft.com> Subject: Re: [PATCH] tools: usb: usbip: adding support for older kernel versions Date: Mon, 25 Mar 2019 10:56:02 -0700 [thread overview] Message-ID: <CA+ASDXOzTkXk_JtBDa4zkMhy3NyWv7Wn1i6YYtwRdeF8j_c2nA@mail.gmail.com> (raw) In-Reply-To: <caee30e2-ddc4-0076-907e-558dbcfe7c52@kernel.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Brian Norris <briannorris@chromium.org> To: shuah <shuah@kernel.org> Cc: David Valleau <valleau@chromium.org>, LKML <linux-kernel@vger.kernel.org>, Linux USB Mailing List <linux-usb@vger.kernel.org>, Michael Grzeschik <m.grzeschik@pengutronix.de>, Valentina Manea <valentina.manea.m@gmail.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Sasha Levin <alexander.levin@microsoft.com> Subject: tools: usb: usbip: adding support for older kernel versions Date: Mon, 25 Mar 2019 10:56:02 -0700 [thread overview] Message-ID: <CA+ASDXOzTkXk_JtBDa4zkMhy3NyWv7Wn1i6YYtwRdeF8j_c2nA@mail.gmail.com> (raw) 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
next prev parent reply other threads:[~2019-03-25 17:56 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-06 21:47 [PATCH] tools: usb: usbip: adding support for older kernel versions David Valleau 2019-03-06 21:47 ` David Valleau 2019-03-16 23:39 ` [PATCH] " shuah 2019-03-16 23:39 ` Shuah Khan 2019-03-18 18:23 ` [PATCH] " Brian Norris 2019-03-18 18:23 ` Brian Norris 2019-03-25 15:51 ` [PATCH] " shuah 2019-03-25 15:51 ` Shuah Khan 2019-03-25 17:56 ` Brian Norris [this message] 2019-03-25 17:56 ` Brian Norris 2019-03-25 22:07 ` [PATCH] " shuah 2019-03-25 22:07 ` Shuah Khan 2019-03-25 23:02 ` [PATCH] " Brian Norris 2019-03-25 23:02 ` Brian Norris 2019-03-26 0:04 ` [PATCH] " shuah 2019-03-26 0:04 ` Shuah Khan 2019-03-26 1:29 ` [PATCH] " Brian Norris 2019-03-26 1:29 ` Brian Norris 2019-03-26 1:59 ` [PATCH] " shuah 2019-03-26 1:59 ` Shuah Khan 2019-03-27 1:16 ` [PATCH] " Brian Norris 2019-03-27 1:16 ` Brian Norris 2019-03-26 0:13 ` [PATCH] " Greg Kroah-Hartman 2019-03-26 0:13 ` Greg Kroah-Hartman 2019-03-26 0:49 ` [PATCH] " Brian Norris 2019-03-26 0:49 ` Brian Norris 2019-03-26 0:55 ` [PATCH] " Greg Kroah-Hartman 2019-03-26 0:55 ` Greg Kroah-Hartman 2019-03-26 1:28 ` [PATCH] " Brian Norris 2019-03-26 1:28 ` Brian Norris
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CA+ASDXOzTkXk_JtBDa4zkMhy3NyWv7Wn1i6YYtwRdeF8j_c2nA@mail.gmail.com \ --to=briannorris@chromium.org \ --cc=alexander.levin@microsoft.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=m.grzeschik@pengutronix.de \ --cc=shuah@kernel.org \ --cc=valentina.manea.m@gmail.com \ --cc=valleau@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.