Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for64bit compat layer

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@xxxxxxx> wrote:
> The changes in this patch will fix the binder interface for use on 64bit
> machines and stand as the base of the 64bit compat support. The changes
> apply to the structures that are passed between the kernel and
> userspace.
>
> Most of the changes applied mirror the change to struct binder_version
> where there is no need for a 64bit wide protocol_version(on 64bit
> machines). The change inlines with the existing 32bit userspace(the
> structure has the same size) and simplifies the compat layer such that
> the same handler can service the BINDER_VERSION ioctl.
>
> Other changes fix the function prototypes for binder_thread_read/write,
> make use of kernel types as well as user-exportable ones and fix
> format specifier issues.
>
> The changes do not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@xxxxxxx>
> ---
> drivers/staging/android/binder.c | 28 ++++++++++++++--------------
> drivers/staging/android/binder.h | 16 ++++++++--------
> 2 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 5794cf6..a2cdd9e 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
...
> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
> }
>
> int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
> - void __user *buffer, int size, signed long *consumed)
> + void __user *buffer, size_t size, size_t *consumed)

What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.

...
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index dbe81ce..8012921 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -48,13 +48,13 @@ enum {
> */
> 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?

> };
>
> /* 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.

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

> };
>
> /* This is the current protocol version. */
> --
> 1.7.9.5
>


--
Arve Hjønnevåg
--
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/