binder: several fixes for frozen notification

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 [email protected]

Acked-by Todd Kjos <[email protected]>

Signed-off-by Carlos Llamas <[email protected]>

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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Signed-off-by Carlos Llamas <[email protected]>

---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/android/binder.c +3 -1

index 2be9f3559ed7..73dc6cbc1681 100644
@@ -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

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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Signed-off-by Carlos Llamas <[email protected]>

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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +13 -16

index 73dc6cbc1681..415fc9759249 100644
@@ -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

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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +3 -14

index 73dc6cbc1681..415fc9759249 100644
@@ -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);

drivers/android/binder.c +1 -1

+       }
 
        binder_node_unlock(ref->node);
        binder_proc_unlock(proc);
--

2.46.1.824.gd892dcdcdd-goog

Quoted 60 lines · 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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +3 -14

index 73dc6cbc1681..415fc9759249 100644
@@ -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);

drivers/android/binder.c binder_request_freeze_notification(struct binder_proc *proc, · line 3897

Quoted 10 lines

drivers/android/binder.c +9 -1

-       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?

Quoted 8 lines

drivers/android/binder.c +1 -1

+       }
 
        binder_node_unlock(ref->node);
        binder_proc_unlock(proc);
--

2.46.1.824.gd892dcdcdd-goog

Quoted 78 lines · Alice Ryhl

Quoted 60 lines · 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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +3 -14

index 73dc6cbc1681..415fc9759249 100644
@@ -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);

drivers/android/binder.c binder_request_freeze_notification(struct binder_proc *proc, · line 3897

Quoted 10 lines

drivers/android/binder.c +9 -1

-       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?

Quoted 4 lines

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?

Quoted 71 lines · Yu-Ting Tseng

Quoted 69 lines · Alice Ryhl

Quoted 67 lines · 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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +9 -15

index 73dc6cbc1681..415fc9759249 100644
@@ -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);

Quoted 8 lines

Quoted 8 lines

drivers/android/binder.c binder_request_freeze_notification(struct binder_proc *proc, · line 3897

Quoted 3 lines

drivers/android/binder.c +3 -0

+               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?

Quoted 7 lines

Quoted 4 lines

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.

Quoted 72 lines · Alice Ryhl

Quoted 71 lines · Yu-Ting Tseng

Quoted 69 lines · Alice Ryhl

Quoted 67 lines · 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 [email protected]

Suggested-by Alice Ryhl <[email protected]>

Link https://lore.kernel.org/all/CAH5fLghapZJ4PbbkC8V5A6Zay-_sgTzwVpwqk6RWWUNKKyJC_Q@mail.gmail.com/ [1] open here

Signed-off-by Carlos Llamas <[email protected]>

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

drivers/android/binder.c +9 -15

index 73dc6cbc1681..415fc9759249 100644
@@ -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);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/android/binder.c binder_request_freeze_notification(struct binder_proc *proc, · line 3897

Quoted 3 lines

drivers/android/binder.c +3 -0

+               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.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 16 lines · Carlos Llamas

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.

Quoted 2 lines

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.

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 [email protected]

Signed-off-by Carlos Llamas <[email protected]>

---
 drivers/android/binder.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/android/binder.c +11 -1

index 415fc9759249..7c09b5e38e32 100644
@@ -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

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 [email protected]

Signed-off-by Carlos Llamas <[email protected]>

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 [email protected]

Signed-off-by Carlos Llamas <[email protected]>

---
 drivers/android/binder.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/android/binder.c +4 -1

index 7c09b5e38e32..ef353ca13c35 100644
@@ -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

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 [email protected]

Signed-off-by Carlos Llamas <[email protected]>

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/[email protected]/