[PATCH 0/4] binder: optimize handle generation logic

[PATCH 0/4] binder: optimize handle generation logic

[Carlos Llamas]

This patchset adds the ability for userspace to select a faster handle
generation logic. This is implemented through a new ioctl that enables
certain functionality on a "per-proc" basis, this is required in order
to maintain backwards compatibility.

Cc: Tim Murray <timmurray@google.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Alice Ryhl <aliceryhl@google.com>  
Cc: Martijn Coenen <maco@android.com>
Cc: Todd Kjos <tkjos@android.com>
Cc: John Stultz <jstultz@google.com>

Carlos Llamas (4):
  binder: introduce BINDER_SET_PROC_FLAGS ioctl
  binder: migrate ioctl to new PF_SPAM_DETECTION
  binder: add support for PF_LARGE_HANDLES
  binder: fix max_thread type inconsistency

 drivers/android/binder.c            | 78 +++++++++++++++++++++++------
 drivers/android/binder_internal.h   | 10 ++--
 include/uapi/linux/android/binder.h | 13 ++++-
 3 files changed, 83 insertions(+), 18 deletions(-)

-- 
2.44.0.683.g7961c838ac-goog

[PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Carlos Llamas]

This new ioctl enables userspace to control the individual behavior of
the 'struct binder_proc' instance via flags. The driver validates and
returns the supported subset. Some existing ioctls are migrated to use
these flags in subsequent commits.

Suggested-by: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c            | 25 +++++++++++++++++++++++++
 drivers/android/binder_internal.h   |  4 +++-
 include/uapi/linux/android/binder.h |  6 ++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bad28cf42010..e0d193bfb237 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
 	return 0;
 }
 
+static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
+				       u32 __user *user)
+{
+	u32 flags;
+
+	if (get_user(flags, user))
+		return -EFAULT;
+
+	binder_inner_proc_lock(proc);
+	flags &= PF_SUPPORTED_FLAGS_MASK;
+	proc->flags = flags;
+	binder_inner_proc_unlock(proc);
+
+	/* confirm supported flags with user */
+	if (put_user(flags, user))
+		return -EFAULT;
+
+	return 0;
+}
+
 static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int ret;
@@ -5542,6 +5562,11 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (ret < 0)
 			goto err;
 		break;
