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(-)
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,
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)
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); }
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;
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
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;
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
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
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-_sgTzwVpwqk6RWWUNKKyJ... [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);
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-_sgTzwVpwqk6RWWUNKKyJ... [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
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-_sgTzwVpwqk6RWWUNKKyJ... [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
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-_sgTzwVpwqk6RWWUNKKyJ... [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
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-_sgTzwVpwqk6RWWUNKKyJ... [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
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-_sgTzwVpwqk6RWWUNKKyJ... [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
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-_sgTzwVpwqk6RWWUNKKyJ... [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:
- return ESRCH
- silently fail and don't allocate a ref->freeze
- allocate a ref->freeze but don't notify the current state
- 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
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:
- return ESRCH
- silently fail and don't allocate a ref->freeze
- allocate a ref->freeze but don't notify the current state
- 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
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",
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
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
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;
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
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
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
linux-stable-mirror@lists.linaro.org
-
Alice Ryhl -
Carlos Llamas -
Todd Kjos -
Yu-Ting Tseng