[PATCH v1 0/9] Android: Add Support for Binder Compat

[PATCH v1 0/9] Android: Add Support for Binder Compat

[Serban Constantinescu]

Hi all,

The patches attached add support for 32bit userspace running on 64bit
kernels. This is the last series of patches needed for basic 32bit Android
bring-up on 64bit kernels.

The series is split into refactoring the binder driver and the addition
of the compat layer. Please note that the helper macros are extended
with compat handling by the last patch of the series.

The patches have been successfully tested on 32 and 64bit platforms.

Thanks for your help,
Best regards,
Serban Constantinescu

Serban Constantinescu (9):
  staging: android: binder: Move some of the logic into subfunction
  staging: android: binder: Add binder_copy_to_user()
  staging: android: binder: Add cmd == CMD_NAME handling
  staging: android: binder: Add align_helper() macro
  staging: android: binder: Add deref_helper() macro
  staging: android: binder: Add size_helper() macro
  staging: android: binder: Add copy_flat_binder_object()
  staging: android: binder: Add binder compat handling to binder.h
  staging: android: binder: Add binder compat layer

 drivers/staging/android/binder.c |  846 ++++++++++++++++++++++++++++----------
 drivers/staging/android/binder.h |  109 +++++
 2 files changed, 732 insertions(+), 223 deletions(-)

-- 
1.7.9.5

[PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Serban Constantinescu]

This patch moves some of the logic for binder_thread_write() into
subfunctions. This way we can share more code with the binder compat
layer.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index eaec1da..233889c 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1700,6 +1700,217 @@ err_no_context_mgr_node:
 		thread->return_error = return_error;
 }
 
+static void bc_increfs_done(struct binder_proc *proc,
+		struct binder_thread *thread, uint32_t cmd,
+		void __user *node_ptr, void __user *cookie)
+{
+	struct binder_node *node;
+
+	node = binder_get_node(proc, node_ptr);
+	if (node == NULL) {
+		binder_user_error("%d:%d %s u%p no match\n",
+			proc->pid, thread->pid,
+			cmd == BC_INCREFS_DONE ?
+			"BC_INCREFS_DONE" :
+			"BC_ACQUIRE_DONE",
+			node_ptr);
+		return;
+	}
+	if (cookie != node->cookie) {
+		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
+			proc->pid, thread->pid,
+			cmd == BC_INCREFS_DONE ?
+			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+			node_ptr, node->debug_id,
+			cookie, node->cookie);
+		return;
+	}
+	if (cmd == BC_ACQUIRE_DONE) {
+		if (node->pending_strong_ref == 0) {
+			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
+				proc->pid, thread->pid,
+				node->debug_id);
+			return;
+		}
+		node->pending_strong_ref = 0;
+	} else {
+		if (node->pending_weak_ref == 0) {
+			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
+				proc->pid, thread->pid,
+				node->debug_id);
+			return;
+		}
+		node->pending_weak_ref = 0;
+	}
+	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
+	binder_debug(BINDER_DEBUG_USER_REFS,
+		     "%d:%d %s node %d ls %d lw %d\n",
+		     proc->pid, thread->pid,
+		     cmd == BC_INCREFS_DONE ?
+		     "BC_INCREFS_DONE" :
+		     "BC_ACQUIRE_DONE",
+		     node->debug_id, node->local_strong_refs,
+		     node->local_weak_refs);
+	return;
+}
+
+static void bc_free_buffer(struct binder_proc *proc,
+		    struct binder_thread *thread, void __user *data_ptr)
+{
+	struct binder_buffer *buffer;
+
+	buffer = binder_buffer_lookup(proc, data_ptr);
+	if (buffer == NULL) {
+		binder_user_error("%d:%d BC_FREE_BUFFER u%p no match\n",
+			proc->pid, thread->pid, data_ptr);
+		return;
+	}
+	if (!buffer->allow_user_free) {
+		binder_user_error("%d:%d BC_FREE_BUFFER u%p matched unreturned buffer\n",
+			proc->pid, thread->pid, data_ptr);
+		return;
+	}
+	binder_debug(BINDER_DEBUG_FREE_BUFFER,
+		     "%d:%d BC_FREE_BUFFER u%p found buffer %d for %s transaction\n",
+		     proc->pid, thread->pid, data_ptr, buffer->debug_id,
+		     buffer->transaction ? "active" : "finished");
+
+	if (buffer->transaction) {
+		buffer->transaction->buffer = NULL;
+		buffer->transaction = NULL;
+	}
+	if (buffer->async_transaction && buffer->target_node) {
+		BUG_ON(!buffer->target_node->has_async_transaction);
+		if (list_empty(&buffer->target_node->async_todo))
+			buffer->target_node->has_async_transaction = 0;
+		else
+			list_move_tail(buffer->target_node->async_todo.next, &thread->todo);
+	}
+	trace_binder_transaction_buffer_release(buffer);
+	binder_transaction_buffer_release(proc, buffer, NULL);
+	binder_free_buf(proc, buffer);
+	return;
+}
+
+static void bc_clear_death_notif(struct binder_proc *proc,
+		    struct binder_thread *thread, uint32_t cmd,
+		    uint32_t target, void __user *cookie)
+{
+	struct binder_ref *ref;
+	struct binder_ref_death *death;
+
+	ref = binder_get_ref(proc, target);
+	if (ref == NULL) {
+		binder_user_error("%d:%d %s invalid ref %d\n",
+			proc->pid, thread->pid,
+			cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+			"BC_REQUEST_DEATH_NOTIFICATION" :
+			"BC_CLEAR_DEATH_NOTIFICATION",
+			target);
+		return;
+	}
+
+	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
+		     "%d:%d %s %p ref %d desc %d s %d w %d for node %d\n",
+		     proc->pid, thread->pid,
+		     cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+		     "BC_REQUEST_DEATH_NOTIFICATION" :
+		     "BC_CLEAR_DEATH_NOTIFICATION",
+		     cookie, ref->debug_id, ref->desc,
+		     ref->strong, ref->weak, ref->node->debug_id);
+
+	if (cmd == BC_REQUEST_DEATH_NOTIFICATION) {
+		if (ref->death) {
+			binder_user_error("%d:%d BC_REQUEST_DEATH_NOTIFICATION death notification already set\n",
+				proc->pid, thread->pid);
+			return;
+		}
+		death = kzalloc(sizeof(*death), GFP_KERNEL);
+		if (death == NULL) {
+			thread->return_error = BR_ERROR;
+			binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+				     "%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
+				     proc->pid, thread->pid);
+			return;
+		}
+		binder_stats_created(BINDER_STAT_DEATH);
+		INIT_LIST_HEAD(&death->work.entry);
+		death->cookie = cookie;
+		ref->death = death;
+		if (ref->node->proc == NULL) {
+			ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+			if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
+				list_add_tail(&ref->death->work.entry, &thread->todo);
+			} else {
+				list_add_tail(&ref->death->work.entry, &proc->todo);
+				wake_up_interruptible(&proc->wait);
+			}
+		}
+	} else {
+		if (ref->death == NULL) {
+			binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification not active\n",
+				proc->pid, thread->pid);
+			return;
+		}
+		death = ref->death;
+		if (death->cookie != cookie) {
+			binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification cookie mismatch %p != %p\n",
+				proc->pid, thread->pid,
+				death->cookie, cookie);
+			return;
+		}
+		ref->death = NULL;
+		if (list_empty(&death->work.entry)) {
+			death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
+			if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
+				list_add_tail(&death->work.entry, &thread->todo);
+			} else {
+				list_add_tail(&death->work.entry, &proc->todo);
+				wake_up_interruptible(&proc->wait);
+			}
+		} else {
+			BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
+			death->work.type = BINDER_WORK_DEAD_BINDER_AND_CLEAR;
+		}
+	}
+	return;
+}
+
+static void bc_dead_binder_done(struct binder_proc *proc,
+		    struct binder_thread *thread, void __user *cookie)
+{
+	struct binder_work *w;
+	struct binder_ref_death *death = NULL;
+
+	list_for_each_entry(w, &proc->delivered_death, entry) {
+		struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);
+		if (tmp_death->cookie == cookie) {
+			death = tmp_death;
+			return;
+		}
+	}
+	binder_debug(BINDER_DEBUG_DEAD_BINDER,
+		     "%d:%d BC_DEAD_BINDER_DONE %p found %p\n",
+		     proc->pid, thread->pid, cookie, death);
+	if (death == NULL) {
+		binder_user_error("%d:%d BC_DEAD_BINDER_DONE %p not found\n",
+			proc->pid, thread->pid, cookie);
+		return;
+	}
+
+	list_del_init(&death->work.entry);
+	if (death->work.type == BINDER_WORK_DEAD_BINDER_AND_CLEAR) {
+		death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
+		if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
+			list_add_tail(&death->work.entry, &thread->todo);
+		} else {
+			list_add_tail(&death->work.entry, &proc->todo);
+			wake_up_interruptible(&proc->wait);
+		}
+	}
+	return;
+}
+
 static int binder_thread_write(struct binder_proc *proc,
 			struct binder_thread *thread,
 			void __user *buffer, size_t size, size_t *consumed)
@@ -1775,7 +1986,6 @@ static int binder_thread_write(struct binder_proc *proc,
 		case BC_ACQUIRE_DONE: {
 			void __user *node_ptr;
 			void __user *cookie;
-			struct binder_node *node;
 
 			if (get_user(node_ptr, (void * __user *)ptr))
 				return -EFAULT;
@@ -1783,48 +1993,7 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (void * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
-			node = binder_get_node(proc, node_ptr);
-			if (node == NULL) {
-				binder_user_error("%d:%d %s u%p no match\n",
-					proc->pid, thread->pid,
-					cmd == BC_INCREFS_DONE ?
-					"BC_INCREFS_DONE" :
-					"BC_ACQUIRE_DONE",
-					node_ptr);
-				break;
-			}
-			if (cookie != node->cookie) {
-				binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
-					proc->pid, thread->pid,
-					cmd == BC_INCREFS_DONE ?
-					"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
-					node_ptr, node->debug_id,
-					cookie, node->cookie);
-				break;
-			}
-			if (cmd == BC_ACQUIRE_DONE) {
-				if (node->pending_strong_ref == 0) {
-					binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
-						proc->pid, thread->pid,
-						node->debug_id);
-					break;
-				}
-				node->pending_strong_ref = 0;
-			} else {
-				if (node->pending_weak_ref == 0) {
-					binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
-						proc->pid, thread->pid,
-						node->debug_id);
-					break;
-				}
-				node->pending_weak_ref = 0;
-			}
-			binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
-			binder_debug(BINDER_DEBUG_USER_REFS,
-				     "%d:%d %s node %d ls %d lw %d\n",
-				     proc->pid, thread->pid,
-				     cmd == BC_INCREFS_DONE ? "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
-				     node->debug_id, node->local_strong_refs, node->local_weak_refs);
+			bc_increfs_done(proc, thread, cmd, node_ptr, cookie);
 			break;
 		}
 		case BC_ATTEMPT_ACQUIRE:
@@ -1836,42 +2005,11 @@ static int binder_thread_write(struct binder_proc *proc,
 
 		case BC_FREE_BUFFER: {
 			void __user *data_ptr;
-			struct binder_buffer *buffer;
 
 			if (get_user(data_ptr, (void * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
-
-			buffer = binder_buffer_lookup(proc, data_ptr);
-			if (buffer == NULL) {
-				binder_user_error("%d:%d BC_FREE_BUFFER u%p no match\n",
-					proc->pid, thread->pid, data_ptr);
-				break;
-			}
-			if (!buffer->allow_user_free) {
-				binder_user_error("%d:%d BC_FREE_BUFFER u%p matched unreturned buffer\n",
-					proc->pid, thread->pid, data_ptr);
-				break;
-			}
-			binder_debug(BINDER_DEBUG_FREE_BUFFER,
-				     "%d:%d BC_FREE_BUFFER u%p found buffer %d for %s transaction\n",
-				     proc->pid, thread->pid, data_ptr, buffer->debug_id,
-				     buffer->transaction ? "active" : "finished");
-
-			if (buffer->transaction) {
-				buffer->transaction->buffer = NULL;
-				buffer->transaction = NULL;
-			}
-			if (buffer->async_transaction && buffer->target_node) {
-				BUG_ON(!buffer->target_node->has_async_transaction);
-				if (list_empty(&buffer->target_node->async_todo))
-					buffer->target_node->has_async_transaction = 0;
-				else
-					list_move_tail(buffer->target_node->async_todo.next, &thread->todo);
-			}
-			trace_binder_transaction_buffer_release(buffer);
-			binder_transaction_buffer_release(proc, buffer, NULL);
-			binder_free_buf(proc, buffer);
+			bc_free_buffer(proc, thread, data_ptr);
 			break;
 		}
 
@@ -1926,8 +2064,6 @@ static int binder_thread_write(struct binder_proc *proc,
 		case BC_CLEAR_DEATH_NOTIFICATION: {
 			uint32_t target;
 			void __user *cookie;
-			struct binder_ref *ref;
-			struct binder_ref_death *death;
 
 			if (get_user(target, (uint32_t __user *)ptr))
 				return -EFAULT;
@@ -1935,117 +2071,18 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (void __user * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
-			ref = binder_get_ref(proc, target);
-			if (ref == NULL) {
-				binder_user_error("%d:%d %s invalid ref %d\n",
-					proc->pid, thread->pid,
-					cmd == BC_REQUEST_DEATH_NOTIFICATION ?
-					"BC_REQUEST_DEATH_NOTIFICATION" :
-					"BC_CLEAR_DEATH_NOTIFICATION",
-					target);
-				break;
-			}
-
-			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
-				     "%d:%d %s %p ref %d desc %d s %d w %d for node %d\n",
-				     proc->pid, thread->pid,
-				     cmd == BC_REQUEST_DEATH_NOTIFICATION ?
-				     "BC_REQUEST_DEATH_NOTIFICATION" :
-				     "BC_CLEAR_DEATH_NOTIFICATION",
-				     cookie, ref->debug_id, ref->desc,
-				     ref->strong, ref->weak, ref->node->debug_id);
-
-			if (cmd == BC_REQUEST_DEATH_NOTIFICATION) {
-				if (ref->death) {
-					binder_user_error("%d:%d BC_REQUEST_DEATH_NOTIFICATION death notification already set\n",
-						proc->pid, thread->pid);
-					break;
-				}
-				death = kzalloc(sizeof(*death), GFP_KERNEL);
-				if (death == NULL) {
-					thread->return_error = BR_ERROR;
-					binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
-						     "%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
-						     proc->pid, thread->pid);
-					break;
-				}
-				binder_stats_created(BINDER_STAT_DEATH);
-				INIT_LIST_HEAD(&death->work.entry);
-				death->cookie = cookie;
-				ref->death = death;
-				if (ref->node->proc == NULL) {
-					ref->death->work.type = BINDER_WORK_DEAD_BINDER;
-					if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
-						list_add_tail(&ref->death->work.entry, &thread->todo);
-					} else {
-						list_add_tail(&ref->death->work.entry, &proc->todo);
-						wake_up_interruptible(&proc->wait);
-					}
-				}
-			} else {
-				if (ref->death == NULL) {
-					binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification not active\n",
-						proc->pid, thread->pid);
-					break;
-				}
-				death = ref->death;
-				if (death->cookie != cookie) {
-					binder_user_error("%d:%d BC_CLEAR_DEATH_NOTIFICATION death notification cookie mismatch %p != %p\n",
-						proc->pid, thread->pid,
-						death->cookie, cookie);
-					break;
-				}
-				ref->death = NULL;
-				if (list_empty(&death->work.entry)) {
-					death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
-					if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
-						list_add_tail(&death->work.entry, &thread->todo);
-					} else {
-						list_add_tail(&death->work.entry, &proc->todo);
-						wake_up_interruptible(&proc->wait);
-					}
-				} else {
-					BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
-					death->work.type = BINDER_WORK_DEAD_BINDER_AND_CLEAR;
-				}
-			}
-		} break;
+			bc_clear_death_notif(proc, thread, cmd, target, cookie);
+			break;
+		}
 		case BC_DEAD_BINDER_DONE: {
-			struct binder_work *w;
 			void __user *cookie;
-			struct binder_ref_death *death = NULL;
+
 			if (get_user(cookie, (void __user * __user *)ptr))
 				return -EFAULT;
-
 			ptr += sizeof(void *);
-			list_for_each_entry(w, &proc->delivered_death, entry) {
-				struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);
-				if (tmp_death->cookie == cookie) {
-					death = tmp_death;
-					break;
-				}
-			}
-			binder_debug(BINDER_DEBUG_DEAD_BINDER,
-				     "%d:%d BC_DEAD_BINDER_DONE %p found %p\n",
-				     proc->pid, thread->pid, cookie, death);
-			if (death == NULL) {
-				binder_user_error("%d:%d BC_DEAD_BINDER_DONE %p not found\n",
-					proc->pid, thread->pid, cookie);
-				break;
-			}
-
-			list_del_init(&death->work.entry);
-			if (death->work.type == BINDER_WORK_DEAD_BINDER_AND_CLEAR) {
-				death->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
-				if (thread->looper & (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
-					list_add_tail(&death->work.entry, &thread->todo);
-				} else {
-					list_add_tail(&death->work.entry, &proc->todo);
-					wake_up_interruptible(&proc->wait);
-				}
-			}
-		} break;
-
+			bc_dead_binder_done(proc, thread, cookie);
+			break;
+		}
 		default:
 			pr_err("%d:%d unknown command %d\n",
 			       proc->pid, thread->pid, cmd);