+	case BINDER_SET_PROC_FLAGS:
+		ret = binder_ioctl_set_proc_flags(proc, ubuf);
+		if (ret < 0)
+			goto err;
+		break;
 	default:
 		ret = -EINVAL;
 		goto err;
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 7270d4d22207..a22e64cddbae 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -346,6 +346,8 @@ struct binder_ref {
  * @cred                  struct cred associated with the `struct file`
  *                        in binder_open()
  *                        (invariant after initialized)
+ * @flags:                enum proc_flags set via BINDER_SET_PROC_FLAGS.
+ *                        (protected by @inner_lock)
  * @deferred_work_node:   element for binder_deferred_list
  *                        (protected by binder_deferred_lock)
  * @deferred_work:        bitmap of deferred work to perform
@@ -409,6 +411,7 @@ struct binder_proc {
 	int pid;
 	struct task_struct *tsk;
 	const struct cred *cred;
+	u32 flags;
 	struct hlist_node deferred_work_node;
 	int deferred_work;
 	int outstanding_txns;
@@ -417,7 +420,6 @@ struct binder_proc {
 	bool sync_recv;
 	bool async_recv;
 	wait_queue_head_t freeze_wait;
-
 	struct list_head todo;
 	struct binder_stats stats;
 	struct list_head delivered_death;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index d44a8118b2ed..281a8e2e734e 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -251,6 +251,11 @@ struct binder_extended_error {
 	__s32	param;
 };
 
+/* Used with BINDER_SET_PROC_FLAGS ioctl */
+enum proc_flags {
+	PF_SUPPORTED_FLAGS_MASK,
+};
+
 enum {
 	BINDER_WRITE_READ		= _IOWR('b', 1, struct binder_write_read),
 	BINDER_SET_IDLE_TIMEOUT		= _IOW('b', 3, __s64),
@@ -266,6 +271,7 @@ enum {
 	BINDER_GET_FROZEN_INFO		= _IOWR('b', 15, struct binder_frozen_status_info),
 	BINDER_ENABLE_ONEWAY_SPAM_DETECTION	= _IOW('b', 16, __u32),
 	BINDER_GET_EXTENDED_ERROR	= _IOWR('b', 17, struct binder_extended_error),
+	BINDER_SET_PROC_FLAGS		= _IOWR('b', 18, __u32),
 };
 
 /*
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 2/4] binder: migrate ioctl to new PF_SPAM_DETECTION

[Carlos Llamas]

Deprecate the BINDER_ENABLE_ONEWAY_SPAM_DETECTION ioctl in favor of the
new PF_SPAM_DETECTION flag. The ioctl can be lumped together with other
flags to avoid a separate system call. The driver still supports the old
ioctl for backwards compatibility.

Suggested-by: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c            | 7 ++++---
 drivers/android/binder_internal.h   | 1 -
 include/uapi/linux/android/binder.h | 8 ++++++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e0d193bfb237..54d27dbf1de2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4486,8 +4486,8 @@ static int binder_thread_read(struct binder_proc *proc,
 		case BINDER_WORK_TRANSACTION_COMPLETE:
 		case BINDER_WORK_TRANSACTION_PENDING:
 		case BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT: {
-			if (proc->oneway_spam_detection_enabled &&
-				   w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
+			if (proc->flags & PF_SPAM_DETECTION &&
+			    w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
 				cmd = BR_ONEWAY_SPAM_SUSPECT;
 			else if (w->type == BINDER_WORK_TRANSACTION_PENDING)
 				cmd = BR_TRANSACTION_PENDING_FROZEN;
@@ -5553,7 +5553,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		}
 		binder_inner_proc_lock(proc);
-		proc->oneway_spam_detection_enabled = (bool)enable;
+		proc->flags &= ~PF_SPAM_DETECTION;
+		proc->flags |= enable & PF_SPAM_DETECTION;
 		binder_inner_proc_unlock(proc);
 		break;
 	}
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index a22e64cddbae..3312fe93a306 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -434,7 +434,6 @@ struct binder_proc {
 	spinlock_t inner_lock;
 	spinlock_t outer_lock;
 	struct dentry *binderfs_entry;
-	bool oneway_spam_detection_enabled;
 };
 
 /**
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 281a8e2e734e..972f402415c2 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -253,7 +253,9 @@ struct binder_extended_error {
 
 /* Used with BINDER_SET_PROC_FLAGS ioctl */
 enum proc_flags {
-	PF_SUPPORTED_FLAGS_MASK,
+	PF_SPAM_DETECTION	= (1 << 0), /* enable oneway spam detection */
+
+	PF_SUPPORTED_FLAGS_MASK = PF_SPAM_DETECTION,
 };
 
 enum {
@@ -269,9 +271,11 @@ enum {
 	BINDER_SET_CONTEXT_MGR_EXT	= _IOW('b', 13, struct flat_binder_object),
 	BINDER_FREEZE			= _IOW('b', 14, struct binder_freeze_info),
 	BINDER_GET_FROZEN_INFO		= _IOWR('b', 15, struct binder_frozen_status_info),
-	BINDER_ENABLE_ONEWAY_SPAM_DETECTION	= _IOW('b', 16, __u32),
 	BINDER_GET_EXTENDED_ERROR	= _IOWR('b', 17, struct binder_extended_error),
 	BINDER_SET_PROC_FLAGS		= _IOWR('b', 18, __u32),
+
+	/* This is deprecated, use BINDER_SET_PROC_FLAGS instead. */
+	BINDER_ENABLE_ONEWAY_SPAM_DETECTION	= _IOW('b', 16, __u32),
 };
 
 /*
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 3/4] binder: add support for PF_LARGE_HANDLES

[Carlos Llamas]

Introduce the PF_LARGE_HANDLES flag to enable the use of monotonically
increasing counters to generate handles. This improves performance in
transactions when dealing with a large number of references.

The legacy logic performs an inorder traversal of an rbtree to find the
smallest unused handle. This limitation is due to userspace using the
handles as indexes (e.g. in vectors). The new logic scales much better
but requires userspace to support large handle numbers.

The benchmark below with 100,000 references shows the performance gains
in binder_get_ref_for_node_olocked() calls with PF_LARGE_HANDLES.

  [  167.855945] binder_get_ref_for_node_olocked: 17us (flag on)
  [  237.088072] binder_get_ref_for_node_olocked: 18178us (flag off)

Suggested-by: Tim Murray <timmurray@google.com>
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c            | 44 ++++++++++++++++++++++-------
 drivers/android/binder_internal.h   |  3 ++
 include/uapi/linux/android/binder.h |  3 +-
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 54d27dbf1de2..f120a24c9ae6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1045,6 +1045,35 @@ static struct binder_ref *binder_get_ref_olocked(struct binder_proc *proc,
 	return NULL;
 }
 
+static u32 next_ref_desc(struct binder_proc *proc, struct binder_node *node)
+{
+	struct binder_ref *ref;
+	struct rb_node *n;
+	u32 desc;
+
+	/* Handle 0 is reserved for context manager refs */
+	if (node == proc->context->binder_context_mgr_node)
+		return 0;
+
+	/* Get the next handle (non-zero) */
+	if (proc->flags & PF_LARGE_HANDLES)
+		return proc->next_ref_desc++ ? : proc->next_ref_desc++;
+
+	/*
+	 * Userspace doesn't support large handles. Find the smallest
+	 * unused descriptor by doing an in-order traversal (slow).
+	 */
+	desc = 1;
+	for (n = rb_first(&proc->refs_by_desc); n; n = rb_next(n)) {
+		ref = rb_entry(n, struct binder_ref, rb_node_desc);
+		if (ref->data.desc > desc)
+			break;
+		desc = ref->data.desc + 1;
+	}
+
+	return desc;
+}
+
 /**
  * binder_get_ref_for_node_olocked() - get the ref associated with given node
  * @proc:	binder_proc that owns the ref
@@ -1068,11 +1097,9 @@ static struct binder_ref *binder_get_ref_for_node_olocked(
 					struct binder_node *node,
 					struct binder_ref *new_ref)
 {
-	struct binder_context *context = proc->context;
 	struct rb_node **p = &proc->refs_by_node.rb_node;
 	struct rb_node *parent = NULL;
 	struct binder_ref *ref;
-	struct rb_node *n;
 
 	while (*p) {
 		parent = *p;
@@ -1095,14 +1122,8 @@ static struct binder_ref *binder_get_ref_for_node_olocked(
 	rb_link_node(&new_ref->rb_node_node, parent, p);
 	rb_insert_color(&new_ref->rb_node_node, &proc->refs_by_node);
 
-	new_ref->data.desc = (node == context->binder_context_mgr_node) ? 0 : 1;
-	for (n = rb_first(&proc->refs_by_desc); n != NULL; n = rb_next(n)) {
-		ref = rb_entry(n, struct binder_ref, rb_node_desc);
-		if (ref->data.desc > new_ref->data.desc)
-			break;
-		new_ref->data.desc = ref->data.desc + 1;
-	}
-
+retry:
+	new_ref->data.desc = next_ref_desc(proc, node);
 	p = &proc->refs_by_desc.rb_node;
 	while (*p) {
 		parent = *p;
@@ -1112,6 +1133,8 @@ static struct binder_ref *binder_get_ref_for_node_olocked(
 			p = &(*p)->rb_left;
 		else if (new_ref->data.desc > ref->data.desc)
 			p = &(*p)->rb_right;
+		else if (proc->flags & PF_LARGE_HANDLES)
+			goto retry;
 		else
 			BUG();
 	}
@@ -5663,6 +5686,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
 	get_task_struct(current->group_leader);
 	proc->tsk = current->group_leader;
 	proc->cred = get_cred(filp->f_cred);
+	proc->next_ref_desc = 1;
 	INIT_LIST_HEAD(&proc->todo);
 	init_waitqueue_head(&proc->freeze_wait);
 	proc->default_priority = task_nice(current);
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 3312fe93a306..221ab7a6384a 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -337,6 +337,8 @@ struct binder_ref {
  *                        (protected by @outer_lock)
  * @refs_by_node:         rbtree of refs ordered by ref->node
  *                        (protected by @outer_lock)
+ * @next_ref_desc:        monotonic wrap-around counter to get the next handle
+ *                        (protected by @outer_lock)
  * @waiting_threads:      threads currently waiting for proc work
  *                        (protected by @inner_lock)
  * @pid                   PID of group_leader of process
@@ -407,6 +409,7 @@ struct binder_proc {
 	struct rb_root nodes;
 	struct rb_root refs_by_desc;
 	struct rb_root refs_by_node;
+	u32 next_ref_desc;
 	struct list_head waiting_threads;
 	int pid;
 	struct task_struct *tsk;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 972f402415c2..d257ab8689ce 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -254,8 +254,9 @@ struct binder_extended_error {
 /* Used with BINDER_SET_PROC_FLAGS ioctl */
 enum proc_flags {
 	PF_SPAM_DETECTION	= (1 << 0), /* enable oneway spam detection */
+	PF_LARGE_HANDLES	= (1 << 1), /* use large reference handles */
 
-	PF_SUPPORTED_FLAGS_MASK = PF_SPAM_DETECTION,
+	PF_SUPPORTED_FLAGS_MASK = (PF_SPAM_DETECTION | PF_LARGE_HANDLES),
 };
 
 enum {
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 4/4] binder: fix max_thread type inconsistency

[Carlos Llamas]

The type defined for the BINDER_SET_MAX_THREADS ioctl was changed from
size_t to __u32 in order to avoid incompatibility issues between 32 and
64-bit kernels. However, the internal types used to copy from user and
store the value were never updated. Use u32 to fix the inconsistency.

Fixes: a9350fc859ae ("staging: android: binder: fix BINDER_SET_MAX_THREADS declaration")
Reported-by: Arve Hjønnevåg <arve@android.com>
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c          | 2 +-
 drivers/android/binder_internal.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f120a24c9ae6..2596cbfa16d0 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5408,7 +5408,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		break;
 	case BINDER_SET_MAX_THREADS: {
-		int max_threads;
+		u32 max_threads;
 
 		if (copy_from_user(&max_threads, ubuf,
 				   sizeof(max_threads))) {
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 221ab7a6384a..3c522698083f 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -426,7 +426,7 @@ struct binder_proc {
 	struct list_head todo;
 	struct binder_stats stats;
 	struct list_head delivered_death;
-	int max_threads;
+	u32 max_threads;
 	int requested_threads;
 	int requested_threads_started;
 	int tmp_ref;
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH 4/4] binder: fix max_thread type inconsistency

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 07:13:44PM +0000, Carlos Llamas wrote:
> The type defined for the BINDER_SET_MAX_THREADS ioctl was changed from
> size_t to __u32 in order to avoid incompatibility issues between 32 and
> 64-bit kernels. However, the internal types used to copy from user and
> store the value were never updated. Use u32 to fix the inconsistency.
> 
> Fixes: a9350fc859ae ("staging: android: binder: fix BINDER_SET_MAX_THREADS declaration")
> Reported-by: Arve Hjønnevåg <arve@android.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c          | 2 +-
>  drivers/android/binder_internal.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Why does only patch 4/4 need to go into the tree now, and as a stable
backport, but the first 3 do not?  Shouldn't this be two different
series of patches, one 3 long, and one 1 long, to go to the different
branches (next and linus)?

thanks,

greg k-h

Re: [PATCH 2/4] binder: migrate ioctl to new PF_SPAM_DETECTION

[Alice Ryhl]

Carlos Llamas <cmllamas@google.com> writes:
> @@ -5553,7 +5553,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			goto err;
>  		}
>  		binder_inner_proc_lock(proc);
> -		proc->oneway_spam_detection_enabled = (bool)enable;
> +		proc->flags &= ~PF_SPAM_DETECTION;
> +		proc->flags |= enable & PF_SPAM_DETECTION;

The bitwise and in `enable & PF_SPAM_DETECTION` only works because
PF_SPAM_DETECTION happens to be equal to 1. This seems pretty fragile to
me. Would you be willing to do this instead?

proc->flags &= ~PF_SPAM_DETECTION;
if (enable)
	proc->flags |= PF_SPAM_DETECTION;


Carlos Llamas <cmllamas@google.com> writes:
> -			if (proc->oneway_spam_detection_enabled &&
> -				   w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> +			if (proc->flags & PF_SPAM_DETECTION &&
> +			    w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)

Maybe I am just not sufficiently familiar with C, but I had to look up
the operator precedence rules for this one. Could we add parenthesises
around `proc->flags & PF_SPAM_DETECTION`? Or even define a macro for it?

Alice

Re: [PATCH 3/4] binder: add support for PF_LARGE_HANDLES

[Alice Ryhl]

On Wed, Apr 17, 2024 at 9:15 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> Introduce the PF_LARGE_HANDLES flag to enable the use of monotonically
> increasing counters to generate handles. This improves performance in
> transactions when dealing with a large number of references.
>
> The legacy logic performs an inorder traversal of an rbtree to find the
> smallest unused handle. This limitation is due to userspace using the
> handles as indexes (e.g. in vectors). The new logic scales much better
> but requires userspace to support large handle numbers.
>
> The benchmark below with 100,000 references shows the performance gains
> in binder_get_ref_for_node_olocked() calls with PF_LARGE_HANDLES.
>
>   [  167.855945] binder_get_ref_for_node_olocked: 17us (flag on)
>   [  237.088072] binder_get_ref_for_node_olocked: 18178us (flag off)
>
> Suggested-by: Tim Murray <timmurray@google.com>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Alice Ryhl]

Carlos Llamas <cmllamas@google.com> writes:
> This new ioctl enables userspace to control the individual behavior of
> the 'struct binder_proc' instance via flags. The driver validates and
> returns the supported subset. Some existing ioctls are migrated to use
> these flags in subsequent commits.
> 
> Suggested-by: Arve Hjønnevåg <arve@android.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
>  drivers/android/binder_internal.h   |  4 +++-
>  include/uapi/linux/android/binder.h |  6 ++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index bad28cf42010..e0d193bfb237 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
>  	return 0;
>  }
>  
> +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> +				       u32 __user *user)
> +{
> +	u32 flags;
> +
> +	if (get_user(flags, user))
> +		return -EFAULT;
> +
> +	binder_inner_proc_lock(proc);
> +	flags &= PF_SUPPORTED_FLAGS_MASK;
> +	proc->flags = flags;
> +	binder_inner_proc_unlock(proc);
> +
> +	/* confirm supported flags with user */
> +	if (put_user(flags, user))
> +		return -EFAULT;
> +
> +	return 0;
> +}

I'm just thinking out loud here, but is this the best API for this
ioctl? Using this API, if I want to toggle the oneway-spam-detection
flag, then I can't do so without knowing the value of all other flags,
and I also need to synchronize all calls to this ioctl.

That's fine for the current use-case where these flags are only set
during startup, but are we confident that no future flag will be toggled
while a process is active?

How about these alternatives?

1. Userspace passes two masks, one containing bits to set, and another
   containing bits to unset. Userspace returns new value of flags. (If
   the same bit is set in both masks, we can fail with EINVAL.)

2. Compare and swap. Userspace passes the expected previous value and
   the desired new value. The kernel returns the actual previous value
   and updates it only if userspace gave the right previous value.

3. Set or unset only. Userspace passes a boolean and a mask. Boolean
   determines whether userspace wants to set or unset the bits set in
   the mask.

I don't know what the usual kernel convention is for this kind of
ioctl, so I'm happy with whatever you all think is best.

Alice

Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Carlos Llamas]

On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote:
> Carlos Llamas <cmllamas@google.com> writes:
> > This new ioctl enables userspace to control the individual behavior of
> > the 'struct binder_proc' instance via flags. The driver validates and
> > returns the supported subset. Some existing ioctls are migrated to use
> > these flags in subsequent commits.
> > 
> > Suggested-by: Arve Hjønnevåg <arve@android.com>
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
> >  drivers/android/binder_internal.h   |  4 +++-
> >  include/uapi/linux/android/binder.h |  6 ++++++
> >  3 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index bad28cf42010..e0d193bfb237 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> >  	return 0;
> >  }
> >  
> > +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> > +				       u32 __user *user)
> > +{
> > +	u32 flags;
> > +
> > +	if (get_user(flags, user))
> > +		return -EFAULT;
> > +
> > +	binder_inner_proc_lock(proc);
> > +	flags &= PF_SUPPORTED_FLAGS_MASK;
> > +	proc->flags = flags;
> > +	binder_inner_proc_unlock(proc);
> > +
> > +	/* confirm supported flags with user */
> > +	if (put_user(flags, user))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> 
> I'm just thinking out loud here, but is this the best API for this
> ioctl? Using this API, if I want to toggle the oneway-spam-detection
> flag, then I can't do so without knowing the value of all other flags,
> and I also need to synchronize all calls to this ioctl.
> 
> That's fine for the current use-case where these flags are only set
> during startup, but are we confident that no future flag will be toggled
> while a process is active?

hmmm, this is a very good point. It would probably lead to userspace
having to cache its flags for every binder instance. This is not a good
solution at all.

> 
> How about these alternatives?
> 
> 1. Userspace passes two masks, one containing bits to set, and another
>    containing bits to unset. Userspace returns new value of flags. (If
>    the same bit is set in both masks, we can fail with EINVAL.)
> 
> 2. Compare and swap. Userspace passes the expected previous value and
>    the desired new value. The kernel returns the actual previous value
>    and updates it only if userspace gave the right previous value.
> 
> 3. Set or unset only. Userspace passes a boolean and a mask. Boolean
>    determines whether userspace wants to set or unset the bits set in
>    the mask.
> 
> I don't know what the usual kernel convention is for this kind of
> ioctl, so I'm happy with whatever you all think is best.

I've never come across these types of alternatives personally. What I've
seen however, is the typical SET/GET ioctl pairs. This is a "simpler"
interface I guess but it has the downside of an extra roundtrip. e.g.

	ioctl(fd, BINDER_GET_PROC_FLAGS, &flags);
	flags |= BF_LARGE_HANDLES;
	ioctl(fd, BINDER_SET_PROC_FLAGS, &flags);

What seems tempting about the SET/GET pair is that we could replace the
BINDER_ENABLE_ONEWAY_SPAM_DETECTION with the SET. Instead of maintaining
legacy code for the "deprecated" ioctl.

wdyt?

I'll have a second look at the alternatives you mentioned. Perhaps I can
reference some other existing ioctl that does something similar.

--
Carlos Llamas

Re: [PATCH 2/4] binder: migrate ioctl to new PF_SPAM_DETECTION

[Carlos Llamas]

On Thu, Apr 18, 2024 at 08:12:22AM +0000, Alice Ryhl wrote:
> Carlos Llamas <cmllamas@google.com> writes:
> > @@ -5553,7 +5553,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  			goto err;
> >  		}
> >  		binder_inner_proc_lock(proc);
> > -		proc->oneway_spam_detection_enabled = (bool)enable;
> > +		proc->flags &= ~PF_SPAM_DETECTION;
> > +		proc->flags |= enable & PF_SPAM_DETECTION;
> 
> The bitwise and in `enable & PF_SPAM_DETECTION` only works because
> PF_SPAM_DETECTION happens to be equal to 1. This seems pretty fragile to
> me. Would you be willing to do this instead?
> 
> proc->flags &= ~PF_SPAM_DETECTION;
> if (enable)
> 	proc->flags |= PF_SPAM_DETECTION;
> 

