Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 4:02 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote:
>> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>> >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>> >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> >> >> <snip>
>> >> >>
>> >> >> > And finally, is this all really needed?  Why not just fix the structures
>> >> >> > to be "correct", and then fix userspace to use the correct structures as
>> >> >> > well, thereby not needing a compat layer at all?
>> >> >>
>> >> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>> >> >> storing those pointers in a __u64 to avoid having to have a
>> >> >> compat_ioctl?
>> >> >
>> >> > Yes, that's the best way to solve the issue, right?
>> >>
>> >> It's the least code, but in exchange you lose all the type safety and
>> >> warnings when copying in and out of the pointers, as well as sparse
>> >> checking on the __user attribute.
>> >
>> > Not if you make the cast right at the beginning, when you first "touch"
>> > the data, but yes, it does take some of the type saftey away, at the
>> > expense of simpler code to mess up :)
>> >
>> >> That doesn't seem like a good tradeoff to me.  In addition it requires
>> >> modifying the existing heavily used 32 bit api, which means a
>> >> mostly-equivalent compat layer added in libbinder to support old
>> >> kernels.
>> >
>> > Wait, I thought that libbinder would have to be changed anyway here, to
>> > handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
>> > already changing it, why not just "do it correctly"?
>>
>> libbinder will need changes to support 64-bit userspace and especially
>> a mixed 64-bit and 32-bit userspace, but this patch series is only
>> addressing a pure 32-bit userspace on a 64-bit kernel.  Support for a
>> 64-bit userspace in Android is obviously going to require a future
>> version of Android including, among other things, libbinder changes.
>> As far as I know, those changes won't need to change the ioctl api,
>> only the layout of the buffers that are passed through the ioctl api.
>
> "only" means you can rearrange things at that point in time, as you will
> have to be doing that anyway :)
>
>> > Or does this patch series mean that no userspace code is changed?  Is
>> > that a "requirement" here?
>>
>> Since this series only addresses 32-bit userspace on 64-bit kernel
>> support there are no associated userspace changes.  Changing the
>> 32-bit api here means that combining the KitKat branch from
>> http://android.googlesource.com with a newer kernel version will not
>> work.
>
> Is that something that anyone has said would work in the past?  It seems
> that other parts of the Android userspace are pretty tied to specific
> kernel features / versions, is this anything new if the binder code had
> to change?

We have explicitly said that Android userspace does not require a
specific kernel version.  I expect to see KitKat devices running at
least 3.4, 3.10, and whatever is the next long term stable version.
Sometimes new kernels need userspace support, but we try to avoid that
as much as possible.  The last major one I can remember was removing
early suspend in 3.4, but in that case I provided a compat 3.4 branch
to allow using 3.4 with an older userspace.  Changing the binder api
is not completely unsupportable for old versions, my guess is some
people would just ship with a new kernel with the binder changes
reverted.

> Anyway, the code as submitted has problems, see my response to the
> second patch, so it's not ready yet anyway :(
>
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/