[PATCH v5 0/6] Android Binder IPC Fixes

[PATCH v5 0/6] Android Binder IPC Fixes

[Serban Constantinescu]

Hi all,

This set of patches will clean-up and fix some of the issues that arise
with the current binder interface when moving to a 64bit kernel. All these
changes will not affect the existing 32bit Android interface and are meant
to stand as the base for the 64bit binder compat layer(kernel or userpsace).

The patch set has been successfully tested with a 64bit Linux userspace and
64bit binder unit-tests.

This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
kernel binder compat layer.

Changes for v5:
1 6/6: Moved patch to the end of the series; changed handle to use __u32 type
2 4/6: Removed some of the alignment/buffer changes introduced in previous
       versions of the patch.

Changes for v4:
1: 5/6: Fix the offset buffer alignment such that it will work for cases where
buffer start + buffer size are not aligned to (void *)

Changes for v3:
1:  Dropped the patch that was replacing uint32_t types with unsigned int
2:  Dropped the patch fixing the IOCTL types(since it has been added to Greg's
    staging tree)
3:  Split one patch into two: 'modify binder_write_read' and '64bit changes'
4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
5:  Modified the binder command IOCTL declarations according to Arve's review

Changes for v2:
1: 1/7: Modified the commit message according to Greg's feedback;
2: 3/7: Merged with the patch fixing the printk format specifiers.

Serban Constantinescu (6):
  staging: android: binder: modify struct binder_write_read to use
    size_t
  staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
  staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
  staging: android: binder: fix alignment issues
  staging: android: binder: replace types with portable ones
  staging: android: binder: fix binder interface for 64bit compat layer

 drivers/staging/android/binder.c |   32 ++++++++++++-------------
 drivers/staging/android/binder.h |   48 +++++++++++++++++++-------------------
 2 files changed, 40 insertions(+), 40 deletions(-)

-- 
1.7.9.5

[PATCH v5 1/6] staging: android: binder: modify struct binder_write_read to use size_t

[Serban Constantinescu]

This change mirrors the userspace operation where struct binder_write_read
members that specify the buffer size and consumed size are size_t elements.

The patch also fixes the binder_thread_write() and binder_thread_read()
functions prototypes to conform with the definition of binder_write_read.

The changes do not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
Acked-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.c |   10 +++++-----
 drivers/staging/android/binder.h |    8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 1567ac2..ce70909 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)
 {
 	uint32_t cmd;
 	void __user *ptr = buffer + *consumed;
@@ -2080,8 +2080,8 @@ static int binder_has_thread_work(struct binder_thread *thread)
 
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
-			      void  __user *buffer, int size,
-			      signed long *consumed, int non_block)
+			      void  __user *buffer, size_t size,
+			      size_t *consumed, int non_block)
 {
 	void __user *ptr = buffer + *consumed;
 	void __user *end = buffer + size;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		}
 		binder_debug(BINDER_DEBUG_READ_WRITE,
-			     "%d:%d write %ld at %08lx, read %ld at %08lx\n",
+			     "%d:%d write %zd at %08lx, read %zd at %08lx\n",
 			     proc->pid, thread->pid, bwr.write_size,
 			     bwr.write_buffer, bwr.read_size, bwr.read_buffer);
 
@@ -2604,7 +2604,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			}
 		}
 		binder_debug(BINDER_DEBUG_READ_WRITE,
-			     "%d:%d wrote %ld of %ld, read return %ld of %ld\n",
+			     "%d:%d wrote %zd of %zd, read return %zd of %zd\n",
 			     proc->pid, thread->pid, bwr.write_consumed, bwr.write_size,
 			     bwr.read_consumed, bwr.read_size);
 		if (copy_to_user(ubuf, &bwr, sizeof(bwr))) {
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dbe81ce..edab249 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -67,11 +67,11 @@ 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 */
 	unsigned long	read_buffer;
 };
 
-- 
1.7.9.5

[PATCH v5 2/6] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration

[Serban Constantinescu]

This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
instead of size_t for setting the max threads. Thus using the same
handler for 32 and 64bit kernels.

This value is stored internally in struct binder_proc and set to 15
on open_binder() in the libbinder API(thus no need for a 64bit size_t
on 64bit platforms).

