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@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.

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.

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