I don't think it is fragile since PF_SPAM_DETECTION is fixed. However,
I agree the code is missing context about the flag being bit 0 and your
version addresses this problem. So I'll take it for v2, thanks! 

> 
> Carlos Llamas <cmllamas@google.com> writes:
> > -			if (proc->oneway_spam_detection_enabled &&
> > -				   w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> > +			if (proc->flags & PF_SPAM_DETECTION &&
> > +			    w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> 
> Maybe I am just not sufficiently familiar with C, but I had to look up
> the operator precedence rules for this one. Could we add parenthesises
> around `proc->flags & PF_SPAM_DETECTION`? Or even define a macro for it?

I think this is fairly common in C but I can definitly add the extra
paranthesis if it helps.

--
Carlos Llamas

Re: [PATCH 4/4] binder: fix max_thread type inconsistency

[Carlos Llamas]

On Thu, Apr 18, 2024 at 06:40:52AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 17, 2024 at 07:13:44PM +0000, Carlos Llamas wrote:
> > The type defined for the BINDER_SET_MAX_THREADS ioctl was changed from
> > size_t to __u32 in order to avoid incompatibility issues between 32 and
> > 64-bit kernels. However, the internal types used to copy from user and
> > store the value were never updated. Use u32 to fix the inconsistency.
> > 
> > Fixes: a9350fc859ae ("staging: android: binder: fix BINDER_SET_MAX_THREADS declaration")
> > Reported-by: Arve Hjønnevåg <arve@android.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  drivers/android/binder.c          | 2 +-
> >  drivers/android/binder_internal.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Why does only patch 4/4 need to go into the tree now, and as a stable
> backport, but the first 3 do not?  Shouldn't this be two different
> series of patches, one 3 long, and one 1 long, to go to the different
> branches (next and linus)?

Yes, that is correct. Only patch 4/4 would need to be picked for linus
now and for stable. The others would go to next. Sorry, I was not aware
that sending them separately would be preferred.

I'll drop 4/4 patch from the series in v2. Let me know if you still need
me to send it again separately.

Thanks,
Carlos Llamas

Re: [PATCH 4/4] binder: fix max_thread type inconsistency

[Greg Kroah-Hartman]

On Sun, Apr 21, 2024 at 12:00:30AM +0000, Carlos Llamas wrote:
> On Thu, Apr 18, 2024 at 06:40:52AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 17, 2024 at 07:13:44PM +0000, Carlos Llamas wrote:
> > > The type defined for the BINDER_SET_MAX_THREADS ioctl was changed from
> > > size_t to __u32 in order to avoid incompatibility issues between 32 and
> > > 64-bit kernels. However, the internal types used to copy from user and
> > > store the value were never updated. Use u32 to fix the inconsistency.
> > > 
> > > Fixes: a9350fc859ae ("staging: android: binder: fix BINDER_SET_MAX_THREADS declaration")
> > > Reported-by: Arve Hjønnevåg <arve@android.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > ---
> > >  drivers/android/binder.c          | 2 +-
> > >  drivers/android/binder_internal.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > Why does only patch 4/4 need to go into the tree now, and as a stable
> > backport, but the first 3 do not?  Shouldn't this be two different
> > series of patches, one 3 long, and one 1 long, to go to the different
> > branches (next and linus)?
> 
> Yes, that is correct. Only patch 4/4 would need to be picked for linus
> now and for stable. The others would go to next. Sorry, I was not aware
> that sending them separately would be preferred.
> 
> I'll drop 4/4 patch from the series in v2. Let me know if you still need
> me to send it again separately.

Please do, thanks!

greg k-h

Re: [PATCH 4/4] binder: fix max_thread type inconsistency

[Carlos Llamas]

On Sun, Apr 21, 2024 at 08:39:23AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Apr 21, 2024 at 12:00:30AM +0000, Carlos Llamas wrote:
> > On Thu, Apr 18, 2024 at 06:40:52AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Apr 17, 2024 at 07:13:44PM +0000, Carlos Llamas wrote:
> > > > The type defined for the BINDER_SET_MAX_THREADS ioctl was changed from
> > > > size_t to __u32 in order to avoid incompatibility issues between 32 and
> > > > 64-bit kernels. However, the internal types used to copy from user and
> > > > store the value were never updated. Use u32 to fix the inconsistency.
> > > > 
> > > > Fixes: a9350fc859ae ("staging: android: binder: fix BINDER_SET_MAX_THREADS declaration")
> > > > Reported-by: Arve Hjønnevåg <arve@android.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > ---
> > > >  drivers/android/binder.c          | 2 +-
> > > >  drivers/android/binder_internal.h | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > Why does only patch 4/4 need to go into the tree now, and as a stable
> > > backport, but the first 3 do not?  Shouldn't this be two different
> > > series of patches, one 3 long, and one 1 long, to go to the different
> > > branches (next and linus)?
> > 
> > Yes, that is correct. Only patch 4/4 would need to be picked for linus
> > now and for stable. The others would go to next. Sorry, I was not aware
> > that sending them separately would be preferred.
> > 
> > I'll drop 4/4 patch from the series in v2. Let me know if you still need
> > me to send it again separately.
> 
> Please do, thanks!
> 
> greg k-h
> 

Ok, done. The separated patch is here:
https://lore.kernel.org/all/20240421173750.3117808-1-cmllamas@google.com/

Thanks,
Carlos Llamas

Re: [PATCH 2/4] binder: migrate ioctl to new PF_SPAM_DETECTION

[Alice Ryhl]

On Sun, Apr 21, 2024 at 1:49 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Thu, Apr 18, 2024 at 08:12:22AM +0000, Alice Ryhl wrote:
> > Carlos Llamas <cmllamas@google.com> writes:
> > > @@ -5553,7 +5553,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > >                     goto err;
> > >             }
> > >             binder_inner_proc_lock(proc);
> > > -           proc->oneway_spam_detection_enabled = (bool)enable;
> > > +           proc->flags &= ~PF_SPAM_DETECTION;
> > > +           proc->flags |= enable & PF_SPAM_DETECTION;
> >
> > The bitwise and in `enable & PF_SPAM_DETECTION` only works because
> > PF_SPAM_DETECTION happens to be equal to 1. This seems pretty fragile to
> > me. Would you be willing to do this instead?
> >
> > proc->flags &= ~PF_SPAM_DETECTION;
> > if (enable)
> >       proc->flags |= PF_SPAM_DETECTION;
> >
>
> I don't think it is fragile since PF_SPAM_DETECTION is fixed. However,
> I agree the code is missing context about the flag being bit 0 and your
> version addresses this problem. So I'll take it for v2, thanks!

Thanks! By fragile I mean that it could result in future mistakes,
e.g. somebody could copy this code and use it elsewhere with a
different bit flag that might not be bit 0.

> > Carlos Llamas <cmllamas@google.com> writes:
> > > -                   if (proc->oneway_spam_detection_enabled &&
> > > -                              w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> > > +                   if (proc->flags & PF_SPAM_DETECTION &&
> > > +                       w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> >
> > Maybe I am just not sufficiently familiar with C, but I had to look up
> > the operator precedence rules for this one. Could we add parenthesises
> > around `proc->flags & PF_SPAM_DETECTION`? Or even define a macro for it?
>
> I think this is fairly common in C but I can definitly add the extra
> paranthesis if it helps.

Yeah, makes sense. Thanks!

With the mentioned changes, you may add:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Alice Ryhl]

On Sun, Apr 21, 2024 at 1:39 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote:
> > Carlos Llamas <cmllamas@google.com> writes:
> > > This new ioctl enables userspace to control the individual behavior of
> > > the 'struct binder_proc' instance via flags. The driver validates and
> > > returns the supported subset. Some existing ioctls are migrated to use
> > > these flags in subsequent commits.
> > >
> > > Suggested-by: Arve Hjønnevåg <arve@android.com>
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > ---
> > >  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
> > >  drivers/android/binder_internal.h   |  4 +++-
> > >  include/uapi/linux/android/binder.h |  6 ++++++
> > >  3 files changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index bad28cf42010..e0d193bfb237 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> > >     return 0;
> > >  }
> > >
> > > +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> > > +                                  u32 __user *user)
> > > +{
> > > +   u32 flags;
> > > +
> > > +   if (get_user(flags, user))
> > > +           return -EFAULT;
> > > +
> > > +   binder_inner_proc_lock(proc);
> > > +   flags &= PF_SUPPORTED_FLAGS_MASK;
> > > +   proc->flags = flags;
> > > +   binder_inner_proc_unlock(proc);
> > > +
> > > +   /* confirm supported flags with user */
> > > +   if (put_user(flags, user))
> > > +           return -EFAULT;
> > > +
> > > +   return 0;
> > > +}
> >
> > I'm just thinking out loud here, but is this the best API for this
> > ioctl? Using this API, if I want to toggle the oneway-spam-detection
> > flag, then I can't do so without knowing the value of all other flags,
> > and I also need to synchronize all calls to this ioctl.
> >
> > That's fine for the current use-case where these flags are only set
> > during startup, but are we confident that no future flag will be toggled
> > while a process is active?
>
> hmmm, this is a very good point. It would probably lead to userspace
> having to cache its flags for every binder instance. This is not a good
> solution at all.
>
> >
> > How about these alternatives?
> >
> > 1. Userspace passes two masks, one containing bits to set, and another
> >    containing bits to unset. Userspace returns new value of flags. (If
> >    the same bit is set in both masks, we can fail with EINVAL.)

