Date:	Wed, 10 Apr 2013 14:01:57 +0100
From:	Serban Constantinescu <Serban.Constantinescu@....com>
To:	Arve Hjønnevåg <arve@...roid.com>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Android Kernel Team <kernel-team@...roid.com>,
	John Stultz <john.stultz@...aro.org>,
	Dave Butcher <Dave.Butcher@....com>
Subject: Re: [PATCH v2 3/7] staging: android: binder: fix binder interface
 for 64bit compat layer

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.

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

>> };
>>
>> /* 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists