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

[Serban Constantinescu]

On 10/04/13 23:12, Arve Hjønnevåg wrote:
> > > > struct flat_binder_object {
> > > > /* 8 bytes for large_flat_header. */
> > > > - unsigned long type;
> > > > - unsigned long flags;
> > > > + __u32 type;
> > > > + __u32 flags;
> > > >
> > > > /* 8 bytes of data. */
> > > > union {
> > > > void __user *binder; /* local object */
> > > > - signed long handle; /* remote object */
> > > > + __s32 handle; /* remote object */
> > >
> > >
> > > Why limit the handle to 32 bits when the pointer that it shares
> > > storage with need to be 64 bit on 64 bit systems?
> >
> >
> > Here I have mirrored the type being passed in handle - a file
> > descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
> > BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
> > the kernel/userspace(as 32bit value on 64bit systems). However this change
> > does not limit the extension of the API since we can read the value as 64bit
> > - binder(on 64bit systems).
> >
> > I can remove this change if you consider that is the better solution.
>
> I was asking if we should just use 64 bit handles on 64 bit systems,
> not adding casts. It would require another union member for a file
> descriptor however.

I will leave this handle as __s32 for this patch set but I will take a 
look into what and if we need to change this for a 64bit system. From a 
top level perspective(a look at binder_*_ref() functions and the 
userspace equivalent) this should work fine as 32bit.

> > > > };
> > > >
> > > > /* extra data associated with local object */
> > > > @@ -67,18 +67,18 @@ struct flat_binder_object {
> > > > */
> > > >
> > > > struct binder_write_read {
> > > > - signed long write_size; /* bytes to write */
> > > > - signed long write_consumed; /* bytes consumed by driver */
> > > > + size_t write_size; /* bytes to write */
> > > > + size_t write_consumed; /* bytes consumed by driver */
> > > > unsigned long write_buffer;
> > > > - signed long read_size; /* bytes to read */
> > > > - signed long read_consumed; /* bytes consumed by driver */
> > > > + size_t read_size; /* bytes to read */
> > > > + size_t read_consumed; /* bytes consumed by driver */
> > >
> > >
> > > What is this change for? You changed from a signed type to an unsigned
> > > type which seems unrelated to adding 64 bit support.
> >
> >
> > See above explanation for binder_thread_write() change, I will break this
> > into its own patch.
> >
> >
> > > > 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.

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/