To add to this one, one could also say that if a bit is set in both,
then the value is toggled.

> > 2. Compare and swap. Userspace passes the expected previous value and
> >    the desired new value. The kernel returns the actual previous value
> >    and updates it only if userspace gave the right previous value.
> >
> > 3. Set or unset only. Userspace passes a boolean and a mask. Boolean
> >    determines whether userspace wants to set or unset the bits set in
> >    the mask.
> >
> > I don't know what the usual kernel convention is for this kind of
> > ioctl, so I'm happy with whatever you all think is best.
>
> I've never come across these types of alternatives personally. What I've
> seen however, is the typical SET/GET ioctl pairs. This is a "simpler"
> interface I guess but it has the downside of an extra roundtrip. e.g.
>
>         ioctl(fd, BINDER_GET_PROC_FLAGS, &flags);
>         flags |= BF_LARGE_HANDLES;
>         ioctl(fd, BINDER_SET_PROC_FLAGS, &flags);
>
> What seems tempting about the SET/GET pair is that we could replace the
> BINDER_ENABLE_ONEWAY_SPAM_DETECTION with the SET. Instead of maintaining
> legacy code for the "deprecated" ioctl.
>
> wdyt?
>
> I'll have a second look at the alternatives you mentioned. Perhaps I can
> reference some other existing ioctl that does something similar.

