[PATCH v3 0/3] Binder: Enable App Freezing Capability

[PATCH v3 0/3] Binder: Enable App Freezing Capability

[Li Li]

From: Li Li <dualli@google.com>

To improve the user experience when switching between recently used
applications, the background applications which are not currently needed
are cached in the memory. Normally, a well designed application will not
consume valuable CPU resources in the background. However, it's possible
some applications are not able or willing to behave as expected, wasting
energy even after being cached.

It is a good idea to freeze those applications when they're only being
kept alive for the sake of faster startup and energy saving. These kernel
patches will provide the necessary infrastructure for user space framework
to freeze and thaw a cached process, check the current freezing status and
correctly deal with outstanding binder transactions to frozen processes.

Changes in v2: avoid panic by using pr_warn for unexpected cases.
Changes in v3: improved errcode logic in binder_proc_transaction().

Marco Ballesio (3):
  binder: BINDER_FREEZE ioctl
  binder: use EINTR for interrupted wait for work
  binder: BINDER_GET_FROZEN_INFO ioctl

 drivers/android/binder.c            | 198 ++++++++++++++++++++++++++--
 drivers/android/binder_internal.h   |  18 +++
 include/uapi/linux/android/binder.h |  20 +++
 3 files changed, 224 insertions(+), 12 deletions(-)

-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH v3 1/3] binder: BINDER_FREEZE ioctl

[Li Li]

From: Marco Ballesio <balejs@google.com>

Frozen tasks can't process binder transactions, so a way is required to
inform transmitting ends of communication failures due to the frozen
state of their receiving counterparts. Additionally, races are possible
between transitions to frozen state and binder transactions enqueued to
a specific process.

Implement BINDER_FREEZE ioctl for user space to inform the binder driver
about the intention to freeze or unfreeze a process. When the ioctl is
called, block the caller until any pending binder transactions toward
the target process are flushed. Return an error to transactions to
processes marked as frozen.

Signed-off-by: Marco Ballesio <balejs@google.com>
Co-developed-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Li Li <dualli@google.com>
---
 drivers/android/binder.c            | 139 ++++++++++++++++++++++++++--
 drivers/android/binder_internal.h   |  12 +++
 include/uapi/linux/android/binder.h |  13 +++
 3 files changed, 154 insertions(+), 10 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..b93ca53bb90f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct binder_transaction *t)
 
 	if (target_proc) {
 		binder_inner_proc_lock(target_proc);
+		target_proc->outstanding_txns--;
+		if (target_proc->outstanding_txns < 0)
+			pr_warn("%s: Unexpected outstanding_txns %d\n",
+				__func__, target_proc->outstanding_txns);
+		if (!target_proc->outstanding_txns && target_proc->is_frozen)
+			wake_up_interruptible_all(&target_proc->freeze_wait);
 		if (t->buffer)
 			t->buffer->transaction = NULL;
 		binder_inner_proc_unlock(target_proc);
@@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct binder_transaction *t,
  * If the @thread parameter is not NULL, the transaction is always queued
  * to the waitlist of that specific thread.
  *
- * Return:	true if the transactions was successfully queued
- *		false if the target process or thread is dead
+ * Return:	0 if the transaction was successfully queued
+ *		BR_DEAD_REPLY if the target process or thread is dead
+ *		BR_FROZEN_REPLY if the target process or thread is frozen
  */
-static bool binder_proc_transaction(struct binder_transaction *t,
+static int binder_proc_transaction(struct binder_transaction *t,
 				    struct binder_proc *proc,
 				    struct binder_thread *thread)
 {
@@ -2354,10 +2361,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
 
 	binder_inner_proc_lock(proc);
 
-	if (proc->is_dead || (thread && thread->is_dead)) {
+	if ((proc->is_frozen && !oneway) || proc->is_dead ||
+			(thread && thread->is_dead)) {
 		binder_inner_proc_unlock(proc);
 		binder_node_unlock(node);
-		return false;
+		return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
 	}
 
 	if (!thread && !pending_async)
@@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
 	if (!pending_async)
 		binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
 
+	proc->outstanding_txns++;
 	binder_inner_proc_unlock(proc);
 	binder_node_unlock(node);
 
-	return true;
+	return 0;
 }
 
 /**
@@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc *proc,
 	if (reply) {
 		binder_enqueue_thread_work(thread, tcomplete);
 		binder_inner_proc_lock(target_proc);
-		if (target_thread->is_dead) {
+		if (target_thread->is_dead || target_proc->is_frozen) {
+			return_error = target_thread->is_dead ?
+				BR_DEAD_REPLY : BR_FROZEN_REPLY;
 			binder_inner_proc_unlock(target_proc);
 			goto err_dead_proc_or_thread;
 		}
 		BUG_ON(t->buffer->async_transaction != 0);
 		binder_pop_transaction_ilocked(target_thread, in_reply_to);
 		binder_enqueue_thread_work_ilocked(target_thread, &t->work);
+		target_proc->outstanding_txns++;
 		binder_inner_proc_unlock(target_proc);
 		wake_up_interruptible_sync(&target_thread->wait);
 		binder_free_transaction(in_reply_to);
@@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
 		t->from_parent = thread->transaction_stack;
 		thread->transaction_stack = t;
 		binder_inner_proc_unlock(proc);
-		if (!binder_proc_transaction(t, target_proc, target_thread)) {
+		return_error = binder_proc_transaction(t,
+				target_proc, target_thread);
+		if (return_error) {
 			binder_inner_proc_lock(proc);
 			binder_pop_transaction_ilocked(thread, t);
 			binder_inner_proc_unlock(proc);
@@ -3048,7 +3062,8 @@ static void binder_transaction(struct binder_proc *proc,
 		BUG_ON(target_node == NULL);
 		BUG_ON(t->buffer->async_transaction != 1);
 		binder_enqueue_thread_work(thread, tcomplete);
-		if (!binder_proc_transaction(t, target_proc, NULL))
+		return_error = binder_proc_transaction(t, target_proc, NULL);
+		if (return_error)
 			goto err_dead_proc_or_thread;
 	}
 	if (target_thread)
@@ -3065,7 +3080,6 @@ static void binder_transaction(struct binder_proc *proc,
 	return;
 
 err_dead_proc_or_thread:
-	return_error = BR_DEAD_REPLY;
 	return_error_line = __LINE__;
 	binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
@@ -4298,6 +4312,9 @@ static void binder_free_proc(struct binder_proc *proc)
 
 	BUG_ON(!list_empty(&proc->todo));
 	BUG_ON(!list_empty(&proc->delivered_death));
+	if (proc->outstanding_txns)
+		pr_warn("%s: Unexpected outstanding_txns %d\n",
+			__func__, proc->outstanding_txns);
 	device = container_of(proc->context, struct binder_device, context);
 	if (refcount_dec_and_test(&device->ref)) {
 		kfree(proc->context->name);
@@ -4359,6 +4376,7 @@ static int binder_thread_release(struct binder_proc *proc,
 			     (t->to_thread == thread) ? "in" : "out");
 
 		if (t->to_thread == thread) {
+			thread->proc->outstanding_txns--;
 			t->to_proc = NULL;
 			t->to_thread = NULL;
 			if (t->buffer) {
@@ -4609,6 +4627,45 @@ static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
 	return 0;
 }
 
+static int binder_ioctl_freeze(struct binder_freeze_info *info,
+			       struct binder_proc *target_proc)
+{
+	int ret = 0;
+
+	if (!info->enable) {
+		binder_inner_proc_lock(target_proc);
+		target_proc->is_frozen = false;
+		binder_inner_proc_unlock(target_proc);
+		return 0;
+	}
+
+	/*
+	 * Freezing the target. Prevent new transactions by
+	 * setting frozen state. If timeout specified, wait
+	 * for transactions to drain.
+	 */
+	binder_inner_proc_lock(target_proc);
+	target_proc->is_frozen = true;
+	binder_inner_proc_unlock(target_proc);
+
+	if (info->timeout_ms > 0)
+		ret = wait_event_interruptible_timeout(
+			target_proc->freeze_wait,
+			(!target_proc->outstanding_txns),
+			msecs_to_jiffies(info->timeout_ms));
+
+	if (!ret && target_proc->outstanding_txns)
+		ret = -EAGAIN;
+
+	if (ret < 0) {
+		binder_inner_proc_lock(target_proc);
+		target_proc->is_frozen = false;
+		binder_inner_proc_unlock(target_proc);
+	}
+
+	return ret;
+}
+
 static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int ret;
@@ -4727,6 +4784,66 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		}
 		break;
 	}
