[PATCH] android: binder: fix type mismatch warning

[PATCH] android: binder: fix type mismatch warning

[Arnd Bergmann]

Allowing binder to expose the 64-bit API on 32-bit kernels caused a
build warning:

drivers/android/binder.c: In function 'binder_transaction_buffer_release':
drivers/android/binder.c:2220:15: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
    fd_array = (u32 *)(parent_buffer + fda->parent_offset);
               ^
drivers/android/binder.c: In function 'binder_translate_fd_array':
drivers/android/binder.c:2445:13: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  fd_array = (u32 *)(parent_buffer + fda->parent_offset);
             ^
drivers/android/binder.c: In function 'binder_fixup_parent':
drivers/android/binder.c:2511:18: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

This adds extra type casts to avoid the warning.

However, there is another problem with the Kconfig option: turning
it on or off creates two incompatible ABI versions, a kernel that
has this enabled cannot run user space that was built without it
or vice versa. A better solution might be to leave the option hidden
until the binder code is fixed to deal with both ABI versions.

Fixes: e8d2ed7db7c3 ("Revert "staging: Fix build issues with new binder API"")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/android/binder.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d055b3f2a207..72af95c9ea22 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2217,7 +2217,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 				       debug_id, (u64)fda->num_fds);
 				continue;
 			}
-			fd_array = (u32 *)(parent_buffer + fda->parent_offset);
+			fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset);
 			for (fd_index = 0; fd_index < fda->num_fds; fd_index++)
 				task_close_fd(proc, fd_array[fd_index]);
 		} break;
@@ -2442,7 +2442,7 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
 	 */
 	parent_buffer = parent->buffer -
 		binder_alloc_get_user_buffer_offset(&target_proc->alloc);
-	fd_array = (u32 *)(parent_buffer + fda->parent_offset);
+	fd_array = (u32 *)(parent_buffer + (uintptr_t)fda->parent_offset);
 	if (!IS_ALIGNED((unsigned long)fd_array, sizeof(u32))) {
 		binder_user_error("%d:%d parent offset not aligned correctly.\n",
 				  proc->pid, thread->pid);
@@ -2508,7 +2508,7 @@ static int binder_fixup_parent(struct binder_transaction *t,
 				  proc->pid, thread->pid);
 		return -EINVAL;
 	}
-	parent_buffer = (u8 *)(parent->buffer -
+	parent_buffer = (u8 *)((uintptr_t)parent->buffer -
 			binder_alloc_get_user_buffer_offset(
 				&target_proc->alloc));
 	*(binder_uintptr_t *)(parent_buffer + bp->parent_offset) = bp->buffer;
-- 
2.9.0

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

[Greg Kroah-Hartman]

On Tue, Sep 05, 2017 at 10:56:13AM +0200, Arnd Bergmann wrote:
> Allowing binder to expose the 64-bit API on 32-bit kernels caused a
> build warning:
> 
> drivers/android/binder.c: In function 'binder_transaction_buffer_release':
> drivers/android/binder.c:2220:15: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>     fd_array = (u32 *)(parent_buffer + fda->parent_offset);
>                ^
> drivers/android/binder.c: In function 'binder_translate_fd_array':
> drivers/android/binder.c:2445:13: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   fd_array = (u32 *)(parent_buffer + fda->parent_offset);
>              ^
> drivers/android/binder.c: In function 'binder_fixup_parent':
> drivers/android/binder.c:2511:18: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
> 
> This adds extra type casts to avoid the warning.
> 
> However, there is another problem with the Kconfig option: turning
> it on or off creates two incompatible ABI versions, a kernel that
> has this enabled cannot run user space that was built without it
> or vice versa. A better solution might be to leave the option hidden
> until the binder code is fixed to deal with both ABI versions.

I don't know if that is ever going to be fixed, as it's not an issue
that you will ever see "in the wild" from what I can tell...

thanks,

greg k-h

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

[Arnd Bergmann]

On Mon, Sep 18, 2017 at 4:02 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>> However, there is another problem with the Kconfig option: turning
>> it on or off creates two incompatible ABI versions, a kernel that
>> has this enabled cannot run user space that was built without it
>> or vice versa. A better solution might be to leave the option hidden
>> until the binder code is fixed to deal with both ABI versions.
>
> I don't know if that is ever going to be fixed, as it's not an issue
> that you will ever see "in the wild" from what I can tell...

Probably not on a native Android device or even a Chromebook that
ships a binder user space together with a kernel, but what about
people using "anbox" or similar projects that allow you to run
Android apps in a container?

It seems like a legitimate use case of the binder modules, but
now there is a kernel Kconfig option that has to match a user
space binary.

       Arnd

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

[Christoph Hellwig]

On Mon, Sep 18, 2017 at 05:35:03PM +0200, Arnd Bergmann wrote:
> Probably not on a native Android device or even a Chromebook that
> ships a binder user space together with a kernel, but what about
> people using "anbox" or similar projects that allow you to run
> Android apps in a container?
> 
> It seems like a legitimate use case of the binder modules, but
> now there is a kernel Kconfig option that has to match a user
> space binary.

Agreed.  It's also against our general policy for userspace interfaces.

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

[Greg Kroah-Hartman]

On Mon, Sep 18, 2017 at 05:35:03PM +0200, Arnd Bergmann wrote:
> On Mon, Sep 18, 2017 at 4:02 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >> However, there is another problem with the Kconfig option: turning
> >> it on or off creates two incompatible ABI versions, a kernel that
> >> has this enabled cannot run user space that was built without it
> >> or vice versa. A better solution might be to leave the option hidden
> >> until the binder code is fixed to deal with both ABI versions.
> >
> > I don't know if that is ever going to be fixed, as it's not an issue
> > that you will ever see "in the wild" from what I can tell...
> 
> Probably not on a native Android device or even a Chromebook that
> ships a binder user space together with a kernel, but what about
> people using "anbox" or similar projects that allow you to run
> Android apps in a container?

Ugh, that's crazy, binder should not be run that way due to a variety of
issues...  Well, as of 4.14 those issues will be gone, so no one is
doing this yet :)