Hmm. I don't think a get/set pair improves the situation much.
Userspace still needs a global mutex for making changes to the flag in
that case. Otherwise, two threads changing two different flags in
parallel could result in a race condition where one of the changes is
lost.

Alice

Re: [PATCH 2/4] binder: migrate ioctl to new PF_SPAM_DETECTION

[Carlos Llamas]

On Mon, Apr 22, 2024 at 10:52:57AM +0200, Alice Ryhl wrote:
> On Sun, Apr 21, 2024 at 1:49 AM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > On Thu, Apr 18, 2024 at 08:12:22AM +0000, Alice Ryhl wrote:
> > > Carlos Llamas <cmllamas@google.com> writes:
> > > > @@ -5553,7 +5553,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > > >                     goto err;
> > > >             }
> > > >             binder_inner_proc_lock(proc);
> > > > -           proc->oneway_spam_detection_enabled = (bool)enable;
> > > > +           proc->flags &= ~PF_SPAM_DETECTION;
> > > > +           proc->flags |= enable & PF_SPAM_DETECTION;
> > >
> > > The bitwise and in `enable & PF_SPAM_DETECTION` only works because
> > > PF_SPAM_DETECTION happens to be equal to 1. This seems pretty fragile to
> > > me. Would you be willing to do this instead?
> > >
> > > proc->flags &= ~PF_SPAM_DETECTION;
> > > if (enable)
> > >       proc->flags |= PF_SPAM_DETECTION;
> > >
> >
> > I don't think it is fragile since PF_SPAM_DETECTION is fixed. However,
> > I agree the code is missing context about the flag being bit 0 and your
> > version addresses this problem. So I'll take it for v2, thanks!
> 
> Thanks! By fragile I mean that it could result in future mistakes,
> e.g. somebody could copy this code and use it elsewhere with a
> different bit flag that might not be bit 0.

