Messages in this thread | | | Date | Thu, 02 May 2013 17:38:27 +0100 | From | Serban Constantinescu <> | Subject | Re: [PATCH v3 0/6] Android Binder IPC Fixes |
| |
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.
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |