| Message ID | 20170905085640.1566593-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 1c363eaece2752c5f8b1b874cb4ae435de06aa66 |
| Headers | show |
| Series | android: binder: fix type mismatch warning | expand |
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
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
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.
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
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.
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
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
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
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
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
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
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
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
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
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.
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;
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(-) -- 2.9.0