> It seems like a legitimate use case of the binder modules, but
> now there is a kernel Kconfig option that has to match a user
> space binary.

So, should we revert that?

I don't really know what to suggest here, sorry...

greg k-h

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

[Christoph Hellwig]

On Mon, Sep 18, 2017 at 05:53:47PM +0200, Greg Kroah-Hartman wrote:
> > It seems like a legitimate use case of the binder modules, but
> > now there is a kernel Kconfig option that has to match a user
> > space binary.
> 
> So, should we revert that?
> 
> I don't really know what to suggest here, sorry...

Keep it as is for now, and encourage people to fix it.  But we should
reject any new patch that makes this worse or adds additional ioctls
that don't follow our normal model.

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

[Greg Kroah-Hartman]

On Mon, Sep 18, 2017 at 09:23:03AM -0700, Christoph Hellwig wrote:
> On Mon, Sep 18, 2017 at 05:53:47PM +0200, Greg Kroah-Hartman wrote:
> > > It seems like a legitimate use case of the binder modules, but
> > > now there is a kernel Kconfig option that has to match a user
> > > space binary.
> > 
> > So, should we revert that?
> > 
> > I don't really know what to suggest here, sorry...
> 
> Keep it as is for now, and encourage people to fix it.  But we should
> reject any new patch that makes this worse or adds additional ioctls
> that don't follow our normal model.

That sounds reasonable to me, thanks.

greg k-h

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

[Arnd Bergmann]

On Mon, Sep 18, 2017 at 6:23 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Sep 18, 2017 at 05:53:47PM +0200, Greg Kroah-Hartman wrote:
>> > It seems like a legitimate use case of the binder modules, but
>> > now there is a kernel Kconfig option that has to match a user
>> > space binary.
>>
>> So, should we revert that?
>>
>> I don't really know what to suggest here, sorry...
>
> Keep it as is for now, and encourage people to fix it.  But we should
> reject any new patch that makes this worse or adds additional ioctls
> that don't follow our normal model.

I would argue we should leave it as non-configurable, but make
a conscious decision which ABI to use for 4.14, as that is going
to be in a the next generation of Android devices:

a) leave the BINDER_CURRENT_PROTOCOL_VERSION==7
interface supported in all 32-bit builds forever, remaining compatible
the ABI that was supported in mainline Linux in all versions until v4.13.
The version 7 interface is incompatible between 32-bit and 64-bit user
space, and there is no compat_ioctl implementation for it, so someone
should fix it by implementing a proper compat_ioctl function.

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

