Date:	Thu, 11 Apr 2013 16:13:24 +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 23:12, Arve Hjønnevåg wrote:
>>>> 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.

I will leave this handle as __s32 for this patch set but I will take a 
look into what and if we need to change this for a 64bit system. From a 
top level perspective(a look at binder_*_ref() functions and the 
userspace equivalent) this should work fine as 32bit.

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

I will think about the best way of getting the pointer size and add it 
to the patch set for binder compat. For this patch set I will only 
modify the protocol_version from long to __s32.

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