[PATCH v2 0/8] binder: several fixes for frozen notification

[PATCH v2 0/8] binder: several fixes for frozen notification

[Carlos Llamas]

These are all fixes for the frozen notification patch [1], which as of
today hasn't landed in mainline yet. As such, this patchset is rebased
on top of the char-misc-next branch.

[1] https://lore.kernel.org/all/20240709070047.4055369-2-yutingtseng@google.com/

Cc: stable@vger.kernel.org
Cc: Yu-Ting Tseng <yutingtseng@google.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Martijn Coenen <maco@google.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Viktor Martensson <vmartensson@google.com>

v1: https://lore.kernel.org/all/20240924184401.76043-1-cmllamas@google.com/

v2:
  * debug output for BINDER_WORK_CLEAR_FREEZE_NOTIFICATION (Alice)
  * allow notifications for dead nodes instead of EINVAL (Alice)
  * add fix for memleak of proc->delivered_freeze
  * add proc->delivered_freeze to debug output
  * collect tags

Carlos Llamas (8):
  binder: fix node UAF in binder_add_freeze_work()
  binder: fix OOB in binder_add_freeze_work()
  binder: fix freeze UAF in binder_release_work()
  binder: fix BINDER_WORK_FROZEN_BINDER debug logs
  binder: fix BINDER_WORK_CLEAR_FREEZE_NOTIFICATION debug logs
  binder: allow freeze notification for dead nodes
  binder: fix memleak of proc->delivered_freeze
  binder: add delivered_freeze to debugfs output

 drivers/android/binder.c | 64 ++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 15 deletions(-)

-- 
2.46.1.824.gd892dcdcdd-goog

[PATCH v2 1/8] binder: fix node UAF in binder_add_freeze_work()

[Carlos Llamas]

In binder_add_freeze_work() we iterate over the proc->nodes with the
proc->inner_lock held. However, this lock is temporarily dropped in
order to acquire the node->lock first (lock nesting order). This can
race with binder_node_release() and trigger a use-after-free:

  ==================================================================
  BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x19c
  Write of size 4 at addr ffff53c04c29dd04 by task freeze/640

  CPU: 5 UID: 0 PID: 640 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #17
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   _raw_spin_lock+0xe4/0x19c
   binder_add_freeze_work+0x148/0x478
   binder_ioctl+0x1e70/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  Allocated by task 637:
   __kmalloc_cache_noprof+0x12c/0x27c
   binder_new_node+0x50/0x700
   binder_transaction+0x35ac/0x6f74
   binder_thread_write+0xfb8/0x42a0
   binder_ioctl+0x18f0/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  Freed by task 637:
   kfree+0xf0/0x330
   binder_thread_read+0x1e88/0x3a68
   binder_ioctl+0x16d8/0x25ac
   __arm64_sys_ioctl+0x124/0x190
  ==================================================================

