[PATCH v3 0/6] Android Binder IPC Fixes

[PATCH v3 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 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 interface for 64bit compat layer
  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

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

-- 
1.7.9.5

[PATCH v3 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>
---
 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 24456a0..b7a8bec 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 v3 2/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 |    8 ++++----
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index b7a8bec..06004cf 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 edab249..2f94d16 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 */
 	};
 
 	/* 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. */
-- 
1.7.9.5

[PATCH v3 3/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>
---
 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 2f94d16..1761541 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 v3 4/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 1761541..c3562c4 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 v3 5/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 |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 06004cf..6436f22 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -658,8 +658,8 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
 		return NULL;
 	}
 
-	size = ALIGN(data_size, sizeof(void *)) +
-		ALIGN(offsets_size, sizeof(void *));
+	size = ALIGN(data_size, sizeof(u32)) +
+		ALIGN(offsets_size, sizeof(u32));
 
 	if (size < data_size || size < offsets_size) {
 		binder_user_error("%d: got transaction with invalid size %zd-%zd\n",
@@ -807,8 +807,8 @@ static void binder_free_buf(struct binder_proc *proc,
 
 	buffer_size = binder_buffer_size(proc, buffer);
 
-	size = ALIGN(buffer->data_size, sizeof(void *)) +
-		ALIGN(buffer->offsets_size, sizeof(void *));
+	size = ALIGN(buffer->data_size, sizeof(u32)) +
+		ALIGN(buffer->offsets_size, sizeof(u32));
 
 	binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
 		     "%d: binder_free_buf %p size %zd buffer_size %zd\n",
@@ -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 v3 6/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>
---
 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 c3562c4..bff1c74 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

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

[Serban Constantinescu]

Hi all,

Any feedback or comments on this patch set?

Thanks,
Serban


On 12/04/13 13:25, 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 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 interface for 64bit compat layer
>    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
>
>   drivers/staging/android/binder.c |   40 ++++++++++++++++-----------------
>   drivers/staging/android/binder.h |   46 +++++++++++++++++++-------------------
>   2 files changed, 43 insertions(+), 43 deletions(-)
>

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

[Arve Hjønnevåg]

On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> Hi all,
>
> Any feedback or comments on this patch set?
>

You don't seem to have addressed my feedback on the previous patch set.

--
Arve Hjønnevåg

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

[Kirill A. Shutemov]

On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
> 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.

I've tried to replace ugly hack with your patches to be able to boot
32-bit userspace on x86-64. It fails. :(

-- 
 Kirill A. Shutemov

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

[Serban Constantinescu]

Hi Arve,

On 30/04/13 00:13, Arve Hjønnevåg wrote:
> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> Hi all,
>>
>> Any feedback or comments on this patch set?
>>
>
> You don't seem to have addressed my feedback on the previous patch set.

For v3 I have modified the following according to your review:

> 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

The following were left out:

> On 11/04/13 22:40, Arve Hjønnevåg wrote:
> 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?

There is no kernel macro that I know which will help here(one that 
springs to mind is PTR_ALIGN but it aligns to (unsigned long) - we need 
one that aligns to (u32)). Any ideas?

> On 11/04/13 21:38, Arve Hjønnevåg wrote:
> 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.

For 32bit systems nothing has changed so they will continue to work as 
before. For 64bit systems the size of binder_version was signed long 
before the patch and __s32 after the patch is applied. Thus a 64bit 
system using the old interface will fail immediately after opening the 
binder driver, while cheeking the binder version (since the 
BINDER_VERSION ioctl will be different pre/post patch - size of 
binder_version differs).

For 64/32 systems once I will have the userspace wrapper ready I will 
add another ioctl(as discussed) that will check if the driver is 64bit 
ready(among the first things to do on binder_open).

Please let me know if there is anything that skipped my review and you 
would like to integrate in this patch set.

Thanks for your feedback and help,
Serban

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

[Serban Constantinescu]

Hi Kirill,

On 30/04/13 08:31, Kirill A. Shutemov wrote:
> On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
>> 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.
>
> I've tried to replace ugly hack with your patches to be able to boot
> 32-bit userspace on x86-64. It fails. :(
>

This patch set fixes the binder interface so that it works on 32/32 or 
64/64 systems and preps the driver for the compat layer - 64bit 
kernel/32bit userspace. As discussed with the Android developers the 
binder compat will live in the userspace. Thus for a 64bit kernel the 
calls coming from 32 or 64bit file-systems will look alike.

Thus for the moment it won't work.

Thanks,
Serban

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

[Kirill A. Shutemov]

On Tue, Apr 30, 2013 at 09:52:34AM +0100, Serban Constantinescu wrote:
> Hi Kirill,
> 
> On 30/04/13 08:31, Kirill A. Shutemov wrote:
> >On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
> >>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.
> >
> >I've tried to replace ugly hack with your patches to be able to boot
> >32-bit userspace on x86-64. It fails. :(
> >
> 
> This patch set fixes the binder interface so that it works on 32/32
> or 64/64 systems and preps the driver for the compat layer - 64bit
> kernel/32bit userspace. As discussed with the Android developers the
> binder compat will live in the userspace.

Does the compat layer already exist or it's a plan?

-- 
 Kirill A. Shutemov

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

[Serban Constantinescu]

On 30/04/13 11:04, Kirill A. Shutemov wrote:
> On Tue, Apr 30, 2013 at 09:52:34AM +0100, Serban Constantinescu wrote:
>> Hi Kirill,
>>
>> On 30/04/13 08:31, Kirill A. Shutemov wrote:
>>> On Fri, Apr 12, 2013 at 01:25:49PM +0100, Serban Constantinescu wrote:
>>>> 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.
>>>
>>> I've tried to replace ugly hack with your patches to be able to boot
>>> 32-bit userspace on x86-64. It fails. :(
>>>
>>
>> This patch set fixes the binder interface so that it works on 32/32
>> or 64/64 systems and preps the driver for the compat layer - 64bit
>> kernel/32bit userspace. As discussed with the Android developers the
>> binder compat will live in the userspace.
>
> Does the compat layer already exist or it's a plan?
We were working with a kernel compat layer. However this wasn't 
encouraged by the Android developers who would prefer a userspace binder 
compat.

The changes in this patch set fix the kernel interface so that it will 
work with the yet to be developed userspace wrapper.

Some support for 64/32 systems has already been integrated - see ashmem.

Thanks,
Serban

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

[Arve Hjønnevåg]

On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> Hi Arve,
>
>
> On 30/04/13 00:13, Arve Hjønnevåg wrote:
>>
>> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
>> <Serban.Constantinescu@arm.com> wrote:
>>>
>>> Hi all,
>>>
>>> Any feedback or comments on this patch set?
>>>
>>
>> You don't seem to have addressed my feedback on the previous patch set.
>
>
> For v3 I have modified the following according to your review:
>
>
>> 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
>
>
> The following were left out:
>
>> On 11/04/13 22:40, Arve Hjønnevåg wrote:
>> 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?
>
>
> There is no kernel macro that I know which will help here(one that springs
> to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that
> aligns to (u32)). Any ideas?
>

Perhaps using __alignof__(struct flat_binder_object) will work. This
is the least important part of that change however. I saw no response
to my concern that your changes can cause less memory to be allocated
than you write to.

>> On 11/04/13 21:38, Arve Hjønnevåg wrote:
>> 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.
>
>
> For 32bit systems nothing has changed so they will continue to work as
> before. For 64bit systems the size of binder_version was signed long before
> the patch and __s32 after the patch is applied. Thus a 64bit system using
> the old interface will fail immediately after opening the binder driver,
> while cheeking the binder version (since the BINDER_VERSION ioctl will be
> different pre/post patch - size of binder_version differs).
>
> For 64/32 systems once I will have the userspace wrapper ready I will add
> another ioctl(as discussed) that will check if the driver is 64bit
> ready(among the first things to do on binder_open).
>

Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
the driver is usable on a 64 bit system?

> Please let me know if there is anything that skipped my review and you would
> like to integrate in this patch set.
>

It may be better to reply to my original emails instead of copying
bits of them here.

> Thanks for your feedback and help,
> Serban
>

--
Arve Hjønnevåg

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

[Serban Constantinescu]

On 01/05/13 00:52, Arve Hjønnevåg wrote:
> On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> Hi Arve,
>>
>>
>> On 30/04/13 00:13, Arve Hjønnevåg wrote:
>>>
>>> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
>>> <Serban.Constantinescu@arm.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Any feedback or comments on this patch set?
>>>>
>>>
>>> You don't seem to have addressed my feedback on the previous patch set.
>>
>>
>> For v3 I have modified the following according to your review:
>>
>>
>>> 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
>>
>>
>> The following were left out:
>>
>>> On 11/04/13 22:40, Arve Hjønnevåg wrote:
>>> 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?
>>
>>
>> There is no kernel macro that I know which will help here(one that springs
>> to mind is PTR_ALIGN but it aligns to (unsigned long) - we need one that
>> aligns to (u32)). Any ideas?
>>
>
> Perhaps using __alignof__(struct flat_binder_object) will work. This
> is the least important part of that change however. I saw no response
> to my concern that your changes can cause less memory to be allocated
> than you write to.

This can happen for situations where (buffer_start + buffer_size) are 
not aligned to (void *), because offset_start is calculated as:

> offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));

Thus you can have a situation where instead of reading offset[i] you 
will read (offset[i] >> 32 | offset[i+1] << 32) (offset is size_t - 
8byte for 64bit systems).

I will address this issue in v4 of the patch set.

>
>>> On 11/04/13 21:38, Arve Hjønnevåg wrote:
>>> 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.
>>
>>
>> For 32bit systems nothing has changed so they will continue to work as
>> before. For 64bit systems the size of binder_version was signed long before
>> the patch and __s32 after the patch is applied. Thus a 64bit system using
>> the old interface will fail immediately after opening the binder driver,
>> while cheeking the binder version (since the BINDER_VERSION ioctl will be
>> different pre/post patch - size of binder_version differs).
>>
>> For 64/32 systems once I will have the userspace wrapper ready I will add
>> another ioctl(as discussed) that will check if the driver is 64bit
>> ready(among the first things to do on binder_open).
>>
>
> Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
> the driver is usable on a 64 bit system?

Leaving the binder_version as a long will cause the BINDER_VERSION ioctl 
to fail just on 64/32 - since the size will be different between 32bit 
compilers and 64bit compilers. The call will succeed on 64/64 and 32/32 
(since they use the same kernel headers).

>> Please let me know if there is anything that skipped my review and you would
>> like to integrate in this patch set.
>>
>
> It may be better to reply to my original emails instead of copying
> bits of them here.

I will do that! I did not understand your initial reply to the buffer 
size issue, my fault!


Thanks for your feedback,
Serban

-- 
Best Regards,

Serban Constantinescu
PDSW Engineer ARM Ltd.