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@xxxxxxxxxx> wrote:
> > On 3/18/19 12:23 PM, Brian Norris wrote:
> > > On Sat, Mar 16, 2019 at 4:40 PM shuah <shuah@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> 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