Re: [PATCH v3 0/6] Android Binder IPC Fixes

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

On 01/05/13 00:52, Arve Hjønnevåg wrote:
> On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> Hi Arve,
>>
>>
>> On 30/04/13 00:13, Arve Hjønnevåg wrote:
>>>
>>> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
>>> <Serban.Constantinescu@arm.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Any feedback or comments on this patch set?
>>>>
>>>
>>> You don't seem to have addressed my feedback on the previous patch set.
>>
>>
>> For v3 I have modified the following according to your review:
>>
>>
>>> Changes for v3:
>>> 1:  Dropped the patch that was replacing uint32_t types with unsigned int
>>> 2:  Dropped the patch fixing the IOCTL types(since it has been added to
>>> Greg's
>>>      staging tree)
>>> 3:  Split one patch into two: 'modify binder_write_read' and '64bit
>>> changes'
>>> 4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>>> review
>>> 5:  Modified the binder command IOCTL declarations according to Arve's
>>> review
>>
>>
>> The following were left out:
>>
>>> On 11/04/13 22:40, Arve Hjønnevåg wrote:
>>> OK, relaxing the alignment requirement for *offp to what the hardware
>>>>
>>>> requires makes sense. Is there any macros in the kernel to help with
>>>> this, instead of hard-coding it to 4 bytes?
>>
>>
>> There is no kernel macro that I know which will help here(one that springs
>> to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that
>> aligns to (u32)). Any ideas?
>>
>
> Perhaps using __alignof__(struct flat_binder_object) will work. This
> is the least important part of that change however. I saw no response
> to my concern that your changes can cause less memory to be allocated
> than you write to.

This can happen for situations where (buffer_start + buffer_size) are 
not aligned to (void *), because offset_start is calculated as:

> offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));

Thus you can have a situation where instead of reading offset[i] you 
will read (offset[i] >> 32 | offset[i+1] << 32) (offset is size_t - 
8byte for 64bit systems).

I will address this issue in v4 of the patch set.

>
>>> On 11/04/13 21:38, Arve Hjønnevåg wrote:
>>> OK, but if you are using this change let a 64 bit user-space know that
>>>>
>>>> the driver has been fixed, then this patch needs to go after the
>>>> patches that change the structures on 64 bit systems.
>>
>>
>> For 32bit systems nothing has changed so they will continue to work as
>> before. For 64bit systems the size of binder_version was signed long before
>> the patch and __s32 after the patch is applied. Thus a 64bit system using
>> the old interface will fail immediately after opening the binder driver,
>> while cheeking the binder version (since the BINDER_VERSION ioctl will be
>> different pre/post patch - size of binder_version differs).
>>
>> For 64/32 systems once I will have the userspace wrapper ready I will add
>> another ioctl(as discussed) that will check if the driver is 64bit
>> ready(among the first things to do on binder_open).
>>
>
> Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
> the driver is usable on a 64 bit system?

Leaving the binder_version as a long will cause the BINDER_VERSION ioctl 
to fail just on 64/32 - since the size will be different between 32bit 
compilers and 64bit compilers. The call will succeed on 64/64 and 32/32 
(since they use the same kernel headers).

>> Please let me know if there is anything that skipped my review and you would
>> like to integrate in this patch set.
>>
>
> It may be better to reply to my original emails instead of copying
> bits of them here.

I will do that! I did not understand your initial reply to the buffer 
size issue, my fault!


Thanks for your feedback,
Serban

-- 
Best Regards,

Serban Constantinescu
PDSW Engineer ARM Ltd.