Message-ID: <20170921074435.GB11294@kroah.com>
Date:   Thu, 21 Sep 2017 09:44:35 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Martijn Coenen <maco@...roid.com>,
        Christoph Hellwig <hch@...radead.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Riley Andrews <riandrews@...roid.com>,
        devel@...verdev.osuosl.org, Todd Kjos <tkjos@...roid.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        John Stultz <john.stultz@...aro.org>
Subject: Re: [PATCH] android: binder: fix type mismatch warning

On Wed, Sep 20, 2017 at 01:37:45PM +0000, Arnd Bergmann wrote:
> On Wed, Sep 20, 2017 at 12:24 PM, Martijn Coenen <maco@...roid.com> wrote:
> > On Wed, Sep 20, 2017 at 11:58 AM, Arnd Bergmann <arnd@...db.de> wrote:
> >>
> >> - Since you say there are existing users of recent 32-bit Android
> >>   including Oreo, I also think that removing support for the v7 ABI
> >>   is no longer an option. I only made that suggestion under the
> >>   assumption that there is no user space requiring that. Linux
> >>   has no real way of "deprecating" an existing ABI, either it is
> >>   currently used and has to stay around, or it is not used at all
> >>   and can be removed without anybody noticing.
> >
> > I don't know of any Android devices shipping with 4.14 - we don't even
> > have a common tree for 4.14 (4.9 is the latest). So I don't think
> > we're hurting anyone in the Android ecosystem if we remove v7 from
> > 4.14. From what I know, it's extremely rare for an Android device to
> > change their kernel version after a device ships, but even if someone
> > hypothetically did update their Android device to use 4.14, they can
> > pretty easily switch the build-time config option. I understand that
> > this is against Linux' philosophy around not breaking userspace, I
> > just want to point out that I think from an Android POV, removing v7
> > from 4.14 is not an issue. I'm not sure if that is good enough.
> 
> I'm not really worried about shipping Android products, for those
> there is no big problem using the compile-time option as they build
> everything together.
> 
> The case that gets interesting is a any kind of user that wants to
> run an Android application on a regular Linux box without
> using virtual machines or emulation, e.g. a an app developer,
> or a user that wants to run some Android app on their desktop
> using anbox.
> 
> This obviously requires enabling the module, but it should not
> require enabling an option to support one version of the user
> space while breaking another version.

To be fair, lots of people, including myself, have always said, "never
run binder on a system that is not a 'real' Android system" for a
variety of good reasons, including security issues.

Now around the 4.10 or so kernel release, most of those issues were
resolved, and with 4.14, all of those have been taken care of (that I
know of), so this should be allowed.  But given that no one has done it
in the past (or should have), I don't know how hard you want to support
"older" situations at all.

> >> If we end up doing a runtime switch between the ABIs instead,
> >> I see two ways of doing that:
> >>
> >> The easiest implementation would make it a module parameter:
> >> Since there is only one instance of the binder in any given
> >> system, and presumably all processes talking to it have to use
> >> the same ABI, this should be sufficient. The main downside is
> >> that it prevents us from having incompatible versions per
> >> namespace if we ever get a containerized version of binder.
> >
> > Namespace support for binder is currently being investigated, but I'm
> > not sure we'd ever have a system where we want to mix v7 and v8. There
> > really is no good reason to use v7 anymore (even on 32-bit mahcines),
> > we just should have removed it from our userspace a lot earlier.
> 
> The only possible reason for v7 is backwards compatibility. I agree
> we don't need a single machine (or container) to support a mix of
> v7 and v8 applications, but I can see someone using a v7 based
> chroot user space today to keep running that in a container in the
> future while using another container for an updated image.
> 
> This is clearly a corner case that we could decide to ignore, it
> just feels like a bit of a hack.

I think it's a corner case that no one will actually run into at all.

Remember, it's only libbinder that is touching the binder device node,
not random applications, so it's not a matter of an application in a
container, but rather, a whole "system" there.  And running whole
"systems" in different containers is not really viable from what I can
tell (but I could be wrong.)

> >> The correct way to do it would be to unify the two versions of
> >> the ABI so they can be used interchangeably: any 32-bit
> >> process would start out with the v7 interface and a 64-bit
> >> process would start out with the v8 interface
> >
> > This wouldn't work with the current implementation - if a 32-bit and
> > 64-bit process communicate, then userspace would make wrong
> > assumptions about the sizes of transactions. Or is this what you're
> > proposing to fix?
> 
> The scenario I had in mind is like this:
> 
> - Any 64-bit process would use the v8 ABI all the time. When the binder
>   device has been opened by a 64-bit process already, this forces
>   all other processes opening it to also use v8.
> 
> - An existing 32-bit process would keep using the v7 ABI, but as long
>   as the the v7 interface is in use, no other process may use the v8 ABI.
>   This would exclude all 64-bit processes, as well as prevent 32-bit
>   processes other than the first one from switching binder to the v8
>   ABI
> 
> - Future binder user space implementations on 32-bit include a
>   call to a new ioctl for switching from v7 to v8. The binder user space
>   calls this immediately after opening the device to tell the kernel that
>   it wants to use the v8 ABI, and from that point on, it behaves like
>   the first case (opened by a 64-bit process).
> 
> To support both ABIs in the same kernel module, that has to convert
> between the data structures. This is not much different between the
> module parameter method and the ioctl switch.
> It can continue using the v8 structures
> internally and then do:
> 
> static bool abi_v8 = !BINDER_IPC_32BIT;
> module_parm(abi_v8, bool, S_IRUGO);
> 
> static long binder_ioctl_v8(struct file *filp, unsigned int cmd,
> unsigned long arg)
> {
>         /* the current binder_ioctl() function, built for v8 */
> }
> 
> static long binder_ioctl_v7(struct file *filp, unsigned int cmd,
> unsigned long arg)
> {
>        if (cmd == BINDER_SET_V8_ABI && !binder_in_use) {
>                abi_v8 = 1;
>                return;
>        }
> 
>        switch (cmd) {
>        /* for each command, convert data structures on stack, and call
> binder_ioctl_v8 */
>        }
> }
> 
> static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
>       if (!abi_v8) {
>                if (IS_ENABLED(CONFIG_64BIT))
>                        return -EINVAL;
> 
>                return binder_ioctl_v7(filp, cmd, arg);
>        }
> 
>          return binder_ioctl_v8(filp, cmd, arg);
> }
> 
> static long binder_compat_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> {
>         if (!abi_v8)
>                 return binder_ioctl_v7(filp, cmd, arg);
> 
>          return binder_ioctl_v8(filp, cmd, arg);
> }

Ugh, messy, but yes, it should work.  However, who is going to write the
userspace test framework to ever test such a scheme?  :)

thanks,

greg k-h

Powered by blists - more mailing lists