We could also add support for switching the ABI dynamically in the
kernel module based on either a module parameter or an ioctl
that a future version of the binder user space can call, to add optional
support for the v8 ABI on 64-bit.

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. We can
probably get away with that because
 - Neither arm64 nor x86_64 are affected by this change.
 - "anbox" would normally only be used on x86_64, but not i386
   or arm32
 - we probably already lost support for Android 4.4 and earlier due
   to other kernel changes
 - any arm32 user space that people actually try to run with a
   mainline kernel should already support the v8 ABI, and might not
   support the v7 ABI at all.
 - we don't support v1 through v6 of the interface either.

      Arnd

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

[Martijn Coenen]

On Mon, Sep 18, 2017 at 9:49 PM, Arnd Bergmann <arnd@arndb.de> 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.

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

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

Martijn

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

[Arnd Bergmann]

On Wed, Sep 20, 2017 at 11:08 AM, Martijn Coenen <maco@android.com> wrote:
> On Mon, Sep 18, 2017 at 9:49 PM, Arnd Bergmann <arnd@arndb.de> 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

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

[Martijn Coenen]

On Wed, Sep 20, 2017 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> - 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

Yeah that would need to be fixed indeed.

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

I definitely don't want to keep it going forward, just maintain the
option for older kernels.

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

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

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

Yes, this would be a lot of work. I'll let others chime in, but I
would still prefer to remove v7 from 4.14 onward altogether.

Thanks,
Martijn

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

[Arnd Bergmann]

On Wed, Sep 20, 2017 at 12:24 PM, Martijn Coenen <maco@android.com> wrote:
> On Wed, Sep 20, 2017 at 11:58 AM, Arnd Bergmann <arnd@arndb.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

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

[Greg Kroah-Hartman]

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@android.com> wrote:
> > On Wed, Sep 20, 2017 at 11:58 AM, Arnd Bergmann <arnd@arndb.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

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

[Martijn Coenen]

On Wed, Sep 20, 2017 at 3:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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.

Ack.

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

I don't think the binder driver is present on any regular Linux box,
so things like anbox actually have to provide it as a module. It looks
like the current version of anbox is actually using the v8 interface,
and is also using a modified version of the binder driver (eg for
dkms, but also other hacks). The other popular one is Shashlik, but
looks like it uses a VM (just for binder). The v7 interface doesn't
work on 64-bit machines at all, so any container userspace using v7
would be seriously limiting the number of machines it could be run on,
whereas v8 runs on both 32 and 64. So I'm not sure we have to be
concerned about things like this, certainly if we have communicated
earlier (as Greg said) that binder shouldn't be used outside Android
pre 4.14.

> The scenario I had in mind is like this:

Thanks for clarifying! I think this works, but it's additional code to
maintain and support for a pretty rare (and unsupported?) scenario. I
understand that hiding ABI behind a config option is not a good
design, and that it must be removed. If we really can't remove v7 from
4.14, I would actually prefer to leave the config option (but flip the
default to v8), and remove it as soon as we think it can be (eg once P
has been in the field for a while).

Thanks,
Martijn

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

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 9:00 AM, Martijn Coenen <maco@android.com> wrote:

>> 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.
>
> I don't think the binder driver is present on any regular Linux box,
> so things like anbox actually have to provide it as a module. It looks
> like the current version of anbox is actually using the v8 interface,
> and is also using a modified version of the binder driver (eg for
> dkms, but also other hacks). The other popular one is Shashlik, but
> looks like it uses a VM (just for binder). The v7 interface doesn't
> work on 64-bit machines at all, so any container userspace using v7
> would be seriously limiting the number of machines it could be run on,
> whereas v8 runs on both 32 and 64. So I'm not sure we have to be
> concerned about things like this, certainly if we have communicated
> earlier (as Greg said) that binder shouldn't be used outside Android
> pre 4.14.

Ok.

>> The scenario I had in mind is like this:
>
> Thanks for clarifying! I think this works, but it's additional code to
> maintain and support for a pretty rare (and unsupported?) scenario. I
> understand that hiding ABI behind a config option is not a good
> design, and that it must be removed. If we really can't remove v7 from
> 4.14, I would actually prefer to leave the config option (but flip the
> default to v8), and remove it as soon as we think it can be (eg once P
> has been in the field for a while).

How would waiting help?