Oh, I see. Yeah that would be a problem.

> 
> > > Carlos Llamas <cmllamas@google.com> writes:
> > > > -                   if (proc->oneway_spam_detection_enabled &&
> > > > -                              w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> > > > +                   if (proc->flags & PF_SPAM_DETECTION &&
> > > > +                       w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> > >
> > > Maybe I am just not sufficiently familiar with C, but I had to look up
> > > the operator precedence rules for this one. Could we add parenthesises
> > > around `proc->flags & PF_SPAM_DETECTION`? Or even define a macro for it?
> >
> > I think this is fairly common in C but I can definitly add the extra
> > paranthesis if it helps.
> 
> Yeah, makes sense. Thanks!
> 
> With the mentioned changes, you may add:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Done. Thanks!

Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Carlos Llamas]

On Mon, Apr 22, 2024 at 10:56:31AM +0200, Alice Ryhl wrote:
> On Sun, Apr 21, 2024 at 1:39 AM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote:
> > > Carlos Llamas <cmllamas@google.com> writes:
> > > > This new ioctl enables userspace to control the individual behavior of
> > > > the 'struct binder_proc' instance via flags. The driver validates and
> > > > returns the supported subset. Some existing ioctls are migrated to use
> > > > these flags in subsequent commits.
> > > >
> > > > Suggested-by: Arve Hjønnevåg <arve@android.com>
> > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > ---
> > > >  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
> > > >  drivers/android/binder_internal.h   |  4 +++-
> > > >  include/uapi/linux/android/binder.h |  6 ++++++
> > > >  3 files changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index bad28cf42010..e0d193bfb237 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> > > >     return 0;
> > > >  }
> > > >
> > > > +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> > > > +                                  u32 __user *user)
> > > > +{
> > > > +   u32 flags;
> > > > +
> > > > +   if (get_user(flags, user))
> > > > +           return -EFAULT;
> > > > +
> > > > +   binder_inner_proc_lock(proc);
> > > > +   flags &= PF_SUPPORTED_FLAGS_MASK;
> > > > +   proc->flags = flags;
> > > > +   binder_inner_proc_unlock(proc);
> > > > +
> > > > +   /* confirm supported flags with user */
> > > > +   if (put_user(flags, user))
> > > > +           return -EFAULT;
> > > > +
> > > > +   return 0;
> > > > +}
> > >
> > > I'm just thinking out loud here, but is this the best API for this
> > > ioctl? Using this API, if I want to toggle the oneway-spam-detection
> > > flag, then I can't do so without knowing the value of all other flags,
> > > and I also need to synchronize all calls to this ioctl.
> > >
> > > That's fine for the current use-case where these flags are only set
> > > during startup, but are we confident that no future flag will be toggled
> > > while a process is active?
> >
> > hmmm, this is a very good point. It would probably lead to userspace
> > having to cache its flags for every binder instance. This is not a good
> > solution at all.
> >
> > >
> > > How about these alternatives?
> > >
> > > 1. Userspace passes two masks, one containing bits to set, and another
> > >    containing bits to unset. Userspace returns new value of flags. (If
> > >    the same bit is set in both masks, we can fail with EINVAL.)
> 
> To add to this one, one could also say that if a bit is set in both,
> then the value is toggled.
> 
> > > 2. Compare and swap. Userspace passes the expected previous value and
> > >    the desired new value. The kernel returns the actual previous value
> > >    and updates it only if userspace gave the right previous value.
> > >
> > > 3. Set or unset only. Userspace passes a boolean and a mask. Boolean
> > >    determines whether userspace wants to set or unset the bits set in
> > >    the mask.
> > >
> > > I don't know what the usual kernel convention is for this kind of
> > > ioctl, so I'm happy with whatever you all think is best.
> >
> > I've never come across these types of alternatives personally. What I've
> > seen however, is the typical SET/GET ioctl pairs. This is a "simpler"
> > interface I guess but it has the downside of an extra roundtrip. e.g.
> >
> >         ioctl(fd, BINDER_GET_PROC_FLAGS, &flags);
> >         flags |= BF_LARGE_HANDLES;
> >         ioctl(fd, BINDER_SET_PROC_FLAGS, &flags);
> >
> > What seems tempting about the SET/GET pair is that we could replace the
> > BINDER_ENABLE_ONEWAY_SPAM_DETECTION with the SET. Instead of maintaining
> > legacy code for the "deprecated" ioctl.
> >
> > wdyt?
> >
> > I'll have a second look at the alternatives you mentioned. Perhaps I can
> > reference some other existing ioctl that does something similar.
> 
> Hmm. I don't think a get/set pair improves the situation much.
> Userspace still needs a global mutex for making changes to the flag in
> that case. Otherwise, two threads changing two different flags in
> parallel could result in a race condition where one of the changes is
> lost.