Fix the race by taking a temporary reference on the node before
releasing the proc->inner lock. This ensures the node remains alive
while in use.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 978740537a1a..4d90203ea048 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5552,6 +5552,7 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
 
 static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 {
+	struct binder_node *prev = NULL;
 	struct rb_node *n;
 	struct binder_ref *ref;
 
@@ -5560,7 +5561,10 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 		struct binder_node *node;
 
 		node = rb_entry(n, struct binder_node, rb_node);
+		binder_inc_node_tmpref_ilocked(node);
 		binder_inner_proc_unlock(proc);
+		if (prev)
+			binder_put_node(prev);
 		binder_node_lock(node);
 		hlist_for_each_entry(ref, &node->refs, node_entry) {
 			/*
@@ -5586,10 +5590,13 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 			}
 			binder_inner_proc_unlock(ref->proc);
 		}
+		prev = node;
 		binder_node_unlock(node);
 		binder_inner_proc_lock(proc);
 	}
 	binder_inner_proc_unlock(proc);
+	if (prev)
+		binder_put_node(prev);
 }
 
 static int binder_ioctl_freeze(struct binder_freeze_info *info,
-- 
2.46.1.824.gd892dcdcdd-goog

[PATCH v2 2/8] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

In binder_add_freeze_work() we iterate over the proc->nodes with the
proc->inner_lock held. However, this lock is temporarily dropped to
acquire the node->lock first (lock nesting order). This can race with
binder_deferred_release() which removes the nodes from the proc->nodes
rbtree and adds them into binder_dead_nodes list. This leads to a broken
iteration in binder_add_freeze_work() as rb_next() will use data from
binder_dead_nodes, triggering an out-of-bounds access:

  ==================================================================
  BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
  Read of size 8 at addr ffffcb84285f7170 by task freeze/660

  CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   rb_next+0xfc/0x124
   binder_add_freeze_work+0x344/0x534
   binder_ioctl+0x1e70/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  The buggy address belongs to the variable:
   binder_dead_nodes+0x10/0x40
  [...]
  ==================================================================

This is possible because proc->nodes (rbtree) and binder_dead_nodes
(list) share entries in binder_node through a union:

	struct binder_node {
	[...]
		union {
			struct rb_node rb_node;
			struct hlist_node dead_node;
		};

Fix the race by checking that the proc is still alive. If not, simply
break out of the iteration.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4d90203ea048..8bca2de6fa24 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5593,6 +5593,8 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 		prev = node;
 		binder_node_unlock(node);
 		binder_inner_proc_lock(proc);
+		if (proc->is_dead)
+			break;
 	}
 	binder_inner_proc_unlock(proc);
 	if (prev)
-- 
2.46.1.824.gd892dcdcdd-goog

[PATCH v2 3/8] binder: fix freeze UAF in binder_release_work()

[Carlos Llamas]

When a binder reference is cleaned up, any freeze work queued in the
associated process should also be removed. Otherwise, the reference is
freed while its ref->freeze.work is still queued in proc->work leading
to a use-after-free issue as shown by the following KASAN report:

  ==================================================================
  BUG: KASAN: slab-use-after-free in binder_release_work+0x398/0x3d0
  Read of size 8 at addr ffff31600ee91488 by task kworker/5:1/211

  CPU: 5 UID: 0 PID: 211 Comm: kworker/5:1 Not tainted 6.11.0-rc7-00382-gfc6c92196396 #22
  Hardware name: linux,dummy-virt (DT)
  Workqueue: events binder_deferred_func
  Call trace:
   binder_release_work+0x398/0x3d0
   binder_deferred_func+0xb60/0x109c
   process_one_work+0x51c/0xbd4
   worker_thread+0x608/0xee8

  Allocated by task 703:
   __kmalloc_cache_noprof+0x130/0x280
   binder_thread_write+0xdb4/0x42a0
   binder_ioctl+0x18f0/0x25ac
   __arm64_sys_ioctl+0x124/0x190
   invoke_syscall+0x6c/0x254

  Freed by task 211:
   kfree+0xc4/0x230
   binder_deferred_func+0xae8/0x109c
   process_one_work+0x51c/0xbd4
   worker_thread+0x608/0xee8
  ==================================================================

This commit fixes the issue by ensuring any queued freeze work is removed
when cleaning up a binder reference.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Acked-by: Todd Kjos <tkjos@android.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8bca2de6fa24..d955135ee37a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1225,6 +1225,12 @@ static void binder_cleanup_ref_olocked(struct binder_ref *ref)
 		binder_dequeue_work(ref->proc, &ref->death->work);
 		binder_stats_deleted(BINDER_STAT_DEATH);
 	}
+
+	if (ref->freeze) {
+		binder_dequeue_work(ref->proc, &ref->freeze->work);
+		binder_stats_deleted(BINDER_STAT_FREEZE);
+	}
+
 	binder_stats_deleted(BINDER_STAT_REF);
 }
 
-- 
2.46.1.824.gd892dcdcdd-goog

[PATCH v2 4/8] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Carlos Llamas]

The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
entries and it shows up as "unknown work" when logged:

  proc 649
  context binder-test
    thread 649: l 00 need_return 0 tr 0
    ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
    unknown work: type 10

This patch add the freeze work type and is now logged as such:

  proc 637
  context binder-test
    thread 637: l 00 need_return 0 tr 0
    ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
    has frozen binder

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Acked-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d955135ee37a..2be9f3559ed7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
 	case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
 		seq_printf(m, "%shas cleared death notification\n", prefix);
 		break;
+	case BINDER_WORK_FROZEN_BINDER:
+		seq_printf(m, "%shas frozen binder\n", prefix);
+		break;
 	default:
 		seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
 		break;
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v2 4/8] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:36 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
> entries and it shows up as "unknown work" when logged:
>
>   proc 649
>   context binder-test
>     thread 649: l 00 need_return 0 tr 0
>     ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
>     unknown work: type 10
>
> This patch add the freeze work type and is now logged as such:
>
>   proc 637
>   context binder-test
>     thread 637: l 00 need_return 0 tr 0
>     ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
>     has frozen binder
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Acked-by: Todd Kjos <tkjos@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

[PATCH v2 5/8] binder: fix BINDER_WORK_CLEAR_FREEZE_NOTIFICATION debug logs

[Carlos Llamas]

proc 699
context binder-test
  thread 699: l 00 need_return 0 tr 0
  ref 25: desc 1 node 20 s 1 w 0 d 00000000c03e09a3
  unknown work: type 11

proc 640
context binder-test
  thread 640: l 00 need_return 0 tr 0
  ref 8: desc 1 node 3 s 1 w 0 d 000000002bb493e1
  has cleared freeze notification

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2be9f3559ed7..73dc6cbc1681 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6411,6 +6411,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
 	case BINDER_WORK_FROZEN_BINDER:
 		seq_printf(m, "%shas frozen binder\n", prefix);
 		break;
+	case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION:
+		seq_printf(m, "%shas cleared freeze notification\n", prefix);
+		break;
 	default:
 		seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
 		break;
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v2 5/8] binder: fix BINDER_WORK_CLEAR_FREEZE_NOTIFICATION debug logs

[Todd Kjos]

On Thu, Sep 26, 2024 at 4:36 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> proc 699
> context binder-test
>   thread 699: l 00 need_return 0 tr 0
>   ref 25: desc 1 node 20 s 1 w 0 d 00000000c03e09a3
>   unknown work: type 11
>
> proc 640
> context binder-test
>   thread 640: l 00 need_return 0 tr 0
>   ref 8: desc 1 node 3 s 1 w 0 d 000000002bb493e1
>   has cleared freeze notification
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 2be9f3559ed7..73dc6cbc1681 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6411,6 +6411,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
>         case BINDER_WORK_FROZEN_BINDER:
>                 seq_printf(m, "%shas frozen binder\n", prefix);
>                 break;
> +       case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION:
> +               seq_printf(m, "%shas cleared freeze notification\n", prefix);
> +               break;
>         default:
>                 seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
>                 break;
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH v2 5/8] binder: fix BINDER_WORK_CLEAR_FREEZE_NOTIFICATION debug logs

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:36 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> proc 699
> context binder-test
>   thread 699: l 00 need_return 0 tr 0
>   ref 25: desc 1 node 20 s 1 w 0 d 00000000c03e09a3
>   unknown work: type 11
>
> proc 640
> context binder-test
>   thread 640: l 00 need_return 0 tr 0
>   ref 8: desc 1 node 3 s 1 w 0 d 000000002bb493e1
>   has cleared freeze notification
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

[PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Carlos Llamas]

Alice points out that binder_request_freeze_notification() should not
return EINVAL when the relevant node is dead [1]. The node can die at
any point even if the user input is valid. Instead, allow the request
to be allocated but skip the initial notification for dead nodes. This
avoids propagating unnecessary errors back to userspace.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 73dc6cbc1681..415fc9759249 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
 {
 	struct binder_ref_freeze *freeze;
 	struct binder_ref *ref;
-	bool is_frozen;
 
 	freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
 	if (!freeze)
@@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
 	}
 
 	binder_node_lock(ref->node);
-
-	if (ref->freeze || !ref->node->proc) {
-		binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
-				  proc->pid, thread->pid,
-				  ref->freeze ? "already set" : "dead node");
+	if (ref->freeze) {
+		binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
+				  proc->pid, thread->pid);
 		binder_node_unlock(ref->node);
 		binder_proc_unlock(proc);
 		kfree(freeze);
 		return -EINVAL;
 	}
-	binder_inner_proc_lock(ref->node->proc);
-	is_frozen = ref->node->proc->is_frozen;
-	binder_inner_proc_unlock(ref->node->proc);
 
 	binder_stats_created(BINDER_STAT_FREEZE);
 	INIT_LIST_HEAD(&freeze->work.entry);
 	freeze->cookie = handle_cookie->cookie;
 	freeze->work.type = BINDER_WORK_FROZEN_BINDER;
-	freeze->is_frozen = is_frozen;
-
 	ref->freeze = freeze;
 
-	binder_inner_proc_lock(proc);
-	binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
-	binder_wakeup_proc_ilocked(proc);
-	binder_inner_proc_unlock(proc);
+	if (ref->node->proc) {
+		binder_inner_proc_lock(ref->node->proc);
+		freeze->is_frozen = ref->node->proc->is_frozen;
+		binder_inner_proc_unlock(ref->node->proc);
+
+		binder_inner_proc_lock(proc);
+		binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+		binder_wakeup_proc_ilocked(proc);
+		binder_inner_proc_unlock(proc);
+	}
 
 	binder_node_unlock(ref->node);
 	binder_proc_unlock(proc);
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Todd Kjos]

On Thu, Sep 26, 2024 at 4:36 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> Alice points out that binder_request_freeze_notification() should not
> return EINVAL when the relevant node is dead [1]. The node can die at
> any point even if the user input is valid. Instead, allow the request
> to be allocated but skip the initial notification for dead nodes. This
> avoids propagating unnecessary errors back to userspace.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 73dc6cbc1681..415fc9759249 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
>  {
>         struct binder_ref_freeze *freeze;
>         struct binder_ref *ref;
> -       bool is_frozen;
>
>         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
>         if (!freeze)
> @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
>         }
>
>         binder_node_lock(ref->node);
> -
> -       if (ref->freeze || !ref->node->proc) {
> -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> -                                 proc->pid, thread->pid,
> -                                 ref->freeze ? "already set" : "dead node");
> +       if (ref->freeze) {
> +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> +                                 proc->pid, thread->pid);
>                 binder_node_unlock(ref->node);
>                 binder_proc_unlock(proc);
>                 kfree(freeze);
>                 return -EINVAL;
>         }
> -       binder_inner_proc_lock(ref->node->proc);
> -       is_frozen = ref->node->proc->is_frozen;
> -       binder_inner_proc_unlock(ref->node->proc);
>
>         binder_stats_created(BINDER_STAT_FREEZE);
>         INIT_LIST_HEAD(&freeze->work.entry);
>         freeze->cookie = handle_cookie->cookie;
>         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> -       freeze->is_frozen = is_frozen;
> -
>         ref->freeze = freeze;
>
> -       binder_inner_proc_lock(proc);
> -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> -       binder_wakeup_proc_ilocked(proc);
> -       binder_inner_proc_unlock(proc);
> +       if (ref->node->proc) {
> +               binder_inner_proc_lock(ref->node->proc);
> +               freeze->is_frozen = ref->node->proc->is_frozen;
> +               binder_inner_proc_unlock(ref->node->proc);
> +
> +               binder_inner_proc_lock(proc);
> +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> +               binder_wakeup_proc_ilocked(proc);
> +               binder_inner_proc_unlock(proc);
> +       }
>
>         binder_node_unlock(ref->node);
>         binder_proc_unlock(proc);
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> Alice points out that binder_request_freeze_notification() should not
> return EINVAL when the relevant node is dead [1]. The node can die at
> any point even if the user input is valid. Instead, allow the request
> to be allocated but skip the initial notification for dead nodes. This
> avoids propagating unnecessary errors back to userspace.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 73dc6cbc1681..415fc9759249 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
>  {
>         struct binder_ref_freeze *freeze;
>         struct binder_ref *ref;
> -       bool is_frozen;
>
>         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
>         if (!freeze)
> @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
>         }
>
>         binder_node_lock(ref->node);
> -
> -       if (ref->freeze || !ref->node->proc) {
> -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> -                                 proc->pid, thread->pid,
> -                                 ref->freeze ? "already set" : "dead node");
> +       if (ref->freeze) {
> +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> +                                 proc->pid, thread->pid);
>                 binder_node_unlock(ref->node);
>                 binder_proc_unlock(proc);
>                 kfree(freeze);
>                 return -EINVAL;
>         }
> -       binder_inner_proc_lock(ref->node->proc);
> -       is_frozen = ref->node->proc->is_frozen;
> -       binder_inner_proc_unlock(ref->node->proc);
>
>         binder_stats_created(BINDER_STAT_FREEZE);
>         INIT_LIST_HEAD(&freeze->work.entry);
>         freeze->cookie = handle_cookie->cookie;
>         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> -       freeze->is_frozen = is_frozen;
> -
>         ref->freeze = freeze;
>
> -       binder_inner_proc_lock(proc);
> -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> -       binder_wakeup_proc_ilocked(proc);
> -       binder_inner_proc_unlock(proc);
> +       if (ref->node->proc) {
> +               binder_inner_proc_lock(ref->node->proc);
> +               freeze->is_frozen = ref->node->proc->is_frozen;
> +               binder_inner_proc_unlock(ref->node->proc);
> +
> +               binder_inner_proc_lock(proc);
> +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> +               binder_wakeup_proc_ilocked(proc);
> +               binder_inner_proc_unlock(proc);

This is not a problem with your change ... but, why exactly are we
scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
death notications, we only schedule it immediately if the process is
dead. So shouldn't we only schedule it if the process is not frozen?

And if the answer is that frozen notifications are always sent
immediately to notify about the current state, then we should also
send one for a dead process ... maybe. I guess a dead process is not
frozen?

> +       }
>
>         binder_node_unlock(ref->node);
>         binder_proc_unlock(proc);
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Yu-Ting Tseng]

On Fri, Sep 27, 2024 at 12:19 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > Alice points out that binder_request_freeze_notification() should not
> > return EINVAL when the relevant node is dead [1]. The node can die at
> > any point even if the user input is valid. Instead, allow the request
> > to be allocated but skip the initial notification for dead nodes. This
> > avoids propagating unnecessary errors back to userspace.
> >
> > Fixes: d579b04a52a1 ("binder: frozen notification")
> > Cc: stable@vger.kernel.org
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  drivers/android/binder.c | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 73dc6cbc1681..415fc9759249 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
> >  {
> >         struct binder_ref_freeze *freeze;
> >         struct binder_ref *ref;
> > -       bool is_frozen;
> >
> >         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> >         if (!freeze)
> > @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
> >         }
> >
> >         binder_node_lock(ref->node);
> > -
> > -       if (ref->freeze || !ref->node->proc) {
> > -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> > -                                 proc->pid, thread->pid,
> > -                                 ref->freeze ? "already set" : "dead node");
> > +       if (ref->freeze) {
> > +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> > +                                 proc->pid, thread->pid);
> >                 binder_node_unlock(ref->node);
> >                 binder_proc_unlock(proc);
> >                 kfree(freeze);
> >                 return -EINVAL;
> >         }
> > -       binder_inner_proc_lock(ref->node->proc);
> > -       is_frozen = ref->node->proc->is_frozen;
> > -       binder_inner_proc_unlock(ref->node->proc);
> >
> >         binder_stats_created(BINDER_STAT_FREEZE);
> >         INIT_LIST_HEAD(&freeze->work.entry);
> >         freeze->cookie = handle_cookie->cookie;
> >         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> > -       freeze->is_frozen = is_frozen;
> > -
> >         ref->freeze = freeze;
> >
> > -       binder_inner_proc_lock(proc);
> > -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> > -       binder_wakeup_proc_ilocked(proc);
> > -       binder_inner_proc_unlock(proc);
> > +       if (ref->node->proc) {
> > +               binder_inner_proc_lock(ref->node->proc);
> > +               freeze->is_frozen = ref->node->proc->is_frozen;
> > +               binder_inner_proc_unlock(ref->node->proc);
> > +
> > +               binder_inner_proc_lock(proc);
> > +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> > +               binder_wakeup_proc_ilocked(proc);
> > +               binder_inner_proc_unlock(proc);
>
> This is not a problem with your change ... but, why exactly are we
> scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
> death notications, we only schedule it immediately if the process is
> dead. So shouldn't we only schedule it if the process is not frozen?
>
> And if the answer is that frozen notifications are always sent
> immediately to notify about the current state, then we should also
> send one for a dead process ... maybe. I guess a dead process is not
> frozen?
Yes this is to immediately notify about the current state (frozen or
unfrozen). A dead process is in neither state so it feels more correct
not to send either?


>
> > +       }
> >
> >         binder_node_unlock(ref->node);
> >         binder_proc_unlock(proc);
> > --
> > 2.46.1.824.gd892dcdcdd-goog
> >

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Alice Ryhl]

On Fri, Sep 27, 2024 at 6:13 PM Yu-Ting Tseng <yutingtseng@google.com> wrote:
>
> On Fri, Sep 27, 2024 at 12:19 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > Alice points out that binder_request_freeze_notification() should not
> > > return EINVAL when the relevant node is dead [1]. The node can die at
> > > any point even if the user input is valid. Instead, allow the request
> > > to be allocated but skip the initial notification for dead nodes. This
> > > avoids propagating unnecessary errors back to userspace.
> > >
> > > Fixes: d579b04a52a1 ("binder: frozen notification")
> > > Cc: stable@vger.kernel.org
> > > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > > Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > ---
> > >  drivers/android/binder.c | 28 +++++++++++++---------------
> > >  1 file changed, 13 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index 73dc6cbc1681..415fc9759249 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > >  {
> > >         struct binder_ref_freeze *freeze;
> > >         struct binder_ref *ref;
> > > -       bool is_frozen;
> > >
> > >         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> > >         if (!freeze)
> > > @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > >         }
> > >
> > >         binder_node_lock(ref->node);
> > > -
> > > -       if (ref->freeze || !ref->node->proc) {
> > > -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> > > -                                 proc->pid, thread->pid,
> > > -                                 ref->freeze ? "already set" : "dead node");
> > > +       if (ref->freeze) {
> > > +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> > > +                                 proc->pid, thread->pid);
> > >                 binder_node_unlock(ref->node);
> > >                 binder_proc_unlock(proc);
> > >                 kfree(freeze);
> > >                 return -EINVAL;
> > >         }
> > > -       binder_inner_proc_lock(ref->node->proc);
> > > -       is_frozen = ref->node->proc->is_frozen;
> > > -       binder_inner_proc_unlock(ref->node->proc);
> > >
> > >         binder_stats_created(BINDER_STAT_FREEZE);
> > >         INIT_LIST_HEAD(&freeze->work.entry);
> > >         freeze->cookie = handle_cookie->cookie;
> > >         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> > > -       freeze->is_frozen = is_frozen;
> > > -
> > >         ref->freeze = freeze;
> > >
> > > -       binder_inner_proc_lock(proc);
> > > -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> > > -       binder_wakeup_proc_ilocked(proc);
> > > -       binder_inner_proc_unlock(proc);
> > > +       if (ref->node->proc) {
> > > +               binder_inner_proc_lock(ref->node->proc);
> > > +               freeze->is_frozen = ref->node->proc->is_frozen;
> > > +               binder_inner_proc_unlock(ref->node->proc);
> > > +
> > > +               binder_inner_proc_lock(proc);
> > > +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> > > +               binder_wakeup_proc_ilocked(proc);
> > > +               binder_inner_proc_unlock(proc);
> >
> > This is not a problem with your change ... but, why exactly are we
> > scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
> > death notications, we only schedule it immediately if the process is
> > dead. So shouldn't we only schedule it if the process is not frozen?
> >
> > And if the answer is that frozen notifications are always sent
> > immediately to notify about the current state, then we should also
> > send one for a dead process ... maybe. I guess a dead process is not
> > frozen?
> Yes this is to immediately notify about the current state (frozen or
> unfrozen). A dead process is in neither state so it feels more correct
> not to send either?

Okay.

On the other hand, I can easily imagine userspace code being written
with the assumption that it'll always get a notification immediately.
That would probably result in deadlocks in the edge case where the
process happens to be dead.

Alice

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Carlos Llamas]

On Fri, Sep 27, 2024 at 06:15:40PM +0200, Alice Ryhl wrote:
> On Fri, Sep 27, 2024 at 6:13 PM Yu-Ting Tseng <yutingtseng@google.com> wrote:
> >
> > On Fri, Sep 27, 2024 at 12:19 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
> > > >
> > > > Alice points out that binder_request_freeze_notification() should not
> > > > return EINVAL when the relevant node is dead [1]. The node can die at
> > > > any point even if the user input is valid. Instead, allow the request
> > > > to be allocated but skip the initial notification for dead nodes. This
> > > > avoids propagating unnecessary errors back to userspace.
> > > >
> > > > Fixes: d579b04a52a1 ("binder: frozen notification")
> > > > Cc: stable@vger.kernel.org
> > > > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > > > Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > ---
> > > >  drivers/android/binder.c | 28 +++++++++++++---------------
> > > >  1 file changed, 13 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index 73dc6cbc1681..415fc9759249 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > > >  {
> > > >         struct binder_ref_freeze *freeze;
> > > >         struct binder_ref *ref;
> > > > -       bool is_frozen;
> > > >
> > > >         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> > > >         if (!freeze)
> > > > @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > > >         }
> > > >
> > > >         binder_node_lock(ref->node);
> > > > -
> > > > -       if (ref->freeze || !ref->node->proc) {
> > > > -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> > > > -                                 proc->pid, thread->pid,
> > > > -                                 ref->freeze ? "already set" : "dead node");
> > > > +       if (ref->freeze) {
> > > > +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> > > > +                                 proc->pid, thread->pid);
> > > >                 binder_node_unlock(ref->node);
> > > >                 binder_proc_unlock(proc);
> > > >                 kfree(freeze);
> > > >                 return -EINVAL;
> > > >         }
> > > > -       binder_inner_proc_lock(ref->node->proc);
> > > > -       is_frozen = ref->node->proc->is_frozen;
> > > > -       binder_inner_proc_unlock(ref->node->proc);
> > > >
> > > >         binder_stats_created(BINDER_STAT_FREEZE);
> > > >         INIT_LIST_HEAD(&freeze->work.entry);
> > > >         freeze->cookie = handle_cookie->cookie;
> > > >         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> > > > -       freeze->is_frozen = is_frozen;
> > > > -
> > > >         ref->freeze = freeze;
> > > >
> > > > -       binder_inner_proc_lock(proc);
> > > > -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> > > > -       binder_wakeup_proc_ilocked(proc);
> > > > -       binder_inner_proc_unlock(proc);
> > > > +       if (ref->node->proc) {
> > > > +               binder_inner_proc_lock(ref->node->proc);
> > > > +               freeze->is_frozen = ref->node->proc->is_frozen;
> > > > +               binder_inner_proc_unlock(ref->node->proc);
> > > > +
> > > > +               binder_inner_proc_lock(proc);
> > > > +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> > > > +               binder_wakeup_proc_ilocked(proc);
> > > > +               binder_inner_proc_unlock(proc);
> > >
> > > This is not a problem with your change ... but, why exactly are we
> > > scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
> > > death notications, we only schedule it immediately if the process is
> > > dead. So shouldn't we only schedule it if the process is not frozen?

For death notifications, we only care about a remote binder's death.
Unlike freeze, in which we have a state that can toggle at any point.
This is important for suspending and resuming transactions to a node.

Sending the freeze notification immediately allows for (1) userspace
knowing the current state of the remote node and (2) avoiding a race
with BINDER_FREEZE ioctl in which we could miss a freeze/thaw.

> > > And if the answer is that frozen notifications are always sent
> > > immediately to notify about the current state, then we should also
> > > send one for a dead process ... maybe. I guess a dead process is not
> > > frozen?
> > Yes this is to immediately notify about the current state (frozen or
> > unfrozen). A dead process is in neither state so it feels more correct
> > not to send either?
> 
> Okay.
> 
> On the other hand, I can easily imagine userspace code being written
> with the assumption that it'll always get a notification immediately.
> That would probably result in deadlocks in the edge case where the
> process happens to be dead.

There are different ways to proceed with this dead node scenario:

1. return ESRCH
2. silently fail and don't allocate a ref->freeze
3. allocate a ref->freeze but don't notify the current state
4. allocate and send a "fake" state notification.

I like 1 just because it is technically the correct thing to do from the
driver's perspective. However, it does complicate things in userspace as
we've discussed. Option 2, could work but it would also fail with EINVAL
if a "clear notification" is sent later anyway. Option 3 changes the
behavior of guaranteeing a notification upon success. Option 4 can cause
trouble on how a "not-frozen" notification is handled in userspace e.g
start sending transactions.

As you can see there is no clear winner here, we have to compromise
something and option #3 is the best we can do IMO.

--
Carlos Llamas

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Alice Ryhl]

On Fri, Sep 27, 2024 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Fri, Sep 27, 2024 at 06:15:40PM +0200, Alice Ryhl wrote:
> > On Fri, Sep 27, 2024 at 6:13 PM Yu-Ting Tseng <yutingtseng@google.com> wrote:
> > >
> > > On Fri, Sep 27, 2024 at 12:19 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
> > > > >
> > > > > Alice points out that binder_request_freeze_notification() should not
> > > > > return EINVAL when the relevant node is dead [1]. The node can die at
> > > > > any point even if the user input is valid. Instead, allow the request
> > > > > to be allocated but skip the initial notification for dead nodes. This
> > > > > avoids propagating unnecessary errors back to userspace.
> > > > >
> > > > > Fixes: d579b04a52a1 ("binder: frozen notification")
> > > > > Cc: stable@vger.kernel.org
> > > > > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > > > > Link: https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1]
> > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > > > ---
> > > > >  drivers/android/binder.c | 28 +++++++++++++---------------
> > > > >  1 file changed, 13 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index 73dc6cbc1681..415fc9759249 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -3856,7 +3856,6 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > > > >  {
> > > > >         struct binder_ref_freeze *freeze;
> > > > >         struct binder_ref *ref;
> > > > > -       bool is_frozen;
> > > > >
> > > > >         freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> > > > >         if (!freeze)
> > > > > @@ -3872,32 +3871,31 @@ binder_request_freeze_notification(struct binder_proc *proc,
> > > > >         }
> > > > >
> > > > >         binder_node_lock(ref->node);
> > > > > -
> > > > > -       if (ref->freeze || !ref->node->proc) {
> > > > > -               binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
> > > > > -                                 proc->pid, thread->pid,
> > > > > -                                 ref->freeze ? "already set" : "dead node");
> > > > > +       if (ref->freeze) {
> > > > > +               binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION already set\n",
> > > > > +                                 proc->pid, thread->pid);
> > > > >                 binder_node_unlock(ref->node);
> > > > >                 binder_proc_unlock(proc);
> > > > >                 kfree(freeze);
> > > > >                 return -EINVAL;
> > > > >         }
> > > > > -       binder_inner_proc_lock(ref->node->proc);
> > > > > -       is_frozen = ref->node->proc->is_frozen;
> > > > > -       binder_inner_proc_unlock(ref->node->proc);
> > > > >
> > > > >         binder_stats_created(BINDER_STAT_FREEZE);
> > > > >         INIT_LIST_HEAD(&freeze->work.entry);
> > > > >         freeze->cookie = handle_cookie->cookie;
> > > > >         freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> > > > > -       freeze->is_frozen = is_frozen;
> > > > > -
> > > > >         ref->freeze = freeze;
> > > > >
> > > > > -       binder_inner_proc_lock(proc);
> > > > > -       binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> > > > > -       binder_wakeup_proc_ilocked(proc);
> > > > > -       binder_inner_proc_unlock(proc);
> > > > > +       if (ref->node->proc) {
> > > > > +               binder_inner_proc_lock(ref->node->proc);
> > > > > +               freeze->is_frozen = ref->node->proc->is_frozen;
> > > > > +               binder_inner_proc_unlock(ref->node->proc);
> > > > > +
> > > > > +               binder_inner_proc_lock(proc);
> > > > > +               binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> > > > > +               binder_wakeup_proc_ilocked(proc);
> > > > > +               binder_inner_proc_unlock(proc);
> > > >
> > > > This is not a problem with your change ... but, why exactly are we
> > > > scheduling the BINDER_WORK_FROZEN_BINDER right after creating it? For
> > > > death notications, we only schedule it immediately if the process is
> > > > dead. So shouldn't we only schedule it if the process is not frozen?
>
> For death notifications, we only care about a remote binder's death.
> Unlike freeze, in which we have a state that can toggle at any point.
> This is important for suspending and resuming transactions to a node.
>
> Sending the freeze notification immediately allows for (1) userspace
> knowing the current state of the remote node and (2) avoiding a race
> with BINDER_FREEZE ioctl in which we could miss a freeze/thaw.
>
> > > > And if the answer is that frozen notifications are always sent
> > > > immediately to notify about the current state, then we should also
> > > > send one for a dead process ... maybe. I guess a dead process is not
> > > > frozen?
> > > Yes this is to immediately notify about the current state (frozen or
> > > unfrozen). A dead process is in neither state so it feels more correct
> > > not to send either?
> >
> > Okay.
> >
> > On the other hand, I can easily imagine userspace code being written
> > with the assumption that it'll always get a notification immediately.
> > That would probably result in deadlocks in the edge case where the
> > process happens to be dead.
>
> There are different ways to proceed with this dead node scenario:
>
> 1. return ESRCH
> 2. silently fail and don't allocate a ref->freeze
> 3. allocate a ref->freeze but don't notify the current state
> 4. allocate and send a "fake" state notification.
>
> I like 1 just because it is technically the correct thing to do from the
> driver's perspective. However, it does complicate things in userspace as
> we've discussed. Option 2, could work but it would also fail with EINVAL
> if a "clear notification" is sent later anyway. Option 3 changes the
> behavior of guaranteeing a notification upon success. Option 4 can cause
> trouble on how a "not-frozen" notification is handled in userspace e.g
> start sending transactions.
>
> As you can see there is no clear winner here, we have to compromise
> something and option #3 is the best we can do IMO.

I am happy with both #3 and #4. I think #1 and #2 are problematic
because they will lead to userspace getting errors on correct use of
Binder.

Alice

Re: [PATCH v2 6/8] binder: allow freeze notification for dead nodes

[Carlos Llamas]

On Mon, Sep 30, 2024 at 03:30:01PM +0200, Alice Ryhl wrote:
> On Fri, Sep 27, 2024 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > There are different ways to proceed with this dead node scenario:
> >
> > 1. return ESRCH
> > 2. silently fail and don't allocate a ref->freeze
> > 3. allocate a ref->freeze but don't notify the current state
> > 4. allocate and send a "fake" state notification.
> >
> > I like 1 just because it is technically the correct thing to do from the
> > driver's perspective. However, it does complicate things in userspace as
> > we've discussed. Option 2, could work but it would also fail with EINVAL
> > if a "clear notification" is sent later anyway. Option 3 changes the
> > behavior of guaranteeing a notification upon success. Option 4 can cause
> > trouble on how a "not-frozen" notification is handled in userspace e.g
> > start sending transactions.
> >
> > As you can see there is no clear winner here, we have to compromise
> > something and option #3 is the best we can do IMO.
> 
> I am happy with both #3 and #4. I think #1 and #2 are problematic
> because they will lead to userspace getting errors on correct use of
> Binder.

After talking with userspace folks it seems that #3 would be their
preferred approach. So this v2 patch it the way to go then!

Thanks,
Carlos Llamas

[PATCH v2 7/8] binder: fix memleak of proc->delivered_freeze

[Carlos Llamas]

If a freeze notification is cleared with BC_CLEAR_FREEZE_NOTIFICATION
before calling binder_freeze_notification_done(), then it is detached
from its reference (e.g. ref->freeze) but the work remains queued in
proc->delivered_freeze. This leads to a memory leak when the process
exits as any pending entries in proc->delivered_freeze are not freed:

  unreferenced object 0xffff38e8cfa36180 (size 64):
    comm "binder-util", pid 655, jiffies 4294936641
    hex dump (first 32 bytes):
      b8 e9 9e c8 e8 38 ff ff b8 e9 9e c8 e8 38 ff ff  .....8.......8..
      0b 00 00 00 00 00 00 00 3c 1f 4b 00 00 00 00 00  ........<.K.....
    backtrace (crc 95983b32):
      [<000000000d0582cf>] kmemleak_alloc+0x34/0x40
      [<000000009c99a513>] __kmalloc_cache_noprof+0x208/0x280
      [<00000000313b1704>] binder_thread_write+0xdec/0x439c
      [<000000000cbd33bb>] binder_ioctl+0x1b68/0x22cc
      [<000000002bbedeeb>] __arm64_sys_ioctl+0x124/0x190
      [<00000000b439adee>] invoke_syscall+0x6c/0x254
      [<00000000173558fc>] el0_svc_common.constprop.0+0xac/0x230
      [<0000000084f72311>] do_el0_svc+0x40/0x58
      [<000000008b872457>] el0_svc+0x38/0x78
      [<00000000ee778653>] el0t_64_sync_handler+0x120/0x12c
      [<00000000a8ec61bf>] el0t_64_sync+0x190/0x194

This patch fixes the leak by ensuring that any pending entries in
proc->delivered_freeze are freed during binder_deferred_release().

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 415fc9759249..7c09b5e38e32 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5155,6 +5155,16 @@ static void binder_release_work(struct binder_proc *proc,
 		} break;
 		case BINDER_WORK_NODE:
 			break;
+		case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
+			struct binder_ref_freeze *freeze;
+
+			freeze = container_of(w, struct binder_ref_freeze, work);
+			binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+				     "undelivered freeze notification, %016llx\n",
+				     (u64)freeze->cookie);
+			kfree(freeze);
+			binder_stats_deleted(BINDER_STAT_FREEZE);
+		} break;
 		default:
 			pr_err("unexpected work type, %d, not freed\n",
 			       wtype);
@@ -6273,6 +6283,7 @@ static void binder_deferred_release(struct binder_proc *proc)
 
 	binder_release_work(proc, &proc->todo);
 	binder_release_work(proc, &proc->delivered_death);
+	binder_release_work(proc, &proc->delivered_freeze);
 
 	binder_debug(BINDER_DEBUG_OPEN_CLOSE,
 		     "%s: %d threads %d, nodes %d (ref %d), refs %d, active transactions %d\n",
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v2 7/8] binder: fix memleak of proc->delivered_freeze

[Todd Kjos]

On Thu, Sep 26, 2024 at 4:37 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> If a freeze notification is cleared with BC_CLEAR_FREEZE_NOTIFICATION
> before calling binder_freeze_notification_done(), then it is detached
> from its reference (e.g. ref->freeze) but the work remains queued in
> proc->delivered_freeze. This leads to a memory leak when the process
> exits as any pending entries in proc->delivered_freeze are not freed:
>
>   unreferenced object 0xffff38e8cfa36180 (size 64):
>     comm "binder-util", pid 655, jiffies 4294936641
>     hex dump (first 32 bytes):
>       b8 e9 9e c8 e8 38 ff ff b8 e9 9e c8 e8 38 ff ff  .....8.......8..
>       0b 00 00 00 00 00 00 00 3c 1f 4b 00 00 00 00 00  ........<.K.....
>     backtrace (crc 95983b32):
>       [<000000000d0582cf>] kmemleak_alloc+0x34/0x40
>       [<000000009c99a513>] __kmalloc_cache_noprof+0x208/0x280
>       [<00000000313b1704>] binder_thread_write+0xdec/0x439c
>       [<000000000cbd33bb>] binder_ioctl+0x1b68/0x22cc
>       [<000000002bbedeeb>] __arm64_sys_ioctl+0x124/0x190
>       [<00000000b439adee>] invoke_syscall+0x6c/0x254
>       [<00000000173558fc>] el0_svc_common.constprop.0+0xac/0x230
>       [<0000000084f72311>] do_el0_svc+0x40/0x58
>       [<000000008b872457>] el0_svc+0x38/0x78
>       [<00000000ee778653>] el0t_64_sync_handler+0x120/0x12c
>       [<00000000a8ec61bf>] el0t_64_sync+0x190/0x194
>
> This patch fixes the leak by ensuring that any pending entries in
> proc->delivered_freeze are freed during binder_deferred_release().
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 415fc9759249..7c09b5e38e32 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5155,6 +5155,16 @@ static void binder_release_work(struct binder_proc *proc,
>                 } break;
>                 case BINDER_WORK_NODE:
>                         break;
> +               case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
> +                       struct binder_ref_freeze *freeze;
> +
> +                       freeze = container_of(w, struct binder_ref_freeze, work);
> +                       binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
> +                                    "undelivered freeze notification, %016llx\n",
> +                                    (u64)freeze->cookie);
> +                       kfree(freeze);
> +                       binder_stats_deleted(BINDER_STAT_FREEZE);
> +               } break;
>                 default:
>                         pr_err("unexpected work type, %d, not freed\n",
>                                wtype);
> @@ -6273,6 +6283,7 @@ static void binder_deferred_release(struct binder_proc *proc)
>
>         binder_release_work(proc, &proc->todo);
>         binder_release_work(proc, &proc->delivered_death);
> +       binder_release_work(proc, &proc->delivered_freeze);
>
>         binder_debug(BINDER_DEBUG_OPEN_CLOSE,
>                      "%s: %d threads %d, nodes %d (ref %d), refs %d, active transactions %d\n",
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH v2 7/8] binder: fix memleak of proc->delivered_freeze

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> If a freeze notification is cleared with BC_CLEAR_FREEZE_NOTIFICATION
> before calling binder_freeze_notification_done(), then it is detached
> from its reference (e.g. ref->freeze) but the work remains queued in
> proc->delivered_freeze. This leads to a memory leak when the process
> exits as any pending entries in proc->delivered_freeze are not freed:
>
>   unreferenced object 0xffff38e8cfa36180 (size 64):
>     comm "binder-util", pid 655, jiffies 4294936641
>     hex dump (first 32 bytes):
>       b8 e9 9e c8 e8 38 ff ff b8 e9 9e c8 e8 38 ff ff  .....8.......8..
>       0b 00 00 00 00 00 00 00 3c 1f 4b 00 00 00 00 00  ........<.K.....
>     backtrace (crc 95983b32):
>       [<000000000d0582cf>] kmemleak_alloc+0x34/0x40
>       [<000000009c99a513>] __kmalloc_cache_noprof+0x208/0x280
>       [<00000000313b1704>] binder_thread_write+0xdec/0x439c
>       [<000000000cbd33bb>] binder_ioctl+0x1b68/0x22cc
>       [<000000002bbedeeb>] __arm64_sys_ioctl+0x124/0x190
>       [<00000000b439adee>] invoke_syscall+0x6c/0x254
>       [<00000000173558fc>] el0_svc_common.constprop.0+0xac/0x230
>       [<0000000084f72311>] do_el0_svc+0x40/0x58
>       [<000000008b872457>] el0_svc+0x38/0x78
>       [<00000000ee778653>] el0t_64_sync_handler+0x120/0x12c
>       [<00000000a8ec61bf>] el0t_64_sync+0x190/0x194
>
> This patch fixes the leak by ensuring that any pending entries in
> proc->delivered_freeze are freed during binder_deferred_release().
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

[PATCH v2 8/8] binder: add delivered_freeze to debugfs output

[Carlos Llamas]

Add the pending proc->delivered_freeze work to the debugfs output. This
information was omitted in the original implementation of the freeze
notification and can be valuable for debugging issues.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 7c09b5e38e32..ef353ca13c35 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6569,6 +6569,10 @@ static void print_binder_proc(struct seq_file *m,
 		seq_puts(m, "  has delivered dead binder\n");
 		break;
 	}
+	list_for_each_entry(w, &proc->delivered_freeze, entry) {
+		seq_puts(m, "  has delivered freeze binder\n");
+		break;
+	}
 	binder_inner_proc_unlock(proc);
 	if (!print_all && m->count == header_pos)
 		m->count = start_pos;
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH v2 8/8] binder: add delivered_freeze to debugfs output

[Todd Kjos]

On Thu, Sep 26, 2024 at 4:37 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> Add the pending proc->delivered_freeze work to the debugfs output. This
> information was omitted in the original implementation of the freeze
> notification and can be valuable for debugging issues.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 7c09b5e38e32..ef353ca13c35 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6569,6 +6569,10 @@ static void print_binder_proc(struct seq_file *m,
>                 seq_puts(m, "  has delivered dead binder\n");
>                 break;
>         }
> +       list_for_each_entry(w, &proc->delivered_freeze, entry) {
> +               seq_puts(m, "  has delivered freeze binder\n");
> +               break;
> +       }
>         binder_inner_proc_unlock(proc);
>         if (!print_all && m->count == header_pos)
>                 m->count = start_pos;
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH v2 8/8] binder: add delivered_freeze to debugfs output

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:37 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> Add the pending proc->delivered_freeze work to the debugfs output. This
> information was omitted in the original implementation of the freeze
> notification and can be valuable for debugging issues.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

Re: [PATCH v2 0/8] binder: several fixes for frozen notification

[Alice Ryhl]

On Fri, Sep 27, 2024 at 1:36 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> These are all fixes for the frozen notification patch [1], which as of
> today hasn't landed in mainline yet. As such, this patchset is rebased
> on top of the char-misc-next branch.
>
> [1] https://lore.kernel.org/all/20240709070047.4055369-2-yutingtseng@google.com/

I looked for other inconsistencies between death and freeze
notifications. I found two:

binder_free_proc has this line:
BUG_ON(!list_empty(&proc->delivered_death));

The top comment has this line:
 * 3) proc->inner_lock : protects the thread and node lists
 *    (proc->threads, proc->waiting_threads, proc->nodes)
 *    and all todo lists associated with the binder_proc
 *    (proc->todo, thread->todo, proc->delivered_death and
 *    node->async_todo), as well as thread->transaction_stack
 *    binder_inner_proc_lock() and binder_inner_proc_unlock()
 *    are used to acq/rel

Both mention delivered_death but not freeze.

Alice