Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

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

On 10/04/13 23:30, Arve Hjønnevåg wrote:
> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> On 10/04/13 00:58, Arve Hjønnevåg wrote:
>>>
>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>>> <serban.constantinescu@arm.com> wrote:
>>>>
>>>> The Android userspace aligns the data written to the binder buffers to
>>>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
>>>> Android userspace we can have a buffer looking like this:
>>>>
>>>> platform    buffer(binder_cmd   pointer)      size
>>>> 32/32                 32b         32b          8B
>>>> 64/32                 32b         64b          12B
>>>> 64/64                 32b         64b          12B
>>>>
>>>> Thus the kernel needs to check that the buffer size is aligned to 4bytes
>>>> not to (void *) that will be 8bytes on 64bit machines.
>>>>
>>>> The change does not affect existing 32bit ABI.
>>>>
>>>
>>> Do we not want the pointers to be 8 byte aligned on 64bit platforms?
>>
>>
>> No since here we do not align pointers we align binder_buffers and offsets
>> in a buffer.
>>
>
> Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we
> should keep the start address of the struct 8 byte aligned as well.

Most of 64bit compilers will try to align pointers within a structure to 
natural boundaries. However all 64bit variants of currently supported 
Android architectures can service unaligned accesses(possibly with a 
performance degradation compared to an aligned access).

You can take a look at alignment requirements for AArch64 here 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf 
chapter 4.

What we are modifying in this patch is the alignment requirements on the 
buffer size(as seen above - arbitrary size 4byte aligned) and the 
alignment check on offp.

Let's take a look at what offp does. The userspace will write object 
references to a buffer using:

>>  820 status_t Parcel::writeObject(const flat_binder_object& val, bool nullMetaData)
>>  ...
>>  826         *reinterpret_cast<flat_binder_object*>(mData+mDataPos) = val;

Buffer
|---------------------------------------|val
|					|
|->mData				|->mDataPos	

where mData is the start of the buffer and mDataPos the current position 
within the buffer(equivalent to offp in the kernel space). Since the 
buffer is guaranteed to be u32 aligned but not u64 aligned the pointer 
to flat_binder_object might live on a unaligned boundary(offp will 
always be aligned to sizeof(u32) - see Parcel::writeAligned()).

However this will happen only on buffers where at the time we write the
next object reference(val) the buffer cursor(mDataPos) happens not to be 
on a multiple of sizeof(void *).

Adding an alignment check in the userspace might be more costly than
servicing the unaligned access(for AArch64 serviced in hardware). Also 
we will save some memory by not adding the padding.

On the other hand if instead of writing a pointer we write a 64bit mutex 
lock to an unaligned address and than try to read it in the kernel side 
things are not guaranteed to be sane. The compiler could make the 
assumption that the lock is natural aligned and use load/store exclusive 
that will fail on an unaligned address. However for this situation we 
can extend the userspace API and add a mutex write primitive like:


> status_t Parcel::writeMutex(mutex lock)
> ...
> *reinterpret_cast<mutex>(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock;

I am not aware of any situation where you will have 64bit mutexes passed 
in a binder buffer but you would probably know more about this. Since 
all writes to the buffer are 32bit aligned a 32bit mutex would not 
suffer any alignment issues.

Let me know what are your thoughts about this.

>> Let's assume that from the userspace we receive a sequence of BC_INCREFS and
>> BC_FREE_BUFFER. According to their definitions the buffer would look like:
>>
>> Buffer:
>> [addr]          [element]
>> 0               BC_INCREFS
>> 4               __u32
>> 8               BC_FREE_BUFFER
>> 12              void *        //(8 bytes for 64bit or 4 bytes for 32bit)
>>
>> Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit systems(4bytes
>> aligned). Same explanation for offp where it represents the offset form the
>> start of the buffer to a flat_binder_object(for example here the offset to
>> void* - 12bytes).
>>
>
> Does this work on every 64 bit system?

See above.


Thanks for your feedback,
Serban