lkml.org 
[lkml]   [2013]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 6/7] staging: android: binder: fix alignment issues
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

--
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/

\
 
 \ /
  Last update: 2013-04-11 21:41    [W:0.056 / U:0.768 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site