The change does not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
Acked-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index edab249..6515dd2 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -86,7 +86,7 @@ struct binder_version {
 
 #define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
 #define	BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
-#define	BINDER_SET_MAX_THREADS		_IOW('b', 5, size_t)
+#define	BINDER_SET_MAX_THREADS		_IOW('b', 5, __u32)
 #define	BINDER_SET_IDLE_PRIORITY	_IOW('b', 6, __s32)
 #define	BINDER_SET_CONTEXT_MGR		_IOW('b', 7, __s32)
 #define	BINDER_THREAD_EXIT		_IOW('b', 8, __s32)
-- 
1.7.9.5

[PATCH v5 3/6] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration

[Serban Constantinescu]

BinderDriverCommands mirror the ioctl usage. Thus the size of the
structure passed through the interface should be used to generate the
ioctl No.

The change reflects the type being passed from the user space-a pointer
to a binder_buffer. This change should not affect the existing 32bit
user space since BC_FREE_BUFFER is computed as:

   #define _IOW(type,nr,size)         \
      ((type) << _IOC_TYPESHIFT) |    \
      ((nr)   << _IOC_NRSHIFT) |      \
      ((size) << _IOC_SIZESHIFT))

and for a 32bit compiler BC_FREE_BUFFER will have the same computed
value. This change will also ease our work in differentiating
BC_FREE_BUFFER from COMPAT_BC_FREE_BUFFER.

The change does not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
Acked-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 6515dd2..b55bba9 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -265,7 +265,7 @@ enum binder_driver_command_protocol {
 	 * Else you have acquired a primary reference on the object.
 	 */
 
-	BC_FREE_BUFFER = _IOW('c', 3, int),
+	BC_FREE_BUFFER = _IOW('c', 3, void *),
 	/*
 	 * void *: ptr to transaction data received on a read
 	 */
-- 
1.7.9.5

[PATCH v5 4/6] staging: android: binder: fix alignment issues

[Serban Constantinescu]

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.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index ce70909..7450d56 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		struct flat_binder_object *fp;
 		if (*offp > buffer->data_size - sizeof(*fp) ||
 		    buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(void *))) {
+		    !IS_ALIGNED(*offp, sizeof(u32))) {
 			pr_err("transaction release %d bad offset %zd, size %zd\n",
 			 debug_id, *offp, buffer->data_size);
 			continue;
@@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
 		struct flat_binder_object *fp;
 		if (*offp > t->buffer->data_size - sizeof(*fp) ||
 		    t->buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(void *))) {
+		    !IS_ALIGNED(*offp, sizeof(u32))) {
 			binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
 					proc->pid, thread->pid, *offp);
 			return_error = BR_FAILED_REPLY;
-- 
1.7.9.5

[PATCH v5 5/6] staging: android: binder: replace types with portable ones

[Serban Constantinescu]

Since this driver is meant to be used on different types of processors
and a portable driver should specify the size a variable expects to be
this patch changes the types used throughout the binder interface.

We use "userspace" types since this header will be exported and used by
the Android filesystem.

The patch does not change in any way the functionality of the binder driver.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
Acked-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/staging/android/binder.h |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index b55bba9..dadfce0 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -123,10 +123,10 @@ struct binder_transaction_data {
 		void	*ptr;	/* target descriptor of return transaction */
 	} target;
 	void		*cookie;	/* target object cookie */
-	unsigned int	code;		/* transaction command */
+	__u32		code;		/* transaction command */
 
 	/* General information about the transaction. */
-	unsigned int	flags;
+	__u32	        flags;
 	pid_t		sender_pid;
 	uid_t		sender_euid;
 	size_t		data_size;	/* number of bytes of data */
@@ -143,7 +143,7 @@ struct binder_transaction_data {
 			/* offsets from buffer to flat_binder_object structs */
 			const void __user	*offsets;
 		} ptr;
-		uint8_t	buf[8];
+		__u8	buf[8];
 	} data;
 };
 