I'm not sure this would ever be a problem, libbinder currently has a
mutex for this kind of things already and it seems unlikely that these
process-wide flags would be toggled outside of initial config. However,
it is worth discussing for sure as things can change.

I'm mainly concern about overloading what should be a very simple ioctl.
With that said, I think one more option that is fairly simple/common is
a SET/CLEAR ioctl pair. A little adaption from your 3rd option I think?
I would be fine with that.

Unfortunately, this would require new ioctl IDs so we would still need
to maintain the ONEWAY_SPAM thing. I suppose that's ok.

Sounds good?

--
Carlos Llamas

Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

[Alice Ryhl]

On Tue, Apr 23, 2024 at 12:48 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Mon, Apr 22, 2024 at 10:56:31AM +0200, Alice Ryhl wrote:
> > On Sun, Apr 21, 2024 at 1:39 AM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote:
> > > > Carlos Llamas <cmllamas@google.com> writes:
> > > > > This new ioctl enables userspace to control the individual behavior of
> > > > > the 'struct binder_proc' instance via flags. The driver validates and
> > > > > returns the supported subset. Some existing ioctls are migrated to use
> > > > > these flags in subsequent commits.
> > > > >
> > > > > Suggested-by: Arve Hjønnevåg <arve@android.com>
> > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > > ---
> > > > >  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
> > > > >  drivers/android/binder_internal.h   |  4 +++-
> > > > >  include/uapi/linux/android/binder.h |  6 ++++++
> > > > >  3 files changed, 34 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index bad28cf42010..e0d193bfb237 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> > > > >     return 0;
> > > > >  }
> > > > >
> > > > > +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> > > > > +                                  u32 __user *user)
> > > > > +{
> > > > > +   u32 flags;
> > > > > +
> > > > > +   if (get_user(flags, user))
> > > > > +           return -EFAULT;
> > > > > +
> > > > > +   binder_inner_proc_lock(proc);
> > > > > +   flags &= PF_SUPPORTED_FLAGS_MASK;
> > > > > +   proc->flags = flags;
> > > > > +   binder_inner_proc_unlock(proc);
> > > > > +
> > > > > +   /* confirm supported flags with user */
> > > > > +   if (put_user(flags, user))
> > > > > +           return -EFAULT;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > >
> > > > I'm just thinking out loud here, but is this the best API for this
> > > > ioctl? Using this API, if I want to toggle the oneway-spam-detection
> > > > flag, then I can't do so without knowing the value of all other flags,
> > > > and I also need to synchronize all calls to this ioctl.
> > > >
> > > > That's fine for the current use-case where these flags are only set
> > > > during startup, but are we confident that no future flag will be toggled
> > > > while a process is active?
> > >
> > > hmmm, this is a very good point. It would probably lead to userspace
> > > having to cache its flags for every binder instance. This is not a good
> > > solution at all.
> > >
> > > >
> > > > How about these alternatives?
> > > >
> > > > 1. Userspace passes two masks, one containing bits to set, and another
> > > >    containing bits to unset. Userspace returns new value of flags. (If
> > > >    the same bit is set in both masks, we can fail with EINVAL.)
> >
> > To add to this one, one could also say that if a bit is set in both,
> > then the value is toggled.
> >
> > > > 2. Compare and swap. Userspace passes the expected previous value and
> > > >    the desired new value. The kernel returns the actual previous value
> > > >    and updates it only if userspace gave the right previous value.
> > > >
> > > > 3. Set or unset only. Userspace passes a boolean and a mask. Boolean
> > > >    determines whether userspace wants to set or unset the bits set in
> > > >    the mask.
> > > >
> > > > I don't know what the usual kernel convention is for this kind of
> > > > ioctl, so I'm happy with whatever you all think is best.
> > >
> > > I've never come across these types of alternatives personally. What I've
> > > seen however, is the typical SET/GET ioctl pairs. This is a "simpler"
> > > interface I guess but it has the downside of an extra roundtrip. e.g.
> > >
> > >         ioctl(fd, BINDER_GET_PROC_FLAGS, &flags);
> > >         flags |= BF_LARGE_HANDLES;
> > >         ioctl(fd, BINDER_SET_PROC_FLAGS, &flags);
> > >
> > > What seems tempting about the SET/GET pair is that we could replace the
> > > BINDER_ENABLE_ONEWAY_SPAM_DETECTION with the SET. Instead of maintaining
> > > legacy code for the "deprecated" ioctl.
> > >
> > > wdyt?
> > >
> > > I'll have a second look at the alternatives you mentioned. Perhaps I can
> > > reference some other existing ioctl that does something similar.
> >
> > Hmm. I don't think a get/set pair improves the situation much.
> > Userspace still needs a global mutex for making changes to the flag in
> > that case. Otherwise, two threads changing two different flags in
> > parallel could result in a race condition where one of the changes is
> > lost.
>
> I'm not sure this would ever be a problem, libbinder currently has a
> mutex for this kind of things already and it seems unlikely that these
> process-wide flags would be toggled outside of initial config. However,
> it is worth discussing for sure as things can change.
>
> I'm mainly concern about overloading what should be a very simple ioctl.
> With that said, I think one more option that is fairly simple/common is
> a SET/CLEAR ioctl pair. A little adaption from your 3rd option I think?
> I would be fine with that.
>
> Unfortunately, this would require new ioctl IDs so we would still need
> to maintain the ONEWAY_SPAM thing. I suppose that's ok.

As long as your decision has taken the things I mentioned into
account, I'm happy with whatever you think is best. If you think a
GET/SET pair is reasonable because binder already has a mutex, then
that's fine with me.

Alice