-- 
1.7.9.5

Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Dan Carpenter]

On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
> +static void bc_dead_binder_done(struct binder_proc *proc,
> +		    struct binder_thread *thread, void __user *cookie)
> +{
> +	struct binder_work *w;
> +	struct binder_ref_death *death = NULL;
> +
> +	list_for_each_entry(w, &proc->delivered_death, entry) {
> +		struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);

Put a blank line after the variable declaration block.  Also break it up
into two lines instead of having the lines be a million characters long.

	list_for_each_entry(w, &proc->delivered_death, entry) {
		struct binder_ref_death *tmp_death;

		tmp_death = container_of(w, struct binder_ref_death, work);

> +		if (tmp_death->cookie == cookie) {
> +			death = tmp_death;
> +			return;

Should be a break here.  This function wasn't tested.

> +		}
> +	}

regards,
dan carpenter

Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Serban Constantinescu]

On 05/12/13 08:00, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
>> +static void bc_dead_binder_done(struct binder_proc *proc,
>> +		    struct binder_thread *thread, void __user *cookie)
>> +{
>> +	struct binder_work *w;
>> +	struct binder_ref_death *death = NULL;
>> +
>> +	list_for_each_entry(w, &proc->delivered_death, entry) {
>> +		struct binder_ref_death *tmp_death = container_of(w, struct binder_ref_death, work);
>
> Put a blank line after the variable declaration block.  Also break it up
> into two lines instead of having the lines be a million characters long.

I will do that, I have followed the same conventions used by the code 
allready there.

>
> 	list_for_each_entry(w, &proc->delivered_death, entry) {
> 		struct binder_ref_death *tmp_death;
>
> 		tmp_death = container_of(w, struct binder_ref_death, work);
>
>> +		if (tmp_death->cookie == cookie) {
>> +			death = tmp_death;
>> +			return;
>
> Should be a break here.  This function wasn't tested.

There should be a break there! I have tested the driver with 32bit 
Android tests, none of which seem to hit this.

Thanks for your review,
Serban C

Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Dan Carpenter]

On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
> +static void bc_increfs_done(struct binder_proc *proc,
> +		struct binder_thread *thread, uint32_t cmd,
> +		void __user *node_ptr, void __user *cookie)
> +{
> +	struct binder_node *node;
> +
> +	node = binder_get_node(proc, node_ptr);
> +	if (node == NULL) {
> +		binder_user_error("%d:%d %s u%p no match\n",
> +			proc->pid, thread->pid,
> +			cmd == BC_INCREFS_DONE ?
> +			"BC_INCREFS_DONE" :
> +			"BC_ACQUIRE_DONE",
> +			node_ptr);
> +		return;
> +	}
> +	if (cookie != node->cookie) {
> +		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
> +			proc->pid, thread->pid,
> +			cmd == BC_INCREFS_DONE ?
> +			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
> +			node_ptr, node->debug_id,
> +			cookie, node->cookie);
> +		return;
> +	}
> +	if (cmd == BC_ACQUIRE_DONE) {
> +		if (node->pending_strong_ref == 0) {
> +			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
> +				proc->pid, thread->pid,
> +				node->debug_id);
> +			return;
> +		}
> +		node->pending_strong_ref = 0;
> +	} else {
> +		if (node->pending_weak_ref == 0) {
> +			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
> +				proc->pid, thread->pid,
> +				node->debug_id);
> +			return;
> +		}
> +		node->pending_weak_ref = 0;
> +	}
> +	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
> +	binder_debug(BINDER_DEBUG_USER_REFS,
> +		     "%d:%d %s node %d ls %d lw %d\n",
> +		     proc->pid, thread->pid,
> +		     cmd == BC_INCREFS_DONE ?
> +		     "BC_INCREFS_DONE" :
> +		     "BC_ACQUIRE_DONE",
> +		     node->debug_id, node->local_strong_refs,
> +		     node->local_weak_refs);
> +	return;
> +}

This function is 52 lines long but at least 32 of those lines are
debug code.

Just extra of lines of code means you have increased the number of bugs
150%.  But what I know is that from a static analysis perspective, debug
code has more defects per line then regular code it's worse than that.
Plus all the extra noise obscures the actual logic and makes the real
code annoying to look at so it's worse still.

If you want to move this stuff out of staging, then remove all the extra
crap so it doesn't look like barf.

regards,
dan carpenter

Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Greg KH]

On Thu, Dec 05, 2013 at 11:18:22AM +0300, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
> > +static void bc_increfs_done(struct binder_proc *proc,
> > +		struct binder_thread *thread, uint32_t cmd,
> > +		void __user *node_ptr, void __user *cookie)
> > +{
> > +	struct binder_node *node;
> > +
> > +	node = binder_get_node(proc, node_ptr);
> > +	if (node == NULL) {
> > +		binder_user_error("%d:%d %s u%p no match\n",
> > +			proc->pid, thread->pid,
> > +			cmd == BC_INCREFS_DONE ?
> > +			"BC_INCREFS_DONE" :
> > +			"BC_ACQUIRE_DONE",
> > +			node_ptr);
> > +		return;
> > +	}
> > +	if (cookie != node->cookie) {
> > +		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
> > +			proc->pid, thread->pid,
> > +			cmd == BC_INCREFS_DONE ?
> > +			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
> > +			node_ptr, node->debug_id,
> > +			cookie, node->cookie);
> > +		return;
> > +	}
> > +	if (cmd == BC_ACQUIRE_DONE) {
> > +		if (node->pending_strong_ref == 0) {
> > +			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
> > +				proc->pid, thread->pid,
> > +				node->debug_id);
> > +			return;
> > +		}
> > +		node->pending_strong_ref = 0;
> > +	} else {
> > +		if (node->pending_weak_ref == 0) {
> > +			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
> > +				proc->pid, thread->pid,
> > +				node->debug_id);
> > +			return;
> > +		}
> > +		node->pending_weak_ref = 0;
> > +	}
> > +	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
> > +	binder_debug(BINDER_DEBUG_USER_REFS,
> > +		     "%d:%d %s node %d ls %d lw %d\n",
> > +		     proc->pid, thread->pid,
> > +		     cmd == BC_INCREFS_DONE ?
> > +		     "BC_INCREFS_DONE" :
> > +		     "BC_ACQUIRE_DONE",
> > +		     node->debug_id, node->local_strong_refs,
> > +		     node->local_weak_refs);
> > +	return;
> > +}
> 
> This function is 52 lines long but at least 32 of those lines are
> debug code.
> 
> Just extra of lines of code means you have increased the number of bugs
> 150%.  But what I know is that from a static analysis perspective, debug
> code has more defects per line then regular code it's worse than that.
> Plus all the extra noise obscures the actual logic and makes the real
> code annoying to look at so it's worse still.
> 
> If you want to move this stuff out of staging, then remove all the extra
> crap so it doesn't look like barf.

I don't ever think the binder code will be moved out of staging for a
variety of reasons, but that doesn't mean it shouldn't be cleaned up :)

thanks,

greg k-h

Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Serban Constantinescu]

On 05/12/13 08:18, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
>> +static void bc_increfs_done(struct binder_proc *proc,
>> +		struct binder_thread *thread, uint32_t cmd,
>> +		void __user *node_ptr, void __user *cookie)
>> +{
>> +	struct binder_node *node;
>> +
>> +	node = binder_get_node(proc, node_ptr);
>> +	if (node == NULL) {
>> +		binder_user_error("%d:%d %s u%p no match\n",
>> +			proc->pid, thread->pid,
>> +			cmd == BC_INCREFS_DONE ?
>> +			"BC_INCREFS_DONE" :
>> +			"BC_ACQUIRE_DONE",
>> +			node_ptr);
>> +		return;
>> +	}
>> +	if (cookie != node->cookie) {
>> +		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
>> +			proc->pid, thread->pid,
>> +			cmd == BC_INCREFS_DONE ?
>> +			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
>> +			node_ptr, node->debug_id,
>> +			cookie, node->cookie);
>> +		return;
>> +	}
>> +	if (cmd == BC_ACQUIRE_DONE) {
>> +		if (node->pending_strong_ref == 0) {
>> +			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
>> +				proc->pid, thread->pid,
>> +				node->debug_id);
>> +			return;
>> +		}
>> +		node->pending_strong_ref = 0;
>> +	} else {
>> +		if (node->pending_weak_ref == 0) {
>> +			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
>> +				proc->pid, thread->pid,
>> +				node->debug_id);
>> +			return;
>> +		}
>> +		node->pending_weak_ref = 0;
>> +	}
>> +	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
>> +	binder_debug(BINDER_DEBUG_USER_REFS,
>> +		     "%d:%d %s node %d ls %d lw %d\n",
>> +		     proc->pid, thread->pid,
>> +		     cmd == BC_INCREFS_DONE ?
>> +		     "BC_INCREFS_DONE" :
>> +		     "BC_ACQUIRE_DONE",
>> +		     node->debug_id, node->local_strong_refs,
>> +		     node->local_weak_refs);
>> +	return;
>> +}
>
> This function is 52 lines long but at least 32 of those lines are
> debug code.
>
> Just extra of lines of code means you have increased the number of bugs
> 150%.  But what I know is that from a static analysis perspective, debug
> code has more defects per line then regular code it's worse than that.
> Plus all the extra noise obscures the actual logic and makes the real
> code annoying to look at so it's worse still.
>
> If you want to move this stuff out of staging, then remove all the extra
> crap so it doesn't look like barf.

This patch moves code around so that some of it can be shared with the
compat layer. I agree that due to the abundance of debug code it is, at
times, hard to have a clear idea of what is actually happening and adds
extra obscurities to the logic.

Thanks,
Serban C

[PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()

[Serban Constantinescu]

This patch adds binder_copy_to_user() to be used for copying binder
commands to user address space. This way we can abstract away the
copy_to_user() calls and add separate handling for the compat layer.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 233889c..6fbb340 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread *thread)
 		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
 }
 