@@ -153,18 +153,18 @@ struct binder_ptr_cookie {
 };
 
 struct binder_pri_desc {
-	int priority;
-	int desc;
+	__s32 priority;
+	__s32 desc;
 };
 
 struct binder_pri_ptr_cookie {
-	int priority;
+	__s32 priority;
 	void *ptr;
 	void *cookie;
 };
 
 enum binder_driver_return_protocol {
-	BR_ERROR = _IOR('r', 0, int),
+	BR_ERROR = _IOR('r', 0, __s32),
 	/*
 	 * int: error code
 	 */
@@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
 	 * binder_transaction_data: the received command.
 	 */
 
-	BR_ACQUIRE_RESULT = _IOR('r', 4, int),
+	BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
 	/*
 	 * not currently supported
 	 * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
@@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
 	 * binder_transaction_data: the sent command.
 	 */
 
-	BC_ACQUIRE_RESULT = _IOW('c', 2, int),
+	BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
 	/*
 	 * not currently supported
 	 * int:  0 if the last BR_ATTEMPT_ACQUIRE was not successful.
@@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
 	 * void *: ptr to transaction data received on a read
 	 */
 
-	BC_INCREFS = _IOW('c', 4, int),
-	BC_ACQUIRE = _IOW('c', 5, int),
-	BC_RELEASE = _IOW('c', 6, int),
-	BC_DECREFS = _IOW('c', 7, int),
+	BC_INCREFS = _IOW('c', 4, __u32),
+	BC_ACQUIRE = _IOW('c', 5, __u32),
+	BC_RELEASE = _IOW('c', 6, __u32),
+	BC_DECREFS = _IOW('c', 7, __u32),
 	/*
 	 * int:	descriptor
 	 */
-- 
1.7.9.5

[PATCH v5 6/6] staging: android: binder: fix binder interface for 64bit compat layer

[Serban Constantinescu]

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 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@arm.com>
---
 drivers/staging/android/binder.c |   20 ++++++++++----------
 drivers/staging/android/binder.h |   10 +++++-----
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7450d56..056afe7 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1271,7 +1271,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		case BINDER_TYPE_WEAK_HANDLE: {
 			struct binder_ref *ref = binder_get_ref(proc, fp->handle);
 			if (ref == NULL) {
-				pr_err("transaction release %d bad handle %ld\n",
+				pr_err("transaction release %d bad handle %d\n",
 				 debug_id, fp->handle);
 				break;
 			}
@@ -1283,13 +1283,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 
 		case BINDER_TYPE_FD:
 			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        fd %ld\n", fp->handle);
+				     "        fd %d\n", fp->handle);
 			if (failed_at)
 				task_close_fd(proc, fp->handle);
 			break;
 
 		default:
-			pr_err("transaction release %d bad object type %lx\n",
+			pr_err("transaction release %d bad object type %x\n",
 				debug_id, fp->type);
 			break;
 		}
@@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc *proc,
 		case BINDER_TYPE_WEAK_HANDLE: {
 			struct binder_ref *ref = binder_get_ref(proc, fp->handle);
 			if (ref == NULL) {
-				binder_user_error("%d:%d got transaction with invalid handle, %ld\n",
+				binder_user_error("%d:%d got transaction with invalid handle, %d\n",
 						proc->pid,
 						thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
@@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc *proc,
 
 			if (reply) {
 				if (!(in_reply_to->flags & TF_ACCEPT_FDS)) {
-					binder_user_error("%d:%d got reply with fd, %ld, but target does not allow fds\n",
+					binder_user_error("%d:%d got reply with fd, %d, but target does not allow fds\n",
 						proc->pid, thread->pid, fp->handle);
 					return_error = BR_FAILED_REPLY;
 					goto err_fd_not_allowed;
 				}
 			} else if (!target_node->accept_fds) {
-				binder_user_error("%d:%d got transaction with fd, %ld, but target does not allow fds\n",
+				binder_user_error("%d:%d got transaction with fd, %d, but target does not allow fds\n",
 					proc->pid, thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
 				goto err_fd_not_allowed;
@@ -1604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
 
 			file = fget(fp->handle);
 			if (file == NULL) {
-				binder_user_error("%d:%d got transaction with invalid fd, %ld\n",
+				binder_user_error("%d:%d got transaction with invalid fd, %d\n",
 					proc->pid, thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
 				goto err_fget_failed;
@@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc *proc,
 			task_fd_install(target_proc, target_fd, file);
 			trace_binder_transaction_fd(t, fp->handle, target_fd);
 			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        fd %ld -> %d\n", fp->handle, target_fd);
+				     "        fd %d -> %d\n", fp->handle, target_fd);
 			/* TODO: fput? */
 			fp->handle = target_fd;
 		} break;
 
 		default:
-			binder_user_error("%d:%d got transaction with invalid object type, %lx\n",
+			binder_user_error("%d:%d got transaction with invalid object type, %x\n",
 				proc->pid, thread->pid, fp->type);
 			return_error = BR_FAILED_REPLY;
 			goto err_bad_object_type;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		}
 		binder_debug(BINDER_DEBUG_READ_WRITE,
-			     "%d:%d write %zd at %08lx, read %zd at %08lx\n",
+			     "%d:%d write %zd at %016lx, read %zd at %016lx\n",
 			     proc->pid, thread->pid, bwr.write_size,
 			     bwr.write_buffer, bwr.read_size, bwr.read_buffer);
 
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dadfce0..b88b263 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 */
+		__u32	    handle;		/* remote object */
 	};
 
 	/* extra data associated with local object */
@@ -78,7 +78,7 @@ struct binder_write_read {
 /* 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;
 };
 
 /* This is the current protocol version. */
@@ -119,7 +119,7 @@ struct binder_transaction_data {
 	 * identifying the target and contents of the transaction.
 	 */
 	union {
-		size_t	handle;	/* target descriptor of command transaction */
+		__u32	handle;	/* target descriptor of command transaction */
 		void	*ptr;	/* target descriptor of return transaction */
 	} target;
 	void		*cookie;	/* target object cookie */
-- 
1.7.9.5

Re: [PATCH v5 0/6] Android Binder IPC Fixes

[Serban Constantinescu]

Hi all,

Any feedback or comments on this patch set?

Thanks,
Serban

On 19/06/13 18:12, Serban Constantinescu wrote:
> Hi all,
>
> This set of patches will clean-up and fix some of the issues that arise
> with the current binder interface when moving to a 64bit kernel. All these
> changes will not affect the existing 32bit Android interface and are meant
> to stand as the base for the 64bit binder compat layer(kernel or userpsace).
>
> The patch set has been successfully tested with a 64bit Linux userspace and
> 64bit binder unit-tests.
>
> This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
> kernel binder compat layer.
>
> Changes for v5:
> 1 6/6: Moved patch to the end of the series; changed handle to use __u32 type
> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
>         versions of the patch.
>
> Changes for v4:
> 1: 5/6: Fix the offset buffer alignment such that it will work for cases where
> buffer start + buffer size are not aligned to (void *)
>
> Changes for v3:
> 1:  Dropped the patch that was replacing uint32_t types with unsigned int
> 2:  Dropped the patch fixing the IOCTL types(since it has been added to Greg's
>      staging tree)
> 3:  Split one patch into two: 'modify binder_write_read' and '64bit changes'
> 4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
> 5:  Modified the binder command IOCTL declarations according to Arve's review
>
> Changes for v2:
> 1: 1/7: Modified the commit message according to Greg's feedback;
> 2: 3/7: Merged with the patch fixing the printk format specifiers.
>
> Serban Constantinescu (6):
>    staging: android: binder: modify struct binder_write_read to use
>      size_t
>    staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
>    staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
>    staging: android: binder: fix alignment issues
>    staging: android: binder: replace types with portable ones
>    staging: android: binder: fix binder interface for 64bit compat layer
>
>   drivers/staging/android/binder.c |   32 ++++++++++++-------------
>   drivers/staging/android/binder.h |   48 +++++++++++++++++++-------------------
>   2 files changed, 40 insertions(+), 40 deletions(-)
>


-- 
Best Regards,

Serban Constantinescu
PDSW Engineer ARM Ltd.

Re: [PATCH v5 4/6] staging: android: binder: fix alignment issues

[Arve Hjønnevåg]

On Wed, Jun 19, 2013 at 10:12 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.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
>  drivers/staging/android/binder.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index ce70909..7450d56 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>                 struct flat_binder_object *fp;
>                 if (*offp > buffer->data_size - sizeof(*fp) ||
>                     buffer->data_size < sizeof(*fp) ||
> -                   !IS_ALIGNED(*offp, sizeof(void *))) {
> +                   !IS_ALIGNED(*offp, sizeof(u32))) {
>                         pr_err("transaction release %d bad offset %zd, size %zd\n",
>                          debug_id, *offp, buffer->data_size);
>                         continue;
> @@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
>                 struct flat_binder_object *fp;
>                 if (*offp > t->buffer->data_size - sizeof(*fp) ||
>                     t->buffer->data_size < sizeof(*fp) ||
> -                   !IS_ALIGNED(*offp, sizeof(void *))) {
> +                   !IS_ALIGNED(*offp, sizeof(u32))) {
>                         binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
>                                         proc->pid, thread->pid, *offp);
>                         return_error = BR_FAILED_REPLY;
> --
> 1.7.9.5
>

Acked-by: Arve Hjønnevåg <arve@android.com>

--
Arve Hjønnevåg

Re: [PATCH v5 5/6] staging: android: binder: replace types with portable ones

[Arve Hjønnevåg]

On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> Since this driver is meant to be used on different types of processors
> and a portable driver should specify the size a variable expects to be
> this patch changes the types used throughout the binder interface.
>
> We use "userspace" types since this header will be exported and used by
> the Android filesystem.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> Acked-by: Arve Hjønnevåg <arve@android.com>
> ---
>  drivers/staging/android/binder.h |   26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index b55bba9..dadfce0 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -123,10 +123,10 @@ struct binder_transaction_data {
>                 void    *ptr;   /* target descriptor of return transaction */
>         } target;
>         void            *cookie;        /* target object cookie */
> -       unsigned int    code;           /* transaction command */
> +       __u32           code;           /* transaction command */
>
>         /* General information about the transaction. */
> -       unsigned int    flags;
> +       __u32           flags;
>         pid_t           sender_pid;
>         uid_t           sender_euid;
>         size_t          data_size;      /* number of bytes of data */
> @@ -143,7 +143,7 @@ struct binder_transaction_data {
>                         /* offsets from buffer to flat_binder_object structs */
>                         const void __user       *offsets;
>                 } ptr;
> -               uint8_t buf[8];
> +               __u8    buf[8];
>         } data;
>  };
>
> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
>  };
>
>  struct binder_pri_desc {
> -       int priority;
> -       int desc;
> +       __s32 priority;
> +       __s32 desc;

desc should be __u32 to be consistent with the other changes you are
making in this and the next patch.

>  };
>
>  struct binder_pri_ptr_cookie {
> -       int priority;
> +       __s32 priority;
>         void *ptr;
>         void *cookie;
>  };
>
>  enum binder_driver_return_protocol {
> -       BR_ERROR = _IOR('r', 0, int),
> +       BR_ERROR = _IOR('r', 0, __s32),
>         /*
>          * int: error code
>          */
> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
>          * binder_transaction_data: the received command.
>          */
>
> -       BR_ACQUIRE_RESULT = _IOR('r', 4, int),
> +       BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
>         /*
>          * not currently supported
>          * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
>          * binder_transaction_data: the sent command.
>          */
>
> -       BC_ACQUIRE_RESULT = _IOW('c', 2, int),
> +       BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
>         /*
>          * not currently supported
>          * int:  0 if the last BR_ATTEMPT_ACQUIRE was not successful.
> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
>          * void *: ptr to transaction data received on a read
>          */
>
> -       BC_INCREFS = _IOW('c', 4, int),
> -       BC_ACQUIRE = _IOW('c', 5, int),
> -       BC_RELEASE = _IOW('c', 6, int),
> -       BC_DECREFS = _IOW('c', 7, int),
> +       BC_INCREFS = _IOW('c', 4, __u32),
> +       BC_ACQUIRE = _IOW('c', 5, __u32),
> +       BC_RELEASE = _IOW('c', 6, __u32),
> +       BC_DECREFS = _IOW('c', 7, __u32),
>         /*
>          * int: descriptor
>          */
> --
> 1.7.9.5
>



--
Arve Hjønnevåg

Re: [PATCH v5 6/6] staging: android: binder: fix binder interface for 64bit compat layer

[Arve Hjønnevåg]

On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
<serban.constantinescu@arm.com> 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 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@arm.com>
> ---
>  drivers/staging/android/binder.c |   20 ++++++++++----------
>  drivers/staging/android/binder.h |   10 +++++-----
>  2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 7450d56..056afe7 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -1271,7 +1271,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>                 case BINDER_TYPE_WEAK_HANDLE: {
>                         struct binder_ref *ref = binder_get_ref(proc, fp->handle);
>                         if (ref == NULL) {
> -                               pr_err("transaction release %d bad handle %ld\n",
> +                               pr_err("transaction release %d bad handle %d\n",
>                                  debug_id, fp->handle);
>                                 break;
>                         }
> @@ -1283,13 +1283,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
>
>                 case BINDER_TYPE_FD:
>                         binder_debug(BINDER_DEBUG_TRANSACTION,
> -                                    "        fd %ld\n", fp->handle);
> +                                    "        fd %d\n", fp->handle);
>                         if (failed_at)
>                                 task_close_fd(proc, fp->handle);
>                         break;
>
>                 default:
> -                       pr_err("transaction release %d bad object type %lx\n",
> +                       pr_err("transaction release %d bad object type %x\n",
>                                 debug_id, fp->type);
>                         break;
>                 }
> @@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc *proc,
>                 case BINDER_TYPE_WEAK_HANDLE: {
>                         struct binder_ref *ref = binder_get_ref(proc, fp->handle);
>                         if (ref == NULL) {
> -                               binder_user_error("%d:%d got transaction with invalid handle, %ld\n",
> +                               binder_user_error("%d:%d got transaction with invalid handle, %d\n",
>                                                 proc->pid,
>                                                 thread->pid, fp->handle);
>                                 return_error = BR_FAILED_REPLY;
> @@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc *proc,
>
>                         if (reply) {
>                                 if (!(in_reply_to->flags & TF_ACCEPT_FDS)) {
> -                                       binder_user_error("%d:%d got reply with fd, %ld, but target does not allow fds\n",
> +                                       binder_user_error("%d:%d got reply with fd, %d, but target does not allow fds\n",
>                                                 proc->pid, thread->pid, fp->handle);
>                                         return_error = BR_FAILED_REPLY;
>                                         goto err_fd_not_allowed;
>                                 }
>                         } else if (!target_node->accept_fds) {
> -                               binder_user_error("%d:%d got transaction with fd, %ld, but target does not allow fds\n",
> +                               binder_user_error("%d:%d got transaction with fd, %d, but target does not allow fds\n",
>                                         proc->pid, thread->pid, fp->handle);
>                                 return_error = BR_FAILED_REPLY;
>                                 goto err_fd_not_allowed;
> @@ -1604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
>
>                         file = fget(fp->handle);
>                         if (file == NULL) {
> -                               binder_user_error("%d:%d got transaction with invalid fd, %ld\n",
> +                               binder_user_error("%d:%d got transaction with invalid fd, %d\n",
>                                         proc->pid, thread->pid, fp->handle);
>                                 return_error = BR_FAILED_REPLY;
>                                 goto err_fget_failed;
> @@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc *proc,
>                         task_fd_install(target_proc, target_fd, file);
>                         trace_binder_transaction_fd(t, fp->handle, target_fd);
>                         binder_debug(BINDER_DEBUG_TRANSACTION,
> -                                    "        fd %ld -> %d\n", fp->handle, target_fd);
> +                                    "        fd %d -> %d\n", fp->handle, target_fd);
>                         /* TODO: fput? */
>                         fp->handle = target_fd;
>                 } break;
>
>                 default:
> -                       binder_user_error("%d:%d got transaction with invalid object type, %lx\n",
> +                       binder_user_error("%d:%d got transaction with invalid object type, %x\n",
>                                 proc->pid, thread->pid, fp->type);
>                         return_error = BR_FAILED_REPLY;
>                         goto err_bad_object_type;
> @@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                         goto err;
>                 }
>                 binder_debug(BINDER_DEBUG_READ_WRITE,
> -                            "%d:%d write %zd at %08lx, read %zd at %08lx\n",
> +                            "%d:%d write %zd at %016lx, read %zd at %016lx\n",
>                              proc->pid, thread->pid, bwr.write_size,
>                              bwr.write_buffer, bwr.read_size, bwr.read_buffer);
>
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index dadfce0..b88b263 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 */
> +               __u32       handle;             /* remote object */
>         };
>
>         /* extra data associated with local object */
> @@ -78,7 +78,7 @@ struct binder_write_read {
>  /* 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;
>  };
>
>  /* This is the current protocol version. */
> @@ -119,7 +119,7 @@ struct binder_transaction_data {
>          * identifying the target and contents of the transaction.
>          */
>         union {
> -               size_t  handle; /* target descriptor of command transaction */
> +               __u32   handle; /* target descriptor of command transaction */
>                 void    *ptr;   /* target descriptor of return transaction */
>         } target;
>         void            *cookie;        /* target object cookie */
> --
> 1.7.9.5
>

Acked-by: Arve Hjønnevåg <arve@android.com>

--
Arve Hjønnevåg

Re: [PATCH v5 0/6] Android Binder IPC Fixes

[Arve Hjønnevåg]

On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> Hi all,
>
> Any feedback or comments on this patch set?
>
> Thanks,
> Serban
>

The new patches look OK, but I would like to also see the patches that
add support for 32 bit user-space on a 64 bit kernel.


>
> On 19/06/13 18:12, Serban Constantinescu wrote:
>>
>> Hi all,
>>
>> This set of patches will clean-up and fix some of the issues that arise
>> with the current binder interface when moving to a 64bit kernel. All these
>> changes will not affect the existing 32bit Android interface and are meant
>> to stand as the base for the 64bit binder compat layer(kernel or
>> userpsace).
>>
>> The patch set has been successfully tested with a 64bit Linux userspace
>> and
>> 64bit binder unit-tests.
>>
>> This patch set has been successfully tested on 32bit platforms(ARMv7
>> VExpress)
>> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an
>> in
>> kernel binder compat layer.
>>
>> Changes for v5:
>> 1 6/6: Moved patch to the end of the series; changed handle to use __u32
>> type
>> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
>>         versions of the patch.
>>
>> Changes for v4:
>> 1: 5/6: Fix the offset buffer alignment such that it will work for cases
>> where
>> buffer start + buffer size are not aligned to (void *)
>>
>> Changes for v3:
>> 1:  Dropped the patch that was replacing uint32_t types with unsigned int
>> 2:  Dropped the patch fixing the IOCTL types(since it has been added to
>> Greg's
>>      staging tree)
>> 3:  Split one patch into two: 'modify binder_write_read' and '64bit
>> changes'
>> 4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>> review
>> 5:  Modified the binder command IOCTL declarations according to Arve's
>> review
>>
>> Changes for v2:
>> 1: 1/7: Modified the commit message according to Greg's feedback;
>> 2: 3/7: Merged with the patch fixing the printk format specifiers.
>>
>> Serban Constantinescu (6):
>>    staging: android: binder: modify struct binder_write_read to use
>>      size_t
>>    staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
>>    staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
>>    staging: android: binder: fix alignment issues
>>    staging: android: binder: replace types with portable ones
>>    staging: android: binder: fix binder interface for 64bit compat layer
>>
>>   drivers/staging/android/binder.c |   32 ++++++++++++-------------
>>   drivers/staging/android/binder.h |   48
>> +++++++++++++++++++-------------------
>>   2 files changed, 40 insertions(+), 40 deletions(-)
>>
>
>
> --
> Best Regards,
>
> Serban Constantinescu
> PDSW Engineer ARM Ltd.
>



--
Arve Hjønnevåg

Re: [PATCH v5 5/6] staging: android: binder: replace types with portable ones

[Serban Constantinescu]

On 03/07/13 23:30, Arve Hjønnevåg wrote:
> On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
> <serban.constantinescu@arm.com> wrote:
>> Since this driver is meant to be used on different types of processors
>> and a portable driver should specify the size a variable expects to be
>> this patch changes the types used throughout the binder interface.
>>
>> We use "userspace" types since this header will be exported and used by
>> the Android filesystem.
>>
>> The patch does not change in any way the functionality of the binder driver.
>>
>> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
>> Acked-by: Arve Hjønnevåg <arve@android.com>
>> ---
>>   drivers/staging/android/binder.h |   26 +++++++++++++-------------
>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
>> index b55bba9..dadfce0 100644
>> --- a/drivers/staging/android/binder.h
>> +++ b/drivers/staging/android/binder.h
>> @@ -123,10 +123,10 @@ struct binder_transaction_data {
>>                  void    *ptr;   /* target descriptor of return transaction */
>>          } target;
>>          void            *cookie;        /* target object cookie */
>> -       unsigned int    code;           /* transaction command */
>> +       __u32           code;           /* transaction command */
>>
>>          /* General information about the transaction. */
>> -       unsigned int    flags;
>> +       __u32           flags;
>>          pid_t           sender_pid;
>>          uid_t           sender_euid;
>>          size_t          data_size;      /* number of bytes of data */
>> @@ -143,7 +143,7 @@ struct binder_transaction_data {
>>                          /* offsets from buffer to flat_binder_object structs */
>>                          const void __user       *offsets;
>>                  } ptr;
>> -               uint8_t buf[8];
>> +               __u8    buf[8];
>>          } data;
>>   };
>>
>> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
>>   };
>>
>>   struct binder_pri_desc {
>> -       int priority;
>> -       int desc;
>> +       __s32 priority;
>> +       __s32 desc;
>
> desc should be __u32 to be consistent with the other changes you are
> making in this and the next patch.

I have added this change to v6 of this patch set which I will send out 
shortly.


Thanks again for your feedback,
Serban
>
>>   };
>>
>>   struct binder_pri_ptr_cookie {
>> -       int priority;
>> +       __s32 priority;
>>          void *ptr;
>>          void *cookie;
>>   };
>>
>>   enum binder_driver_return_protocol {
>> -       BR_ERROR = _IOR('r', 0, int),
>> +       BR_ERROR = _IOR('r', 0, __s32),
>>          /*
>>           * int: error code
>>           */
>> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
>>           * binder_transaction_data: the received command.
>>           */
>>
>> -       BR_ACQUIRE_RESULT = _IOR('r', 4, int),
>> +       BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
>>          /*
>>           * not currently supported
>>           * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
>> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
>>           * binder_transaction_data: the sent command.
>>           */
>>
>> -       BC_ACQUIRE_RESULT = _IOW('c', 2, int),
>> +       BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
>>          /*
>>           * not currently supported
>>           * int:  0 if the last BR_ATTEMPT_ACQUIRE was not successful.
>> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
>>           * void *: ptr to transaction data received on a read
>>           */
>>
>> -       BC_INCREFS = _IOW('c', 4, int),
>> -       BC_ACQUIRE = _IOW('c', 5, int),
>> -       BC_RELEASE = _IOW('c', 6, int),
>> -       BC_DECREFS = _IOW('c', 7, int),
>> +       BC_INCREFS = _IOW('c', 4, __u32),
>> +       BC_ACQUIRE = _IOW('c', 5, __u32),
>> +       BC_RELEASE = _IOW('c', 6, __u32),
>> +       BC_DECREFS = _IOW('c', 7, __u32),
>>          /*
>>           * int: descriptor
>>           */
>> --
>> 1.7.9.5
>>
>
>
>
> --
> Arve Hjønnevåg
>

Re: [PATCH v5 0/6] Android Binder IPC Fixes

[Serban Constantinescu]

On 03/07/13 23:32, Arve Hjønnevåg wrote:
> On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> Hi all,
>>
>> Any feedback or comments on this patch set?
>>
>> Thanks,
>> Serban
>>
>
> The new patches look OK, but I would like to also see the patches that
> add support for 32 bit user-space on a 64 bit kernel.

I have added the last change to v6 and kept Arve's ack.

I will rebase my binder kernel compat on this latest version and polish 
the patch set such that is ready for upstream. I should have something 
ready for your review soon.

Thanks for your help,
Serban Constantinescu

>
>>
>> On 19/06/13 18:12, Serban Constantinescu wrote:
>>>
>>> Hi all,
>>>
>>> This set of patches will clean-up and fix some of the issues that arise
>>> with the current binder interface when moving to a 64bit kernel. All these
>>> changes will not affect the existing 32bit Android interface and are meant
>>> to stand as the base for the 64bit binder compat layer(kernel or
>>> userpsace).
>>>
>>> The patch set has been successfully tested with a 64bit Linux userspace
>>> and
>>> 64bit binder unit-tests.
>>>
>>> This patch set has been successfully tested on 32bit platforms(ARMv7
>>> VExpress)
>>> and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an
>>> in
>>> kernel binder compat layer.
>>>
>>> Changes for v5:
>>> 1 6/6: Moved patch to the end of the series; changed handle to use __u32
>>> type
>>> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
>>>          versions of the patch.
>>>
>>> Changes for v4:
>>> 1: 5/6: Fix the offset buffer alignment such that it will work for cases
>>> where
>>> buffer start + buffer size are not aligned to (void *)
>>>
>>> Changes for v3:
>>> 1:  Dropped the patch that was replacing uint32_t types with unsigned int
>>> 2:  Dropped the patch fixing the IOCTL types(since it has been added to
>>> Greg's
>>>       staging tree)
>>> 3:  Split one patch into two: 'modify binder_write_read' and '64bit
>>> changes'
>>> 4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>>> review
>>> 5:  Modified the binder command IOCTL declarations according to Arve's
>>> review
>>>
>>> Changes for v2:
>>> 1: 1/7: Modified the commit message according to Greg's feedback;
>>> 2: 3/7: Merged with the patch fixing the printk format specifiers.
>>>
>>> Serban Constantinescu (6):
>>>     staging: android: binder: modify struct binder_write_read to use
>>>       size_t
>>>     staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
>>>     staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
>>>     staging: android: binder: fix alignment issues
>>>     staging: android: binder: replace types with portable ones
>>>     staging: android: binder: fix binder interface for 64bit compat layer
>>>
>>>    drivers/staging/android/binder.c |   32 ++++++++++++-------------
>>>    drivers/staging/android/binder.h |   48
>>> +++++++++++++++++++-------------------
>>>    2 files changed, 40 insertions(+), 40 deletions(-)
>>>
>>
>>
>> --
>> Best Regards,
>>
>> Serban Constantinescu
>> PDSW Engineer ARM Ltd.
>>
>
>
>
> --
> Arve Hjønnevåg
>