+	case BINDER_FREEZE: {
+		struct binder_freeze_info info;
+		struct binder_proc **target_procs = NULL, *target_proc;
+		int target_procs_count = 0, i = 0;
+
+		ret = 0;
+
+		if (copy_from_user(&info, ubuf, sizeof(info))) {
+			ret = -EFAULT;
+			goto err;
+		}
+
+		mutex_lock(&binder_procs_lock);
+		hlist_for_each_entry(target_proc, &binder_procs, proc_node) {
+			if (target_proc->pid == info.pid)
+				target_procs_count++;
+		}
+
+		if (target_procs_count == 0) {
+			mutex_unlock(&binder_procs_lock);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		target_procs = kcalloc(target_procs_count,
+				       sizeof(struct binder_proc *),
+				       GFP_KERNEL);
+
+		if (!target_procs) {
+			mutex_unlock(&binder_procs_lock);
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		hlist_for_each_entry(target_proc, &binder_procs, proc_node) {
+			if (target_proc->pid != info.pid)
+				continue;
+
+			binder_inner_proc_lock(target_proc);
+			target_proc->tmp_ref++;
+			binder_inner_proc_unlock(target_proc);
+
+			target_procs[i++] = target_proc;
+		}
+		mutex_unlock(&binder_procs_lock);
+
+		for (i = 0; i < target_procs_count; i++) {
+			if (ret >= 0)
+				ret = binder_ioctl_freeze(&info,
+							  target_procs[i]);
+
+			binder_proc_dec_tmpref(target_procs[i]);
+		}
+
+		kfree(target_procs);
+
+		if (ret < 0)
+			goto err;
+		break;
+	}
 	default:
 		ret = -EINVAL;
 		goto err;
@@ -4823,6 +4940,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
 	get_task_struct(current->group_leader);
 	proc->tsk = current->group_leader;
 	INIT_LIST_HEAD(&proc->todo);
+	init_waitqueue_head(&proc->freeze_wait);
 	proc->default_priority = task_nice(current);
 	/* binderfs stashes devices in i_private */
 	if (is_binderfs_device(nodp)) {
@@ -5035,6 +5153,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 	proc->tmp_ref++;
 
 	proc->is_dead = true;
+	proc->is_frozen = false;
 	threads = 0;
 	active_transactions = 0;
 	while ((n = rb_first(&proc->threads))) {
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 6cd79011e35d..e6a53e98c6da 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -367,9 +367,18 @@ struct binder_ref {
  *                        (protected by binder_deferred_lock)
  * @deferred_work:        bitmap of deferred work to perform
  *                        (protected by binder_deferred_lock)
+ * @outstanding_txns:     number of transactions to be transmitted before
+ *                        processes in freeze_wait are woken up
+ *                        (protected by @inner_lock)
  * @is_dead:              process is dead and awaiting free
  *                        when outstanding transactions are cleaned up
  *                        (protected by @inner_lock)
+ * @is_frozen:            process is frozen and unable to service
+ *                        binder transactions
+ *                        (protected by @inner_lock)
+ * @freeze_wait:          waitqueue of processes waiting for all outstanding
+ *                        transactions to be processed
+ *                        (protected by @inner_lock)
  * @todo:                 list of work for this process
  *                        (protected by @inner_lock)
  * @stats:                per-process binder statistics
@@ -410,7 +419,10 @@ struct binder_proc {
 	struct task_struct *tsk;
 	struct hlist_node deferred_work_node;
 	int deferred_work;
+	int outstanding_txns;
 	bool is_dead;
+	bool is_frozen;
+	wait_queue_head_t freeze_wait;
 
 	struct list_head todo;
 	struct binder_stats stats;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index ec84ad106568..7eb5b818b3c1 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -217,6 +217,12 @@ struct binder_node_info_for_ref {
 	__u32            reserved3;
 };
 
+struct binder_freeze_info {
+	__u32            pid;
+	__u32            enable;
+	__u32            timeout_ms;
+};
+
 #define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS		_IOW('b', 5, __u32)
@@ -227,6 +233,7 @@ struct binder_node_info_for_ref {
 #define BINDER_GET_NODE_DEBUG_INFO	_IOWR('b', 11, struct binder_node_debug_info)
 #define BINDER_GET_NODE_INFO_FOR_REF	_IOWR('b', 12, struct binder_node_info_for_ref)
 #define BINDER_SET_CONTEXT_MGR_EXT	_IOW('b', 13, struct flat_binder_object)
+#define BINDER_FREEZE			_IOW('b', 14, struct binder_freeze_info)
 
 /*
  * NOTE: Two special error codes you should check for when calling
@@ -408,6 +415,12 @@ enum binder_driver_return_protocol {
 	 * The last transaction (either a bcTRANSACTION or
 	 * a bcATTEMPT_ACQUIRE) failed (e.g. out of memory).  No parameters.
 	 */
+
+	BR_FROZEN_REPLY = _IO('r', 18),
+	/*
+	 * The target of the last transaction (either a bcTRANSACTION or
+	 * a bcATTEMPT_ACQUIRE) is frozen.  No parameters.
+	 */
 };
 
 enum binder_driver_command_protocol {
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v3 1/3] binder: BINDER_FREEZE ioctl

[Todd Kjos]

On Mon, Mar 15, 2021 at 6:16 PM Li Li <dualli@chromium.org> wrote:
>
> From: Marco Ballesio <balejs@google.com>
>
> Frozen tasks can't process binder transactions, so a way is required to
> inform transmitting ends of communication failures due to the frozen
> state of their receiving counterparts. Additionally, races are possible
> between transitions to frozen state and binder transactions enqueued to
> a specific process.
>
> Implement BINDER_FREEZE ioctl for user space to inform the binder driver
> about the intention to freeze or unfreeze a process. When the ioctl is
> called, block the caller until any pending binder transactions toward
> the target process are flushed. Return an error to transactions to
> processes marked as frozen.
>
> Signed-off-by: Marco Ballesio <balejs@google.com>
> Co-developed-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Li Li <dualli@google.com>

For the series, you can add

Acked-by: Todd Kjos <tkjos@google.com>


> ---
>  drivers/android/binder.c            | 139 ++++++++++++++++++++++++++--
>  drivers/android/binder_internal.h   |  12 +++
>  include/uapi/linux/android/binder.h |  13 +++
>  3 files changed, 154 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56a..b93ca53bb90f 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct binder_transaction *t)
>
>         if (target_proc) {
>                 binder_inner_proc_lock(target_proc);
> +               target_proc->outstanding_txns--;
> +               if (target_proc->outstanding_txns < 0)
> +                       pr_warn("%s: Unexpected outstanding_txns %d\n",
> +                               __func__, target_proc->outstanding_txns);
> +               if (!target_proc->outstanding_txns && target_proc->is_frozen)
> +                       wake_up_interruptible_all(&target_proc->freeze_wait);
>                 if (t->buffer)
>                         t->buffer->transaction = NULL;
>                 binder_inner_proc_unlock(target_proc);
> @@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct binder_transaction *t,
>   * If the @thread parameter is not NULL, the transaction is always queued
>   * to the waitlist of that specific thread.
>   *
> - * Return:     true if the transactions was successfully queued
> - *             false if the target process or thread is dead
> + * Return:     0 if the transaction was successfully queued
> + *             BR_DEAD_REPLY if the target process or thread is dead
> + *             BR_FROZEN_REPLY if the target process or thread is frozen
>   */
> -static bool binder_proc_transaction(struct binder_transaction *t,
> +static int binder_proc_transaction(struct binder_transaction *t,
>                                     struct binder_proc *proc,
>                                     struct binder_thread *thread)
>  {
> @@ -2354,10 +2361,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
>
>         binder_inner_proc_lock(proc);
>
> -       if (proc->is_dead || (thread && thread->is_dead)) {
> +       if ((proc->is_frozen && !oneway) || proc->is_dead ||
> +                       (thread && thread->is_dead)) {
>                 binder_inner_proc_unlock(proc);
>                 binder_node_unlock(node);
> -               return false;
> +               return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
>         }
>
>         if (!thread && !pending_async)
> @@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
>         if (!pending_async)
>                 binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
>
> +       proc->outstanding_txns++;
>         binder_inner_proc_unlock(proc);
>         binder_node_unlock(node);
>
> -       return true;
> +       return 0;
>  }
>
>  /**
> @@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc *proc,
>         if (reply) {
>                 binder_enqueue_thread_work(thread, tcomplete);
>                 binder_inner_proc_lock(target_proc);
> -               if (target_thread->is_dead) {
> +               if (target_thread->is_dead || target_proc->is_frozen) {
> +                       return_error = target_thread->is_dead ?
> +                               BR_DEAD_REPLY : BR_FROZEN_REPLY;
>                         binder_inner_proc_unlock(target_proc);
>                         goto err_dead_proc_or_thread;
>                 }
>                 BUG_ON(t->buffer->async_transaction != 0);
>                 binder_pop_transaction_ilocked(target_thread, in_reply_to);
>                 binder_enqueue_thread_work_ilocked(target_thread, &t->work);
> +               target_proc->outstanding_txns++;
>                 binder_inner_proc_unlock(target_proc);
>                 wake_up_interruptible_sync(&target_thread->wait);
>                 binder_free_transaction(in_reply_to);
> @@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
>                 t->from_parent = thread->transaction_stack;
>                 thread->transaction_stack = t;
>                 binder_inner_proc_unlock(proc);
> -               if (!binder_proc_transaction(t, target_proc, target_thread)) {
> +               return_error = binder_proc_transaction(t,
> +                               target_proc, target_thread);
> +               if (return_error) {
>                         binder_inner_proc_lock(proc);
>                         binder_pop_transaction_ilocked(thread, t);
>                         binder_inner_proc_unlock(proc);
> @@ -3048,7 +3062,8 @@ static void binder_transaction(struct binder_proc *proc,
>                 BUG_ON(target_node == NULL);
>                 BUG_ON(t->buffer->async_transaction != 1);
>                 binder_enqueue_thread_work(thread, tcomplete);
> -               if (!binder_proc_transaction(t, target_proc, NULL))
> +               return_error = binder_proc_transaction(t, target_proc, NULL);
> +               if (return_error)
>                         goto err_dead_proc_or_thread;
>         }
>         if (target_thread)
> @@ -3065,7 +3080,6 @@ static void binder_transaction(struct binder_proc *proc,
>         return;
>
>  err_dead_proc_or_thread:
> -       return_error = BR_DEAD_REPLY;
>         return_error_line = __LINE__;
>         binder_dequeue_work(proc, tcomplete);
>  err_translate_failed:
> @@ -4298,6 +4312,9 @@ static void binder_free_proc(struct binder_proc *proc)
>
>         BUG_ON(!list_empty(&proc->todo));
>         BUG_ON(!list_empty(&proc->delivered_death));
> +       if (proc->outstanding_txns)
> +               pr_warn("%s: Unexpected outstanding_txns %d\n",
> +                       __func__, proc->outstanding_txns);
>         device = container_of(proc->context, struct binder_device, context);
>         if (refcount_dec_and_test(&device->ref)) {
>                 kfree(proc->context->name);
> @@ -4359,6 +4376,7 @@ static int binder_thread_release(struct binder_proc *proc,
>                              (t->to_thread == thread) ? "in" : "out");
>
>                 if (t->to_thread == thread) {
> +                       thread->proc->outstanding_txns--;
>                         t->to_proc = NULL;
>                         t->to_thread = NULL;
>                         if (t->buffer) {
> @@ -4609,6 +4627,45 @@ static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
>         return 0;
>  }
>
> +static int binder_ioctl_freeze(struct binder_freeze_info *info,
> +                              struct binder_proc *target_proc)
> +{
> +       int ret = 0;
> +
> +       if (!info->enable) {
> +               binder_inner_proc_lock(target_proc);
> +               target_proc->is_frozen = false;
> +               binder_inner_proc_unlock(target_proc);
> +               return 0;
> +       }
> +
> +       /*
> +        * Freezing the target. Prevent new transactions by
> +        * setting frozen state. If timeout specified, wait
> +        * for transactions to drain.
> +        */
> +       binder_inner_proc_lock(target_proc);
> +       target_proc->is_frozen = true;
> +       binder_inner_proc_unlock(target_proc);
> +
> +       if (info->timeout_ms > 0)
> +               ret = wait_event_interruptible_timeout(
> +                       target_proc->freeze_wait,
> +                       (!target_proc->outstanding_txns),
> +                       msecs_to_jiffies(info->timeout_ms));
> +
> +       if (!ret && target_proc->outstanding_txns)
> +               ret = -EAGAIN;
> +
> +       if (ret < 0) {
> +               binder_inner_proc_lock(target_proc);
> +               target_proc->is_frozen = false;
> +               binder_inner_proc_unlock(target_proc);
> +       }
> +
> +       return ret;
> +}
> +
>  static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>         int ret;
> @@ -4727,6 +4784,66 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                 }
>                 break;
>         }
> +       case BINDER_FREEZE: {
> +               struct binder_freeze_info info;
> +               struct binder_proc **target_procs = NULL, *target_proc;
> +               int target_procs_count = 0, i = 0;
> +
> +               ret = 0;
> +
> +               if (copy_from_user(&info, ubuf, sizeof(info))) {
> +                       ret = -EFAULT;
> +                       goto err;
> +               }
> +
> +               mutex_lock(&binder_procs_lock);
> +               hlist_for_each_entry(target_proc, &binder_procs, proc_node) {
> +                       if (target_proc->pid == info.pid)
> +                               target_procs_count++;
> +               }
> +
> +               if (target_procs_count == 0) {
> +                       mutex_unlock(&binder_procs_lock);
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
> +
> +               target_procs = kcalloc(target_procs_count,
> +                                      sizeof(struct binder_proc *),
> +                                      GFP_KERNEL);
> +
> +               if (!target_procs) {
> +                       mutex_unlock(&binder_procs_lock);
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
> +
> +               hlist_for_each_entry(target_proc, &binder_procs, proc_node) {
> +                       if (target_proc->pid != info.pid)
> +                               continue;
> +
> +                       binder_inner_proc_lock(target_proc);
> +                       target_proc->tmp_ref++;
> +                       binder_inner_proc_unlock(target_proc);
> +
> +                       target_procs[i++] = target_proc;
> +               }
> +               mutex_unlock(&binder_procs_lock);
> +
> +               for (i = 0; i < target_procs_count; i++) {
> +                       if (ret >= 0)
> +                               ret = binder_ioctl_freeze(&info,
> +                                                         target_procs[i]);
> +
> +                       binder_proc_dec_tmpref(target_procs[i]);
> +               }
> +
> +               kfree(target_procs);
> +
> +               if (ret < 0)
> +                       goto err;
> +               break;
> +       }
>         default:
>                 ret = -EINVAL;
>                 goto err;
> @@ -4823,6 +4940,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
>         get_task_struct(current->group_leader);
>         proc->tsk = current->group_leader;
>         INIT_LIST_HEAD(&proc->todo);
> +       init_waitqueue_head(&proc->freeze_wait);
>         proc->default_priority = task_nice(current);
>         /* binderfs stashes devices in i_private */
>         if (is_binderfs_device(nodp)) {
> @@ -5035,6 +5153,7 @@ static void binder_deferred_release(struct binder_proc *proc)
>         proc->tmp_ref++;
>
>         proc->is_dead = true;
> +       proc->is_frozen = false;
>         threads = 0;
>         active_transactions = 0;
>         while ((n = rb_first(&proc->threads))) {
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 6cd79011e35d..e6a53e98c6da 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -367,9 +367,18 @@ struct binder_ref {
>   *                        (protected by binder_deferred_lock)
>   * @deferred_work:        bitmap of deferred work to perform
>   *                        (protected by binder_deferred_lock)
> + * @outstanding_txns:     number of transactions to be transmitted before
> + *                        processes in freeze_wait are woken up
> + *                        (protected by @inner_lock)
>   * @is_dead:              process is dead and awaiting free
>   *                        when outstanding transactions are cleaned up
>   *                        (protected by @inner_lock)
> + * @is_frozen:            process is frozen and unable to service
> + *                        binder transactions
> + *                        (protected by @inner_lock)
> + * @freeze_wait:          waitqueue of processes waiting for all outstanding
> + *                        transactions to be processed
> + *                        (protected by @inner_lock)
>   * @todo:                 list of work for this process
>   *                        (protected by @inner_lock)
>   * @stats:                per-process binder statistics
> @@ -410,7 +419,10 @@ struct binder_proc {
>         struct task_struct *tsk;
>         struct hlist_node deferred_work_node;
>         int deferred_work;
> +       int outstanding_txns;
>         bool is_dead;
> +       bool is_frozen;
> +       wait_queue_head_t freeze_wait;
>
>         struct list_head todo;
>         struct binder_stats stats;
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index ec84ad106568..7eb5b818b3c1 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -217,6 +217,12 @@ struct binder_node_info_for_ref {
>         __u32            reserved3;
>  };
>
> +struct binder_freeze_info {
> +       __u32            pid;
> +       __u32            enable;
> +       __u32            timeout_ms;
> +};
> +
>  #define BINDER_WRITE_READ              _IOWR('b', 1, struct binder_write_read)
>  #define BINDER_SET_IDLE_TIMEOUT                _IOW('b', 3, __s64)
>  #define BINDER_SET_MAX_THREADS         _IOW('b', 5, __u32)
> @@ -227,6 +233,7 @@ struct binder_node_info_for_ref {
>  #define BINDER_GET_NODE_DEBUG_INFO     _IOWR('b', 11, struct binder_node_debug_info)
>  #define BINDER_GET_NODE_INFO_FOR_REF   _IOWR('b', 12, struct binder_node_info_for_ref)
>  #define BINDER_SET_CONTEXT_MGR_EXT     _IOW('b', 13, struct flat_binder_object)
> +#define BINDER_FREEZE                  _IOW('b', 14, struct binder_freeze_info)
>
>  /*
>   * NOTE: Two special error codes you should check for when calling
> @@ -408,6 +415,12 @@ enum binder_driver_return_protocol {
>          * The last transaction (either a bcTRANSACTION or
>          * a bcATTEMPT_ACQUIRE) failed (e.g. out of memory).  No parameters.
>          */
> +
> +       BR_FROZEN_REPLY = _IO('r', 18),
> +       /*
> +        * The target of the last transaction (either a bcTRANSACTION or
> +        * a bcATTEMPT_ACQUIRE) is frozen.  No parameters.
> +        */
>  };
>
>  enum binder_driver_command_protocol {
> --
> 2.31.0.rc2.261.g7f71774620-goog
>

Re: [PATCH v3 1/3] binder: BINDER_FREEZE ioctl

[Christian Brauner]

On Mon, Mar 15, 2021 at 06:16:28PM -0700, Li Li wrote:
> From: Marco Ballesio <balejs@google.com>
> 
> Frozen tasks can't process binder transactions, so a way is required to
> inform transmitting ends of communication failures due to the frozen
> state of their receiving counterparts. Additionally, races are possible
> between transitions to frozen state and binder transactions enqueued to
> a specific process.
> 
> Implement BINDER_FREEZE ioctl for user space to inform the binder driver
> about the intention to freeze or unfreeze a process. When the ioctl is
> called, block the caller until any pending binder transactions toward
> the target process are flushed. Return an error to transactions to
> processes marked as frozen.
> 
> Signed-off-by: Marco Ballesio <balejs@google.com>
> Co-developed-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Li Li <dualli@google.com>
> ---
>  drivers/android/binder.c            | 139 ++++++++++++++++++++++++++--
>  drivers/android/binder_internal.h   |  12 +++
>  include/uapi/linux/android/binder.h |  13 +++
>  3 files changed, 154 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56a..b93ca53bb90f 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct binder_transaction *t)
>  
>  	if (target_proc) {
>  		binder_inner_proc_lock(target_proc);
> +		target_proc->outstanding_txns--;
> +		if (target_proc->outstanding_txns < 0)
> +			pr_warn("%s: Unexpected outstanding_txns %d\n",
> +				__func__, target_proc->outstanding_txns);
> +		if (!target_proc->outstanding_txns && target_proc->is_frozen)
> +			wake_up_interruptible_all(&target_proc->freeze_wait);
>  		if (t->buffer)
>  			t->buffer->transaction = NULL;
>  		binder_inner_proc_unlock(target_proc);
> @@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct binder_transaction *t,
>   * If the @thread parameter is not NULL, the transaction is always queued
>   * to the waitlist of that specific thread.
>   *
> - * Return:	true if the transactions was successfully queued
> - *		false if the target process or thread is dead
> + * Return:	0 if the transaction was successfully queued
> + *		BR_DEAD_REPLY if the target process or thread is dead
> + *		BR_FROZEN_REPLY if the target process or thread is frozen
>   */
> -static bool binder_proc_transaction(struct binder_transaction *t,
> +static int binder_proc_transaction(struct binder_transaction *t,
>  				    struct binder_proc *proc,
>  				    struct binder_thread *thread)
>  {
> @@ -2354,10 +2361,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
>  
>  	binder_inner_proc_lock(proc);
>  
> -	if (proc->is_dead || (thread && thread->is_dead)) {
> +	if ((proc->is_frozen && !oneway) || proc->is_dead ||
> +			(thread && thread->is_dead)) {
>  		binder_inner_proc_unlock(proc);
>  		binder_node_unlock(node);
> -		return false;
> +		return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
>  	}
>  
>  	if (!thread && !pending_async)
> @@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct binder_transaction *t,
>  	if (!pending_async)
>  		binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
>  
> +	proc->outstanding_txns++;
>  	binder_inner_proc_unlock(proc);
>  	binder_node_unlock(node);
>  
> -	return true;
> +	return 0;
>  }
>  
>  /**
> @@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc *proc,
>  	if (reply) {
>  		binder_enqueue_thread_work(thread, tcomplete);
>  		binder_inner_proc_lock(target_proc);
> -		if (target_thread->is_dead) {
> +		if (target_thread->is_dead || target_proc->is_frozen) {
> +			return_error = target_thread->is_dead ?
> +				BR_DEAD_REPLY : BR_FROZEN_REPLY;
>  			binder_inner_proc_unlock(target_proc);
>  			goto err_dead_proc_or_thread;
>  		}
>  		BUG_ON(t->buffer->async_transaction != 0);
>  		binder_pop_transaction_ilocked(target_thread, in_reply_to);
>  		binder_enqueue_thread_work_ilocked(target_thread, &t->work);
> +		target_proc->outstanding_txns++;
>  		binder_inner_proc_unlock(target_proc);
>  		wake_up_interruptible_sync(&target_thread->wait);
>  		binder_free_transaction(in_reply_to);
> @@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
>  		t->from_parent = thread->transaction_stack;
>  		thread->transaction_stack = t;
>  		binder_inner_proc_unlock(proc);
> -		if (!binder_proc_transaction(t, target_proc, target_thread)) {
> +		return_error = binder_proc_transaction(t,
> +				target_proc, target_thread);
> +		if (return_error) {
>  			binder_inner_proc_lock(proc);
>  			binder_pop_transaction_ilocked(thread, t);
>  			binder_inner_proc_unlock(proc);
> @@ -3048,7 +3062,8 @@ static void binder_transaction(struct binder_proc *proc,
>  		BUG_ON(target_node == NULL);
>  		BUG_ON(t->buffer->async_transaction != 1);
>  		binder_enqueue_thread_work(thread, tcomplete);
> -		if (!binder_proc_transaction(t, target_proc, NULL))
> +		return_error = binder_proc_transaction(t, target_proc, NULL);
> +		if (return_error)
>  			goto err_dead_proc_or_thread;
>  	}
>  	if (target_thread)
> @@ -3065,7 +3080,6 @@ static void binder_transaction(struct binder_proc *proc,
>  	return;
>  
>  err_dead_proc_or_thread:
> -	return_error = BR_DEAD_REPLY;
>  	return_error_line = __LINE__;
>  	binder_dequeue_work(proc, tcomplete);
>  err_translate_failed:
> @@ -4298,6 +4312,9 @@ static void binder_free_proc(struct binder_proc *proc)
>  
>  	BUG_ON(!list_empty(&proc->todo));
>  	BUG_ON(!list_empty(&proc->delivered_death));
> +	if (proc->outstanding_txns)
> +		pr_warn("%s: Unexpected outstanding_txns %d\n",
> +			__func__, proc->outstanding_txns);
>  	device = container_of(proc->context, struct binder_device, context);
>  	if (refcount_dec_and_test(&device->ref)) {
>  		kfree(proc->context->name);
> @@ -4359,6 +4376,7 @@ static int binder_thread_release(struct binder_proc *proc,
>  			     (t->to_thread == thread) ? "in" : "out");
>  
>  		if (t->to_thread == thread) {
> +			thread->proc->outstanding_txns--;
>  			t->to_proc = NULL;
>  			t->to_thread = NULL;
>  			if (t->buffer) {
> @@ -4609,6 +4627,45 @@ static int binder_ioctl_get_node_debug_info(struct binder_proc *proc,
>  	return 0;
>  }
>  
> +static int binder_ioctl_freeze(struct binder_freeze_info *info,
> +			       struct binder_proc *target_proc)
> +{
> +	int ret = 0;
> +
> +	if (!info->enable) {
> +		binder_inner_proc_lock(target_proc);
> +		target_proc->is_frozen = false;
> +		binder_inner_proc_unlock(target_proc);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Freezing the target. Prevent new transactions by
> +	 * setting frozen state. If timeout specified, wait
> +	 * for transactions to drain.
> +	 */
> +	binder_inner_proc_lock(target_proc);
> +	target_proc->is_frozen = true;
> +	binder_inner_proc_unlock(target_proc);
> +
> +	if (info->timeout_ms > 0)
> +		ret = wait_event_interruptible_timeout(
> +			target_proc->freeze_wait,
> +			(!target_proc->outstanding_txns),
> +			msecs_to_jiffies(info->timeout_ms));
> +
> +	if (!ret && target_proc->outstanding_txns)
> +		ret = -EAGAIN;
> +
> +	if (ret < 0) {
> +		binder_inner_proc_lock(target_proc);
> +		target_proc->is_frozen = false;
> +		binder_inner_proc_unlock(target_proc);
> +	}
> +
> +	return ret;
> +}
> +
>  static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	int ret;
> @@ -4727,6 +4784,66 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		}
>  		break;
>  	}
> +	case BINDER_FREEZE: {
> +		struct binder_freeze_info info;
> +		struct binder_proc **target_procs = NULL, *target_proc;
> +		int target_procs_count = 0, i = 0;
> +
> +		ret = 0;
> +
> +		if (copy_from_user(&info, ubuf, sizeof(info))) {

This is not at all a blocker to this I just want to point out that if
you wanted to you could ensure that this API is extensible with
guaranteed forward- and backward compatibility by switching to the
copy_struct_from_user() API that Aleksa and I added and that is nowadays
used in a bunch of syscalls (openat2((), clone3(), mount_setattr(),
sched_setattr() etc.). For the seccomp notifier work we did we extended
this concept to ioctls(), see

kernel/seccomp.c:

static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
				 unsigned long arg)
{
	struct seccomp_filter *filter = file->private_data;
	void __user *buf = (void __user *)arg;

	/* Fixed-size ioctls */
	switch (cmd) {
	case SECCOMP_IOCTL_NOTIF_RECV:
		return seccomp_notify_recv(filter, buf);
	case SECCOMP_IOCTL_NOTIF_SEND:
		return seccomp_notify_send(filter, buf);
	case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
	case SECCOMP_IOCTL_NOTIF_ID_VALID:
		return seccomp_notify_id_valid(filter, buf);
	}

	/* Extensible Argument ioctls */
#define EA_IOCTL(cmd)	((cmd) & ~(IOC_INOUT | IOCSIZE_MASK))
	switch (EA_IOCTL(cmd)) {
	case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD):
		return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));
	default:
		return -EINVAL;
	}
}

Christian

[PATCH v3 2/3] binder: use EINTR for interrupted wait for work

[Li Li]

From: Marco Ballesio <balejs@google.com>

when interrupted by a signal, binder_wait_for_work currently returns
-ERESTARTSYS. This error code isn't propagated to user space, but a way
to handle interruption due to signals must be provided to code using
this API.

Replace this instance of -ERESTARTSYS with -EINTR, which is propagated
to user space.

Test: built, booted, interrupted a worker thread within
binder_wait_for_work
Signed-off-by: Marco Ballesio <balejs@google.com>
Signed-off-by: Li Li <dualli@google.com>
---
 drivers/android/binder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b93ca53bb90f..fe16c455a76e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3710,7 +3710,7 @@ static int binder_wait_for_work(struct binder_thread *thread,
 		binder_inner_proc_lock(proc);
 		list_del_init(&thread->waiting_thread_node);
 		if (signal_pending(current)) {
-			ret = -ERESTARTSYS;
+			ret = -EINTR;
 			break;
 		}
 	}
@@ -4853,7 +4853,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (thread)
 		thread->looper_need_return = false;
 	wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2);
-	if (ret && ret != -ERESTARTSYS)
+	if (ret && ret != -EINTR)
 		pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
 err_unlocked:
 	trace_binder_ioctl_done(ret);
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH v3 3/3] binder: BINDER_GET_FROZEN_INFO ioctl

[Li Li]

From: Marco Ballesio <balejs@google.com>

User space needs to know if binder transactions occurred to frozen
processes. Introduce a new BINDER_GET_FROZEN ioctl and keep track of
transactions occurring to frozen proceses.

Signed-off-by: Marco Ballesio <balejs@google.com>
Signed-off-by: Li Li <dualli@google.com>
---
 drivers/android/binder.c            | 55 +++++++++++++++++++++++++++++
 drivers/android/binder_internal.h   |  6 ++++
 include/uapi/linux/android/binder.h |  7 ++++
 3 files changed, 68 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fe16c455a76e..e1a484ab0366 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2360,6 +2360,10 @@ static int binder_proc_transaction(struct binder_transaction *t,
 	}
 
 	binder_inner_proc_lock(proc);
+	if (proc->is_frozen) {
+		proc->sync_recv |= !oneway;
+		proc->async_recv |= oneway;
+	}
 
 	if ((proc->is_frozen && !oneway) || proc->is_dead ||
 			(thread && thread->is_dead)) {
@@ -4634,6 +4638,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
 
 	if (!info->enable) {
 		binder_inner_proc_lock(target_proc);
+		target_proc->sync_recv = false;
+		target_proc->async_recv = false;
 		target_proc->is_frozen = false;
 		binder_inner_proc_unlock(target_proc);
 		return 0;
@@ -4645,6 +4651,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
 	 * for transactions to drain.
 	 */
 	binder_inner_proc_lock(target_proc);
+	target_proc->sync_recv = false;
+	target_proc->async_recv = false;
 	target_proc->is_frozen = true;
 	binder_inner_proc_unlock(target_proc);
 
@@ -4666,6 +4674,33 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
 	return ret;
 }
 
+static int binder_ioctl_get_freezer_info(
+				struct binder_frozen_status_info *info)
+{
+	struct binder_proc *target_proc;
+	bool found = false;
+
+	info->sync_recv = 0;
+	info->async_recv = 0;
+
+	mutex_lock(&binder_procs_lock);
+	hlist_for_each_entry(target_proc, &binder_procs, proc_node) {
+		if (target_proc->pid == info->pid) {
+			found = true;
+			binder_inner_proc_lock(target_proc);
+			info->sync_recv |= target_proc->sync_recv;
+			info->async_recv |= target_proc->async_recv;
+			binder_inner_proc_unlock(target_proc);
+		}
+	}
+	mutex_unlock(&binder_procs_lock);
+
+	if (!found)
+		return -EINVAL;
+
+	return 0;
+}
+
 static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int ret;
@@ -4844,6 +4879,24 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		break;
 	}
+	case BINDER_GET_FROZEN_INFO: {
+		struct binder_frozen_status_info info;
+
+		if (copy_from_user(&info, ubuf, sizeof(info))) {
+			ret = -EFAULT;
+			goto err;
+		}
+
+		ret = binder_ioctl_get_freezer_info(&info);
+		if (ret < 0)
+			goto err;
+
+		if (copy_to_user(ubuf, &info, sizeof(info))) {
+			ret = -EFAULT;
+			goto err;
+		}
+		break;
+	}
 	default:
 		ret = -EINVAL;
 		goto err;
@@ -5154,6 +5207,8 @@ static void binder_deferred_release(struct binder_proc *proc)
 
 	proc->is_dead = true;
 	proc->is_frozen = false;
+	proc->sync_recv = false;
+	proc->async_recv = false;
 	threads = 0;
 	active_transactions = 0;
 	while ((n = rb_first(&proc->threads))) {
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index e6a53e98c6da..2872a7de68e1 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -376,6 +376,10 @@ struct binder_ref {
  * @is_frozen:            process is frozen and unable to service
  *                        binder transactions
  *                        (protected by @inner_lock)
+ * @sync_recv:            process received sync transactions since last frozen
+ *                        (protected by @inner_lock)
+ * @async_recv:           process received async transactions since last frozen
+ *                        (protected by @inner_lock)
  * @freeze_wait:          waitqueue of processes waiting for all outstanding
  *                        transactions to be processed
  *                        (protected by @inner_lock)
@@ -422,6 +426,8 @@ struct binder_proc {
 	int outstanding_txns;
 	bool is_dead;
 	bool is_frozen;
+	bool sync_recv;
+	bool async_recv;
 	wait_queue_head_t freeze_wait;
 
 	struct list_head todo;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 7eb5b818b3c1..156070d18c4f 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -223,6 +223,12 @@ struct binder_freeze_info {
 	__u32            timeout_ms;
 };
 
+struct binder_frozen_status_info {
+	__u32            pid;
+	__u32            sync_recv;
+	__u32            async_recv;
+};
+
 #define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
 #define BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
 #define BINDER_SET_MAX_THREADS		_IOW('b', 5, __u32)
@@ -234,6 +240,7 @@ struct binder_freeze_info {
 #define BINDER_GET_NODE_INFO_FOR_REF	_IOWR('b', 12, struct binder_node_info_for_ref)
 #define BINDER_SET_CONTEXT_MGR_EXT	_IOW('b', 13, struct flat_binder_object)
 #define BINDER_FREEZE			_IOW('b', 14, struct binder_freeze_info)
+#define BINDER_GET_FROZEN_INFO		_IOWR('b', 15, struct binder_frozen_status_info)
 
 /*
  * NOTE: Two special error codes you should check for when calling
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability

[Christian Brauner]

On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote:
> From: Li Li <dualli@google.com>
> 
> To improve the user experience when switching between recently used
> applications, the background applications which are not currently needed
> are cached in the memory. Normally, a well designed application will not
> consume valuable CPU resources in the background. However, it's possible
> some applications are not able or willing to behave as expected, wasting
> energy even after being cached.
> 
> It is a good idea to freeze those applications when they're only being
> kept alive for the sake of faster startup and energy saving. These kernel
> patches will provide the necessary infrastructure for user space framework
> to freeze and thaw a cached process, check the current freezing status and
> correctly deal with outstanding binder transactions to frozen processes.
> 
> Changes in v2: avoid panic by using pr_warn for unexpected cases.
> Changes in v3: improved errcode logic in binder_proc_transaction().
> 
> Marco Ballesio (3):
>   binder: BINDER_FREEZE ioctl
>   binder: use EINTR for interrupted wait for work
>   binder: BINDER_GET_FROZEN_INFO ioctl
> 
>  drivers/android/binder.c            | 198 ++++++++++++++++++++++++++--
>  drivers/android/binder_internal.h   |  18 +++
>  include/uapi/linux/android/binder.h |  20 +++
>  3 files changed, 224 insertions(+), 12 deletions(-)

[+Cc Jann]

Christian

Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability

[Jann Horn]

On Wed, Mar 17, 2021 at 7:00 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote:
> > To improve the user experience when switching between recently used
> > applications, the background applications which are not currently needed
> > are cached in the memory. Normally, a well designed application will not
> > consume valuable CPU resources in the background. However, it's possible
> > some applications are not able or willing to behave as expected, wasting
> > energy even after being cached.
> >
> > It is a good idea to freeze those applications when they're only being
> > kept alive for the sake of faster startup and energy saving. These kernel
> > patches will provide the necessary infrastructure for user space framework
> > to freeze and thaw a cached process, check the current freezing status and
> > correctly deal with outstanding binder transactions to frozen processes.

I just have some comments on the overall design:

This seems a bit convoluted to me; and I'm not sure whether this is
really something the kernel should get involved in, or whether this
patchset is operating at the right layer.

If there are non-binder threads that are misbehaving, could you
instead stop all those threads in pure userspace code (e.g. by sending
a thread-directed signal to all of them and letting the signal handler
sleep on a futex); and if the binder thread receives a transaction
that should be handled, wake up those threads again?

Or alternatively you could detect that the application is being woken
up frequently even though it's supposed to be idle (e.g. using
information from procfs), and kill it since you consider it to be
misbehaving?

Or if there are specific usage patterns you see frequently that you
consider to be wasting CPU resources (e.g. setting an interval timer
that fires in short intervals), you could try to delay such timers.


With your current approach, you're baking the assumption that all IPC
goes through binder into the kernel API; things like passing a file
descriptor to a pipe through binder or using shared futexes are no
longer usable for cross-process communication without making more
kernel changes. I'm not sure whether that's a good idea. On top of
that, if you freeze a process while it is in the middle of some
operation, resources associated with the operation will probably stay
in use for quite some time; for example, if an app is in the middle of
downloading some data over HTTP, and you freeze it, this may cause the
TCP connection to remain active and consume resources for send/receive
buffers on both the device and the server.

Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability

[Todd Kjos]

On Wed, Mar 17, 2021 at 1:17 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Mar 17, 2021 at 7:00 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote:
> > > To improve the user experience when switching between recently used
> > > applications, the background applications which are not currently needed
> > > are cached in the memory. Normally, a well designed application will not
> > > consume valuable CPU resources in the background. However, it's possible
> > > some applications are not able or willing to behave as expected, wasting
> > > energy even after being cached.
> > >
> > > It is a good idea to freeze those applications when they're only being
> > > kept alive for the sake of faster startup and energy saving. These kernel
> > > patches will provide the necessary infrastructure for user space framework
> > > to freeze and thaw a cached process, check the current freezing status and
> > > correctly deal with outstanding binder transactions to frozen processes.
>
> I just have some comments on the overall design:
>
> This seems a bit convoluted to me; and I'm not sure whether this is
> really something the kernel should get involved in, or whether this
> patchset is operating at the right layer.

The issue is that there is lot's of per-process state in the binder
driver that needs to be quiesced prior to freezing the process (using
the standard freeze mechanism of
Documentation/admin-guide/cgroup-v1/freezer-subsystem.rst). That's all
this series does... quiesces binder state prior to freeze and then
re-enable transactions when the process is thawed.

>
> If there are non-binder threads that are misbehaving, could you
> instead stop all those threads in pure userspace code (e.g. by sending
> a thread-directed signal to all of them and letting the signal handler
> sleep on a futex); and if the binder thread receives a transaction
> that should be handled, wake up those threads again?

It is not an issue of stopping threads. It's an issue of quiescing
binder for a process so clients aren't blocked waiting for a response
from a frozen process that may never handle the transaction. This
series causes the soon-to-be-frozen process to reject new transactions
and allows user-space to detect when the transactions have drained
from the queues prior to freezing the process.

>
> Or alternatively you could detect that the application is being woken
> up frequently even though it's supposed to be idle (e.g. using
> information from procfs), and kill it since you consider it to be
> misbehaving?
>
> Or if there are specific usage patterns you see frequently that you
> consider to be wasting CPU resources (e.g. setting an interval timer
> that fires in short intervals), you could try to delay such timers.
>
>
> With your current approach, you're baking the assumption that all IPC
> goes through binder into the kernel API; things like passing a file
> descriptor to a pipe through binder or using shared futexes are no

No, we're dealing with an issue that is particular to binder IPC when
freezing a process. I suspect that other IPC mechanisms do not have
this issue -- and if any do for Android, then they would need
equivalent pre-freeze/post-freeze mechanisms. So far in the testing of
freezing in Android R, there haven't been issues with pipes or futexes
that required this kind of explicit quiescing (at least none that I
know of -- dualli@, please comment if there have been these kinds of
issues).

> longer usable for cross-process communication without making more
> kernel changes. I'm not sure whether that's a good idea. On top of
> that, if you freeze a process while it is in the middle of some
> operation, resources associated with the operation will probably stay
> in use for quite some time; for example, if an app is in the middle of
> downloading some data over HTTP, and you freeze it, this may cause the
> TCP connection to remain active and consume resources for send/receive
> buffers on both the device and the server.

Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability

[Li Li]

On Thu, Mar 18, 2021 at 9:20 AM Todd Kjos <tkjos@google.com> wrote:
>
> On Wed, Mar 17, 2021 at 1:17 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Wed, Mar 17, 2021 at 7:00 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > > On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote:
> > > > To improve the user experience when switching between recently used
> > > > applications, the background applications which are not currently needed
> > > > are cached in the memory. Normally, a well designed application will not
> > > > consume valuable CPU resources in the background. However, it's possible
> > > > some applications are not able or willing to behave as expected, wasting
> > > > energy even after being cached.
> > > >
> > > > It is a good idea to freeze those applications when they're only being
> > > > kept alive for the sake of faster startup and energy saving. These kernel
> > > > patches will provide the necessary infrastructure for user space framework
> > > > to freeze and thaw a cached process, check the current freezing status and
> > > > correctly deal with outstanding binder transactions to frozen processes.
> >
> > I just have some comments on the overall design:
> >
> > This seems a bit convoluted to me; and I'm not sure whether this is
> > really something the kernel should get involved in, or whether this
> > patchset is operating at the right layer.
>
> The issue is that there is lot's of per-process state in the binder
> driver that needs to be quiesced prior to freezing the process (using
> the standard freeze mechanism of
> Documentation/admin-guide/cgroup-v1/freezer-subsystem.rst). That's all
> this series does... quiesces binder state prior to freeze and then
> re-enable transactions when the process is thawed.
>
> >
> > If there are non-binder threads that are misbehaving, could you
> > instead stop all those threads in pure userspace code (e.g. by sending
> > a thread-directed signal to all of them and letting the signal handler
> > sleep on a futex); and if the binder thread receives a transaction
> > that should be handled, wake up those threads again?
>
> It is not an issue of stopping threads. It's an issue of quiescing
> binder for a process so clients aren't blocked waiting for a response
> from a frozen process that may never handle the transaction. This
> series causes the soon-to-be-frozen process to reject new transactions
> and allows user-space to detect when the transactions have drained
> from the queues prior to freezing the process.
>
> >
> > Or alternatively you could detect that the application is being woken
> > up frequently even though it's supposed to be idle (e.g. using
> > information from procfs), and kill it since you consider it to be
> > misbehaving?
> >
> > Or if there are specific usage patterns you see frequently that you
> > consider to be wasting CPU resources (e.g. setting an interval timer
> > that fires in short intervals), you could try to delay such timers.
> >
> >
> > With your current approach, you're baking the assumption that all IPC
> > goes through binder into the kernel API; things like passing a file
> > descriptor to a pipe through binder or using shared futexes are no
>
> No, we're dealing with an issue that is particular to binder IPC when
> freezing a process. I suspect that other IPC mechanisms do not have
> this issue -- and if any do for Android, then they would need
> equivalent pre-freeze/post-freeze mechanisms. So far in the testing of
> freezing in Android R, there haven't been issues with pipes or futexes
> that required this kind of explicit quiescing (at least none that I
> know of -- dualli@, please comment if there have been these kinds of
> issues).
Correct, currently there's no evidence the similar things should be applied
to other IPC mechanisms. But we'll keep an eye on this.

>
> > longer usable for cross-process communication without making more
> > kernel changes. I'm not sure whether that's a good idea. On top of
> > that, if you freeze a process while it is in the middle of some
> > operation, resources associated with the operation will probably stay
> > in use for quite some time; for example, if an app is in the middle of
> > downloading some data over HTTP, and you freeze it, this may cause the
> > TCP connection to remain active and consume resources for send/receive
> > buffers on both the device and the server.