If we are reasonably sure that it's not a problem, then let's remove
the v7 support now and  see if anyone complains (and put it back
if they did have a reason to need it).

If we can assume that the upstream binder driver (unlike
third-party modules for anbox, which are not affected as you
say) is only ever used with a platform specific full Android
build, the question is what upgrade scenarios a device might
need to support.

The relevant cases would seem to be:

1. updating a 32-bit Android Oreo (or earlier) OS from linux-4.9
    (or earlier) to linux-4.14 (or later) without updating libbinder:
    This would require keeping around compile-time option
    at least, for as long new kernels are supported on Oreo.
2. updating a 32-bit kernel to a 64-bit kernel on hardware
    that allows this, without changing anything else:
    Theoretically fixable, but this would require significant
    code changes in the kernel module, similar to what I
    described above.
3. upgrading from Android Oreo (or earlier) to Android P
    while upgrading the kernel at the same time: should
    be no problem.
4. upgrading from Android Oreo (or earlier) to Android P
    libbinder without upgrading the kernel: this might
    require patching the old kernels to enable the v8 ABI.
    Not our problem here, as it only concerns older kernels.

What assumptions can we make about the two problematic
cases (1 and 2) here? Is it reasonable to require device
makes to always update libbinder together with the kernel?
Could that run into problems after a botched upgrade that
reverts back to an older kernel but keeps the new user space
or vice versa?

       Arnd

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

[Martijn Coenen]

On Fri, Sep 22, 2017 at 11:12 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> How would waiting help?

Once P drops support for v7, all P userspaces (including containerized
ones) need to be v8. After a while, the number of non-Android
userspaces < P with v7 would become practically zero. But it's really
hard to draw a line when it is "good enough", so I agree with you it
doesn't really help, it just reduces the chance that somebody will be
affected by it.

> The relevant cases would seem to be:
>
> 1. updating a 32-bit Android Oreo (or earlier) OS from linux-4.9
>     (or earlier) to linux-4.14 (or later) without updating libbinder:
>     This would require keeping around compile-time option
>     at least, for as long new kernels are supported on Oreo.

I think it is unlikely a vendor would upgrade their kernel from
linux-4.9 to linux-4.14 on an already launched Android device - it is
just too much work. Vendors ideally should pick an LTS kernel and
stick with it. But of course we can't rule it out. If it does happen,
I don't think it would be strange to expect vendors to upgrade their
userspace along with it. Many vendors (sadly) have a ton of patches on
top of mainline, that may contain non-ABI-stable interfaces in the
first place, and therefore would require a userspace push anyway.

> 2. updating a 32-bit kernel to a 64-bit kernel on hardware
>     that allows this, without changing anything else:
>     Theoretically fixable, but this would require significant
>     code changes in the kernel module, similar to what I
>     described above.

This is an issue independent of what we do here; if you used the v7
interface in your pre-4.14 32-bit kernel, you have to migrate to v8
simply because the v7 interface can't work with a (pre-4.14) 64-bit
kernel. If you upgrade to a 4.14 kernel from which v7 is removed, you
fall back to the previous case. Such a change would anyway require
building and deploying a new Android userspace, assuming you'd want to
allow running 64-bit userspace applications on such a kernel. I'm not
sure whether there are good reasons to switch to a 64-bit kernel while
maintaining a 32-bit only userspace.

> What assumptions can we make about the two problematic
> cases (1 and 2) here? Is it reasonable to require device
> makes to always update libbinder together with the kernel?

I personally think this is reasonable. Rolling out an update to an
Android device involves a lot of work; if you're going to update the
kernel, you'll often want to use the opportunity to update userspace
as well for things like security fixes. Project Treble has made it
significantly easier to update individual parts of Android.
Unfortunately binder is so pervasive that it touches pretty much
everything - migrating from v7 to v8 will require a full userspace
push. Again, I don't think this is a problem, because:
1) Launched Android devices tend to stick with the kernel version they
launched with (so v7 will stay available to them)
2) If they do want to upgrade, I don't think it's unreasonable to ask
them to do a userspace push along with it
3) New devices launching with 4.14 (and P) will have no choice to use
anything but v8

> Could that run into problems after a botched upgrade that
> reverts back to an older kernel but keeps the new user space
> or vice versa?

Our current A/B partition scheme keeps a complete copy of both the old
kernel and userspace when upgrading; if the upgrade doesn't boot, we
revert both back to the old state.