Re: [PATCH] android: binder: fix type mismatch warning

[Arnd Bergmann]

On Wed, Sep 20, 2017 at 11:08 AM, Martijn Coenen <maco@xxxxxxxxxxx> wrote:
> On Mon, Sep 18, 2017 at 9:49 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> The current Kconfig comment says that v7 of the ABI is also
>> incompatible with Android 4.5 and later user space. Can someone
>> confirm that?
>
> That is not actually true - v7 does work with all versions of Android
> (up to and including Oreo). In fact, we've polled some vendors, and
> all vendors with a 32-bit SoC are using the v7 interface, even on
> recent Android versions.

Ah, good to know, thanks for providing that data point!

>> The code looks like it's written under the assumption
>> that user space would dynamically pick between v7 and v8 based
>> on the return value of ioctl(..., BINDER_VERSION).
>
> It's a build-time configuration option in Android userspace.

Ok.

>> b) Treat the fact that we implemented the v7 binder ABI as a bug,
>> since real Android uses v8, removing all traces of the v7 code from
>> the kernel, and requiring users of Android 4.4 and earlier to upgrade
>> their binder to a version that runs on modern kernels.
>
> This would be my proposal as well. In fact, we have already planned to
> officially remove support for the v7 interface in Android P, and
> require all devices using Android P to use the 64-bit v8 interface
> (regardless of whether the machine is 32-bit or 64-bit). The one
> caveat is that for stable/longterm, I think we want to leave the
> config option, but perhaps flip the default (to v8). The main reason
> is that some vendors may have existing devices on the v7 interface,
> and maybe they're just pushing security updates. We don't want to
> force them to switch to 64-bit just by pulling in the latest stable.
> Does that sound reasonable?

I see multiple problems with that:

- On stable mainline kernels (unlike android-common), the v8
  interface has never been available as a build option, and making
  it user-selectable will required additional patches to make it
  actually build on 32-bit ARM. This is fixable if Greg is willing
  to take the backports into stable kernels

- Having a compile-time option to pick between two incompatible
  ABIs is a really bad idea, we don't do that anywhere else in
  the kernel except for things like the choice between big-endian
  and little-endian kernels that can't be done otherwise. If we
  want to keep supporting both ABIs going forward, I think it
  needs to be a runtime option instead, to allow users to migrate
  between kernel versions.

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

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.

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, and any new
version of the 32-bit binder user space would issue a special
ioctl to switch to the v8 version. If we ever get a v9 version
of the ABI, that would then add another set of ioctl commands
with different numbers that don't conflict with the v8 interface
but are implemented by the same kernel module.
Actually implementing the combined v7/v8 ioctl stuff is
of course nontrivial.

     Arnd