+static int binder_copy_to_user(uint32_t cmd, void *parcel,
+			       void __user **ptr, size_t size)
+{
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+	if (copy_to_user(*ptr, parcel, size))
+		return -EFAULT;
+	*ptr += size;
+	return 0;
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      void  __user *buffer, size_t size,
@@ -2263,15 +2275,12 @@ retry:
 				node->has_weak_ref = 0;
 			}
 			if (cmd != BR_NOOP) {
-				if (put_user(cmd, (uint32_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(uint32_t);
-				if (put_user(node->ptr, (void * __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(void *);
-				if (put_user(node->cookie, (void * __user *)ptr))
+				struct binder_ptr_cookie tmp;
+
+				tmp.ptr = node->ptr;
+				tmp.cookie = node->cookie;
+				if (binder_copy_to_user(cmd, &tmp, &ptr, sizeof(struct binder_ptr_cookie)))
 					return -EFAULT;
-				ptr += sizeof(void *);
 
 				binder_stat_br(proc, thread, cmd);
 				binder_debug(BINDER_DEBUG_USER_REFS,
@@ -2306,12 +2315,10 @@ retry:
 				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
 			else
 				cmd = BR_DEAD_BINDER;
-			if (put_user(cmd, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-			if (put_user(death->cookie, (void * __user *)ptr))
+
+			if (binder_copy_to_user(cmd, &death->cookie, &ptr, sizeof(void *)))
 				return -EFAULT;
-			ptr += sizeof(void *);
+
 			binder_stat_br(proc, thread, cmd);
 			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
 				     "%d:%d %s %p\n",
@@ -2373,12 +2380,8 @@ retry:
 					ALIGN(t->buffer->data_size,
 					    sizeof(void *));
 
-		if (put_user(cmd, (uint32_t __user *)ptr))
-			return -EFAULT;
-		ptr += sizeof(uint32_t);
-		if (copy_to_user(ptr, &tr, sizeof(tr)))
+		if (binder_copy_to_user(cmd, &tr, &ptr, sizeof(struct binder_transaction_data)))
 			return -EFAULT;
-		ptr += sizeof(tr);
 
 		trace_binder_transaction_received(t);
 		binder_stat_br(proc, thread, cmd);
-- 
1.7.9.5

Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()

[Greg KH]

On Wed, Dec 04, 2013 at 06:09:34PM +0000, Serban Constantinescu wrote:
> This patch adds binder_copy_to_user() to be used for copying binder
> commands to user address space. This way we can abstract away the
> copy_to_user() calls and add separate handling for the compat layer.
> 
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
>  drivers/staging/android/binder.c |   39 ++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 233889c..6fbb340 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread *thread)
>  		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
>  }
>  
> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
> +			       void __user **ptr, size_t size)
> +{
> +	if (put_user(cmd, (uint32_t __user *)*ptr))
> +		return -EFAULT;
> +	*ptr += sizeof(uint32_t);
> +	if (copy_to_user(*ptr, parcel, size))
> +		return -EFAULT;
> +	*ptr += size;
> +	return 0;
> +}

I know what you are trying to do here, but ick, why not just use the
structure involved in the copying out here?  Or just copy the thing out
in one "chunk", not two different calls, which should make this go
faster, right?


> +
>  static int binder_thread_read(struct binder_proc *proc,
>  			      struct binder_thread *thread,
>  			      void  __user *buffer, size_t size,
> @@ -2263,15 +2275,12 @@ retry:
>  				node->has_weak_ref = 0;
>  			}
>  			if (cmd != BR_NOOP) {
> -				if (put_user(cmd, (uint32_t __user *)ptr))
> -					return -EFAULT;
> -				ptr += sizeof(uint32_t);
> -				if (put_user(node->ptr, (void * __user *)ptr))
> -					return -EFAULT;
> -				ptr += sizeof(void *);
> -				if (put_user(node->cookie, (void * __user *)ptr))
> +				struct binder_ptr_cookie tmp;
> +
> +				tmp.ptr = node->ptr;
> +				tmp.cookie = node->cookie;
> +				if (binder_copy_to_user(cmd, &tmp, &ptr, sizeof(struct binder_ptr_cookie)))
>  					return -EFAULT;
> -				ptr += sizeof(void *);

Are you sure this is correct?  You are now no longer incrementing ptr
anymore, is that ok with the larger loop here?


>  
>  				binder_stat_br(proc, thread, cmd);
>  				binder_debug(BINDER_DEBUG_USER_REFS,
> @@ -2306,12 +2315,10 @@ retry:
>  				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
>  			else
>  				cmd = BR_DEAD_BINDER;
> -			if (put_user(cmd, (uint32_t __user *)ptr))
> -				return -EFAULT;
> -			ptr += sizeof(uint32_t);
> -			if (put_user(death->cookie, (void * __user *)ptr))
> +
> +			if (binder_copy_to_user(cmd, &death->cookie, &ptr, sizeof(void *)))
>  				return -EFAULT;
> -			ptr += sizeof(void *);
> +

Same here, no more ptr incrementing.


>  			binder_stat_br(proc, thread, cmd);
>  			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
>  				     "%d:%d %s %p\n",
> @@ -2373,12 +2380,8 @@ retry:
>  					ALIGN(t->buffer->data_size,
>  					    sizeof(void *));
>  
> -		if (put_user(cmd, (uint32_t __user *)ptr))
> -			return -EFAULT;
> -		ptr += sizeof(uint32_t);
> -		if (copy_to_user(ptr, &tr, sizeof(tr)))
> +		if (binder_copy_to_user(cmd, &tr, &ptr, sizeof(struct binder_transaction_data)))
>  			return -EFAULT;
> -		ptr += sizeof(tr);

And here, no more ptr incrementing.

thanks,

greg k-h

Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()

[Serban Constantinescu]

On 04/12/13 23:17, Greg KH wrote:
> On Wed, Dec 04, 2013 at 06:09:34PM +0000, Serban Constantinescu wrote:
>> This patch adds binder_copy_to_user() to be used for copying binder
>> commands to user address space. This way we can abstract away the
>> copy_to_user() calls and add separate handling for the compat layer.
>>
>> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
>> ---
>>   drivers/staging/android/binder.c |   39 ++++++++++++++++++++------------------
>>   1 file changed, 21 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>> index 233889c..6fbb340 100644
>> --- a/drivers/staging/android/binder.c
>> +++ b/drivers/staging/android/binder.c
>> @@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread *thread)
>>   		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
>>   }
>>
>> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
>> +			       void __user **ptr, size_t size)
>> +{
>> +	if (put_user(cmd, (uint32_t __user *)*ptr))
>> +		return -EFAULT;
>> +	*ptr += sizeof(uint32_t);
>> +	if (copy_to_user(*ptr, parcel, size))
>> +		return -EFAULT;
>> +	*ptr += size;
>> +	return 0;
>> +}
>
> I know what you are trying to do here, but ick, why not just use the
> structure involved in the copying out here?  Or just copy the thing out
> in one "chunk", not two different calls, which should make this go
> faster, right?

Ick... agree. I do this split here for the compat handling added in the
next patches, where the cmd will have to be converted to a 32bit compat
cmd and the size, parcel ,passed here will change to a 32bit compat
size, parcel.

This patch makes more sense when looking at the following snippet:

*<snippet from patch 9/9>*

> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
> +                              void __user **ptr, size_t size)
> +{
> +       if (!is_compat_task()) {
> +               if (put_user(cmd, (uint32_t __user *)*ptr))
> +                       return -EFAULT;
> +               *ptr += sizeof(uint32_t);
> +               if (copy_to_user(*ptr, parcel, size))
> +                       return -EFAULT;
> +               *ptr += size;
> +               return 0;
> +       }
> +       switch (cmd) {
> +       case BR_INCREFS:
> +       case BR_ACQUIRE:
> +       case BR_RELEASE:
> +       case BR_DECREFS: {
> +               struct binder_ptr_cookie *fp;
> +               struct compat_binder_ptr_cookie tmp;
> +
> +               cmd = compat_change_size(cmd, sizeof(tmp));

Passing the cmd, and the structure to be copied to binder_copy_to_user()
allows me to add this extra handling here. Where, first, cmd is changed
to a compat cmd, and then copied to userspace - i.e:

I change

> cmd = BR_INCREFS = _IOR('r', 7, struct binder_ptr_cookie)

to

> cmd = COMPAT_BR_INCREFS = _IOR('r', 7, struct compat_binder_ptr_cookie)

where

> struct binder_ptr_cookie {
> 	void* ptr;
> 	void* cookie;
> };

and

> struct compat_binder_ptr_cookie {
> 	compat_uptr_t ptr;
> 	compat_uptr_t cookie;
> };

Thus BR_INCREFS will be different between 32bit userspace and 64bit kernel.

> +               BUG_ON((cmd != COMPAT_BR_INCREFS) &&
> +                      (cmd != COMPAT_BR_ACQUIRE) &&
> +                      (cmd != COMPAT_BR_RELEASE) &&
> +                      (cmd != COMPAT_BR_DECREFS));
> +
> +               fp = (struct binder_ptr_cookie *) parcel;
> +               tmp.ptr = ptr_to_compat(fp->ptr);
> +               tmp.cookie = ptr_to_compat(fp->cookie);
> +               if (put_user(cmd, (uint32_t __user *)*ptr))
> +                       return -EFAULT;
> +               *ptr += sizeof(uint32_t);
> +               if (copy_to_user(*ptr, &tmp, sizeof(tmp)))
> +                       return -EFAULT;
> +               *ptr += sizeof(tmp);

Also, since the size of the parcel will differ when copied to a compat
task (32bit userspace) I increment the buffer ptr here, rather then
doing this in binder_thread_read().

This way we can safely move to the next cmd, by incrementing the buffer
ptr accordingly whether 32 or 64bit structures are needed.

*</sippet from patch 9/9>*


>> +
>>   static int binder_thread_read(struct binder_proc *proc,
>>   			      struct binder_thread *thread,
>>   			      void  __user *buffer, size_t size,
>> @@ -2263,15 +2275,12 @@ retry:
>>   				node->has_weak_ref = 0;
>>   			}
>>   			if (cmd != BR_NOOP) {
>> -				if (put_user(cmd, (uint32_t __user *)ptr))
>> -					return -EFAULT;
>> -				ptr += sizeof(uint32_t);
>> -				if (put_user(node->ptr, (void * __user *)ptr))
>> -					return -EFAULT;
>> -				ptr += sizeof(void *);
>> -				if (put_user(node->cookie, (void * __user *)ptr))
>> +				struct binder_ptr_cookie tmp;
>> +
>> +				tmp.ptr = node->ptr;
>> +				tmp.cookie = node->cookie;
>> +				if (binder_copy_to_user(cmd, &tmp, &ptr, sizeof(struct binder_ptr_cookie)))
>>   					return -EFAULT;
>> -				ptr += sizeof(void *);
>
> Are you sure this is correct?  You are now no longer incrementing ptr
> anymore, is that ok with the larger loop here?

Obscure, I should document this in the commit message! It is correct, I
increment the buffer in binder_copy_to_user() with a different size,
depending on whether the structures are copied to 32bit or 64bit
userspace. See above explanation for more detail.

>
>
>>
>>   				binder_stat_br(proc, thread, cmd);
>>   				binder_debug(BINDER_DEBUG_USER_REFS,
>> @@ -2306,12 +2315,10 @@ retry:
>>   				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
>>   			else
>>   				cmd = BR_DEAD_BINDER;
>> -			if (put_user(cmd, (uint32_t __user *)ptr))
>> -				return -EFAULT;
>> -			ptr += sizeof(uint32_t);
>> -			if (put_user(death->cookie, (void * __user *)ptr))
>> +
>> +			if (binder_copy_to_user(cmd, &death->cookie, &ptr, sizeof(void *)))
>>   				return -EFAULT;
>> -			ptr += sizeof(void *);
>> +
>
> Same here, no more ptr incrementing.

See above.
>
>
>>   			binder_stat_br(proc, thread, cmd);
>>   			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
>>   				     "%d:%d %s %p\n",
>> @@ -2373,12 +2380,8 @@ retry:
>>   					ALIGN(t->buffer->data_size,
>>   					    sizeof(void *));
>>
>> -		if (put_user(cmd, (uint32_t __user *)ptr))
>> -			return -EFAULT;
>> -		ptr += sizeof(uint32_t);
>> -		if (copy_to_user(ptr, &tr, sizeof(tr)))
>> +		if (binder_copy_to_user(cmd, &tr, &ptr, sizeof(struct binder_transaction_data)))
>>   			return -EFAULT;
>> -		ptr += sizeof(tr);
>
> And here, no more ptr incrementing.
See above.

Thanks for your feedback Greg,
Serban C

Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()

[Dan Carpenter]

On Wed, Dec 04, 2013 at 06:09:34PM +0000, Serban Constantinescu wrote:
> This patch adds binder_copy_to_user() to be used for copying binder
> commands to user address space. This way we can abstract away the
> copy_to_user() calls and add separate handling for the compat layer.
> 
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
>  drivers/staging/android/binder.c |   39 ++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 233889c..6fbb340 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread *thread)
>  		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
>  }
>  
> +static int binder_copy_to_user(uint32_t cmd, void *parcel,
> +			       void __user **ptr, size_t size)
> +{

Bike shedding:  Put the **ptr as the first argument.
binder_copy_to_user(dest, cmd, src, size);

> +	if (put_user(cmd, (uint32_t __user *)*ptr))
> +		return -EFAULT;
> +	*ptr += sizeof(uint32_t);
> +	if (copy_to_user(*ptr, parcel, size))
> +		return -EFAULT;
> +	*ptr += size;
> +	return 0;
> +}

regards,
dan carpenter

[PATCH v1 3/9] staging: android: binder: Add cmd == CMD_NAME handling

[Serban Constantinescu]

This patch modifies the functions that need to be passed the explicit
command to use a boolean flag. This way we can reuse the code for 64bit
compat commands.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 6fbb340..16109cd 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1701,7 +1701,7 @@ err_no_context_mgr_node:
 }
 
 static void bc_increfs_done(struct binder_proc *proc,
-		struct binder_thread *thread, uint32_t cmd,
+		struct binder_thread *thread, bool acquire,
 		void __user *node_ptr, void __user *cookie)
 {
 	struct binder_node *node;
@@ -1710,22 +1710,22 @@ static void bc_increfs_done(struct binder_proc *proc,
 	if (node == NULL) {
 		binder_user_error("%d:%d %s u%p no match\n",
 			proc->pid, thread->pid,
-			cmd == BC_INCREFS_DONE ?
-			"BC_INCREFS_DONE" :
-			"BC_ACQUIRE_DONE",
+			acquire ?
+			"BC_ACQUIRE_DONE" :
+			"BC_INCREFS_DONE",
 			node_ptr);
 		return;
 	}
 	if (cookie != node->cookie) {
 		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
 			proc->pid, thread->pid,
-			cmd == BC_INCREFS_DONE ?
-			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+			acquire ?
+			"BC_ACQUIRE_DONE" : "BC_INCREFS_DONE",
 			node_ptr, node->debug_id,
 			cookie, node->cookie);
 		return;
 	}
-	if (cmd == BC_ACQUIRE_DONE) {
+	if (acquire) {
 		if (node->pending_strong_ref == 0) {
 			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
 				proc->pid, thread->pid,
@@ -1742,13 +1742,13 @@ static void bc_increfs_done(struct binder_proc *proc,
 		}
 		node->pending_weak_ref = 0;
 	}
-	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
+	binder_dec_node(node, acquire, 0);
 	binder_debug(BINDER_DEBUG_USER_REFS,
 		     "%d:%d %s node %d ls %d lw %d\n",
 		     proc->pid, thread->pid,
-		     cmd == BC_INCREFS_DONE ?
-		     "BC_INCREFS_DONE" :
-		     "BC_ACQUIRE_DONE",
+		     acquire ?
+		     "BC_ACQUIRE_DONE" :
+		     "BC_INCREFS_DONE",
 		     node->debug_id, node->local_strong_refs,
 		     node->local_weak_refs);
 	return;
@@ -1793,7 +1793,7 @@ static void bc_free_buffer(struct binder_proc *proc,
 }
 
 static void bc_clear_death_notif(struct binder_proc *proc,
-		    struct binder_thread *thread, uint32_t cmd,
+		    struct binder_thread *thread, bool request,
 		    uint32_t target, void __user *cookie)
 {
 	struct binder_ref *ref;
@@ -1803,7 +1803,7 @@ static void bc_clear_death_notif(struct binder_proc *proc,
 	if (ref == NULL) {
 		binder_user_error("%d:%d %s invalid ref %d\n",
 			proc->pid, thread->pid,
-			cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+			request ?
 			"BC_REQUEST_DEATH_NOTIFICATION" :
 			"BC_CLEAR_DEATH_NOTIFICATION",
 			target);
@@ -1813,13 +1813,13 @@ static void bc_clear_death_notif(struct binder_proc *proc,
 	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
 		     "%d:%d %s %p ref %d desc %d s %d w %d for node %d\n",
 		     proc->pid, thread->pid,
-		     cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+		     request ?
 		     "BC_REQUEST_DEATH_NOTIFICATION" :
 		     "BC_CLEAR_DEATH_NOTIFICATION",
 		     cookie, ref->debug_id, ref->desc,
 		     ref->strong, ref->weak, ref->node->debug_id);
 
-	if (cmd == BC_REQUEST_DEATH_NOTIFICATION) {
+	if (request) {
 		if (ref->death) {
 			binder_user_error("%d:%d BC_REQUEST_DEATH_NOTIFICATION death notification already set\n",
 				proc->pid, thread->pid);
@@ -1993,7 +1993,7 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (void * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
-			bc_increfs_done(proc, thread, cmd, node_ptr, cookie);
+			bc_increfs_done(proc, thread, cmd == BC_ACQUIRE_DONE, node_ptr, cookie);
 			break;
 		}
 		case BC_ATTEMPT_ACQUIRE:
@@ -2071,7 +2071,8 @@ static int binder_thread_write(struct binder_proc *proc,
 			if (get_user(cookie, (void __user * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
-			bc_clear_death_notif(proc, thread, cmd, target, cookie);
+			bc_clear_death_notif(proc, thread, cmd == BC_REQUEST_DEATH_NOTIFICATION,
+					     target, cookie);
 			break;
 		}
 		case BC_DEAD_BINDER_DONE: {
-- 
1.7.9.5

Re: [PATCH v1 3/9] staging: android: binder: Add cmd == CMD_NAME handling

[Dan Carpenter]

On Wed, Dec 04, 2013 at 06:09:35PM +0000, Serban Constantinescu wrote:
> This patch modifies the functions that need to be passed the explicit
> command to use a boolean flag. This way we can reuse the code for 64bit
> compat commands.
> 

I don't understand this at all.  cmd seems like it should be 32 bits
on both arches.

regards,
dan carpenter

Re: [PATCH v1 3/9] staging: android: binder: Add cmd == CMD_NAME handling

[Serban Constantinescu]

On 05/12/13 08:40, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:35PM +0000, Serban Constantinescu wrote:
>> This patch modifies the functions that need to be passed the explicit
>> command to use a boolean flag. This way we can reuse the code for 64bit
>> compat commands.
>>
>
> I don't understand this at all.  cmd seems like it should be 32 bits
> on both arches.
Command is 32bit for both arches but does not have the same value. Here 
is what happens, this patch make more sense when looking at the compat 
layer.

The binder commands differ between 32bit userspace and 64bit kernels. E.g:

> BR_INCREFS = _IOR('r', 7, struct binder_ptr_cookie)

> COMPAT_BR_INCREFS = _IOR('r', 7, struct compat_binder_ptr_cookie)

> struct compat_binder_ptr_cookie {
>     compat_uptr_t ptr;
>     compat_uptr_t cookie;
> };

> struct binder_ptr_cookie {
>     void *ptr;
>     void *cookie;
> };

(the IOR() macro encodes the size of the transaction - struct 
binder_ptr_cookie).

This enables me to use the same handler for:

* native 64bit kernel/ 64bit userspace

>           case BC_INCREFS_DONE:
>         case BC_ACQUIRE_DONE: {
>                  void __user *node_ptr;
>                  void *cookie;
>
>                  if (get_user(node_ptr, (void * __user *)ptr))
>                        return -EFAULT;
>                  ptr += sizeof(void *);
>                  if (get_user(cookie, (void * __user *)ptr))
>                        return -EFAULT;
>                  ptr += sizeof(void *);
>                  bc_increfs_done(proc, thread, cmd == BC_ACQUIRE_DONE, node_ptr, cookie);
>                  break;
>           }

* compat 64bit kernel/ 32bit userspace.

> +       case COMPAT_BC_INCREFS_DONE:
> +       case COMPAT_BC_ACQUIRE_DONE: {
> +               compat_uptr_t node_ptr;
> +               compat_uptr_t cookie;
> +
> +               if (get_user(node_ptr, (compat_uptr_t __user *)*ptr))
> +                       return -EFAULT;
> +               *ptr += sizeof(compat_uptr_t);
> +               if (get_user(cookie, (compat_uptr_t __user *)*ptr))
> +                       return -EFAULT;
> +               *ptr += sizeof(compat_uptr_t);
> +               bc_increfs_done(proc, thread, cmd == COMPAT_BC_ACQUIRE_DONE,
> +                       compat_ptr(node_ptr), compat_ptr(cookie));
> +               break;

where the prototype for bc_increfs_done() has changed to:

>  static void bc_increfs_done(struct binder_proc *proc,
> -        struct binder_thread *thread, uint32_t cmd,
> +        struct binder_thread *thread, bool acquire,

Obsucre on its own, I should add more details in the commit message.

Thanks,
Serban C

[PATCH v1 4/9] staging: android: binder: Add align_helper() macro

[Serban Constantinescu]

This patch adds align_helper() macro that will be used for enforcing
the desired alignment on 64bit systems where the alignment will differ
depending on the userspace used (32bit /64bit).

This patch is a temporary patch that will be extended with 32bit compat
handling.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 16109cd..6719a53 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -140,6 +140,8 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
 			binder_stop_on_user_error = 2; \
 	} while (0)
 
+#define align_helper(ptr)	    ALIGN(ptr, sizeof(void *))
+
 enum binder_stat_types {
 	BINDER_STAT_PROC,
 	BINDER_STAT_THREAD,
@@ -1239,7 +1241,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	if (buffer->target_node)
 		binder_dec_node(buffer->target_node, 1, 0);
 
-	offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
+	offp = (size_t *)(buffer->data + align_helper(buffer->data_size));
 	if (failed_at)
 		off_end = failed_at;
 	else
@@ -1472,7 +1474,7 @@ static void binder_transaction(struct binder_proc *proc,
 	if (target_node)
 		binder_inc_node(target_node, 1, 0, NULL);
 
-	offp = (size_t *)(t->buffer->data + ALIGN(tr->data_size, sizeof(void *)));
+	offp = (size_t *)(t->buffer->data + align_helper(tr->data_size));
 
 	if (copy_from_user(t->buffer->data, tr->data.ptr.buffer, tr->data_size)) {
 		binder_user_error("%d:%d got transaction with invalid data ptr\n",
@@ -2378,8 +2380,7 @@ retry:
 		tr.data.ptr.buffer = (void *)t->buffer->data +
 					proc->user_buffer_offset;
 		tr.data.ptr.offsets = tr.data.ptr.buffer +
-					ALIGN(t->buffer->data_size,
-					    sizeof(void *));
+					align_helper(t->buffer->data_size);
 
 		if (binder_copy_to_user(cmd, &tr, &ptr, sizeof(struct binder_transaction_data)))
 			return -EFAULT;
-- 
1.7.9.5

Re: [PATCH v1 4/9] staging: android: binder: Add align_helper() macro

[Dan Carpenter]

On Wed, Dec 04, 2013 at 06:09:36PM +0000, Serban Constantinescu wrote:
> This patch adds align_helper() macro that will be used for enforcing
> the desired alignment on 64bit systems where the alignment will differ
> depending on the userspace used (32bit /64bit).
> 
> This patch is a temporary patch that will be extended with 32bit compat
> handling.
> 
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
>  drivers/staging/android/binder.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 16109cd..6719a53 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -140,6 +140,8 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
>  			binder_stop_on_user_error = 2; \
>  	} while (0)
>  
> +#define align_helper(ptr)	    ALIGN(ptr, sizeof(void *))
> +

This is a terrible name.

regards,
dan carpenter

[PATCH v1 5/9] staging: android: binder: Add deref_helper() macro

[Serban Constantinescu]

This patch adds a deref_helper() macro that will be used to dereference
the binder offsets on 64bit systems where the offset is either a 32bit
or a 64bit value, depending on the userpace used (32bit /64bit)

This patch is a temporary patch that will be extended with 32bit compat
handling.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 6719a53..95c2581 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -141,6 +141,7 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
 	} while (0)
 
 #define align_helper(ptr)	    ALIGN(ptr, sizeof(void *))
+#define deref_helper(ptr)	    (*(typeof(size_t *))ptr)
 
 enum binder_stat_types {
 	BINDER_STAT_PROC,
@@ -1230,7 +1231,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 					      struct binder_buffer *buffer,
 					      size_t *failed_at)
 {
-	size_t *offp, *off_end;
+	void *offp, *off_end;
 	int debug_id = buffer->debug_id;
 
 	binder_debug(BINDER_DEBUG_TRANSACTION,
@@ -1246,16 +1247,16 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		off_end = failed_at;
 	else
 		off_end = (void *)offp + buffer->offsets_size;
-	for (; offp < off_end; offp++) {
+	for (; offp < off_end; offp += sizeof(size_t)) {
 		struct flat_binder_object *fp;
-		if (*offp > buffer->data_size - sizeof(*fp) ||
+		if (deref_helper(offp) > buffer->data_size - sizeof(*fp) ||
 		    buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(u32))) {
+		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
 			pr_err("transaction release %d bad offset %zd, size %zd\n",
-			 debug_id, *offp, buffer->data_size);
+			 debug_id, deref_helper(offp), buffer->data_size);
 			continue;
 		}
-		fp = (struct flat_binder_object *)(buffer->data + *offp);
+		fp = (struct flat_binder_object *)(buffer->data + deref_helper(offp));
 		switch (fp->type) {
 		case BINDER_TYPE_BINDER:
 		case BINDER_TYPE_WEAK_BINDER: {
@@ -1305,7 +1306,7 @@ static void binder_transaction(struct binder_proc *proc,
 {
 	struct binder_transaction *t;
 	struct binder_work *tcomplete;
-	size_t *offp, *off_end;
+	void *offp, *off_end;
 	struct binder_proc *target_proc;
 	struct binder_thread *target_thread = NULL;
 	struct binder_node *target_node = NULL;
@@ -1495,17 +1496,17 @@ static void binder_transaction(struct binder_proc *proc,
 		goto err_bad_offset;
 	}
 	off_end = (void *)offp + tr->offsets_size;
-	for (; offp < off_end; offp++) {
+	for (; offp < off_end; offp += sizeof(size_t)) {
 		struct flat_binder_object *fp;
-		if (*offp > t->buffer->data_size - sizeof(*fp) ||
+		if (deref_helper(offp) > t->buffer->data_size - sizeof(*fp) ||
 		    t->buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(u32))) {
+		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
 			binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
-					proc->pid, thread->pid, *offp);
+					proc->pid, thread->pid, deref_helper(offp));
 			return_error = BR_FAILED_REPLY;
 			goto err_bad_offset;
 		}
-		fp = (struct flat_binder_object *)(t->buffer->data + *offp);
+		fp = (struct flat_binder_object *)(t->buffer->data + deref_helper(offp));
 		switch (fp->type) {
 		case BINDER_TYPE_BINDER:
 		case BINDER_TYPE_WEAK_BINDER: {
-- 
1.7.9.5

[PATCH v1 6/9] staging: android: binder: Add size_helper() macro

[Serban Constantinescu]

This patch adds size_helper() macro that will be used for indexing into
different buffers on 64bit systems where the size of particular
structures will differ depending on the userspace used (32bit /64bit).

This patch is a temporary patch that will be extended with 32bit compat
handling.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 95c2581..6d22717 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -142,6 +142,7 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
 
 #define align_helper(ptr)	    ALIGN(ptr, sizeof(void *))
 #define deref_helper(ptr)	    (*(typeof(size_t *))ptr)
+#define size_helper(x)		    sizeof(x)
 
 enum binder_stat_types {
 	BINDER_STAT_PROC,
@@ -1247,10 +1248,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		off_end = failed_at;
 	else
 		off_end = (void *)offp + buffer->offsets_size;
-	for (; offp < off_end; offp += sizeof(size_t)) {
+	for (; offp < off_end; offp += size_helper(size_t)) {
 		struct flat_binder_object *fp;
-		if (deref_helper(offp) > buffer->data_size - sizeof(*fp) ||
-		    buffer->data_size < sizeof(*fp) ||
+		if (deref_helper(offp) > buffer->data_size - size_helper(*fp) ||
+		    buffer->data_size < size_helper(*fp) ||
 		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
 			pr_err("transaction release %d bad offset %zd, size %zd\n",
 			 debug_id, deref_helper(offp), buffer->data_size);
@@ -1489,17 +1490,17 @@ static void binder_transaction(struct binder_proc *proc,
 		return_error = BR_FAILED_REPLY;
 		goto err_copy_data_failed;
 	}
-	if (!IS_ALIGNED(tr->offsets_size, sizeof(size_t))) {
+	if (!IS_ALIGNED(tr->offsets_size, size_helper(size_t))) {
 		binder_user_error("%d:%d got transaction with invalid offsets size, %zd\n",
 				proc->pid, thread->pid, tr->offsets_size);
 		return_error = BR_FAILED_REPLY;
 		goto err_bad_offset;
 	}
 	off_end = (void *)offp + tr->offsets_size;
-	for (; offp < off_end; offp += sizeof(size_t)) {
+	for (; offp < off_end; offp += size_helper(size_t)) {
 		struct flat_binder_object *fp;
-		if (deref_helper(offp) > t->buffer->data_size - sizeof(*fp) ||
-		    t->buffer->data_size < sizeof(*fp) ||
+		if (deref_helper(offp) > t->buffer->data_size - size_helper(*fp) ||
+		    t->buffer->data_size < size_helper(*fp) ||
 		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
 			binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
 					proc->pid, thread->pid, deref_helper(offp));
@@ -2229,7 +2230,7 @@ retry:
 			break;
 		}
 
-		if (end - ptr < sizeof(tr) + 4)
+		if (end - ptr < size_helper(tr) + 4)
 			break;
 
 		switch (w->type) {
-- 
1.7.9.5

[PATCH v1 7/9] staging: android: binder: Add copy_flat_binder_object()

[Serban Constantinescu]

This patch adds copy_flat_binder_object macro() that will help
dereference struct flat_binder_object on 64bit systems where the
structure differs between 32bit and 64bit userspace.

This patch is a temporary patch that will be extended with 32bit compat
handling.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 6d22717..855d348 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -144,6 +144,11 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
 #define deref_helper(ptr)	    (*(typeof(size_t *))ptr)
 #define size_helper(x)		    sizeof(x)
 
+static inline struct flat_binder_object *copy_flat_binder_object(void __user *ptr)
+{
+	return (struct flat_binder_object *)ptr;
+}
+
 enum binder_stat_types {
 	BINDER_STAT_PROC,
 	BINDER_STAT_THREAD,
@@ -1257,7 +1262,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 			 debug_id, deref_helper(offp), buffer->data_size);
 			continue;
 		}
-		fp = (struct flat_binder_object *)(buffer->data + deref_helper(offp));
+		fp = copy_flat_binder_object(buffer->data + deref_helper(offp));
 		switch (fp->type) {
 		case BINDER_TYPE_BINDER:
 		case BINDER_TYPE_WEAK_BINDER: {
@@ -1507,7 +1512,7 @@ static void binder_transaction(struct binder_proc *proc,
 			return_error = BR_FAILED_REPLY;
 			goto err_bad_offset;
 		}
-		fp = (struct flat_binder_object *)(t->buffer->data + deref_helper(offp));
+		fp = copy_flat_binder_object(t->buffer->data + deref_helper(offp));
 		switch (fp->type) {
 		case BINDER_TYPE_BINDER:
 		case BINDER_TYPE_WEAK_BINDER: {
-- 
1.7.9.5

[PATCH v1 8/9] staging: android: binder: Add binder compat handling to binder.h

[Serban Constantinescu]

This patch adds all the needed compat structures to binder.h. All the
structures defined in this patch mirror the structure and size of 32bit
ones.

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

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index cbe3451..6c9849d 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -22,6 +22,10 @@
 
 #include <linux/ioctl.h>
 
+#ifdef CONFIG_COMPAT
+#include <linux/compat.h>
+#endif
+
 #define B_PACK_CHARS(c1, c2, c3, c4) \
 	((((c1)<<24)) | (((c2)<<16)) | (((c3)<<8)) | (c4))
 #define B_TYPE_LARGE 0x85
@@ -326,5 +330,110 @@ enum binder_driver_command_protocol {
 	 */
 };
 
+/* Support for 32bit userspace on a 64bit system */
+#ifdef CONFIG_COMPAT
+struct compat_flat_binder_object {
+	/* 8 bytes for large_flat_header. */
+	__u32		type;
+	__u32		flags;
+
+	/* 8 bytes of data. */
+	union {
+		compat_uptr_t	binder;	/* local object */
+		__u32	handle;	        /* remote object */
+	};
+
+	/* extra data associated with local object */
+	compat_uptr_t	cookie;
+};
+
+struct compat_binder_write_read {
+	compat_size_t	write_size;     /* bytes to write */
+	compat_size_t	write_consumed;	/* bytes consumed by driver */
+	compat_ulong_t	write_buffer;
+	compat_size_t	read_size;      /* bytes to read */
+	compat_size_t	read_consumed;	/* bytes consumed by driver */
+	compat_ulong_t	read_buffer;
+};
+
+#define COMPAT_BINDER_WRITE_READ	_IOWR('b', 1, struct compat_binder_write_read)
+
+struct compat_binder_transaction_data {
+	/* The first two are only used for bcTRANSACTION and brTRANSACTION,
+	 * identifying the target and contents of the transaction.
+	 */
+	union {
+		__u32	handle;		    /* target descriptor of command transaction */
+		compat_uptr_t	ptr;	/* target descriptor of return transaction */
+	} target;
+	compat_uptr_t	cookie;	/* target object cookie */
+	__u32		code;	    /* transaction command */
+
+	/* General information about the transaction. */
+	__u32	flags;
+	pid_t	sender_pid;
+	uid_t	sender_euid;
+	compat_size_t	data_size;	    /* number of bytes of data */
+	compat_size_t	offsets_size;	/* number of bytes of offsets */
+
+	/* If this transaction is inline, the data immediately
+	 * follows here; otherwise, it ends with a pointer to
+	 * the data buffer.
+	 */
+	union {
+		struct {
+			/* transaction data */
+			compat_uptr_t	buffer;
+			/* offsets from buffer to flat_binder_object structs */
+			compat_uptr_t	offsets;
+		} ptr;
+		__u8	buf[8];
+	} data;
+};
+
+struct compat_binder_ptr_cookie {
+	compat_uptr_t ptr;
+	compat_uptr_t cookie;
+};
+
+/* legacy - not used anymore */
+struct compat_binder_pri_ptr_cookie {
+	__s32 priority;
+	compat_uptr_t ptr;
+	compat_uptr_t cookie;
+};
+
+enum compat_binder_driver_return_protocol {
+	COMPAT_BR_TRANSACTION = _IOR('r', 2, struct compat_binder_transaction_data),
+	COMPAT_BR_REPLY = _IOR('r', 3, struct compat_binder_transaction_data),
+
+	COMPAT_BR_INCREFS = _IOR('r', 7, struct compat_binder_ptr_cookie),
+	COMPAT_BR_ACQUIRE = _IOR('r', 8, struct compat_binder_ptr_cookie),
+	COMPAT_BR_RELEASE = _IOR('r', 9, struct compat_binder_ptr_cookie),
+	COMPAT_BR_DECREFS = _IOR('r', 10, struct compat_binder_ptr_cookie),
+
+	/* legacy - not used anymore */
+	COMPAT_BR_ATTEMPT_ACQUIRE = _IOR('r', 11, struct compat_binder_pri_ptr_cookie),
+
+	COMPAT_BR_DEAD_BINDER = _IOR('r', 15, compat_uptr_t),
+	COMPAT_BR_CLEAR_DEATH_NOTIFICATION_DONE = _IOR('r', 16, compat_uptr_t),
+};
+
+enum compat_binder_driver_command_protocol {
+	COMPAT_BC_TRANSACTION = _IOW('c', 0, struct compat_binder_transaction_data),
+	COMPAT_BC_REPLY = _IOW('c', 1, struct compat_binder_transaction_data),
+
+	COMPAT_BC_FREE_BUFFER = _IOW('c', 3, compat_uptr_t),
+
+	COMPAT_BC_INCREFS_DONE = _IOW('c', 8, struct compat_binder_ptr_cookie),
+	COMPAT_BC_ACQUIRE_DONE = _IOW('c', 9, struct compat_binder_ptr_cookie),
+
+	COMPAT_BC_REQUEST_DEATH_NOTIFICATION = _IOW('c', 14, struct compat_binder_ptr_cookie),
+	COMPAT_BC_CLEAR_DEATH_NOTIFICATION = _IOW('c', 15, struct compat_binder_ptr_cookie),
+
+	COMPAT_BC_DEAD_BINDER_DONE = _IOW('c', 16, compat_uptr_t),
+};
+#endif /* CONFIG_COMPAT */
+
 #endif /* _LINUX_BINDER_H */
 
-- 
1.7.9.5

[PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Serban Constantinescu]

This patch adds support for 32bit userspace running on 64bit kernels.
All the changes done in this patch have been tested on 32bit and 64bit
systems.

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

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 855d348..941973a 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -37,6 +37,7 @@
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
 #include <linux/pid_namespace.h>
+#include <linux/compat.h>
 
 #include "binder.h"
 #include "binder_trace.h"
@@ -140,6 +141,77 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
 			binder_stop_on_user_error = 2; \
 	} while (0)
 
+#ifdef CONFIG_COMPAT
+static inline void writeback_flat_binder_object(struct flat_binder_object *fp,
+					 void __user *ptr)
+{
+	struct compat_flat_binder_object *tmp_fp;
+
+	tmp_fp = (struct compat_flat_binder_object *) ptr;
+	tmp_fp->type = fp->type;
+	tmp_fp->flags = fp->flags;
+	tmp_fp->binder = ptr_to_compat(fp->binder);
+	tmp_fp->cookie = ptr_to_compat(fp->cookie);
+}
+
+static inline struct flat_binder_object *copy_flat_binder_object(void __user *ptr)
+{
+	struct flat_binder_object *fp;
+	struct compat_flat_binder_object *tmp_fp;
+
+	if (!is_compat_task())
+		return (struct flat_binder_object *) ptr;
+
+	fp = kzalloc(sizeof(*fp), GFP_KERNEL);
+	if (fp == NULL)
+		return NULL;
+
+	tmp_fp = (struct compat_flat_binder_object *) ptr;
+	/* copy compat struct */
+	fp->type = tmp_fp->type;
+	fp->flags = tmp_fp->flags;
+	fp->binder = compat_ptr(tmp_fp->binder);
+	fp->cookie = compat_ptr(tmp_fp->cookie);
+
+	return fp;
+}
+
+static inline uint32_t compat_change_size(uint32_t cmd, size_t size)
+{
+	uint32_t compat_cmd;
+
+	compat_cmd = cmd & ~IOCSIZE_MASK;
+	compat_cmd = compat_cmd | ((size << _IOC_SIZESHIFT) & IOCSIZE_MASK);
+	return compat_cmd;
+}
+
+#define align_helper(x) (						    \
+	is_compat_task() ?						    \
+	ALIGN(x, sizeof(compat_uptr_t)) :				    \
+	ALIGN(x, sizeof(void *))					    \
+	)
+
+#define deref_helper(x) (						    \
+	is_compat_task() ?						    \
+	(size_t) *(compat_size_t *)x :					    \
+	*(size_t *)x							    \
+	)
+
+#define size_helper(x) ({						    \
+	size_t __size;							    \
+	if (!is_compat_task())						    \
+		__size = sizeof(x);					    \
+	else if (sizeof(x) == sizeof(struct flat_binder_object))	    \
+		__size = sizeof(struct compat_flat_binder_object);	    \
+	else if (sizeof(x) == sizeof(struct binder_transaction_data))	    \
+		__size = sizeof(struct compat_binder_transaction_data);	    \
+	else if (sizeof(x) == sizeof(size_t))				    \
+		__size = sizeof(compat_size_t);				    \
+	else								    \
+		 BUG();							    \
+	__size;								    \
+	})
+#else
 #define align_helper(ptr)	    ALIGN(ptr, sizeof(void *))
 #define deref_helper(ptr)	    (*(typeof(size_t *))ptr)
 #define size_helper(x)		    sizeof(x)
@@ -148,6 +220,7 @@ static inline struct flat_binder_object *copy_flat_binder_object(void __user *pt
 {
 	return (struct flat_binder_object *)ptr;
 }
+#endif
 
 enum binder_stat_types {
 	BINDER_STAT_PROC,
@@ -1254,7 +1327,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 	else
 		off_end = (void *)offp + buffer->offsets_size;
 	for (; offp < off_end; offp += size_helper(size_t)) {
-		struct flat_binder_object *fp;
+		struct flat_binder_object *fp = NULL;
 		if (deref_helper(offp) > buffer->data_size - size_helper(*fp) ||
 		    buffer->data_size < size_helper(*fp) ||
 		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
@@ -1303,6 +1376,8 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 				debug_id, fp->type);
 			break;
 		}
+		if (is_compat_task())
+			kfree(fp);
 	}
 }
 
@@ -1321,6 +1396,7 @@ static void binder_transaction(struct binder_proc *proc,
 	struct binder_transaction *in_reply_to = NULL;
 	struct binder_transaction_log_entry *e;
 	uint32_t return_error;
+	struct flat_binder_object *fp = NULL;
 
 	e = binder_transaction_log_add(&binder_transaction_log);
 	e->call_type = reply ? 2 : !!(tr->flags & TF_ONE_WAY);
@@ -1503,7 +1579,6 @@ static void binder_transaction(struct binder_proc *proc,
 	}
 	off_end = (void *)offp + tr->offsets_size;
 	for (; offp < off_end; offp += size_helper(size_t)) {
-		struct flat_binder_object *fp;
 		if (deref_helper(offp) > t->buffer->data_size - size_helper(*fp) ||
 		    t->buffer->data_size < size_helper(*fp) ||
 		    !IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
@@ -1639,6 +1714,12 @@ static void binder_transaction(struct binder_proc *proc,
 			return_error = BR_FAILED_REPLY;
 			goto err_bad_object_type;
 		}
+#ifdef CONFIG_COMPAT
+		if (is_compat_task()) {
+			writeback_flat_binder_object(fp, t->buffer->data + deref_helper(offp));
+			kfree(fp);
+		}
+#endif
 	}
 	if (reply) {
 		BUG_ON(t->buffer->async_transaction != 0);
@@ -1674,6 +1755,8 @@ err_binder_new_node_failed:
 err_bad_object_type:
 err_bad_offset:
 err_copy_data_failed:
+	if (is_compat_task())
+		kfree(fp);
 	trace_binder_transaction_failed_buffer_release(t->buffer);
 	binder_transaction_buffer_release(target_proc, t->buffer, offp);
 	t->buffer->transaction = NULL;
@@ -1920,6 +2003,95 @@ static void bc_dead_binder_done(struct binder_proc *proc,
 	return;
 }
 
+#ifdef CONFIG_COMPAT
+static int compat_binder_thread_write(struct binder_proc *proc,
+		struct binder_thread *thread, uint32_t cmd,
+		void __user **ptr)
+{
+	BUG_ON(!is_compat_task());
+	switch (cmd) {
+	case COMPAT_BC_INCREFS_DONE:
+	case COMPAT_BC_ACQUIRE_DONE: {
+		compat_uptr_t node_ptr;
+		compat_uptr_t cookie;
+
+		if (get_user(node_ptr, (compat_uptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(compat_uptr_t);
+		if (get_user(cookie, (compat_uptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(compat_uptr_t);
+		bc_increfs_done(proc, thread, cmd == COMPAT_BC_ACQUIRE_DONE,
+			compat_ptr(node_ptr), compat_ptr(cookie));
+		break;
+	}
+
+	case COMPAT_BC_FREE_BUFFER: {
+		compat_uptr_t data_ptr;
+
+		if (get_user(data_ptr, (compat_uptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(compat_uptr_t);
+		bc_free_buffer(proc, thread, compat_ptr(data_ptr));
+		break;
+	}
+
+	case COMPAT_BC_TRANSACTION:
+	case COMPAT_BC_REPLY: {
+		struct binder_transaction_data tr;
+		struct compat_binder_transaction_data tmp_tr;
+
+		if (copy_from_user(&tmp_tr, *ptr, sizeof(tmp_tr)))
+			return -EFAULT;
+		*ptr += sizeof(tmp_tr);
+
+		memset(&tr, 0, sizeof(tr));
+		/* copy from compat struct */
+		tr.target.ptr = compat_ptr(tmp_tr.target.ptr);
+		tr.cookie = compat_ptr(tmp_tr.cookie);
+		tr.code = tmp_tr.code;
+		tr.flags = tmp_tr.flags;
+		tr.sender_pid = tmp_tr.sender_pid;
+		tr.sender_euid = tmp_tr.sender_euid;
+		tr.data_size = (size_t) tmp_tr.data_size;
+		tr.offsets_size = (size_t) tmp_tr.offsets_size;
+		tr.data.ptr.buffer = compat_ptr(tmp_tr.data.ptr.buffer);
+		tr.data.ptr.offsets = compat_ptr(tmp_tr.data.ptr.offsets);
+
+		binder_transaction(proc, thread, &tr, cmd == COMPAT_BC_REPLY);
+		break;
+	}
+	case COMPAT_BC_REQUEST_DEATH_NOTIFICATION:
+	case COMPAT_BC_CLEAR_DEATH_NOTIFICATION: {
+		uint32_t target;
+		compat_uptr_t cookie;
+
+		if (get_user(target, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (get_user(cookie, (compat_uptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(compat_uptr_t);
+		bc_clear_death_notif(proc, thread, cmd == COMPAT_BC_REQUEST_DEATH_NOTIFICATION,
+			target, compat_ptr(cookie));
+		break;
+	}
+	case COMPAT_BC_DEAD_BINDER_DONE: {
+		compat_uptr_t cookie;
+
+		if (get_user(cookie, (compat_uptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(compat_uptr_t);
+		bc_dead_binder_done(proc, thread, compat_ptr(cookie));
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+#endif
+
 static int binder_thread_write(struct binder_proc *proc,
 			struct binder_thread *thread,
 			void __user *buffer, size_t size, size_t *consumed)
@@ -2094,6 +2266,10 @@ static int binder_thread_write(struct binder_proc *proc,
 			break;
 		}
 		default:
+#ifdef CONFIG_COMPAT
+			if (is_compat_task() && (!compat_binder_thread_write(proc, thread, cmd, &ptr)))
+				break;
+#endif
 			pr_err("%d:%d unknown command %d\n",
 			       proc->pid, thread->pid, cmd);
 			return -EINVAL;
@@ -2127,6 +2303,103 @@ static int binder_has_thread_work(struct binder_thread *thread)
 		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
 }
 
+#ifdef CONFIG_COMPAT
+static int binder_copy_to_user(uint32_t cmd, void *parcel,
+			       void __user **ptr, size_t size)
+{
+	if (!is_compat_task()) {
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (copy_to_user(*ptr, parcel, size))
+			return -EFAULT;
+		*ptr += size;
+		return 0;
+	}
+	switch (cmd) {
+	case BR_INCREFS:
+	case BR_ACQUIRE:
+	case BR_RELEASE:
+	case BR_DECREFS: {
+		struct binder_ptr_cookie *fp;
+		struct compat_binder_ptr_cookie tmp;
+
+		cmd = compat_change_size(cmd, sizeof(tmp));
+		BUG_ON((cmd != COMPAT_BR_INCREFS) &&
+		       (cmd != COMPAT_BR_ACQUIRE) &&
+		       (cmd != COMPAT_BR_RELEASE) &&
+		       (cmd != COMPAT_BR_DECREFS));
+
+		fp = (struct binder_ptr_cookie *) parcel;
+		tmp.ptr = ptr_to_compat(fp->ptr);
+		tmp.cookie = ptr_to_compat(fp->cookie);
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (copy_to_user(*ptr, &tmp, sizeof(tmp)))
+			return -EFAULT;
+		*ptr += sizeof(tmp);
+
+		break;
+	}
+	case BR_DEAD_BINDER:
+	case BR_CLEAR_DEATH_NOTIFICATION_DONE: {
+		compat_uptr_t tmp;
+
+		cmd = compat_change_size(cmd, sizeof(tmp));
+		BUG_ON((cmd != COMPAT_BR_DEAD_BINDER) &&
+		       (cmd != COMPAT_BR_CLEAR_DEATH_NOTIFICATION_DONE));
+
+		tmp = ptr_to_compat((void *)*(uintptr_t *) parcel);
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (copy_to_user(*ptr, &tmp, sizeof(tmp)))
+			return -EFAULT;
+		*ptr += sizeof(tmp);
+
+		break;
+	}
+	case BR_REPLY:
+	case BR_TRANSACTION: {
+		struct binder_transaction_data *fp;
+		struct compat_binder_transaction_data tmp;
+
+		memset(&tmp, 0, sizeof(tmp));
+		cmd = compat_change_size(cmd, sizeof(tmp));
+		BUG_ON((cmd != COMPAT_BR_REPLY) &&
+		       (cmd != COMPAT_BR_TRANSACTION));
+
+		fp = (struct binder_transaction_data *) parcel;
+		/* copy to compat struct */
+		tmp.target.ptr = ptr_to_compat(fp->target.ptr);
+		tmp.cookie = ptr_to_compat(fp->cookie);
+		tmp.code = fp->code;
+		tmp.flags = fp->flags;
+		tmp.sender_pid = fp->sender_pid;
+		tmp.sender_euid = fp->sender_euid;
+		tmp.data_size = (compat_size_t) fp->data_size;
+		tmp.offsets_size = (compat_size_t) fp->offsets_size;
+		tmp.data.ptr.buffer = ptr_to_compat((void *)fp->data.ptr.buffer);
+		tmp.data.ptr.offsets = ptr_to_compat((void *)fp->data.ptr.offsets);
+
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (copy_to_user(*ptr, &tmp, sizeof(tmp)))
+			return -EFAULT;
+		*ptr += sizeof(tmp);
+
+		break;
+	}
+	default:
+		pr_err("unexpected user copy, cmd %d, ptr %p\n",
+			cmd, (void *)*(uintptr_t *)ptr);
+		return -EFAULT;
+	}
+	return 0;
+}
+#else
 static int binder_copy_to_user(uint32_t cmd, void *parcel,
 			       void __user **ptr, size_t size)
 {
@@ -2138,6 +2411,7 @@ static int binder_copy_to_user(uint32_t cmd, void *parcel,
 	*ptr += size;
 	return 0;
 }
+#endif
 
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
@@ -2729,6 +3003,80 @@ err_unlocked:
 	return ret;
 }
 
+/* TODO: add tracepoint */
+#ifdef CONFIG_COMPAT
+static long compat_binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	struct binder_thread *thread;
+	struct compat_binder_write_read bwr;
+	struct binder_proc *proc = filp->private_data;
+	void __user *ubuf = compat_ptr(arg);
+
+	/* pr_info("compat_binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg); */
+
+	if (cmd != COMPAT_BINDER_WRITE_READ)
+		return binder_ioctl(filp, cmd, arg);
+
+	BUG_ON(!is_compat_task());
+	binder_lock(__func__);
+	thread = binder_get_thread(proc);
+	if (thread == NULL) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	/* COMPAT_BINDER_WRITE_READ */
+	if (copy_from_user(&bwr, ubuf, sizeof(bwr))) {
+		ret = -EFAULT;
+		goto err;
+	}
+	binder_debug(BINDER_DEBUG_READ_WRITE,
+	    "compat: %d:%d write %d at %016x, read %d at %016x\n",
+		    proc->pid, thread->pid, bwr.write_size,
+		    bwr.write_buffer, bwr.read_size, bwr.read_buffer);
+
+	if (bwr.write_size > 0) {
+		size_t tmp_write_consumed = (size_t)bwr.write_consumed;
+		ret = binder_thread_write(proc, thread, compat_ptr(bwr.write_buffer), (size_t)bwr.write_size, &tmp_write_consumed);
+		bwr.write_consumed = (compat_size_t)tmp_write_consumed;
+		if (ret < 0) {
+			bwr.read_consumed = 0;
+			if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
+				ret = -EFAULT;
+			goto err;
+		}
+	}
+
+	if (bwr.read_size > 0) {
+		size_t tmp_read_consumed = (size_t)bwr.read_consumed;
+		ret = binder_thread_read(proc, thread, compat_ptr(bwr.read_buffer), (size_t)bwr.read_size, &tmp_read_consumed, filp->f_flags & O_NONBLOCK);
+		bwr.read_consumed = (compat_size_t)tmp_read_consumed;
+		if (!list_empty(&proc->todo))
+			wake_up_interruptible(&proc->wait);
+		if (ret < 0) {
+			if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
+				ret = -EFAULT;
+			goto err;
+		}
+	}
+	binder_debug(BINDER_DEBUG_READ_WRITE,
+		    "compat: %d:%d wrote %d of %d, read return %d of %d\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)))
+		ret = -EFAULT;
+
+err:
+	if (thread)
+		thread->looper &= ~BINDER_LOOPER_STATE_NEED_RETURN;
+	binder_unlock(__func__);
+	if (ret && ret != -ERESTARTSYS)
+		pr_info("%d:%d compat ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
+	return ret;
+}
+#endif
+
 static void binder_vma_open(struct vm_area_struct *vma)
 {
 	struct binder_proc *proc = vma->vm_private_data;
@@ -3546,6 +3894,9 @@ static const struct file_operations binder_fops = {
 	.owner = THIS_MODULE,
 	.poll = binder_poll,
 	.unlocked_ioctl = binder_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_binder_ioctl,
+#endif
 	.mmap = binder_mmap,
 	.open = binder_open,
 	.flush = binder_flush,
-- 
1.7.9.5

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Greg KH]

On Wed, Dec 04, 2013 at 06:09:41PM +0000, Serban Constantinescu wrote:
> +#define size_helper(x) ({						    \
> +	size_t __size;							    \
> +	if (!is_compat_task())						    \
> +		__size = sizeof(x);					    \
> +	else if (sizeof(x) == sizeof(struct flat_binder_object))	    \
> +		__size = sizeof(struct compat_flat_binder_object);	    \
> +	else if (sizeof(x) == sizeof(struct binder_transaction_data))	    \
> +		__size = sizeof(struct compat_binder_transaction_data);	    \
> +	else if (sizeof(x) == sizeof(size_t))				    \
> +		__size = sizeof(compat_size_t);				    \
> +	else								    \
> +		 BUG();							    \
> +	__size;								    \
> +	})

Ick.

First off, no driver should ever be able to crash the kernel, which you
just did.

Second, almost none of those "if" lines will ever be hit, why did you
include it all?

And finally, is this all really needed?  Why not just fix the structures
to be "correct", and then fix userspace to use the correct structures as
well, thereby not needing a compat layer at all?

You have the chance to fix the api properly, why not take it and do it,
making all of this unnecessary.

thanks,

greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
<snip>

> And finally, is this all really needed?  Why not just fix the structures
> to be "correct", and then fix userspace to use the correct structures as
> well, thereby not needing a compat layer at all?

Some of the binder ioctls take userspace pointers.  Are you suggesting
storing those pointers in a __u64 to avoid having to have a
compat_ioctl?

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Greg KH]

On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> <snip>
> 
> > And finally, is this all really needed?  Why not just fix the structures
> > to be "correct", and then fix userspace to use the correct structures as
> > well, thereby not needing a compat layer at all?
> 
> Some of the binder ioctls take userspace pointers.  Are you suggesting
> storing those pointers in a __u64 to avoid having to have a
> compat_ioctl?

Yes, that's the best way to solve the issue, right?

thanks,

greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> <snip>
>>
>> > And finally, is this all really needed?  Why not just fix the structures
>> > to be "correct", and then fix userspace to use the correct structures as
>> > well, thereby not needing a compat layer at all?
>>
>> Some of the binder ioctls take userspace pointers.  Are you suggesting
>> storing those pointers in a __u64 to avoid having to have a
>> compat_ioctl?
>
> Yes, that's the best way to solve the issue, right?

It's the least code, but in exchange you lose all the type safety and
warnings when copying in and out of the pointers, as well as sparse
checking on the __user attribute.  That doesn't seem like a good
tradeoff to me.  In addition it requires modifying the existing
heavily used 32 bit api, which means a mostly-equivalent compat layer
added in libbinder to support old kernels.

I would suggest fixing the 32-bit structures to use fixed sizes where
appropriate (__u32 instead of unsigned long) while maintaining
compatibility, and then using compat_ioctl where necessary to handle
pointers.

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Greg KH]

On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> <snip>
> >>
> >> > And finally, is this all really needed?  Why not just fix the structures
> >> > to be "correct", and then fix userspace to use the correct structures as
> >> > well, thereby not needing a compat layer at all?
> >>
> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
> >> storing those pointers in a __u64 to avoid having to have a
> >> compat_ioctl?
> >
> > Yes, that's the best way to solve the issue, right?
> 
> It's the least code, but in exchange you lose all the type safety and
> warnings when copying in and out of the pointers, as well as sparse
> checking on the __user attribute.

Not if you make the cast right at the beginning, when you first "touch"
the data, but yes, it does take some of the type saftey away, at the
expense of simpler code to mess up :)

> That doesn't seem like a good tradeoff to me.  In addition it requires
> modifying the existing heavily used 32 bit api, which means a
> mostly-equivalent compat layer added in libbinder to support old
> kernels.

Wait, I thought that libbinder would have to be changed anyway here, to
handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
already changing it, why not just "do it correctly"?

Or does this patch series mean that no userspace code is changed?  Is
that a "requirement" here?

thanks,

greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> <snip>
>> >>
>> >> > And finally, is this all really needed?  Why not just fix the structures
>> >> > to be "correct", and then fix userspace to use the correct structures as
>> >> > well, thereby not needing a compat layer at all?
>> >>
>> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>> >> storing those pointers in a __u64 to avoid having to have a
>> >> compat_ioctl?
>> >
>> > Yes, that's the best way to solve the issue, right?
>>
>> It's the least code, but in exchange you lose all the type safety and
>> warnings when copying in and out of the pointers, as well as sparse
>> checking on the __user attribute.
>
> Not if you make the cast right at the beginning, when you first "touch"
> the data, but yes, it does take some of the type saftey away, at the
> expense of simpler code to mess up :)
>
>> That doesn't seem like a good tradeoff to me.  In addition it requires
>> modifying the existing heavily used 32 bit api, which means a
>> mostly-equivalent compat layer added in libbinder to support old
>> kernels.
>
> Wait, I thought that libbinder would have to be changed anyway here, to
> handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
> already changing it, why not just "do it correctly"?

libbinder will need changes to support 64-bit userspace and especially
a mixed 64-bit and 32-bit userspace, but this patch series is only
addressing a pure 32-bit userspace on a 64-bit kernel.  Support for a
64-bit userspace in Android is obviously going to require a future
version of Android including, among other things, libbinder changes.
As far as I know, those changes won't need to change the ioctl api,
only the layout of the buffers that are passed through the ioctl api.

> Or does this patch series mean that no userspace code is changed?  Is
> that a "requirement" here?

Since this series only addresses 32-bit userspace on 64-bit kernel
support there are no associated userspace changes.  Changing the
32-bit api here means that combining the KitKat branch from
http://android.googlesource.com with a newer kernel version will not
work.

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Greg KH]

On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote:
> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
> >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
> >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> <snip>
> >> >>
> >> >> > And finally, is this all really needed?  Why not just fix the structures
> >> >> > to be "correct", and then fix userspace to use the correct structures as
> >> >> > well, thereby not needing a compat layer at all?
> >> >>
> >> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
> >> >> storing those pointers in a __u64 to avoid having to have a
> >> >> compat_ioctl?
> >> >
> >> > Yes, that's the best way to solve the issue, right?
> >>
> >> It's the least code, but in exchange you lose all the type safety and
> >> warnings when copying in and out of the pointers, as well as sparse
> >> checking on the __user attribute.
> >
> > Not if you make the cast right at the beginning, when you first "touch"
> > the data, but yes, it does take some of the type saftey away, at the
> > expense of simpler code to mess up :)
> >
> >> That doesn't seem like a good tradeoff to me.  In addition it requires
> >> modifying the existing heavily used 32 bit api, which means a
> >> mostly-equivalent compat layer added in libbinder to support old
> >> kernels.
> >
> > Wait, I thought that libbinder would have to be changed anyway here, to
> > handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
> > already changing it, why not just "do it correctly"?
> 
> libbinder will need changes to support 64-bit userspace and especially
> a mixed 64-bit and 32-bit userspace, but this patch series is only
> addressing a pure 32-bit userspace on a 64-bit kernel.  Support for a
> 64-bit userspace in Android is obviously going to require a future
> version of Android including, among other things, libbinder changes.
> As far as I know, those changes won't need to change the ioctl api,
> only the layout of the buffers that are passed through the ioctl api.

"only" means you can rearrange things at that point in time, as you will
have to be doing that anyway :)

> > Or does this patch series mean that no userspace code is changed?  Is
> > that a "requirement" here?
> 
> Since this series only addresses 32-bit userspace on 64-bit kernel
> support there are no associated userspace changes.  Changing the
> 32-bit api here means that combining the KitKat branch from
> http://android.googlesource.com with a newer kernel version will not
> work.

Is that something that anyone has said would work in the past?  It seems
that other parts of the Android userspace are pretty tied to specific
kernel features / versions, is this anything new if the binder code had
to change?

Anyway, the code as submitted has problems, see my response to the
second patch, so it's not ready yet anyway :(

thanks,

greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 4:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote:
>> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>> >> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>> >> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> >> <snip>
>> >> >>
>> >> >> > And finally, is this all really needed?  Why not just fix the structures
>> >> >> > to be "correct", and then fix userspace to use the correct structures as
>> >> >> > well, thereby not needing a compat layer at all?
>> >> >>
>> >> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>> >> >> storing those pointers in a __u64 to avoid having to have a
>> >> >> compat_ioctl?
>> >> >
>> >> > Yes, that's the best way to solve the issue, right?
>> >>
>> >> It's the least code, but in exchange you lose all the type safety and
>> >> warnings when copying in and out of the pointers, as well as sparse
>> >> checking on the __user attribute.
>> >
>> > Not if you make the cast right at the beginning, when you first "touch"
>> > the data, but yes, it does take some of the type saftey away, at the
>> > expense of simpler code to mess up :)
>> >
>> >> That doesn't seem like a good tradeoff to me.  In addition it requires
>> >> modifying the existing heavily used 32 bit api, which means a
>> >> mostly-equivalent compat layer added in libbinder to support old
>> >> kernels.
>> >
>> > Wait, I thought that libbinder would have to be changed anyway here, to
>> > handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
>> > already changing it, why not just "do it correctly"?
>>
>> libbinder will need changes to support 64-bit userspace and especially
>> a mixed 64-bit and 32-bit userspace, but this patch series is only
>> addressing a pure 32-bit userspace on a 64-bit kernel.  Support for a
>> 64-bit userspace in Android is obviously going to require a future
>> version of Android including, among other things, libbinder changes.
>> As far as I know, those changes won't need to change the ioctl api,
>> only the layout of the buffers that are passed through the ioctl api.
>
> "only" means you can rearrange things at that point in time, as you will
> have to be doing that anyway :)
>
>> > Or does this patch series mean that no userspace code is changed?  Is
>> > that a "requirement" here?
>>
>> Since this series only addresses 32-bit userspace on 64-bit kernel
>> support there are no associated userspace changes.  Changing the
>> 32-bit api here means that combining the KitKat branch from
>> http://android.googlesource.com with a newer kernel version will not
>> work.
>
> Is that something that anyone has said would work in the past?  It seems
> that other parts of the Android userspace are pretty tied to specific
> kernel features / versions, is this anything new if the binder code had
> to change?

We have explicitly said that Android userspace does not require a
specific kernel version.  I expect to see KitKat devices running at
least 3.4, 3.10, and whatever is the next long term stable version.
Sometimes new kernels need userspace support, but we try to avoid that
as much as possible.  The last major one I can remember was removing
early suspend in 3.4, but in that case I provided a compat 3.4 branch
to allow using 3.4 with an older userspace.  Changing the binder api
is not completely unsupportable for old versions, my guess is some
people would just ship with a new kernel with the binder changes
reverted.

> Anyway, the code as submitted has problems, see my response to the
> second patch, so it's not ready yet anyway :(
>
> thanks,
>
> greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Arve Hjønnevåg]

On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> <snip>
>> >>
>> >> > And finally, is this all really needed?  Why not just fix the structures
>> >> > to be "correct", and then fix userspace to use the correct structures as
>> >> > well, thereby not needing a compat layer at all?
>> >>
>> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>> >> storing those pointers in a __u64 to avoid having to have a
>> >> compat_ioctl?
>> >
>> > Yes, that's the best way to solve the issue, right?
>>
>> It's the least code, but in exchange you lose all the type safety and
>> warnings when copying in and out of the pointers, as well as sparse
>> checking on the __user attribute.
>
> Not if you make the cast right at the beginning, when you first "touch"
> the data, but yes, it does take some of the type saftey away, at the
> expense of simpler code to mess up :)
>
>> That doesn't seem like a good tradeoff to me.  In addition it requires
>> modifying the existing heavily used 32 bit api, which means a
>> mostly-equivalent compat layer added in libbinder to support old
>> kernels.
>
> Wait, I thought that libbinder would have to be changed anyway here, to
> handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
> already changing it, why not just "do it correctly"?
>

Yes libbinder will have to be changed to support calls between 32 bit
and 64 bit processes, so I don't see much value in a patchset that
only supports all 32 bit or all 64 bit processes. If user space is
fixed to use 64 bit pointers on a 64 bit system, then much of the code
added in this patchset becomes useless (and probably harmful as it
appears to prevent 32 bit processes from communicating with 64 bit
processes).

> Or does this patch series mean that no userspace code is changed?  Is
> that a "requirement" here?
>

I don't think we need to support old 32 bit userspace framework code
on a 64 bit system. I think it is more important to not prevent mixed
mode systems.

-- 
Arve Hjønnevåg

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Serban Constantinescu]

Hi all,

Thanks for your feedback! Sadly enough, being in a different time-zone, 
is not useful.

Sorry for the confusion related to why is this patch needed or not. I
should highlight a bit more what is the patch enabling and what would be 
the different alternatives, at least from my perspective.

*64bit kernel/ 32bit userspace*

This patch series adds support for 32bit userspace running on 64bit 
kernels. Thus by applying this patch to your kernel you will be able to 
use any existing 32bit Android userspace on your 64bit platform, running 
a 64bit kernel. That is pure 32bit userspace with no 64bit support!

This means *no modifications to the 32bit userspace*. Therefore any 
applications or userspace side drivers, that uses the binder interface 
at a native level will not have to be modified. These kind of 
applications are not "good citizens" - the native binder API is not 
exposed in the Android NDK. However I do not know how many applications 
do this and if breaking the compatibility is a concernt for 32bit 
userspace running on 64bit kernels.

Below you will find more information on one of the alternative 
solutions, the userspace compat.

*32bit kernel/ 32bit userspace*
*64bit kernel/ 64bit userspace*

My last series of binder patches, accepted into 3.12 revision of the 
Linux kernel, audits the binder interface for 64bit. A kernel with these 
changes applied can be used with a pure 64bit userspace (this does not 
include support for 32bit applications coexisting with 64bit ones). The 
userspace side needs trivial changes to libbinder.so and servicemanager, 
that I will upstream ASAP to AOSP, and that work for 32/32 systems and 
64/64 systems without modifying the existing 32bit interface ABI (most 
of the changes are related to uint32_t != void *).

> Author: Serban Constantinescu <serban.constantinescu@arm.com>
> Date:   Thu Jul 4 10:54:48 2013 +0100
>
> staging: android: binder: fix binder interface for 64bit compat layer

*64bit kernel/ 64bit coexisting with 32bit userspace

Please note that *none of the above solutions fix this yet*. To 
understand why this is a special case please take a look at
how the binder driver works, seen from a high level perspective:

>           ServiceManager
> App1  <---------------------> App2

Thus, for two apps to exchange information between each other they will 
have to communicate with the userspace governor, ServiceManager. All the 
interaction between App1, App2 and ServiceManager is done through a 
combination of libbinder.so (Userspace HAL) and the binder driver. Note 
that there can only been one ServiceManager process, that is set during 
the userspace boot process.

Now lets consider the case that Arve described earlier 32bit 
Applications coexisting with 64bit ones. In this case all the commands 
and interface used will have to be the same, the ones used by the 64bit 
ServiceManager. For this the kernel entry point for 32bit compat will 
have to be changed to:

> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -3893,9 +3893,13 @@ static const struct file_operations binder_fops = {
>         .owner = THIS_MODULE,
>         .poll = binder_poll,
>         .unlocked_ioctl = binder_ioctl,
> -#ifdef CONFIG_COMPAT
> +#if defined(CONFIG_COMPAT) && !defined(COEXIST_32BIT_64BIT)
>         .compat_ioctl = compat_binder_ioctl,
>  #endif
> +
> +#if defined(COEXIST_32BIT_64BIT)
> +       .compat_ioctl = binder_ioctl,
> +#endif
>         .mmap = binder_mmap,
>         .open = binder_open,
>         .flush = binder_flush,

thus from the perspective of a kernel that works with a 64bit userspace 
where 64bit applications coexist with 32bit applications the handling 
will be the same for both 32bit and 64bit processes. This will also 
involve modifying libbinder.so to use 64bit structures like:

> --- a/libs/binder/IPCThreadState.cpp
> +++ b/libs/binder/IPCThreadState.cpp
> +#if defined(COEXIST_32BIT_64BIT) && !defined(__LP64__)
> +/*
> + * For running 32-bit processes on a 64-bit system, make the interaction with the
> + * binder driver the same from this, when built for 32-bit, as for a 64-bit process.
> + */
> +struct coexist_binder_write_read {
> +    uint64_t    write_size;     /* __LP64__ size_t: bytes to write */
> +    uint64_t    write_consumed; /* __LP64__ size_t: bytes consumed by driver */
> +    uint64_t    write_buffer;   /* __LP64__ unsigned long */
> +    uint64_t    read_size;      /* __LP64__ size_t: bytes to read */
> +    uint64_t    read_consumed;  /* __LP64__ size_t: bytes consumed by driver */
> +    uint64_t    read_buffer;    /* __LP64__ unsigned long */
> +};

"Good citizen" Android applications will go through these changes (since 
they should only use the binder Java API), and so shouldn't encounter 
any problem. Any 32bit applications that use the binder interface at a 
native level will not work with this model (they should not do so!)· 
This is exactly what the kernel compat "guarantees" *only* for 64/32bit 
systems.

The cleaner solution from the binder kernel perspective is to hook the 
compat_ioctl to binder_ioctl and do all this clean-up in the userspace. 
This way the same userspace compat can be used for  64bit kernel/32 bit 
userspace, 64bit kernel/ 64 bit coexisting with 32bit userspace. Note - 
this will mean that 64bit systems with 32bit userspace will be tied to a 
Android version >= the version with COEXIST_32BIT_64BIT support, and if 
the binder interface is used at a lower level than it should be your app 
will not work!

>> Or does this patch series mean that no userspace code is changed?  Is
>> that a "requirement" here?

This is what this series boils down to... Do we need to keep this 
compatibility for 64/32 and aford breaking it for 64/64-32 ?

>>
>
> I don't think we need to support old 32 bit userspace framework code
> on a 64 bit system. I think it is more important to not prevent mixed
> mode systems.

See above snippet for:
> static const struct file_operations binder_fops.
a kernel build flag could differentiate between your target.

Plese let me know if I have missed any other solution and if there is a 
better alternative. Overall I intend to find a solution rather then 
create more mess. The binder driver is complicated as it is and does not 
need more complexity added, however moving forward we will have to 
support some of these systems.

Thanks for all your feedback,
Let me know your thoughts on this,
Serban Constantinescu

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Greg KH]

On Thu, Dec 05, 2013 at 06:31:25PM +0000, Serban Constantinescu wrote:
> Hi all,
> 
> Thanks for your feedback! Sadly enough, being in a different
> time-zone, is not useful.
> 
> Sorry for the confusion related to why is this patch needed or not. I
> should highlight a bit more what is the patch enabling and what
> would be the different alternatives, at least from my perspective.
> 
> *64bit kernel/ 32bit userspace*
> 
> This patch series adds support for 32bit userspace running on 64bit
> kernels. Thus by applying this patch to your kernel you will be able
> to use any existing 32bit Android userspace on your 64bit platform,
> running a 64bit kernel. That is pure 32bit userspace with no 64bit
> support!
> 
> This means *no modifications to the 32bit userspace*. Therefore any
> applications or userspace side drivers, that uses the binder
> interface at a native level will not have to be modified. These kind
> of applications are not "good citizens" - the native binder API is
> not exposed in the Android NDK. However I do not know how many
> applications do this and if breaking the compatibility is a concernt
> for 32bit userspace running on 64bit kernels.

Um, I thought we were assured that the _only_ user of the kernel binder
interface was libbinder.  If other programs are touching this interface
"directly", you have bigger problems then just a 32/64 bit issue, and
that needs to be fixed.

In other words, you should be totally safe in modifying libbinder as
well as the kernel interface at the same time.

Now if you really want to do that or not is another issue, but it should
be possible (and in my opinion, the better option...)

thanks,

greg k-h

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Octavian Purdila]

On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg <arve@android.com> wrote:
> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>>> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>>> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> >> <snip>
>>> >>
>>> >> > And finally, is this all really needed?  Why not just fix the structures
>>> >> > to be "correct", and then fix userspace to use the correct structures as
>>> >> > well, thereby not needing a compat layer at all?
>>> >>
>>> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>>> >> storing those pointers in a __u64 to avoid having to have a
>>> >> compat_ioctl?
>>> >
>>> > Yes, that's the best way to solve the issue, right?
>>>
>>> It's the least code, but in exchange you lose all the type safety and
>>> warnings when copying in and out of the pointers, as well as sparse
>>> checking on the __user attribute.
>>
>> Not if you make the cast right at the beginning, when you first "touch"
>> the data, but yes, it does take some of the type saftey away, at the
>> expense of simpler code to mess up :)
>>
>>> That doesn't seem like a good tradeoff to me.  In addition it requires
>>> modifying the existing heavily used 32 bit api, which means a
>>> mostly-equivalent compat layer added in libbinder to support old
>>> kernels.
>>
>> Wait, I thought that libbinder would have to be changed anyway here, to
>> handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
>> already changing it, why not just "do it correctly"?
>>
>
> Yes libbinder will have to be changed to support calls between 32 bit
> and 64 bit processes, so I don't see much value in a patchset that
> only supports all 32 bit or all 64 bit processes. If user space is
> fixed to use 64 bit pointers on a 64 bit system, then much of the code
> added in this patchset becomes useless (and probably harmful as it
> appears to prevent 32 bit processes from communicating with 64 bit
> processes).
>

Hi,

Coincidentally, I have been working on a compat layer myself lately.
It is implemented in the binder driver with no changes in libbinder
and it includes support for mixed mode.

Unless you think that the kernel compat layer is a dead end, I can
post the patches here for review. IMO the kernel compat layer gives
you greater flexibility because it keeps the 32bit ABI unchanged. Of
course it comes with the price of increased complexity.

Thanks,
Tavi

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Arve Hjønnevåg]

On Mon, Dec 9, 2013 at 7:01 PM, Octavian Purdila <tavi.purdila@gmail.com> wrote:
> On Thu, Dec 5, 2013 at 4:02 AM, Arve Hjønnevåg <arve@android.com> wrote:
>> On Wed, Dec 4, 2013 at 2:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote:
>>>> On Wed, Dec 4, 2013 at 1:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>> > On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote:
>>>> >> On Wed, Dec 4, 2013 at 10:35 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>> >> <snip>
>>>> >>
>>>> >> > And finally, is this all really needed?  Why not just fix the structures
>>>> >> > to be "correct", and then fix userspace to use the correct structures as
>>>> >> > well, thereby not needing a compat layer at all?
>>>> >>
>>>> >> Some of the binder ioctls take userspace pointers.  Are you suggesting
>>>> >> storing those pointers in a __u64 to avoid having to have a
>>>> >> compat_ioctl?
>>>> >
>>>> > Yes, that's the best way to solve the issue, right?
>>>>
>>>> It's the least code, but in exchange you lose all the type safety and
>>>> warnings when copying in and out of the pointers, as well as sparse
>>>> checking on the __user attribute.
>>>
>>> Not if you make the cast right at the beginning, when you first "touch"
>>> the data, but yes, it does take some of the type saftey away, at the
>>> expense of simpler code to mess up :)
>>>
>>>> That doesn't seem like a good tradeoff to me.  In addition it requires
>>>> modifying the existing heavily used 32 bit api, which means a
>>>> mostly-equivalent compat layer added in libbinder to support old
>>>> kernels.
>>>
>>> Wait, I thought that libbinder would have to be changed anyway here, to
>>> handle 64bit kernels (in both 32 and 64bit userspace).  Since you are
>>> already changing it, why not just "do it correctly"?
>>>
>>
>> Yes libbinder will have to be changed to support calls between 32 bit
>> and 64 bit processes, so I don't see much value in a patchset that
>> only supports all 32 bit or all 64 bit processes. If user space is
>> fixed to use 64 bit pointers on a 64 bit system, then much of the code
>> added in this patchset becomes useless (and probably harmful as it
>> appears to prevent 32 bit processes from communicating with 64 bit
>> processes).
>>
>
> Hi,
>
> Coincidentally, I have been working on a compat layer myself lately.
> It is implemented in the binder driver with no changes in libbinder
> and it includes support for mixed mode.
>
> Unless you think that the kernel compat layer is a dead end, I can
> post the patches here for review. IMO the kernel compat layer gives
> you greater flexibility because it keeps the 32bit ABI unchanged. Of
> course it comes with the price of increased complexity.
>
> Thanks,
> Tavi

Assuming you are talking about a kernel compat layer that translates
the flat_binder_object structs as they pass between 32 bit and 64 bit
processes, that will not always work. The data portion of the message
sometimes contain size values that are invisible to the kernel, but
these values will be wrong if the kernel move data to make room for a
different size flat_binder_object.

-- 
Arve Hjønnevåg

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Octavian Purdila]

On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg <arve@android.com> wrote:
>
> Assuming you are talking about a kernel compat layer that translates
> the flat_binder_object structs as they pass between 32 bit and 64 bit
> processes, that will not always work. The data portion of the message
> sometimes contain size values that are invisible to the kernel, but
> these values will be wrong if the kernel move data to make room for a
> different size flat_binder_object.
>

Hi Arve,

Yes, I was talking about translating flat_binder_objects.

I understand the potential issue for the user data payload, however,
since most applications will use libbinder, the only problematic case
is readIntPtr/writeIntPtr, which we can deprecate and convert
applications that use it to readInt64. AFAICS there is only one user
in the AOSP for this API (libmedia).

If you are referring to data blobs that application parses I don't
think there is anything we can do, even at libbinder level.

Can you give me an example of the sort of problems you see?

Thanks,
Tavi

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Arve Hjønnevåg]

On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg <arve@android.com> wrote:
>>
>> Assuming you are talking about a kernel compat layer that translates
>> the flat_binder_object structs as they pass between 32 bit and 64 bit
>> processes, that will not always work. The data portion of the message
>> sometimes contain size values that are invisible to the kernel, but
>> these values will be wrong if the kernel move data to make room for a
>> different size flat_binder_object.
>>
>
> Hi Arve,
>
> Yes, I was talking about translating flat_binder_objects.
>
> I understand the potential issue for the user data payload, however,
> since most applications will use libbinder, the only problematic case
> is readIntPtr/writeIntPtr, which we can deprecate and convert
> applications that use it to readInt64. AFAICS there is only one user
> in the AOSP for this API (libmedia).
>
> If you are referring to data blobs that application parses I don't
> think there is anything we can do, even at libbinder level.
>
> Can you give me an example of the sort of problems you see?
>
> Thanks,
> Tavi

The specific problem I was told about can be found in
frameworks/base/core/java/android/os/Bundle.java, but there could be
other. The size of the bundle is stored in the parcel so the end of
the bundle will be wrong if the bundle contains a flat_binder_object
that the driver changes the size of. However, since the sending
process gets the parcel size from libbinder, changing libbinder to
always use the 64 bit version of flat_binder_object should work with
this code.

-- 
Arve Hjønnevåg

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Octavian Purdila]

On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg <arve@android.com> wrote:
> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg <arve@android.com> wrote:
>>>
>>> Assuming you are talking about a kernel compat layer that translates
>>> the flat_binder_object structs as they pass between 32 bit and 64 bit
>>> processes, that will not always work. The data portion of the message
>>> sometimes contain size values that are invisible to the kernel, but
>>> these values will be wrong if the kernel move data to make room for a
>>> different size flat_binder_object.
>>>
>>
>> Hi Arve,
>>
>> Yes, I was talking about translating flat_binder_objects.
>>
>> I understand the potential issue for the user data payload, however,
>> since most applications will use libbinder, the only problematic case
>> is readIntPtr/writeIntPtr, which we can deprecate and convert
>> applications that use it to readInt64. AFAICS there is only one user
>> in the AOSP for this API (libmedia).
>>
>> If you are referring to data blobs that application parses I don't
>> think there is anything we can do, even at libbinder level.
>>
>> Can you give me an example of the sort of problems you see?
>>
>> Thanks,
>> Tavi
>
> The specific problem I was told about can be found in
> frameworks/base/core/java/android/os/Bundle.java, but there could be
> other. The size of the bundle is stored in the parcel so the end of
> the bundle will be wrong if the bundle contains a flat_binder_object
> that the driver changes the size of. However, since the sending
> process gets the parcel size from libbinder, changing libbinder to
> always use the 64 bit version of flat_binder_object should work with
> this code.
>

AFAICS the bundle size is stored as an int32 so there is no bit width
issues between the sending and receiving process.

When I mentioned that flat_binder_objects will be translated, I meant
the whole transaction will be translated to preserve its user payload
intact. Something like this:

        32bit                        64bit
  +----------------+    +--------------------+
  | o o oo  o    xx| -> | oo oooo  oo    yyyy|
  +----------------+    +--------------------+

where o are binder objects, spaces are user data and x,y are offsets
pointers to binder objects (they are size_t so they need translation
as well).

As long as the application does not use absolute offsets in the
payload and as long as the data types stored in the payload are fixed
bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of
intptr) doing the translation in kernel should be fine. I checked
libbinder and both assumptions seems to be true (except in a few cases
for the later which I already mentioned)

So, what am I missing?

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Arve Hjønnevåg]

On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg <arve@android.com> wrote:
>> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila
>> <octavian.purdila@intel.com> wrote:
>>> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg <arve@android.com> wrote:
>>>>
>>>> Assuming you are talking about a kernel compat layer that translates
>>>> the flat_binder_object structs as they pass between 32 bit and 64 bit
>>>> processes, that will not always work. The data portion of the message
>>>> sometimes contain size values that are invisible to the kernel, but
>>>> these values will be wrong if the kernel move data to make room for a
>>>> different size flat_binder_object.
>>>>
>>>
>>> Hi Arve,
>>>
>>> Yes, I was talking about translating flat_binder_objects.
>>>
>>> I understand the potential issue for the user data payload, however,
>>> since most applications will use libbinder, the only problematic case
>>> is readIntPtr/writeIntPtr, which we can deprecate and convert
>>> applications that use it to readInt64. AFAICS there is only one user
>>> in the AOSP for this API (libmedia).
>>>
>>> If you are referring to data blobs that application parses I don't
>>> think there is anything we can do, even at libbinder level.
>>>
>>> Can you give me an example of the sort of problems you see?
>>>
>>> Thanks,
>>> Tavi
>>
>> The specific problem I was told about can be found in
>> frameworks/base/core/java/android/os/Bundle.java, but there could be
>> other. The size of the bundle is stored in the parcel so the end of
>> the bundle will be wrong if the bundle contains a flat_binder_object
>> that the driver changes the size of. However, since the sending
>> process gets the parcel size from libbinder, changing libbinder to
>> always use the 64 bit version of flat_binder_object should work with
>> this code.
>>
>
> AFAICS the bundle size is stored as an int32 so there is no bit width
> issues between the sending and receiving process.
>
> When I mentioned that flat_binder_objects will be translated, I meant
> the whole transaction will be translated to preserve its user payload
> intact. Something like this:
>
>         32bit                        64bit
>   +----------------+    +--------------------+
>   | o o oo  o    xx| -> | oo oooo  oo    yyyy|
>   +----------------+    +--------------------+
>
> where o are binder objects, spaces are user data and x,y are offsets
> pointers to binder objects (they are size_t so they need translation
> as well).
>
> As long as the application does not use absolute offsets in the
> payload and as long as the data types stored in the payload are fixed
> bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of
> intptr) doing the translation in kernel should be fine. I checked
> libbinder and both assumptions seems to be true (except in a few cases
> for the later which I already mentioned)
>
> So, what am I missing?

The bundle size is wrong in the receiving process if the driver change
the size of a flat_binder_object stored in the bundle.

A simplified example where a flat_binder_object is a single pointer,
and a bundle only adds a size to the data stored:
If you send a bundle with an object and an int <inta> followed by an
int <intb> from a 32 bit process to a 64 bit process you would get
this transformation of the data (offsets not shown):
bundle(obj, inta), int(intb) => parcel_32(8,obj,inta,intb) =>
parcel_64(8,objl,objh,inta,intb) => bundle(obj), int(inta), int(intb)

The first bundle argument is missing an item in the receiving process,
and the second argument is <inta> instead of <intb>.

-- 
Arve Hjønnevåg

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Octavian Purdila]

On Fri, Dec 13, 2013 at 7:14 AM, Arve Hjønnevåg <arve@android.com> wrote:
> On Thu, Dec 12, 2013 at 12:45 AM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Thu, Dec 12, 2013 at 1:00 AM, Arve Hjønnevåg <arve@android.com> wrote:
>>> On Wed, Dec 11, 2013 at 10:10 AM, Octavian Purdila
>>> <octavian.purdila@intel.com> wrote:
>>>> On Wed, Dec 11, 2013 at 5:21 AM, Arve Hjønnevåg <arve@android.com> wrote:
>>>>>
>>>>> Assuming you are talking about a kernel compat layer that translates
>>>>> the flat_binder_object structs as they pass between 32 bit and 64 bit
>>>>> processes, that will not always work. The data portion of the message
>>>>> sometimes contain size values that are invisible to the kernel, but
>>>>> these values will be wrong if the kernel move data to make room for a
>>>>> different size flat_binder_object.
>>>>>
>>>>
>>>> Hi Arve,
>>>>
>>>> Yes, I was talking about translating flat_binder_objects.
>>>>
>>>> I understand the potential issue for the user data payload, however,
>>>> since most applications will use libbinder, the only problematic case
>>>> is readIntPtr/writeIntPtr, which we can deprecate and convert
>>>> applications that use it to readInt64. AFAICS there is only one user
>>>> in the AOSP for this API (libmedia).
>>>>
>>>> If you are referring to data blobs that application parses I don't
>>>> think there is anything we can do, even at libbinder level.
>>>>
>>>> Can you give me an example of the sort of problems you see?
>>>>
>>>> Thanks,
>>>> Tavi
>>>
>>> The specific problem I was told about can be found in
>>> frameworks/base/core/java/android/os/Bundle.java, but there could be
>>> other. The size of the bundle is stored in the parcel so the end of
>>> the bundle will be wrong if the bundle contains a flat_binder_object
>>> that the driver changes the size of. However, since the sending
>>> process gets the parcel size from libbinder, changing libbinder to
>>> always use the 64 bit version of flat_binder_object should work with
>>> this code.
>>>
>>
>> AFAICS the bundle size is stored as an int32 so there is no bit width
>> issues between the sending and receiving process.
>>
>> When I mentioned that flat_binder_objects will be translated, I meant
>> the whole transaction will be translated to preserve its user payload
>> intact. Something like this:
>>
>>         32bit                        64bit
>>   +----------------+    +--------------------+
>>   | o o oo  o    xx| -> | oo oooo  oo    yyyy|
>>   +----------------+    +--------------------+
>>
>> where o are binder objects, spaces are user data and x,y are offsets
>> pointers to binder objects (they are size_t so they need translation
>> as well).
>>
>> As long as the application does not use absolute offsets in the
>> payload and as long as the data types stored in the payload are fixed
>> bit width across the 32bit/64bit ABI (e.g. int32, int64 instead of
>> intptr) doing the translation in kernel should be fine. I checked
>> libbinder and both assumptions seems to be true (except in a few cases
>> for the later which I already mentioned)
>>
>> So, what am I missing?
>
> The bundle size is wrong in the receiving process if the driver change
> the size of a flat_binder_object stored in the bundle.
>
> A simplified example where a flat_binder_object is a single pointer,
> and a bundle only adds a size to the data stored:
> If you send a bundle with an object and an int <inta> followed by an
> int <intb> from a 32 bit process to a 64 bit process you would get
> this transformation of the data (offsets not shown):
> bundle(obj, inta), int(intb) => parcel_32(8,obj,inta,intb) =>
> parcel_64(8,objl,objh,inta,intb) => bundle(obj), int(inta), int(intb)
>
> The first bundle argument is missing an item in the receiving process,
> and the second argument is <inta> instead of <intb>.
>

Thanks Arve, I understand the problem now. When I first looked at the
code I thought that the bundle stores the number of objects instead of
its size.

In this case, changing libbinder (and ServiceManager) to use 64bit
structures when talking with 64bit kernels seems the only reasonable
approach.

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[One Thousand Gnomes]

On Wed, 4 Dec 2013 10:35:54 -0800
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Dec 04, 2013 at 06:09:41PM +0000, Serban Constantinescu wrote:
> > +#define size_helper(x) ({						    \
> > +	size_t __size;							    \
> > +	if (!is_compat_task())						    \
> > +		__size = sizeof(x);					    \
> > +	else if (sizeof(x) == sizeof(struct flat_binder_object))	    \
> > +		__size = sizeof(struct compat_flat_binder_object);	    \
> > +	else if (sizeof(x) == sizeof(struct binder_transaction_data))	    \
> > +		__size = sizeof(struct compat_binder_transaction_data);	    \
> > +	else if (sizeof(x) == sizeof(size_t))				    \
> > +		__size = sizeof(compat_size_t);				    \
> > +	else								    \
> > +		 BUG();							    \
> > +	__size;								    \
> > +	})
> 
> Ick.
> 
> First off, no driver should ever be able to crash the kernel, which you
> just did.

And which would appear to mean that this code hasn't actually been
tested - at least not properly with error cases ?

You talk about type safety too but your code is already full of
"put_user(node->ptr, (void * __user *)ptr))"

The binder_copy_to_user thing is pretty confusingly named - it sounds
like a wrapper but is in fact a whole set of operations with two
different values and extra cookie structures and the like

Would something like

binder_put_userptr(mode->ptr, &ptr)

perhaps be a shade easier to follow as a set of changes, and less clunky ?


And 3/9 you could have done a clean up .. instead of replacing endless
repeats of 

-			cmd == BC_INCREFS_DONE ?
-			"BC_INCREFS_DONE" :
-			"BC_ACQUIRE_DONE",
+			acquire ?
+			"BC_ACQUIRE_DONE" :
+			"BC_INCREFS_DONE",

couldn't you do that bit in just one place ?

Ditto


-			cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+			request ?
 			"BC_REQUEST_DEATH_NOTIFICATION" :
 			"BC_CLEAR_DEATH_NOTIFICATION",


The "_helper" stuff with type and size magic also really obfuscates the
code horribly


+static inline struct flat_binder_object *copy_flat_binder_object(void
__user *ptr) +{
+	return (struct flat_binder_object *)ptr;
+}


What were you arguing about type safety again ?


While I'm tempted to answer "and that children is what happens when you
don't take your interfaces mainstream and peer review them in the first
place" I appreciate it won't help ;-)

I think I'd rather see the structures fixed up to be correct and properly
type stable for 64bit on a 64bit box including u64 user pointers.

For 32bit then yes you probably have to do something icky like


struct binder_foo64 {
}

struct binder_foo_compat {
}

#if 32bit
#define binder_foo binder_foo_compat
#else
#define binder_foo binder_foo64
#endif

but I do think it would make the rest of the code look less like a lesson
on pointer and GNU extension obfuscation and when 32bit finally gets
buried the uglies can be removed.

Alan

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[Colin Cross]

On Wed, Dec 4, 2013 at 3:21 PM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Wed, 4 Dec 2013 10:35:54 -0800
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Wed, Dec 04, 2013 at 06:09:41PM +0000, Serban Constantinescu wrote:
>> > +#define size_helper(x) ({                                              \
>> > +   size_t __size;                                                      \
>> > +   if (!is_compat_task())                                              \
>> > +           __size = sizeof(x);                                         \
>> > +   else if (sizeof(x) == sizeof(struct flat_binder_object))            \
>> > +           __size = sizeof(struct compat_flat_binder_object);          \
>> > +   else if (sizeof(x) == sizeof(struct binder_transaction_data))       \
>> > +           __size = sizeof(struct compat_binder_transaction_data);     \
>> > +   else if (sizeof(x) == sizeof(size_t))                               \
>> > +           __size = sizeof(compat_size_t);                             \
>> > +   else                                                                \
>> > +            BUG();                                                     \
>> > +   __size;                                                             \
>> > +   })
>>
>> Ick.
>>
>> First off, no driver should ever be able to crash the kernel, which you
>> just did.
>
> And which would appear to mean that this code hasn't actually been
> tested - at least not properly with error cases ?
>
> You talk about type safety too but your code is already full of
> "put_user(node->ptr, (void * __user *)ptr))"

None of this (the patch series or the original code) is mine.  My
question was more of a general one on designing ioctls, as well as
concerns with changing the existing 32-bit api.

Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer

[One Thousand Gnomes]

> None of this (the patch series or the original code) is mine.  My

Sorry mistraced the attribution sequence

> question was more of a general one on designing ioctls, as well as
> concerns with changing the existing 32-bit api.

I think in general my advice would be:

If its already been screwed up
- sort the structures out for the 64bit version
- make the 32bit version and compat ones clean copies of what they are now
- don't try and pull stunts with alignof and other such tricks because
	- some poor bugger will have to debug it one day (and it might be
		  you some years after you forget how it worked)
	- most of our security holes show up in complex data parsing paths
- figure out whether its better to do compat fixups or just have 64bit use
  new structures and new ioctl numbers with the 32bit ones working but
  with 32bit offsets regardless of 32/64bit CPU mode.


and for new stuff
- design for 64bit safety in advance
- watch the padding rules and user alignment
- consider leaving some spare zero space on the end of the structs

Alan