by Arve Hjønnevåg

[permalink] [raw]

Subject: Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

On Thu, Apr 11, 2013 at 8:13 AM, Serban Constantinescu
<[email protected]> wrote:
...
>>>
>>>>> unsigned long read_buffer;
>>>>> };
>>>>>
>>>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>>>> struct binder_version {
>>>>> /* driver protocol version -- increment with incompatible change */
>>>>> - signed long protocol_version;
>>>>> + __s32 protocol_version;
>>>>
>>>>
>>>>
>>>> How does user-space know if it should use 32 bit or 64 bit pointers.
>>>> Without this change, the BINDER_VERSION ioctl would only match when
>>>> the size of long matches.
>>>
>>>
>>>
>>> The userspace can check the values returned by uname(). That will
>>> determine
>>> if the kernel is 32 or 64bit and depending on this select what binder
>>> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
>>> using protocol_version as 64bit signed long(that is old kernel versions
>>> with
>>> no 64bit support).
>>>
>>> Leaving this value as signed long would mean that older versions of the
>>> binder(without 64bit support) will pass the check. Furthermore the
>>> protocol
>>> version will probably never exceed the values that could be represented
>>> on
>>> 32bit. It will also mean that BINDER_VERSION will have a different
>>> userspace/kernel handler for 64/32 systems.
>>>
>>> Let me know what are your thoughts related to these changes,
>>> Thanks for your feedback,
>>> Serban
>>>
>>
>> I think user-space should get the binder pointer size from the binder
>> driver, not elsewhere. Since the current driver does not appear to be
>> functional on a 64 bit system, I think adding an ioctl to get the
>> size, or encoding it into the binder version (use an unsigned type if
>> you do this), would be best.
>
>
> I will think about the best way of getting the pointer size and add it to
> the patch set for binder compat. For this patch set I will only modify the
> protocol_version from long to __s32.
>

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.

--
Arve Hj?nnev?g

2013-04-11 21:40:59

by Arve Hjønnevåg

[permalink] [raw]

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

On Thu, Apr 11, 2013 at 12:02 PM, Serban Constantinescu
<[email protected]> wrote:
> On 10/04/13 23:30, Arve Hj?nnev?g wrote:
>>
>> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
>> <[email protected]> wrote:
>>>
>>> On 10/04/13 00:58, Arve Hj?nnev?g wrote:
>>>>
>>>>
>>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>>>> <[email protected]> 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.
>

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?

I don't think there is any reason to not keep the binder_buffer and
offsets buffer that the kernel allocates aligned to 8 bytes on a 64
bit system. Also, I don't see any changes to where the offsets buffer
starts in this patch, so if datasize is not 8 bytes aligned you seem
to allocate less memory than you use.

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

Storing a mutex in a parcel does not make sense. The data in the
parcel is a copy of the data passed in, and the parcel seen by the
remote process is a copy of the parcel that sender created.

--
Arve Hj?nnev?g