Re: [PATCH v3 0/6] Android Binder IPC Fixes
From: Serban Constantinescu
Date: Thu May 02 2013 - 12:38:34 EST
On 01/05/13 00:52, Arve HjÃnnevÃg wrote:
On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
<Serban.Constantinescu@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/