Re: [PATCH 1/2] Staging: android: binder: Add support for 32bit binder calls in a 64bit kernel

[Serban Constantinescu <serban.constantinescu@arm.com>]

On 04/12/12 16:17, Greg KH wrote:
> On Tue, Dec 04, 2012 at 10:44:13AM +0000, Serban Constantinescu wrote:
>> Android's IPC, Binder, does not support calls from a 32-bit userspace
>> in a 64 bit kernel. This patch adds support for syscalls coming from a
>> 32-bit userspace in a 64-bit kernel.
>>
>> Most of the changes were applied to types that change sizes between
>> 32 and 64 bit world. This will also fix some of the issues around
>> checking the size of an incoming transaction package in the ioctl
>> switch. Since  the transaction's ioctl number are generated using
>> _IOC(dir,type,nr,size), a different userspace size will generate
>> a different ioctl number, thus switching by _IOC_NR is a better
>> solution.
>>
>> The patch has been successfully tested on ARMv8 AEM and Versatile
>> Express V2P-CA9.
>
> Has it also been properly tested on a 32bit kernel and userspace to
> verify that nothing broke?

We have tested this patch set with a 32-bit kernel (on a Versatile
Express platform with an 4xA9 (ARMv7a)) as well as a 64-bit kernel (on
ARMv8 simulation platform). For both test cases we used the same 32-bit
Android file system (ICS, JB). Both platforms have been running browser
loads for days without any problems. We are currently extending the test
cases by running Android CTS test.

>
> I was wondering when someone would notice that this code was not going
> to work for this type of system, nice to see that you are working to fix
> it up.  But, I'll reask Dan's question here, why not use the compat32
> ioctl interface instead?  Shouldn't that be the easier way to do this?

Binder uses a 2 layer ioctl structure i.e. you can't know from the top
of the ioctl handler the size of the incoming package. Therefore adding
a wrapper for a 64bit kernel is not an option. Should a 64bit Android
ever appear we would probably want to support 32bit legacy applications.
For this we will need the same binder/ashmem driver to service both a
32bit application as well as a 64bit application in a 64bit kernel.
Therefore I guess the way forward will be to support 32bit file systems
in a 64bit kernel without any change to the existing user space
(implemented in this patch) and at some point extend the ioctl range
with the needed functionality for 64bit user space.

>
> Also, one meta comment, never use the uint32_t types, use the native
> kernel types (u32 and the like.)  If you are crossing the user/kernel
> boundry, use the other correct types for those data structures (__u32
> and the like).  What you did here is mix and match things so much that I
> really can't verify that it is all correct.

I have tried to in-line my changes with the types already used in the
driver but I will update to using the suggested types.

>
> thanks,
>
> greg k-h
>

Thanks for the feedback,
Serban Constantinescu
`

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.