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

From: Serban Constantinescu
Date: Thu Apr 11 2013 - 15:02:24 EST


On 10/04/13 23:30, Arve Hjønnevåg wrote:
On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
<Serban.Constantinescu@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/