Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for64bit compat layer

[Arve Hjønnevåg]

On Wed, Apr 10, 2013 at 6:01 AM, Serban Constantinescu
<Serban.Constantinescu@xxxxxxx> wrote:
> On 10/04/13 00:48, Arve Hjønnevåg wrote:
>>>
>>> diff --git a/drivers/staging/android/binder.c
>>> b/drivers/staging/android/binder.c
>>> index 5794cf6..a2cdd9e 100644
>>> --- a/drivers/staging/android/binder.c
>>> +++ b/drivers/staging/android/binder.c
>>
>> ...
>>>
>>> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
>>> }
>>>
>>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>>> *thread,
>>> - void __user *buffer, int size, signed long *consumed)
>>> + void __user *buffer, size_t size, size_t *consumed)
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> This change is related to the userspace handling of the struct
> binder_write_read. The userspace writes write_size and read_size as size_t
> values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).
>
> On return from a BINDER_WRITE_READ ioctl write_consumed and read_consumed
> are checked against positive values(these values will represent the
> difference between the start of the buffer cursor and the current buffer
> start - positive values since buffer cursor = buffer start ++).
>
> However if there is any plan for these values to be handled as signed longs
> at some point we can change the patch such that we modify just the function
> prototype to:
>
>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>> *thread,
>>  -               void __user *buffer, int size, signed long *consumed)
>>  +               void __user *buffer, signed long size, signed long
>> *consumed)
>
>
> I will break this change into its own patch such that it is easier to grasp.
>

OK.

>
>>
>> ...
>>>
>>> diff --git a/drivers/staging/android/binder.h
>>> b/drivers/staging/android/binder.h
>>> index dbe81ce..8012921 100644
>>> --- a/drivers/staging/android/binder.h
>>> +++ b/drivers/staging/android/binder.h
>>> @@ -48,13 +48,13 @@ enum {
>>> */
>>> struct flat_binder_object {
>>> /* 8 bytes for large_flat_header. */
>>> - unsigned long type;
>>> - unsigned long flags;
>>> + __u32 type;
>>> + __u32 flags;
>>>
>>> /* 8 bytes of data. */
>>> union {
>>> void __user *binder; /* local object */
>>> - signed long handle; /* remote object */
>>> + __s32 handle; /* remote object */
>>
>>
>> Why limit the handle to 32 bits when the pointer that it shares
>> storage with need to be 64 bit on 64 bit systems?
>
>
> Here I have mirrored the type being passed in handle - a file
> descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
> BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
> the kernel/userspace(as 32bit value on 64bit systems). However this change
> does not limit the extension of the API since we can read the value as 64bit
> - binder(on 64bit systems).
>
> I can remove this change if you consider that is the better solution.
>

I was asking if we should just use 64 bit handles on 64 bit systems,
not adding casts. It would require another union member for a file
descriptor however.

>
>>> };
>>>
>>> /* extra data associated with local object */
>>> @@ -67,18 +67,18 @@ struct flat_binder_object {
>>> */
>>>
>>> struct binder_write_read {
>>> - signed long write_size; /* bytes to write */
>>> - signed long write_consumed; /* bytes consumed by driver */
>>> + size_t write_size; /* bytes to write */
>>> + size_t write_consumed; /* bytes consumed by driver */
>>> unsigned long write_buffer;
>>> - signed long read_size; /* bytes to read */
>>> - signed long read_consumed; /* bytes consumed by driver */
>>> + size_t read_size; /* bytes to read */
>>> + size_t read_consumed; /* bytes consumed by driver */
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> See above explanation for binder_thread_write() change, I will break this
> into its own patch.
>
>
>>> unsigned long read_buffer;
>>> };
>>>
>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>> struct binder_version {
>>> /* driver protocol version -- increment with incompatible change */
>>> - signed long protocol_version;
>>> + __s32 protocol_version;
>>
>>
>> How does user-space know if it should use 32 bit or 64 bit pointers.
>> Without this change, the BINDER_VERSION ioctl would only match when
>> the size of long matches.
>
>
> The userspace can check the values returned by uname(). That will determine
> if the kernel is 32 or 64bit and depending on this select what binder
> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
> using protocol_version as 64bit signed long(that is old kernel versions with
> no 64bit support).
>
> Leaving this value as signed long would mean that older versions of the
> binder(without 64bit support) will pass the check. Furthermore the protocol
> version will probably never exceed the values that could be represented on
> 32bit. It will also mean that BINDER_VERSION will have a different
> userspace/kernel handler for 64/32 systems.
>
> Let me know what are your thoughts related to these changes,
> Thanks for your feedback,
> Serban
>

I think user-space should get the binder pointer size from the binder
driver, not elsewhere. Since the current driver does not appear to be
functional on a 64 bit system, I think adding an ioctl to get the
size, or encoding it into the binder version (use an unsigned type if
you do this), would be best.

--
Arve Hjønnevåg
--
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/