Message-ID: <CAK8P3a0Eyk5ONhE4tyHa3ppPDtER7FVWR9Dyb8sUmdzEQ8AQ3A@mail.gmail.com>
Date:   Wed, 20 Sep 2017 13:37:45 +0000
From:   Arnd Bergmann <arnd@...db.de>
To:     Martijn Coenen <maco@...roid.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.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 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.

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

>> 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);
}

        Arnd

Powered by blists - more mailing lists