* [PATCH v3] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
@ 2024-06-24 17:20 ` Yu-Ting Tseng
2024-06-25 3:01 ` Yu-Ting Tseng
2024-06-24 17:20 ` [PATCH v3 1/1] " Yu-Ting Tseng
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-06-24 17:20 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Yu-Ting Tseng (1):
binder: frozen notification
drivers/android/binder.c | 300 +++++++++++++++++++++++++++-
drivers/android/binder_internal.h | 23 ++-
include/uapi/linux/android/binder.h | 35 ++++
3 files changed, 354 insertions(+), 4 deletions(-)
> freeze was allocated with kzalloc(), you could drop the "= false".
Done.
> If !node->proc then process is dead. Do we really need to continue?
Update the code to return an error early if the process is already dead.
> This access to node->proc->* doesn't seem safe
Added locking.
> Why do we queue this notification?
Yes, this is to get the current state back to userspace. The userspace API delivers an initial event for the current state upon a listener registration, which makes it easier to track what the latest state is.
> I'm looking at the death notification code and it seems it only queues a
BR_ERROR after failing to allocate a "death" and that other errors are
silently ignored?
Sure. Please let me know if you think we need a change here.
> these could be just bitfields.
Done
> freeze->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION
Fixed. Working on a userspace test. Will post a link when it's ready.
base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
--
2.45.2.741.gdbec12cfda-goog
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3] binder: frozen notification
2024-06-24 17:20 ` [PATCH v3] " Yu-Ting Tseng
@ 2024-06-25 3:01 ` Yu-Ting Tseng
0 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-06-25 3:01 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel
On Mon, Jun 24, 2024 at 10:22 AM Yu-Ting Tseng <yutingtseng@google.com> wrote:
>
> Yu-Ting Tseng (1):
> binder: frozen notification
>
> drivers/android/binder.c | 300 +++++++++++++++++++++++++++-
> drivers/android/binder_internal.h | 23 ++-
> include/uapi/linux/android/binder.h | 35 ++++
> 3 files changed, 354 insertions(+), 4 deletions(-)
>
> > freeze was allocated with kzalloc(), you could drop the "= false".
> Done.
>
> > If !node->proc then process is dead. Do we really need to continue?
> Update the code to return an error early if the process is already dead.
>
> > This access to node->proc->* doesn't seem safe
> Added locking.
>
> > Why do we queue this notification?
> Yes, this is to get the current state back to userspace. The userspace API delivers an initial event for the current state upon a listener registration, which makes it easier to track what the latest state is.
>
> > I'm looking at the death notification code and it seems it only queues a
> BR_ERROR after failing to allocate a "death" and that other errors are
> silently ignored?
> Sure. Please let me know if you think we need a change here.
>
> > these could be just bitfields.
> Done
>
> > freeze->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION
> Fixed. Working on a userspace test. Will post a link when it's ready.
New test now included in the aosp patch:
https://android-review.googlesource.com/c/platform/frameworks/native/+/3070045/6/libs/binder/tests/binderDriverInterfaceTest.cpp
>
> base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
> --
> 2.45.2.741.gdbec12cfda-goog
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/1] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
2024-06-24 17:20 ` [PATCH v3] " Yu-Ting Tseng
@ 2024-06-24 17:20 ` Yu-Ting Tseng
2024-06-28 18:42 ` Carlos Llamas
2024-07-01 18:23 ` [PATCH v4] " Yu-Ting Tseng
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-06-24 17:20 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Frozen processes present a significant challenge in binder transactions.
When a process is frozen, it cannot, by design, accept and/or respond to
binder transactions. As a result, the sender needs to adjust its
behavior, such as postponing transactions until the peer process
unfreezes. However, there is currently no way to subscribe to these
state change events, making it impossible to implement frozen-aware
behaviors efficiently.
Introduce a binder API for subscribing to frozen state change events.
This allows programs to react to changes in peer process state,
mitigating issues related to binder transactions sent to frozen
processes.
Implementation details:
For a given binder_ref, the state of frozen notification can be one of
the followings:
1. Userspace doesn't want a notification. binder_ref->freeze is null.
2. Userspace wants a notification but none is in flight.
list_empty(&binder_ref->freeze->work.entry) = true
3. A notification is in flight and waiting to be read by userspace.
binder_ref_freeze.sent is false.
4. A notification was read by userspace and kernel is waiting for an ack.
binder_ref_freeze.sent is true.
When a notification is in flight, new state change events are coalesced into
the existing binder_ref_freeze struct. If userspace hasn't picked up the
notification yet, the driver simply rewrites the state. Otherwise, the
notification is flagged as requiring a resend, which will be performed
once userspace acks the original notification that's inflight.
See https://android-review.googlesource.com/c/platform/frameworks/native/+/3070045
for how userspace is going to use this feature.
Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
---
drivers/android/binder.c | 300 +++++++++++++++++++++++++++-
drivers/android/binder_internal.h | 23 ++-
include/uapi/linux/android/binder.h | 35 ++++
3 files changed, 354 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b21a7b246a0d..d3dacfe0926d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3763,6 +3763,174 @@ static void binder_transaction(struct binder_proc *proc,
}
}
+static int
+binder_request_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+
+ /*
+ * Allocate memory for freeze notification before taking lock.
+ */
+ freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
+ if (!freeze) {
+ thread->return_error.cmd = BR_ERROR;
+ binder_enqueue_thread_work(thread, &thread->return_error.work);
+ binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+ "%d:%d BC_REQUEST_FREEZE_NOTIFICATION failed\n",
+ proc->pid, thread->pid);
+ return -ENOMEM;
+ }
+
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ kfree(freeze);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (ref->freeze || !ref->node->proc) {
+ if (ref->freeze) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION freeze notification already set\n",
+ proc->pid, thread->pid);
+ } else {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION process is already dead\n",
+ proc->pid, thread->pid);
+ }
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ kfree(freeze);
+ return -EINVAL;
+ }
+ binder_stats_created(BINDER_STAT_FREEZE);
+ INIT_LIST_HEAD(&freeze->work.entry);
+ freeze->cookie = handle_cookie->cookie;
+ ref->freeze = freeze;
+ ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
+
+ binder_inner_proc_lock(ref->node->proc);
+ ref->freeze->is_frozen = ref->node->proc->is_frozen;
+ binder_inner_proc_unlock(ref->node->proc);
+
+ binder_inner_proc_lock(proc);
+ binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ binder_inner_proc_unlock(proc);
+
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_clear_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (!ref->freeze) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
+ proc->pid, thread->pid);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ freeze = ref->freeze;
+ if (freeze->cookie != handle_cookie->cookie) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
+ proc->pid, thread->pid, (u64)freeze->cookie,
+ (u64)handle_cookie->cookie);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ ref->freeze = NULL;
+ binder_inner_proc_lock(proc);
+ if (list_empty(&freeze->work.entry)) {
+ freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
+ if (thread->looper &
+ (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
+ binder_enqueue_thread_work_ilocked(thread, &freeze->work);
+ } else {
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
+ } else {
+ /*
+ * There is already a freeze notification. Take it over and rewrite
+ * the work type. If it was already sent, flag it for re-sending;
+ * Otherwise it's pending and will be sent soon.
+ */
+ freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
+ if (freeze->sent)
+ freeze->resend = true;
+ }
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_freeze_notification_done(struct binder_proc *proc,
+ struct binder_thread *thread,
+ binder_uintptr_t cookie)
+{
+ struct binder_ref_freeze *freeze = NULL;
+ struct binder_work *w;
+
+ binder_inner_proc_lock(proc);
+ list_for_each_entry(w, &proc->delivered_freeze, entry) {
+ struct binder_ref_freeze *tmp_freeze =
+ container_of(w, struct binder_ref_freeze, work);
+
+ if (tmp_freeze->cookie == cookie) {
+ freeze = tmp_freeze;
+ break;
+ }
+ }
+ if (!freeze) {
+ binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
+ proc->pid, thread->pid, (u64)cookie);
+ binder_inner_proc_unlock(proc);
+ return -EINVAL;
+ }
+ binder_dequeue_work_ilocked(&freeze->work);
+ freeze->sent = false;
+ if (freeze->resend) {
+ freeze->resend = false;
+ if (thread->looper &
+ (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
+ binder_enqueue_thread_work_ilocked(thread, &freeze->work);
+ } else {
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
+ }
+ binder_inner_proc_unlock(proc);
+ return 0;
+}
+
/**
* binder_free_buf() - free the specified buffer
* @proc: binder proc that owns buffer
@@ -4246,6 +4414,44 @@ static int binder_thread_write(struct binder_proc *proc,
binder_inner_proc_unlock(proc);
} break;
+ case BC_REQUEST_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_request_freeze_notification(proc, thread,
+ &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_FREEZE_NOTIFICATION_DONE: {
+ binder_uintptr_t cookie;
+ int error;
+
+ if (get_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+
+ ptr += sizeof(cookie);
+ error = binder_freeze_notification_done(proc, thread, cookie);
+ if (error)
+ return error;
+ } break;
+
default:
pr_err("%d:%d unknown command %u\n",
proc->pid, thread->pid, cmd);
@@ -4635,6 +4841,45 @@ static int binder_thread_read(struct binder_proc *proc,
if (cmd == BR_DEAD_BINDER)
goto done; /* DEAD_BINDER notifications can cause transactions */
} break;
+
+ case BINDER_WORK_FROZEN_BINDER: {
+ struct binder_ref_freeze *freeze;
+ struct binder_frozen_state_info info;
+
+ freeze = container_of(w, struct binder_ref_freeze, work);
+ info.is_frozen = freeze->is_frozen;
+ info.cookie = freeze->cookie;
+ freeze->sent = true;
+ binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
+ binder_inner_proc_unlock(proc);
+
+ if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (copy_to_user(ptr, &info, sizeof(info)))
+ return -EFAULT;
+ ptr += sizeof(info);
+ binder_stat_br(proc, thread, BR_FROZEN_BINDER);
+ goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
+ } break;
+
+ case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_ref_freeze *freeze =
+ container_of(w, struct binder_ref_freeze, work);
+ binder_uintptr_t cookie = freeze->cookie;
+
+ binder_inner_proc_unlock(proc);
+ kfree(freeze);
+ binder_stats_deleted(BINDER_STAT_FREEZE);
+ if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (put_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(binder_uintptr_t);
+ binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
+ } break;
+
default:
binder_inner_proc_unlock(proc);
pr_err("%d:%d: bad work type %d\n",
@@ -5242,6 +5487,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
return false;
}
+static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
+{
+ struct rb_node *n;
+ struct binder_ref *ref;
+
+ binder_inner_proc_lock(proc);
+ for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
+ struct binder_node *node;
+
+ node = rb_entry(n, struct binder_node, rb_node);
+ binder_inner_proc_unlock(proc);
+ binder_node_lock(node);
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ /*
+ * Need the node lock to synchronize
+ * with new notification requests and the
+ * inner lock to synchronize with queued
+ * freeze notifications.
+ */
+ binder_inner_proc_lock(ref->proc);
+ if (!ref->freeze) {
+ binder_inner_proc_unlock(ref->proc);
+ continue;
+ }
+ ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
+ if (list_empty(&ref->freeze->work.entry)) {
+ ref->freeze->is_frozen = is_frozen;
+ binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
+ binder_wakeup_proc_ilocked(ref->proc);
+ } else {
+ if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
+ ref->freeze->resend = true;
+ ref->freeze->is_frozen = is_frozen;
+ }
+ binder_inner_proc_unlock(ref->proc);
+ }
+ binder_node_unlock(node);
+ binder_inner_proc_lock(proc);
+ }
+ binder_inner_proc_unlock(proc);
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -5253,6 +5540,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ binder_add_freeze_work(target_proc, false);
return 0;
}
@@ -5266,6 +5554,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
target_proc->async_recv = false;
target_proc->is_frozen = true;
binder_inner_proc_unlock(target_proc);
+ binder_add_freeze_work(target_proc, true);
if (info->timeout_ms > 0)
ret = wait_event_interruptible_timeout(
@@ -5658,6 +5947,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
binder_stats_created(BINDER_STAT_PROC);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
+ INIT_LIST_HEAD(&proc->delivered_freeze);
INIT_LIST_HEAD(&proc->waiting_threads);
filp->private_data = proc;
@@ -6209,7 +6499,9 @@ static const char * const binder_return_strings[] = {
"BR_FAILED_REPLY",
"BR_FROZEN_REPLY",
"BR_ONEWAY_SPAM_SUSPECT",
- "BR_TRANSACTION_PENDING_FROZEN"
+ "BR_TRANSACTION_PENDING_FROZEN",
+ "BR_FROZEN_BINDER",
+ "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_command_strings[] = {
@@ -6232,6 +6524,9 @@ static const char * const binder_command_strings[] = {
"BC_DEAD_BINDER_DONE",
"BC_TRANSACTION_SG",
"BC_REPLY_SG",
+ "BC_REQUEST_FREEZE_NOTIFICATION",
+ "BC_CLEAR_FREEZE_NOTIFICATION",
+ "BC_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_objstat_strings[] = {
@@ -6241,7 +6536,8 @@ static const char * const binder_objstat_strings[] = {
"ref",
"death",
"transaction",
- "transaction_complete"
+ "transaction_complete",
+ "freeze",
};
static void print_binder_stats(struct seq_file *m, const char *prefix,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 5b7c80b99ae8..2415fe53b3e9 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -129,12 +129,13 @@ enum binder_stat_types {
BINDER_STAT_DEATH,
BINDER_STAT_TRANSACTION,
BINDER_STAT_TRANSACTION_COMPLETE,
+ BINDER_STAT_FREEZE,
BINDER_STAT_COUNT
};
struct binder_stats {
- atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
- atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
+ atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
+ atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
atomic_t obj_created[BINDER_STAT_COUNT];
atomic_t obj_deleted[BINDER_STAT_COUNT];
};
@@ -159,6 +160,8 @@ struct binder_work {
BINDER_WORK_DEAD_BINDER,
BINDER_WORK_DEAD_BINDER_AND_CLEAR,
BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
+ BINDER_WORK_FROZEN_BINDER,
+ BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
} type;
};
@@ -275,6 +278,16 @@ struct binder_ref_death {
binder_uintptr_t cookie;
};
+struct binder_ref_freeze {
+ struct binder_work work;
+ binder_uintptr_t cookie;
+ bool is_frozen;
+ struct {
+ u8 sent:1;
+ u8 resend:1;
+ };
+};
+
/**
* struct binder_ref_data - binder_ref counts and id
* @debug_id: unique ID for the ref
@@ -307,6 +320,8 @@ struct binder_ref_data {
* @node indicates the node must be freed
* @death: pointer to death notification (ref_death) if requested
* (protected by @node->lock)
+ * @freeze: pointer to freeze notification (ref_freeze) if requested
+ * (protected by @node->lock)
*
* Structure to track references from procA to target node (on procB). This
* structure is unsafe to access without holding @proc->outer_lock.
@@ -323,6 +338,7 @@ struct binder_ref {
struct binder_proc *proc;
struct binder_node *node;
struct binder_ref_death *death;
+ struct binder_ref_freeze *freeze;
};
/**
@@ -374,6 +390,8 @@ struct binder_ref {
* (atomics, no lock needed)
* @delivered_death: list of delivered death notification
* (protected by @inner_lock)
+ * @delivered_freeze: list of delivered freeze notification
+ * (protected by @inner_lock)
* @max_threads: cap on number of binder threads
* (protected by @inner_lock)
* @requested_threads: number of binder threads requested but not
@@ -421,6 +439,7 @@ struct binder_proc {
struct list_head todo;
struct binder_stats stats;
struct list_head delivered_death;
+ struct list_head delivered_freeze;
u32 max_threads;
int requested_threads;
int requested_threads_started;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index d44a8118b2ed..a358cde61ee4 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -236,6 +236,11 @@ struct binder_frozen_status_info {
__u32 async_recv;
};
+struct binder_frozen_state_info {
+ binder_uintptr_t cookie;
+ __u32 is_frozen;
+};
+
/* struct binder_extened_error - extended error information
* @id: identifier for the failed operation
* @command: command as defined by binder_driver_return_protocol
@@ -467,6 +472,17 @@ enum binder_driver_return_protocol {
/*
* The target of the last async transaction is frozen. No parameters.
*/
+
+ BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
+ /*
+ * The cookie and a boolean (is_frozen) that indicates whether the process
+ * transitioned into a frozen or an unfrozen state.
+ */
+
+ BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
enum binder_driver_command_protocol {
@@ -550,6 +566,25 @@ enum binder_driver_command_protocol {
/*
* binder_transaction_data_sg: the sent command.
*/
+
+ BC_REQUEST_FREEZE_NOTIFICATION =
+ _IOW('c', 19, struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
+ struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
#endif /* _UAPI_LINUX_BINDER_H */
--
2.45.2.741.gdbec12cfda-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 1/1] binder: frozen notification
2024-06-24 17:20 ` [PATCH v3 1/1] " Yu-Ting Tseng
@ 2024-06-28 18:42 ` Carlos Llamas
0 siblings, 0 replies; 20+ messages in thread
From: Carlos Llamas @ 2024-06-28 18:42 UTC (permalink / raw)
To: Yu-Ting Tseng
Cc: tkjos, gregkh, arve, maco, joel, brauner, surenb, aliceryhl,
kernel-team, linux-kernel
On Mon, Jun 24, 2024 at 10:20:34AM -0700, Yu-Ting Tseng wrote:
> Frozen processes present a significant challenge in binder transactions.
> When a process is frozen, it cannot, by design, accept and/or respond to
> binder transactions. As a result, the sender needs to adjust its
> behavior, such as postponing transactions until the peer process
> unfreezes. However, there is currently no way to subscribe to these
> state change events, making it impossible to implement frozen-aware
> behaviors efficiently.
>
> Introduce a binder API for subscribing to frozen state change events.
> This allows programs to react to changes in peer process state,
> mitigating issues related to binder transactions sent to frozen
> processes.
>
> Implementation details:
> For a given binder_ref, the state of frozen notification can be one of
> the followings:
> 1. Userspace doesn't want a notification. binder_ref->freeze is null.
> 2. Userspace wants a notification but none is in flight.
> list_empty(&binder_ref->freeze->work.entry) = true
> 3. A notification is in flight and waiting to be read by userspace.
> binder_ref_freeze.sent is false.
> 4. A notification was read by userspace and kernel is waiting for an ack.
> binder_ref_freeze.sent is true.
>
> When a notification is in flight, new state change events are coalesced into
> the existing binder_ref_freeze struct. If userspace hasn't picked up the
> notification yet, the driver simply rewrites the state. Otherwise, the
> notification is flagged as requiring a resend, which will be performed
> once userspace acks the original notification that's inflight.
>
> See https://android-review.googlesource.com/c/platform/frameworks/native/+/3070045
I believe the external link would be https://r.android.com/3070045
> for how userspace is going to use this feature.
>
> Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
> ---
> drivers/android/binder.c | 300 +++++++++++++++++++++++++++-
> drivers/android/binder_internal.h | 23 ++-
> include/uapi/linux/android/binder.h | 35 ++++
> 3 files changed, 354 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index b21a7b246a0d..d3dacfe0926d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3763,6 +3763,174 @@ static void binder_transaction(struct binder_proc *proc,
> }
> }
>
> +static int
> +binder_request_freeze_notification(struct binder_proc *proc,
> + struct binder_thread *thread,
> + struct binder_handle_cookie *handle_cookie)
> +{
> + struct binder_ref_freeze *freeze;
> + struct binder_ref *ref;
> +
> + /*
> + * Allocate memory for freeze notification before taking lock.
> + */
> + freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> + if (!freeze) {
> + thread->return_error.cmd = BR_ERROR;
> + binder_enqueue_thread_work(thread, &thread->return_error.work);
> + binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
> + "%d:%d BC_REQUEST_FREEZE_NOTIFICATION failed\n",
> + proc->pid, thread->pid);
> + return -ENOMEM;
> + }
A couple notes on this allocation:
(1) drop the BR_ERROR, at least until we define a global policy for
propagating errors. For now the "-ENOMEM" will be enough.
(2) typically OOM error logs are discouraged, the infra already dumps
necessary info. Please remove the debug log.
(3) I don't think the comment is necessary.
This should all just be:
freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
if (!freeze)
return -ENOMEM;
> +
> + binder_proc_lock(proc);
> + ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
> + if (!ref) {
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
> + proc->pid, thread->pid, handle_cookie->handle);
> + binder_proc_unlock(proc);
> + kfree(freeze);
> + return -EINVAL;
> + }
> +
> + binder_node_lock(ref->node);
> +
> + if (ref->freeze || !ref->node->proc) {
> + if (ref->freeze) {
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION freeze notification already set\n",
> + proc->pid, thread->pid);
> + } else {
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION process is already dead\n",
> + proc->pid, thread->pid);
> + }
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + kfree(freeze);
> + return -EINVAL;
> + }
> + binder_stats_created(BINDER_STAT_FREEZE);
> + INIT_LIST_HEAD(&freeze->work.entry);
> + freeze->cookie = handle_cookie->cookie;
> + ref->freeze = freeze;
> + ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
Are we ok modifying the ref->freeze->* space here without the
inner_lock? I believe this is OK as we can't race with the other
operations?
> +
> + binder_inner_proc_lock(ref->node->proc);
> + ref->freeze->is_frozen = ref->node->proc->is_frozen;
> + binder_inner_proc_unlock(ref->node->proc);
> +
> + binder_inner_proc_lock(proc);
> + binder_enqueue_work_ilocked(&ref->freeze->work, &proc->todo);
> + binder_wakeup_proc_ilocked(proc);
> + binder_inner_proc_unlock(proc);
> +
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return 0;
> +}
> +
> +static int
> +binder_clear_freeze_notification(struct binder_proc *proc,
> + struct binder_thread *thread,
> + struct binder_handle_cookie *handle_cookie)
> +{
> + struct binder_ref_freeze *freeze;
> + struct binder_ref *ref;
> +
> + binder_proc_lock(proc);
> + ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
> + if (!ref) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
> + proc->pid, thread->pid, handle_cookie->handle);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> +
> + binder_node_lock(ref->node);
> +
> + if (!ref->freeze) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
> + proc->pid, thread->pid);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> + freeze = ref->freeze;
> + if (freeze->cookie != handle_cookie->cookie) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
> + proc->pid, thread->pid, (u64)freeze->cookie,
> + (u64)handle_cookie->cookie);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> + ref->freeze = NULL;
> + binder_inner_proc_lock(proc);
> + if (list_empty(&freeze->work.entry)) {
> + freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
> + if (thread->looper &
> + (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
> + binder_enqueue_thread_work_ilocked(thread, &freeze->work);
I'm not entirely sure why we attempt to queue this as thread work. It
seems more complicated that it needs to be. Do you know why not handle
this as proc work every time instead?
> + } else {
> + binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> + binder_wakeup_proc_ilocked(proc);
> + }
> + } else {
> + /*
> + * There is already a freeze notification. Take it over and rewrite
> + * the work type. If it was already sent, flag it for re-sending;
> + * Otherwise it's pending and will be sent soon.
> + */
> + freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
We set the work type to CLEAR regardless, might as well factor it out.
> + if (freeze->sent)
> + freeze->resend = true;
> + }
> + binder_inner_proc_unlock(proc);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return 0;
> +}
> +
> +static int
> +binder_freeze_notification_done(struct binder_proc *proc,
> + struct binder_thread *thread,
> + binder_uintptr_t cookie)
> +{
> + struct binder_ref_freeze *freeze = NULL;
> + struct binder_work *w;
> +
> + binder_inner_proc_lock(proc);
> + list_for_each_entry(w, &proc->delivered_freeze, entry) {
> + struct binder_ref_freeze *tmp_freeze =
> + container_of(w, struct binder_ref_freeze, work);
> +
> + if (tmp_freeze->cookie == cookie) {
> + freeze = tmp_freeze;
> + break;
> + }
> + }
> + if (!freeze) {
> + binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
> + proc->pid, thread->pid, (u64)cookie);
> + binder_inner_proc_unlock(proc);
> + return -EINVAL;
> + }
> + binder_dequeue_work_ilocked(&freeze->work);
> + freeze->sent = false;
> + if (freeze->resend) {
> + freeze->resend = false;
> + if (thread->looper &
> + (BINDER_LOOPER_STATE_REGISTERED | BINDER_LOOPER_STATE_ENTERED)) {
> + binder_enqueue_thread_work_ilocked(thread, &freeze->work);
> + } else {
> + binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> + binder_wakeup_proc_ilocked(proc);
> + }
Yeah I don't know why we go through the trouble of trying to handle this
as thread work.
> + }
> + binder_inner_proc_unlock(proc);
> + return 0;
> +}
> +
> /**
> * binder_free_buf() - free the specified buffer
> * @proc: binder proc that owns buffer
> @@ -4246,6 +4414,44 @@ static int binder_thread_write(struct binder_proc *proc,
> binder_inner_proc_unlock(proc);
> } break;
>
> + case BC_REQUEST_FREEZE_NOTIFICATION: {
> + struct binder_handle_cookie handle_cookie;
> + int error;
> +
> + if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
> + return -EFAULT;
> + ptr += sizeof(handle_cookie);
> + error = binder_request_freeze_notification(proc, thread,
> + &handle_cookie);
> + if (error)
> + return error;
> + } break;
> +
> + case BC_CLEAR_FREEZE_NOTIFICATION: {
> + struct binder_handle_cookie handle_cookie;
> + int error;
> +
> + if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
> + return -EFAULT;
> + ptr += sizeof(handle_cookie);
> + error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
> + if (error)
> + return error;
> + } break;
> +
> + case BC_FREEZE_NOTIFICATION_DONE: {
> + binder_uintptr_t cookie;
> + int error;
> +
> + if (get_user(cookie, (binder_uintptr_t __user *)ptr))
> + return -EFAULT;
> +
> + ptr += sizeof(cookie);
> + error = binder_freeze_notification_done(proc, thread, cookie);
> + if (error)
> + return error;
> + } break;
> +
> default:
> pr_err("%d:%d unknown command %u\n",
> proc->pid, thread->pid, cmd);
> @@ -4635,6 +4841,45 @@ static int binder_thread_read(struct binder_proc *proc,
> if (cmd == BR_DEAD_BINDER)
> goto done; /* DEAD_BINDER notifications can cause transactions */
> } break;
> +
> + case BINDER_WORK_FROZEN_BINDER: {
> + struct binder_ref_freeze *freeze;
> + struct binder_frozen_state_info info;
> +
> + freeze = container_of(w, struct binder_ref_freeze, work);
> + info.is_frozen = freeze->is_frozen;
> + info.cookie = freeze->cookie;
> + freeze->sent = true;
It's a bit confusing to me the locking here. It seems the freeze->*
space is protected by the proc->inner_lock right? We read/write several
elements here with only that lock held.
> + binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
> + binder_inner_proc_unlock(proc);
> +
> + if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(uint32_t);
> + if (copy_to_user(ptr, &info, sizeof(info)))
> + return -EFAULT;
I believe you should zero the 'info' before copy_to_user(). This might
leak kernel's stack memory to userspace if there is any padding.
> + ptr += sizeof(info);
> + binder_stat_br(proc, thread, BR_FROZEN_BINDER);
> + goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
> + } break;
> +
> + case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
> + struct binder_ref_freeze *freeze =
> + container_of(w, struct binder_ref_freeze, work);
> + binder_uintptr_t cookie = freeze->cookie;
> +
> + binder_inner_proc_unlock(proc);
> + kfree(freeze);
> + binder_stats_deleted(BINDER_STAT_FREEZE);
> + if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(uint32_t);
> + if (put_user(cookie, (binder_uintptr_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(binder_uintptr_t);
> + binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
> + } break;
> +
> default:
> binder_inner_proc_unlock(proc);
> pr_err("%d:%d: bad work type %d\n",
> @@ -5242,6 +5487,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
> return false;
> }
>
> +static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
> +{
> + struct rb_node *n;
> + struct binder_ref *ref;
> +
> + binder_inner_proc_lock(proc);
> + for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
> + struct binder_node *node;
> +
> + node = rb_entry(n, struct binder_node, rb_node);
> + binder_inner_proc_unlock(proc);
> + binder_node_lock(node);
> + hlist_for_each_entry(ref, &node->refs, node_entry) {
So we traverse every single reference of every single node in this proc
looking for references subscribed to freeze, correct? I guess we don't
have the structures to provide something faster. Speed isn't critical
either here.
> + /*
> + * Need the node lock to synchronize
> + * with new notification requests and the
> + * inner lock to synchronize with queued
> + * freeze notifications.
> + */
> + binder_inner_proc_lock(ref->proc);
> + if (!ref->freeze) {
> + binder_inner_proc_unlock(ref->proc);
> + continue;
> + }
> + ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> + if (list_empty(&ref->freeze->work.entry)) {
> + ref->freeze->is_frozen = is_frozen;
> + binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
> + binder_wakeup_proc_ilocked(ref->proc);
> + } else {
> + if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
> + ref->freeze->resend = true;
> + ref->freeze->is_frozen = is_frozen;
> + }
> + binder_inner_proc_unlock(ref->proc);
> + }
> + binder_node_unlock(node);
> + binder_inner_proc_lock(proc);
> + }
> + binder_inner_proc_unlock(proc);
> +}
> +
> static int binder_ioctl_freeze(struct binder_freeze_info *info,
> struct binder_proc *target_proc)
> {
> @@ -5253,6 +5540,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
> target_proc->async_recv = false;
> target_proc->is_frozen = false;
> binder_inner_proc_unlock(target_proc);
> + binder_add_freeze_work(target_proc, false);
> return 0;
> }
>
> @@ -5266,6 +5554,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
> target_proc->async_recv = false;
> target_proc->is_frozen = true;
> binder_inner_proc_unlock(target_proc);
> + binder_add_freeze_work(target_proc, true);
What if there a EAGAIN error following this below? I think you want to
update the freeze work about the 'proc->is_frozen = false' change.
>
> if (info->timeout_ms > 0)
> ret = wait_event_interruptible_timeout(
> @@ -5658,6 +5947,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
> binder_stats_created(BINDER_STAT_PROC);
> proc->pid = current->group_leader->pid;
> INIT_LIST_HEAD(&proc->delivered_death);
> + INIT_LIST_HEAD(&proc->delivered_freeze);
> INIT_LIST_HEAD(&proc->waiting_threads);
> filp->private_data = proc;
>
> @@ -6209,7 +6499,9 @@ static const char * const binder_return_strings[] = {
> "BR_FAILED_REPLY",
> "BR_FROZEN_REPLY",
> "BR_ONEWAY_SPAM_SUSPECT",
> - "BR_TRANSACTION_PENDING_FROZEN"
> + "BR_TRANSACTION_PENDING_FROZEN",
> + "BR_FROZEN_BINDER",
> + "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
> };
>
> static const char * const binder_command_strings[] = {
> @@ -6232,6 +6524,9 @@ static const char * const binder_command_strings[] = {
> "BC_DEAD_BINDER_DONE",
> "BC_TRANSACTION_SG",
> "BC_REPLY_SG",
> + "BC_REQUEST_FREEZE_NOTIFICATION",
> + "BC_CLEAR_FREEZE_NOTIFICATION",
> + "BC_FREEZE_NOTIFICATION_DONE",
> };
>
> static const char * const binder_objstat_strings[] = {
> @@ -6241,7 +6536,8 @@ static const char * const binder_objstat_strings[] = {
> "ref",
> "death",
> "transaction",
> - "transaction_complete"
> + "transaction_complete",
> + "freeze",
> };
>
> static void print_binder_stats(struct seq_file *m, const char *prefix,
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 5b7c80b99ae8..2415fe53b3e9 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -129,12 +129,13 @@ enum binder_stat_types {
> BINDER_STAT_DEATH,
> BINDER_STAT_TRANSACTION,
> BINDER_STAT_TRANSACTION_COMPLETE,
> + BINDER_STAT_FREEZE,
> BINDER_STAT_COUNT
> };
>
> struct binder_stats {
> - atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
> - atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> + atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
> + atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
> atomic_t obj_created[BINDER_STAT_COUNT];
> atomic_t obj_deleted[BINDER_STAT_COUNT];
> };
> @@ -159,6 +160,8 @@ struct binder_work {
> BINDER_WORK_DEAD_BINDER,
> BINDER_WORK_DEAD_BINDER_AND_CLEAR,
> BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
> + BINDER_WORK_FROZEN_BINDER,
> + BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
> } type;
> };
>
> @@ -275,6 +278,16 @@ struct binder_ref_death {
> binder_uintptr_t cookie;
> };
>
> +struct binder_ref_freeze {
> + struct binder_work work;
> + binder_uintptr_t cookie;
> + bool is_frozen;
any reason why is_frozen didn't make it to the bitfields?
> + struct {
No need for a struct
> + u8 sent:1;
> + u8 resend:1;
You could still keep the original bool for bitfields too if you want.
> + };
> +};
> +
> /**
> * struct binder_ref_data - binder_ref counts and id
> * @debug_id: unique ID for the ref
> @@ -307,6 +320,8 @@ struct binder_ref_data {
> * @node indicates the node must be freed
> * @death: pointer to death notification (ref_death) if requested
> * (protected by @node->lock)
> + * @freeze: pointer to freeze notification (ref_freeze) if requested
> + * (protected by @node->lock)
> *
> * Structure to track references from procA to target node (on procB). This
> * structure is unsafe to access without holding @proc->outer_lock.
> @@ -323,6 +338,7 @@ struct binder_ref {
> struct binder_proc *proc;
> struct binder_node *node;
> struct binder_ref_death *death;
> + struct binder_ref_freeze *freeze;
> };
>
> /**
> @@ -374,6 +390,8 @@ struct binder_ref {
> * (atomics, no lock needed)
> * @delivered_death: list of delivered death notification
> * (protected by @inner_lock)
> + * @delivered_freeze: list of delivered freeze notification
> + * (protected by @inner_lock)
> * @max_threads: cap on number of binder threads
> * (protected by @inner_lock)
> * @requested_threads: number of binder threads requested but not
> @@ -421,6 +439,7 @@ struct binder_proc {
> struct list_head todo;
> struct binder_stats stats;
> struct list_head delivered_death;
> + struct list_head delivered_freeze;
> u32 max_threads;
> int requested_threads;
> int requested_threads_started;
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index d44a8118b2ed..a358cde61ee4 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -236,6 +236,11 @@ struct binder_frozen_status_info {
> __u32 async_recv;
> };
>
> +struct binder_frozen_state_info {
> + binder_uintptr_t cookie;
> + __u32 is_frozen;
> +};
> +
> /* struct binder_extened_error - extended error information
> * @id: identifier for the failed operation
> * @command: command as defined by binder_driver_return_protocol
> @@ -467,6 +472,17 @@ enum binder_driver_return_protocol {
> /*
> * The target of the last async transaction is frozen. No parameters.
> */
> +
> + BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
> + /*
> + * The cookie and a boolean (is_frozen) that indicates whether the process
> + * transitioned into a frozen or an unfrozen state.
> + */
> +
> + BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
> + /*
> + * void *: cookie
> + */
> };
>
> enum binder_driver_command_protocol {
> @@ -550,6 +566,25 @@ enum binder_driver_command_protocol {
> /*
> * binder_transaction_data_sg: the sent command.
> */
> +
> + BC_REQUEST_FREEZE_NOTIFICATION =
> + _IOW('c', 19, struct binder_handle_cookie),
> + /*
> + * int: handle
> + * void *: cookie
> + */
> +
> + BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
> + struct binder_handle_cookie),
> + /*
> + * int: handle
> + * void *: cookie
> + */
> +
> + BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
> + /*
> + * void *: cookie
> + */
> };
>
> #endif /* _UAPI_LINUX_BINDER_H */
> --
> 2.45.2.741.gdbec12cfda-goog
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
2024-06-24 17:20 ` [PATCH v3] " Yu-Ting Tseng
2024-06-24 17:20 ` [PATCH v3 1/1] " Yu-Ting Tseng
@ 2024-07-01 18:23 ` Yu-Ting Tseng
2024-07-01 18:27 ` Yu-Ting Tseng
` (5 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-01 18:23 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
> I believe the external link would be https://r.android.com/3070045
Fixed
> BR_ERROR and binder_debug
Removed
> Are we ok modifying the ref->freeze->* space here without the
inner_lock?
Good catch. Moved freeze->* access earlier before it's assigned to ref->freeze so that's safe without locking.
> I'm not entirely sure why we attempt to queue this as thread work
Was mimicking how it's done with death notification. Removed.
> We set the work type to CLEAR regardless, might as well factor it out.
Done
> It seems the freeze->* space is protected by the proc->inner_lock right
Yes, freeze->* is protected by the proc inner lock. ref->freeze is protected by the node lock.
> I believe you should zero the 'info' before copy_to_user()
Done.
> So we traverse every single reference of every single node in this proc
looking for references subscribed to freeze, correct?
That's correct.
> What if there a EAGAIN error following this below?
Fixed. Moved binder_add_freeze_work(target_proc, true) near the end of the function where we know whether freezing is successful.
> any reason why is_frozen didn't make it to the bitfields?
Fixed
> No need for a struct
Fixed
> You could still keep the original bool for bitfields too if you want.
Sure. Done.
Yu-Ting Tseng (1):
binder: frozen notification
drivers/android/binder.c | 300 +++++++++++++++++++++++++++-
drivers/android/binder_internal.h | 23 ++-
include/uapi/linux/android/binder.h | 35 ++++
3 files changed, 354 insertions(+), 4 deletions(-)
base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v4] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
` (2 preceding siblings ...)
2024-07-01 18:23 ` [PATCH v4] " Yu-Ting Tseng
@ 2024-07-01 18:27 ` Yu-Ting Tseng
2024-07-03 4:18 ` Carlos Llamas
2024-07-03 17:08 ` [PATCH v5] " Yu-Ting Tseng
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-01 18:27 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Frozen processes present a significant challenge in binder transactions.
When a process is frozen, it cannot, by design, accept and/or respond to
binder transactions. As a result, the sender needs to adjust its
behavior, such as postponing transactions until the peer process
unfreezes. However, there is currently no way to subscribe to these
state change events, making it impossible to implement frozen-aware
behaviors efficiently.
Introduce a binder API for subscribing to frozen state change events.
This allows programs to react to changes in peer process state,
mitigating issues related to binder transactions sent to frozen
processes.
Implementation details:
For a given binder_ref, the state of frozen notification can be one of
the followings:
1. Userspace doesn't want a notification. binder_ref->freeze is null.
2. Userspace wants a notification but none is in flight.
list_empty(&binder_ref->freeze->work.entry) = true
3. A notification is in flight and waiting to be read by userspace.
binder_ref_freeze.sent is false.
4. A notification was read by userspace and kernel is waiting for an ack.
binder_ref_freeze.sent is true.
When a notification is in flight, new state change events are coalesced into
the existing binder_ref_freeze struct. If userspace hasn't picked up the
notification yet, the driver simply rewrites the state. Otherwise, the
notification is flagged as requiring a resend, which will be performed
once userspace acks the original notification that's inflight.
See https://r.android.com/3070045 for how userspace is going to use this
feature.
Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
---
drivers/android/binder.c | 287 +++++++++++++++++++++++++++-
drivers/android/binder_internal.h | 21 +-
include/uapi/linux/android/binder.h | 36 ++++
3 files changed, 340 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b21a7b246a0d..e1bd9810c2af 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3763,6 +3763,159 @@ static void binder_transaction(struct binder_proc *proc,
}
}
+static int
+binder_request_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+ bool is_frozen;
+
+ freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
+ if (!freeze)
+ return -ENOMEM;
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ kfree(freeze);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (ref->freeze || !ref->node->proc) {
+ if (ref->freeze) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION freeze notification already set\n",
+ proc->pid, thread->pid);
+ } else {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION process is already dead\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 = ref->node->proc->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);
+
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_clear_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (!ref->freeze) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
+ proc->pid, thread->pid);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ freeze = ref->freeze;
+ binder_inner_proc_lock(proc);
+ if (freeze->cookie != handle_cookie->cookie) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
+ proc->pid, thread->pid, (u64)freeze->cookie,
+ (u64)handle_cookie->cookie);
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ ref->freeze = NULL;
+ /*
+ * Take the existing freeze object and overwrite its work type. There are three cases here:
+ * 1. No pending notification. In this case just add the work to the queue.
+ * 2. An notification was sent and is pending an ack from userspace. Once an ack arrives, we
+ * should resend with the new work type.
+ * 3. A notification is pending to be sent. Since the work is already in the queue, nothing
+ * needs to be done here.
+ */
+ freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
+ if (list_empty(&freeze->work.entry)) {
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ } else if (freeze->sent) {
+ freeze->resend = true;
+ }
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_freeze_notification_done(struct binder_proc *proc,
+ struct binder_thread *thread,
+ binder_uintptr_t cookie)
+{
+ struct binder_ref_freeze *freeze = NULL;
+ struct binder_work *w;
+
+ binder_inner_proc_lock(proc);
+ list_for_each_entry(w, &proc->delivered_freeze, entry) {
+ struct binder_ref_freeze *tmp_freeze =
+ container_of(w, struct binder_ref_freeze, work);
+
+ if (tmp_freeze->cookie == cookie) {
+ freeze = tmp_freeze;
+ break;
+ }
+ }
+ if (!freeze) {
+ binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
+ proc->pid, thread->pid, (u64)cookie);
+ binder_inner_proc_unlock(proc);
+ return -EINVAL;
+ }
+ binder_dequeue_work_ilocked(&freeze->work);
+ freeze->sent = false;
+ if (freeze->resend) {
+ freeze->resend = false;
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
+ binder_inner_proc_unlock(proc);
+ return 0;
+}
+
/**
* binder_free_buf() - free the specified buffer
* @proc: binder proc that owns buffer
@@ -4246,6 +4399,44 @@ static int binder_thread_write(struct binder_proc *proc,
binder_inner_proc_unlock(proc);
} break;
+ case BC_REQUEST_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_request_freeze_notification(proc, thread,
+ &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_FREEZE_NOTIFICATION_DONE: {
+ binder_uintptr_t cookie;
+ int error;
+
+ if (get_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+
+ ptr += sizeof(cookie);
+ error = binder_freeze_notification_done(proc, thread, cookie);
+ if (error)
+ return error;
+ } break;
+
default:
pr_err("%d:%d unknown command %u\n",
proc->pid, thread->pid, cmd);
@@ -4635,6 +4826,46 @@ static int binder_thread_read(struct binder_proc *proc,
if (cmd == BR_DEAD_BINDER)
goto done; /* DEAD_BINDER notifications can cause transactions */
} break;
+
+ case BINDER_WORK_FROZEN_BINDER: {
+ struct binder_ref_freeze *freeze;
+ struct binder_frozen_state_info info;
+ memset(&info, 0, sizeof(info));
+
+ freeze = container_of(w, struct binder_ref_freeze, work);
+ info.is_frozen = freeze->is_frozen;
+ info.cookie = freeze->cookie;
+ freeze->sent = true;
+ binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
+ binder_inner_proc_unlock(proc);
+
+ if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (copy_to_user(ptr, &info, sizeof(info)))
+ return -EFAULT;
+ ptr += sizeof(info);
+ binder_stat_br(proc, thread, BR_FROZEN_BINDER);
+ goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
+ } break;
+
+ case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_ref_freeze *freeze =
+ container_of(w, struct binder_ref_freeze, work);
+ binder_uintptr_t cookie = freeze->cookie;
+
+ binder_inner_proc_unlock(proc);
+ kfree(freeze);
+ binder_stats_deleted(BINDER_STAT_FREEZE);
+ if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (put_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(binder_uintptr_t);
+ binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
+ } break;
+
default:
binder_inner_proc_unlock(proc);
pr_err("%d:%d: bad work type %d\n",
@@ -5242,6 +5473,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
return false;
}
+static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
+{
+ struct rb_node *n;
+ struct binder_ref *ref;
+
+ binder_inner_proc_lock(proc);
+ for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
+ struct binder_node *node;
+
+ node = rb_entry(n, struct binder_node, rb_node);
+ binder_inner_proc_unlock(proc);
+ binder_node_lock(node);
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ /*
+ * Need the node lock to synchronize
+ * with new notification requests and the
+ * inner lock to synchronize with queued
+ * freeze notifications.
+ */
+ binder_inner_proc_lock(ref->proc);
+ if (!ref->freeze) {
+ binder_inner_proc_unlock(ref->proc);
+ continue;
+ }
+ ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
+ if (list_empty(&ref->freeze->work.entry)) {
+ ref->freeze->is_frozen = is_frozen;
+ binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
+ binder_wakeup_proc_ilocked(ref->proc);
+ } else {
+ if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
+ ref->freeze->resend = true;
+ ref->freeze->is_frozen = is_frozen;
+ }
+ binder_inner_proc_unlock(ref->proc);
+ }
+ binder_node_unlock(node);
+ binder_inner_proc_lock(proc);
+ }
+ binder_inner_proc_unlock(proc);
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -5253,6 +5526,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ binder_add_freeze_work(target_proc, false);
return 0;
}
@@ -5285,6 +5559,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
binder_inner_proc_lock(target_proc);
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ } else {
+ binder_add_freeze_work(target_proc, true);
}
return ret;
@@ -5658,6 +5934,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
binder_stats_created(BINDER_STAT_PROC);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
+ INIT_LIST_HEAD(&proc->delivered_freeze);
INIT_LIST_HEAD(&proc->waiting_threads);
filp->private_data = proc;
@@ -6209,7 +6486,9 @@ static const char * const binder_return_strings[] = {
"BR_FAILED_REPLY",
"BR_FROZEN_REPLY",
"BR_ONEWAY_SPAM_SUSPECT",
- "BR_TRANSACTION_PENDING_FROZEN"
+ "BR_TRANSACTION_PENDING_FROZEN",
+ "BR_FROZEN_BINDER",
+ "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_command_strings[] = {
@@ -6232,6 +6511,9 @@ static const char * const binder_command_strings[] = {
"BC_DEAD_BINDER_DONE",
"BC_TRANSACTION_SG",
"BC_REPLY_SG",
+ "BC_REQUEST_FREEZE_NOTIFICATION",
+ "BC_CLEAR_FREEZE_NOTIFICATION",
+ "BC_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_objstat_strings[] = {
@@ -6241,7 +6523,8 @@ static const char * const binder_objstat_strings[] = {
"ref",
"death",
"transaction",
- "transaction_complete"
+ "transaction_complete",
+ "freeze",
};
static void print_binder_stats(struct seq_file *m, const char *prefix,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 5b7c80b99ae8..3e4b35873c64 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -129,12 +129,13 @@ enum binder_stat_types {
BINDER_STAT_DEATH,
BINDER_STAT_TRANSACTION,
BINDER_STAT_TRANSACTION_COMPLETE,
+ BINDER_STAT_FREEZE,
BINDER_STAT_COUNT
};
struct binder_stats {
- atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
- atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
+ atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
+ atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
atomic_t obj_created[BINDER_STAT_COUNT];
atomic_t obj_deleted[BINDER_STAT_COUNT];
};
@@ -159,6 +160,8 @@ struct binder_work {
BINDER_WORK_DEAD_BINDER,
BINDER_WORK_DEAD_BINDER_AND_CLEAR,
BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
+ BINDER_WORK_FROZEN_BINDER,
+ BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
} type;
};
@@ -275,6 +278,14 @@ struct binder_ref_death {
binder_uintptr_t cookie;
};
+struct binder_ref_freeze {
+ struct binder_work work;
+ binder_uintptr_t cookie;
+ bool is_frozen:1;
+ bool sent:1;
+ bool resend:1;
+};
+
/**
* struct binder_ref_data - binder_ref counts and id
* @debug_id: unique ID for the ref
@@ -307,6 +318,8 @@ struct binder_ref_data {
* @node indicates the node must be freed
* @death: pointer to death notification (ref_death) if requested
* (protected by @node->lock)
+ * @freeze: pointer to freeze notification (ref_freeze) if requested
+ * (protected by @node->lock)
*
* Structure to track references from procA to target node (on procB). This
* structure is unsafe to access without holding @proc->outer_lock.
@@ -323,6 +336,7 @@ struct binder_ref {
struct binder_proc *proc;
struct binder_node *node;
struct binder_ref_death *death;
+ struct binder_ref_freeze *freeze;
};
/**
@@ -374,6 +388,8 @@ struct binder_ref {
* (atomics, no lock needed)
* @delivered_death: list of delivered death notification
* (protected by @inner_lock)
+ * @delivered_freeze: list of delivered freeze notification
+ * (protected by @inner_lock)
* @max_threads: cap on number of binder threads
* (protected by @inner_lock)
* @requested_threads: number of binder threads requested but not
@@ -421,6 +437,7 @@ struct binder_proc {
struct list_head todo;
struct binder_stats stats;
struct list_head delivered_death;
+ struct list_head delivered_freeze;
u32 max_threads;
int requested_threads;
int requested_threads_started;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index d44a8118b2ed..1fd92021a573 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -236,6 +236,12 @@ struct binder_frozen_status_info {
__u32 async_recv;
};
+struct binder_frozen_state_info {
+ binder_uintptr_t cookie;
+ __u32 is_frozen;
+ __u32 reserved;
+};
+
/* struct binder_extened_error - extended error information
* @id: identifier for the failed operation
* @command: command as defined by binder_driver_return_protocol
@@ -467,6 +473,17 @@ enum binder_driver_return_protocol {
/*
* The target of the last async transaction is frozen. No parameters.
*/
+
+ BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
+ /*
+ * The cookie and a boolean (is_frozen) that indicates whether the process
+ * transitioned into a frozen or an unfrozen state.
+ */
+
+ BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
enum binder_driver_command_protocol {
@@ -550,6 +567,25 @@ enum binder_driver_command_protocol {
/*
* binder_transaction_data_sg: the sent command.
*/
+
+ BC_REQUEST_FREEZE_NOTIFICATION =
+ _IOW('c', 19, struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
+ struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
#endif /* _UAPI_LINUX_BINDER_H */
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4] binder: frozen notification
2024-07-01 18:27 ` Yu-Ting Tseng
@ 2024-07-03 4:18 ` Carlos Llamas
0 siblings, 0 replies; 20+ messages in thread
From: Carlos Llamas @ 2024-07-03 4:18 UTC (permalink / raw)
To: Yu-Ting Tseng
Cc: tkjos, gregkh, arve, maco, joel, brauner, surenb, aliceryhl,
kernel-team, linux-kernel
On Mon, Jul 01, 2024 at 11:27:33AM -0700, Yu-Ting Tseng wrote:
> Frozen processes present a significant challenge in binder transactions.
> When a process is frozen, it cannot, by design, accept and/or respond to
> binder transactions. As a result, the sender needs to adjust its
> behavior, such as postponing transactions until the peer process
> unfreezes. However, there is currently no way to subscribe to these
> state change events, making it impossible to implement frozen-aware
> behaviors efficiently.
>
> Introduce a binder API for subscribing to frozen state change events.
> This allows programs to react to changes in peer process state,
> mitigating issues related to binder transactions sent to frozen
> processes.
>
> Implementation details:
> For a given binder_ref, the state of frozen notification can be one of
> the followings:
> 1. Userspace doesn't want a notification. binder_ref->freeze is null.
> 2. Userspace wants a notification but none is in flight.
> list_empty(&binder_ref->freeze->work.entry) = true
> 3. A notification is in flight and waiting to be read by userspace.
> binder_ref_freeze.sent is false.
> 4. A notification was read by userspace and kernel is waiting for an ack.
> binder_ref_freeze.sent is true.
>
> When a notification is in flight, new state change events are coalesced into
> the existing binder_ref_freeze struct. If userspace hasn't picked up the
> notification yet, the driver simply rewrites the state. Otherwise, the
> notification is flagged as requiring a resend, which will be performed
> once userspace acks the original notification that's inflight.
>
> See https://r.android.com/3070045 for how userspace is going to use this
> feature.
>
> Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
> ---
> drivers/android/binder.c | 287 +++++++++++++++++++++++++++-
> drivers/android/binder_internal.h | 21 +-
> include/uapi/linux/android/binder.h | 36 ++++
> 3 files changed, 340 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index b21a7b246a0d..e1bd9810c2af 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3763,6 +3763,159 @@ static void binder_transaction(struct binder_proc *proc,
> }
> }
>
> +static int
> +binder_request_freeze_notification(struct binder_proc *proc,
> + struct binder_thread *thread,
> + struct binder_handle_cookie *handle_cookie)
> +{
> + struct binder_ref_freeze *freeze;
> + struct binder_ref *ref;
> + bool is_frozen;
> +
> + freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
> + if (!freeze)
> + return -ENOMEM;
> + binder_proc_lock(proc);
> + ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
> + if (!ref) {
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
> + proc->pid, thread->pid, handle_cookie->handle);
> + binder_proc_unlock(proc);
> + kfree(freeze);
> + return -EINVAL;
> + }
> +
> + binder_node_lock(ref->node);
> +
> + if (ref->freeze || !ref->node->proc) {
> + if (ref->freeze) {
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION freeze notification already set\n",
> + binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION process is already dead\n",
> + proc->pid, thread->pid);
> + }
nit: can you merge the branches into a single printk? maybe something
like the following:
binder_user_error("%d:%d invalid BC_REQUEST_FREEZE_NOTIFICATION %s\n",
proc->pid, thread->pid,
ref->freeze ? "already set" : "dead node");
> + 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 = ref->node->proc->is_frozen;
I believe you meant to use the 'is_frozen' here. Otherwise it doesn't
seem to be used after assignment.
> +
> + 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);
> +
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return 0;
> +}
> +
> +static int
> +binder_clear_freeze_notification(struct binder_proc *proc,
> + struct binder_thread *thread,
> + struct binder_handle_cookie *handle_cookie)
> +{
> + struct binder_ref_freeze *freeze;
> + struct binder_ref *ref;
> +
> + binder_proc_lock(proc);
> + ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
> + if (!ref) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
> + proc->pid, thread->pid, handle_cookie->handle);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> +
> + binder_node_lock(ref->node);
> +
> + if (!ref->freeze) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
> + proc->pid, thread->pid);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> + freeze = ref->freeze;
> + binder_inner_proc_lock(proc);
> + if (freeze->cookie != handle_cookie->cookie) {
> + binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
> + proc->pid, thread->pid, (u64)freeze->cookie,
> + (u64)handle_cookie->cookie);
> + binder_inner_proc_unlock(proc);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return -EINVAL;
> + }
> + ref->freeze = NULL;
You detach ref->freeze here and handle the kfree(freeze) later which
seems good. However, what happens when a reference is released with an
active ref->freeze? It seems to me this is a memory leak. This could
accumulate significantly as more references get dropped in thit way.
I think you want to add a kfree(ref->freeze) to binder_free_ref().
> + /*
> + * Take the existing freeze object and overwrite its work type. There are three cases here:
> + * 1. No pending notification. In this case just add the work to the queue.
> + * 2. An notification was sent and is pending an ack from userspace. Once an ack arrives, we
Typo s/An/A/
> + * should resend with the new work type.
> + * 3. A notification is pending to be sent. Since the work is already in the queue, nothing
> + * needs to be done here.
> + */
> + freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
> + if (list_empty(&freeze->work.entry)) {
> + binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> + binder_wakeup_proc_ilocked(proc);
> + } else if (freeze->sent) {
> + freeze->resend = true;
> + }
> + binder_inner_proc_unlock(proc);
> + binder_node_unlock(ref->node);
> + binder_proc_unlock(proc);
> + return 0;
> +}
> +
> +static int
> +binder_freeze_notification_done(struct binder_proc *proc,
> + struct binder_thread *thread,
> + binder_uintptr_t cookie)
> +{
> + struct binder_ref_freeze *freeze = NULL;
> + struct binder_work *w;
> +
> + binder_inner_proc_lock(proc);
> + list_for_each_entry(w, &proc->delivered_freeze, entry) {
> + struct binder_ref_freeze *tmp_freeze =
> + container_of(w, struct binder_ref_freeze, work);
> +
> + if (tmp_freeze->cookie == cookie) {
> + freeze = tmp_freeze;
> + break;
> + }
> + }
> + if (!freeze) {
> + binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
> + proc->pid, thread->pid, (u64)cookie);
> + binder_inner_proc_unlock(proc);
> + return -EINVAL;
> + }
> + binder_dequeue_work_ilocked(&freeze->work);
> + freeze->sent = false;
> + if (freeze->resend) {
> + freeze->resend = false;
> + binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
> + binder_wakeup_proc_ilocked(proc);
> + }
> + binder_inner_proc_unlock(proc);
> + return 0;
> +}
> +
> /**
> * binder_free_buf() - free the specified buffer
> * @proc: binder proc that owns buffer
> @@ -4246,6 +4399,44 @@ static int binder_thread_write(struct binder_proc *proc,
> binder_inner_proc_unlock(proc);
> } break;
>
> + case BC_REQUEST_FREEZE_NOTIFICATION: {
> + struct binder_handle_cookie handle_cookie;
> + int error;
> +
> + if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
> + return -EFAULT;
> + ptr += sizeof(handle_cookie);
> + error = binder_request_freeze_notification(proc, thread,
> + &handle_cookie);
> + if (error)
> + return error;
> + } break;
> +
> + case BC_CLEAR_FREEZE_NOTIFICATION: {
> + struct binder_handle_cookie handle_cookie;
> + int error;
> +
> + if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
> + return -EFAULT;
> + ptr += sizeof(handle_cookie);
> + error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
> + if (error)
> + return error;
> + } break;
> +
> + case BC_FREEZE_NOTIFICATION_DONE: {
> + binder_uintptr_t cookie;
> + int error;
> +
> + if (get_user(cookie, (binder_uintptr_t __user *)ptr))
> + return -EFAULT;
> +
> + ptr += sizeof(cookie);
> + error = binder_freeze_notification_done(proc, thread, cookie);
> + if (error)
> + return error;
> + } break;
> +
> default:
> pr_err("%d:%d unknown command %u\n",
> proc->pid, thread->pid, cmd);
> @@ -4635,6 +4826,46 @@ static int binder_thread_read(struct binder_proc *proc,
> if (cmd == BR_DEAD_BINDER)
> goto done; /* DEAD_BINDER notifications can cause transactions */
> } break;
> +
> + case BINDER_WORK_FROZEN_BINDER: {
> + struct binder_ref_freeze *freeze;
> + struct binder_frozen_state_info info;
> + memset(&info, 0, sizeof(info));
The memset should have a blank line after the declarations. This should
trigger a checkpatch issue I think.
> +
> + freeze = container_of(w, struct binder_ref_freeze, work);
> + info.is_frozen = freeze->is_frozen;
> + info.cookie = freeze->cookie;
> + freeze->sent = true;
> + binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
> + binder_inner_proc_unlock(proc);
> +
> + if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(uint32_t);
> + if (copy_to_user(ptr, &info, sizeof(info)))
> + return -EFAULT;
> + ptr += sizeof(info);
> + binder_stat_br(proc, thread, BR_FROZEN_BINDER);
> + goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
> + } break;
> +
> + case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
> + struct binder_ref_freeze *freeze =
> + container_of(w, struct binder_ref_freeze, work);
> + binder_uintptr_t cookie = freeze->cookie;
> +
> + binder_inner_proc_unlock(proc);
> + kfree(freeze);
> + binder_stats_deleted(BINDER_STAT_FREEZE);
> + if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(uint32_t);
> + if (put_user(cookie, (binder_uintptr_t __user *)ptr))
> + return -EFAULT;
> + ptr += sizeof(binder_uintptr_t);
> + binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
> + } break;
> +
> default:
> binder_inner_proc_unlock(proc);
> pr_err("%d:%d: bad work type %d\n",
> @@ -5242,6 +5473,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
> return false;
> }
>
> +static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
> +{
> + struct rb_node *n;
> + struct binder_ref *ref;
> +
> + binder_inner_proc_lock(proc);
> + for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
> + struct binder_node *node;
> +
> + node = rb_entry(n, struct binder_node, rb_node);
> + binder_inner_proc_unlock(proc);
> + binder_node_lock(node);
> + hlist_for_each_entry(ref, &node->refs, node_entry) {
> + /*
> + * Need the node lock to synchronize
> + * with new notification requests and the
> + * inner lock to synchronize with queued
> + * freeze notifications.
> + */
> + binder_inner_proc_lock(ref->proc);
> + if (!ref->freeze) {
> + binder_inner_proc_unlock(ref->proc);
> + continue;
> + }
> + ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
> + if (list_empty(&ref->freeze->work.entry)) {
> + ref->freeze->is_frozen = is_frozen;
> + binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
> + binder_wakeup_proc_ilocked(ref->proc);
> + } else {
> + if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
> + ref->freeze->resend = true;
> + ref->freeze->is_frozen = is_frozen;
> + }
> + binder_inner_proc_unlock(ref->proc);
> + }
> + binder_node_unlock(node);
> + binder_inner_proc_lock(proc);
> + }
> + binder_inner_proc_unlock(proc);
> +}
> +
> static int binder_ioctl_freeze(struct binder_freeze_info *info,
> struct binder_proc *target_proc)
> {
> @@ -5253,6 +5526,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
> target_proc->async_recv = false;
> target_proc->is_frozen = false;
> binder_inner_proc_unlock(target_proc);
> + binder_add_freeze_work(target_proc, false);
> return 0;
> }
>
> @@ -5285,6 +5559,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
> binder_inner_proc_lock(target_proc);
> target_proc->is_frozen = false;
> binder_inner_proc_unlock(target_proc);
> + } else {
> + binder_add_freeze_work(target_proc, true);
> }
>
> return ret;
> @@ -5658,6 +5934,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
> binder_stats_created(BINDER_STAT_PROC);
> proc->pid = current->group_leader->pid;
> INIT_LIST_HEAD(&proc->delivered_death);
> + INIT_LIST_HEAD(&proc->delivered_freeze);
> INIT_LIST_HEAD(&proc->waiting_threads);
> filp->private_data = proc;
>
> @@ -6209,7 +6486,9 @@ static const char * const binder_return_strings[] = {
> "BR_FAILED_REPLY",
> "BR_FROZEN_REPLY",
> "BR_ONEWAY_SPAM_SUSPECT",
> - "BR_TRANSACTION_PENDING_FROZEN"
> + "BR_TRANSACTION_PENDING_FROZEN",
> + "BR_FROZEN_BINDER",
> + "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
> };
>
> static const char * const binder_command_strings[] = {
> @@ -6232,6 +6511,9 @@ static const char * const binder_command_strings[] = {
> "BC_DEAD_BINDER_DONE",
> "BC_TRANSACTION_SG",
> "BC_REPLY_SG",
> + "BC_REQUEST_FREEZE_NOTIFICATION",
> + "BC_CLEAR_FREEZE_NOTIFICATION",
> + "BC_FREEZE_NOTIFICATION_DONE",
> };
>
> static const char * const binder_objstat_strings[] = {
> @@ -6241,7 +6523,8 @@ static const char * const binder_objstat_strings[] = {
> "ref",
> "death",
> "transaction",
> - "transaction_complete"
> + "transaction_complete",
> + "freeze",
> };
>
> static void print_binder_stats(struct seq_file *m, const char *prefix,
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 5b7c80b99ae8..3e4b35873c64 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -129,12 +129,13 @@ enum binder_stat_types {
> BINDER_STAT_DEATH,
> BINDER_STAT_TRANSACTION,
> BINDER_STAT_TRANSACTION_COMPLETE,
> + BINDER_STAT_FREEZE,
> BINDER_STAT_COUNT
> };
>
> struct binder_stats {
> - atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
> - atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> + atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
> + atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
> atomic_t obj_created[BINDER_STAT_COUNT];
> atomic_t obj_deleted[BINDER_STAT_COUNT];
> };
> @@ -159,6 +160,8 @@ struct binder_work {
> BINDER_WORK_DEAD_BINDER,
> BINDER_WORK_DEAD_BINDER_AND_CLEAR,
> BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
> + BINDER_WORK_FROZEN_BINDER,
> + BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
> } type;
> };
>
> @@ -275,6 +278,14 @@ struct binder_ref_death {
> binder_uintptr_t cookie;
> };
>
> +struct binder_ref_freeze {
> + struct binder_work work;
> + binder_uintptr_t cookie;
> + bool is_frozen:1;
> + bool sent:1;
> + bool resend:1;
> +};
> +
> /**
> * struct binder_ref_data - binder_ref counts and id
> * @debug_id: unique ID for the ref
> @@ -307,6 +318,8 @@ struct binder_ref_data {
> * @node indicates the node must be freed
> * @death: pointer to death notification (ref_death) if requested
> * (protected by @node->lock)
> + * @freeze: pointer to freeze notification (ref_freeze) if requested
> + * (protected by @node->lock)
> *
> * Structure to track references from procA to target node (on procB). This
> * structure is unsafe to access without holding @proc->outer_lock.
> @@ -323,6 +336,7 @@ struct binder_ref {
> struct binder_proc *proc;
> struct binder_node *node;
> struct binder_ref_death *death;
> + struct binder_ref_freeze *freeze;
> };
>
> /**
> @@ -374,6 +388,8 @@ struct binder_ref {
> * (atomics, no lock needed)
> * @delivered_death: list of delivered death notification
> * (protected by @inner_lock)
> + * @delivered_freeze: list of delivered freeze notification
> + * (protected by @inner_lock)
> * @max_threads: cap on number of binder threads
> * (protected by @inner_lock)
> * @requested_threads: number of binder threads requested but not
> @@ -421,6 +437,7 @@ struct binder_proc {
> struct list_head todo;
> struct binder_stats stats;
> struct list_head delivered_death;
> + struct list_head delivered_freeze;
> u32 max_threads;
> int requested_threads;
> int requested_threads_started;
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index d44a8118b2ed..1fd92021a573 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -236,6 +236,12 @@ struct binder_frozen_status_info {
> __u32 async_recv;
> };
>
> +struct binder_frozen_state_info {
> + binder_uintptr_t cookie;
> + __u32 is_frozen;
> + __u32 reserved;
> +};
> +
> /* struct binder_extened_error - extended error information
> * @id: identifier for the failed operation
> * @command: command as defined by binder_driver_return_protocol
> @@ -467,6 +473,17 @@ enum binder_driver_return_protocol {
> /*
> * The target of the last async transaction is frozen. No parameters.
> */
> +
> + BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
> + /*
> + * The cookie and a boolean (is_frozen) that indicates whether the process
> + * transitioned into a frozen or an unfrozen state.
> + */
> +
> + BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
> + /*
> + * void *: cookie
> + */
> };
>
> enum binder_driver_command_protocol {
> @@ -550,6 +567,25 @@ enum binder_driver_command_protocol {
> /*
> * binder_transaction_data_sg: the sent command.
> */
> +
> + BC_REQUEST_FREEZE_NOTIFICATION =
> + _IOW('c', 19, struct binder_handle_cookie),
> + /*
> + * int: handle
> + * void *: cookie
> + */
> +
> + BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
> + struct binder_handle_cookie),
> + /*
> + * int: handle
> + * void *: cookie
> + */
> +
> + BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
> + /*
> + * void *: cookie
> + */
> };
>
> #endif /* _UAPI_LINUX_BINDER_H */
> --
> 2.45.2.803.g4e1b14247a-goog
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
` (3 preceding siblings ...)
2024-07-01 18:27 ` Yu-Ting Tseng
@ 2024-07-03 17:08 ` Yu-Ting Tseng
2024-07-03 17:08 ` Yu-Ting Tseng
` (3 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-03 17:08 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
> can you merge the branches into a single printk?
Done
> I believe you meant to use the 'is_frozen' here
Fixed
> I think you want to add a kfree(ref->freeze) to binder_free_ref().
Done
> Typo s/An/A/
Done
> The memset should have a blank line after the declarations. This should trigger a checkpatch issue I think.
Fixed. Reran checkpatch and there is no other issues flagged now.
(except for line-too-long which doesn't really have a fix)
Also merged the binder_features change into this patch as requested.
Yu-Ting Tseng (1):
binder: frozen notification
drivers/android/binder.c | 284 +++++++++++++++++-
drivers/android/binder_internal.h | 21 +-
drivers/android/binderfs.c | 8 +
include/uapi/linux/android/binder.h | 36 +++
.../filesystems/binderfs/binderfs_test.c | 1 +
5 files changed, 346 insertions(+), 4 deletions(-)
base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v5] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
` (4 preceding siblings ...)
2024-07-03 17:08 ` [PATCH v5] " Yu-Ting Tseng
@ 2024-07-03 17:08 ` Yu-Ting Tseng
2024-07-03 17:58 ` [PATCH v6 0/2] " Yu-Ting Tseng
` (2 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-03 17:08 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Frozen processes present a significant challenge in binder transactions.
When a process is frozen, it cannot, by design, accept and/or respond to
binder transactions. As a result, the sender needs to adjust its
behavior, such as postponing transactions until the peer process
unfreezes. However, there is currently no way to subscribe to these
state change events, making it impossible to implement frozen-aware
behaviors efficiently.
Introduce a binder API for subscribing to frozen state change events.
This allows programs to react to changes in peer process state,
mitigating issues related to binder transactions sent to frozen
processes.
Implementation details:
For a given binder_ref, the state of frozen notification can be one of
the followings:
1. Userspace doesn't want a notification. binder_ref->freeze is null.
2. Userspace wants a notification but none is in flight.
list_empty(&binder_ref->freeze->work.entry) = true
3. A notification is in flight and waiting to be read by userspace.
binder_ref_freeze.sent is false.
4. A notification was read by userspace and kernel is waiting for an ack.
binder_ref_freeze.sent is true.
When a notification is in flight, new state change events are coalesced into
the existing binder_ref_freeze struct. If userspace hasn't picked up the
notification yet, the driver simply rewrites the state. Otherwise, the
notification is flagged as requiring a resend, which will be performed
once userspace acks the original notification that's inflight.
See https://r.android.com/3070045 for how userspace is going to use this
feature.
Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
---
drivers/android/binder.c | 284 +++++++++++++++++-
drivers/android/binder_internal.h | 21 +-
drivers/android/binderfs.c | 8 +
include/uapi/linux/android/binder.h | 36 +++
.../filesystems/binderfs/binderfs_test.c | 1 +
5 files changed, 346 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b21a7b246a0d..77f318127a6e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1295,6 +1295,7 @@ static void binder_free_ref(struct binder_ref *ref)
if (ref->node)
binder_free_node(ref->node);
kfree(ref->death);
+ kfree(ref->freeze);
kfree(ref);
}
@@ -3763,6 +3764,155 @@ static void binder_transaction(struct binder_proc *proc,
}
}
+static int
+binder_request_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+ bool is_frozen;
+
+ freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
+ if (!freeze)
+ return -ENOMEM;
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ kfree(freeze);
+ return -EINVAL;
+ }
+
+ 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");
+ 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);
+
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_clear_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (!ref->freeze) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
+ proc->pid, thread->pid);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ freeze = ref->freeze;
+ binder_inner_proc_lock(proc);
+ if (freeze->cookie != handle_cookie->cookie) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
+ proc->pid, thread->pid, (u64)freeze->cookie,
+ (u64)handle_cookie->cookie);
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ ref->freeze = NULL;
+ /*
+ * Take the existing freeze object and overwrite its work type. There are three cases here:
+ * 1. No pending notification. In this case just add the work to the queue.
+ * 2. A notification was sent and is pending an ack from userspace. Once an ack arrives, we
+ * should resend with the new work type.
+ * 3. A notification is pending to be sent. Since the work is already in the queue, nothing
+ * needs to be done here.
+ */
+ freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
+ if (list_empty(&freeze->work.entry)) {
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ } else if (freeze->sent) {
+ freeze->resend = true;
+ }
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_freeze_notification_done(struct binder_proc *proc,
+ struct binder_thread *thread,
+ binder_uintptr_t cookie)
+{
+ struct binder_ref_freeze *freeze = NULL;
+ struct binder_work *w;
+
+ binder_inner_proc_lock(proc);
+ list_for_each_entry(w, &proc->delivered_freeze, entry) {
+ struct binder_ref_freeze *tmp_freeze =
+ container_of(w, struct binder_ref_freeze, work);
+
+ if (tmp_freeze->cookie == cookie) {
+ freeze = tmp_freeze;
+ break;
+ }
+ }
+ if (!freeze) {
+ binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
+ proc->pid, thread->pid, (u64)cookie);
+ binder_inner_proc_unlock(proc);
+ return -EINVAL;
+ }
+ binder_dequeue_work_ilocked(&freeze->work);
+ freeze->sent = false;
+ if (freeze->resend) {
+ freeze->resend = false;
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
+ binder_inner_proc_unlock(proc);
+ return 0;
+}
+
/**
* binder_free_buf() - free the specified buffer
* @proc: binder proc that owns buffer
@@ -4246,6 +4396,44 @@ static int binder_thread_write(struct binder_proc *proc,
binder_inner_proc_unlock(proc);
} break;
+ case BC_REQUEST_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_request_freeze_notification(proc, thread,
+ &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_FREEZE_NOTIFICATION_DONE: {
+ binder_uintptr_t cookie;
+ int error;
+
+ if (get_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+
+ ptr += sizeof(cookie);
+ error = binder_freeze_notification_done(proc, thread, cookie);
+ if (error)
+ return error;
+ } break;
+
default:
pr_err("%d:%d unknown command %u\n",
proc->pid, thread->pid, cmd);
@@ -4635,6 +4823,46 @@ static int binder_thread_read(struct binder_proc *proc,
if (cmd == BR_DEAD_BINDER)
goto done; /* DEAD_BINDER notifications can cause transactions */
} break;
+
+ case BINDER_WORK_FROZEN_BINDER: {
+ struct binder_ref_freeze *freeze;
+ struct binder_frozen_state_info info;
+
+ memset(&info, 0, sizeof(info));
+ freeze = container_of(w, struct binder_ref_freeze, work);
+ info.is_frozen = freeze->is_frozen;
+ info.cookie = freeze->cookie;
+ freeze->sent = true;
+ binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
+ binder_inner_proc_unlock(proc);
+
+ if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (copy_to_user(ptr, &info, sizeof(info)))
+ return -EFAULT;
+ ptr += sizeof(info);
+ binder_stat_br(proc, thread, BR_FROZEN_BINDER);
+ goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
+ } break;
+
+ case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_ref_freeze *freeze =
+ container_of(w, struct binder_ref_freeze, work);
+ binder_uintptr_t cookie = freeze->cookie;
+
+ binder_inner_proc_unlock(proc);
+ kfree(freeze);
+ binder_stats_deleted(BINDER_STAT_FREEZE);
+ if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (put_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(binder_uintptr_t);
+ binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
+ } break;
+
default:
binder_inner_proc_unlock(proc);
pr_err("%d:%d: bad work type %d\n",
@@ -5242,6 +5470,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
return false;
}
+static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
+{
+ struct rb_node *n;
+ struct binder_ref *ref;
+
+ binder_inner_proc_lock(proc);
+ for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
+ struct binder_node *node;
+
+ node = rb_entry(n, struct binder_node, rb_node);
+ binder_inner_proc_unlock(proc);
+ binder_node_lock(node);
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ /*
+ * Need the node lock to synchronize
+ * with new notification requests and the
+ * inner lock to synchronize with queued
+ * freeze notifications.
+ */
+ binder_inner_proc_lock(ref->proc);
+ if (!ref->freeze) {
+ binder_inner_proc_unlock(ref->proc);
+ continue;
+ }
+ ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
+ if (list_empty(&ref->freeze->work.entry)) {
+ ref->freeze->is_frozen = is_frozen;
+ binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
+ binder_wakeup_proc_ilocked(ref->proc);
+ } else {
+ if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
+ ref->freeze->resend = true;
+ ref->freeze->is_frozen = is_frozen;
+ }
+ binder_inner_proc_unlock(ref->proc);
+ }
+ binder_node_unlock(node);
+ binder_inner_proc_lock(proc);
+ }
+ binder_inner_proc_unlock(proc);
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -5253,6 +5523,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ binder_add_freeze_work(target_proc, false);
return 0;
}
@@ -5285,6 +5556,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
binder_inner_proc_lock(target_proc);
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ } else {
+ binder_add_freeze_work(target_proc, true);
}
return ret;
@@ -5658,6 +5931,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
binder_stats_created(BINDER_STAT_PROC);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
+ INIT_LIST_HEAD(&proc->delivered_freeze);
INIT_LIST_HEAD(&proc->waiting_threads);
filp->private_data = proc;
@@ -6209,7 +6483,9 @@ static const char * const binder_return_strings[] = {
"BR_FAILED_REPLY",
"BR_FROZEN_REPLY",
"BR_ONEWAY_SPAM_SUSPECT",
- "BR_TRANSACTION_PENDING_FROZEN"
+ "BR_TRANSACTION_PENDING_FROZEN",
+ "BR_FROZEN_BINDER",
+ "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_command_strings[] = {
@@ -6232,6 +6508,9 @@ static const char * const binder_command_strings[] = {
"BC_DEAD_BINDER_DONE",
"BC_TRANSACTION_SG",
"BC_REPLY_SG",
+ "BC_REQUEST_FREEZE_NOTIFICATION",
+ "BC_CLEAR_FREEZE_NOTIFICATION",
+ "BC_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_objstat_strings[] = {
@@ -6241,7 +6520,8 @@ static const char * const binder_objstat_strings[] = {
"ref",
"death",
"transaction",
- "transaction_complete"
+ "transaction_complete",
+ "freeze",
};
static void print_binder_stats(struct seq_file *m, const char *prefix,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 5b7c80b99ae8..3e4b35873c64 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -129,12 +129,13 @@ enum binder_stat_types {
BINDER_STAT_DEATH,
BINDER_STAT_TRANSACTION,
BINDER_STAT_TRANSACTION_COMPLETE,
+ BINDER_STAT_FREEZE,
BINDER_STAT_COUNT
};
struct binder_stats {
- atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
- atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
+ atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
+ atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
atomic_t obj_created[BINDER_STAT_COUNT];
atomic_t obj_deleted[BINDER_STAT_COUNT];
};
@@ -159,6 +160,8 @@ struct binder_work {
BINDER_WORK_DEAD_BINDER,
BINDER_WORK_DEAD_BINDER_AND_CLEAR,
BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
+ BINDER_WORK_FROZEN_BINDER,
+ BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
} type;
};
@@ -275,6 +278,14 @@ struct binder_ref_death {
binder_uintptr_t cookie;
};
+struct binder_ref_freeze {
+ struct binder_work work;
+ binder_uintptr_t cookie;
+ bool is_frozen:1;
+ bool sent:1;
+ bool resend:1;
+};
+
/**
* struct binder_ref_data - binder_ref counts and id
* @debug_id: unique ID for the ref
@@ -307,6 +318,8 @@ struct binder_ref_data {
* @node indicates the node must be freed
* @death: pointer to death notification (ref_death) if requested
* (protected by @node->lock)
+ * @freeze: pointer to freeze notification (ref_freeze) if requested
+ * (protected by @node->lock)
*
* Structure to track references from procA to target node (on procB). This
* structure is unsafe to access without holding @proc->outer_lock.
@@ -323,6 +336,7 @@ struct binder_ref {
struct binder_proc *proc;
struct binder_node *node;
struct binder_ref_death *death;
+ struct binder_ref_freeze *freeze;
};
/**
@@ -374,6 +388,8 @@ struct binder_ref {
* (atomics, no lock needed)
* @delivered_death: list of delivered death notification
* (protected by @inner_lock)
+ * @delivered_freeze: list of delivered freeze notification
+ * (protected by @inner_lock)
* @max_threads: cap on number of binder threads
* (protected by @inner_lock)
* @requested_threads: number of binder threads requested but not
@@ -421,6 +437,7 @@ struct binder_proc {
struct list_head todo;
struct binder_stats stats;
struct list_head delivered_death;
+ struct list_head delivered_freeze;
u32 max_threads;
int requested_threads;
int requested_threads_started;
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 3001d754ac36..ad1fa7abc323 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -58,6 +58,7 @@ enum binderfs_stats_mode {
struct binder_features {
bool oneway_spam_detection;
bool extended_error;
+ bool freeze_notification;
};
static const struct constant_table binderfs_param_stats[] = {
@@ -74,6 +75,7 @@ static const struct fs_parameter_spec binderfs_fs_parameters[] = {
static struct binder_features binder_features = {
.oneway_spam_detection = true,
.extended_error = true,
+ .freeze_notification = true,
};
static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
@@ -608,6 +610,12 @@ static int init_binder_features(struct super_block *sb)
if (IS_ERR(dentry))
return PTR_ERR(dentry);
+ dentry = binderfs_create_file(dir, "freeze_notification",
+ &binder_features_fops,
+ &binder_features.freeze_notification);
+ if (IS_ERR(dentry))
+ return PTR_ERR(dentry);
+
return 0;
}
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index d44a8118b2ed..1fd92021a573 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -236,6 +236,12 @@ struct binder_frozen_status_info {
__u32 async_recv;
};
+struct binder_frozen_state_info {
+ binder_uintptr_t cookie;
+ __u32 is_frozen;
+ __u32 reserved;
+};
+
/* struct binder_extened_error - extended error information
* @id: identifier for the failed operation
* @command: command as defined by binder_driver_return_protocol
@@ -467,6 +473,17 @@ enum binder_driver_return_protocol {
/*
* The target of the last async transaction is frozen. No parameters.
*/
+
+ BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
+ /*
+ * The cookie and a boolean (is_frozen) that indicates whether the process
+ * transitioned into a frozen or an unfrozen state.
+ */
+
+ BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
enum binder_driver_command_protocol {
@@ -550,6 +567,25 @@ enum binder_driver_command_protocol {
/*
* binder_transaction_data_sg: the sent command.
*/
+
+ BC_REQUEST_FREEZE_NOTIFICATION =
+ _IOW('c', 19, struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
+ struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
#endif /* _UAPI_LINUX_BINDER_H */
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 5f362c0fd890..319567f0fae1 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -65,6 +65,7 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
static const char * const binder_features[] = {
"oneway_spam_detection",
"extended_error",
+ "freeze_notification",
};
change_mountns(_metadata);
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 0/2] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
` (5 preceding siblings ...)
2024-07-03 17:08 ` Yu-Ting Tseng
@ 2024-07-03 17:58 ` Yu-Ting Tseng
2024-07-04 13:00 ` Greg KH
2024-07-03 17:58 ` [PATCH v6 1/2] " Yu-Ting Tseng
2024-07-03 17:58 ` [PATCH v6 2/2] binder: frozen notification binder_features flag Yu-Ting Tseng
8 siblings, 1 reply; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-03 17:58 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Separated the binder_features change into its own patch.
Yu-Ting Tseng (2):
binder: frozen notification
binder: frozen notification binder_features flag
drivers/android/binder.c | 284 +++++++++++++++++-
drivers/android/binder_internal.h | 21 +-
drivers/android/binderfs.c | 8 +
include/uapi/linux/android/binder.h | 36 +++
.../filesystems/binderfs/binderfs_test.c | 1 +
5 files changed, 346 insertions(+), 4 deletions(-)
base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v6 0/2] binder: frozen notification
2024-07-03 17:58 ` [PATCH v6 0/2] " Yu-Ting Tseng
@ 2024-07-04 13:00 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2024-07-04 13:00 UTC (permalink / raw)
To: Yu-Ting Tseng
Cc: cmllamas, tkjos, arve, maco, joel, brauner, surenb, aliceryhl,
kernel-team, linux-kernel
On Wed, Jul 03, 2024 at 10:58:42AM -0700, Yu-Ting Tseng wrote:
> Separated the binder_features change into its own patch.
>
> Yu-Ting Tseng (2):
> binder: frozen notification
> binder: frozen notification binder_features flag
>
> drivers/android/binder.c | 284 +++++++++++++++++-
> drivers/android/binder_internal.h | 21 +-
> drivers/android/binderfs.c | 8 +
> include/uapi/linux/android/binder.h | 36 +++
> .../filesystems/binderfs/binderfs_test.c | 1 +
> 5 files changed, 346 insertions(+), 4 deletions(-)
>
>
> base-commit: 14d7c92f8df9c0964ae6f8b813c1b3ac38120825
This email thread is crazy, sorry, look at how it shows up in sane email
clients:
https://lore.kernel.org/all/20240703175843.3639568-2-yutingtseng@google.com/
Please just send a new series, as it's own thread, so we can figure out
what to do here.
And for some reason I have 2 different copies of the v6 series, one sent
to the list, and one not, which makes no sense either. For that reason
alone I can't take this :(
Please, realize what this looks like from a reviewer's point of view,
you want to make it simple and easy for us to know what to do...
Please fix up and send a v7.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v6 1/2] binder: frozen notification
2024-06-24 15:56 ` Yu-Ting Tseng
` (6 preceding siblings ...)
2024-07-03 17:58 ` [PATCH v6 0/2] " Yu-Ting Tseng
@ 2024-07-03 17:58 ` Yu-Ting Tseng
2024-07-03 17:58 ` [PATCH v6 2/2] binder: frozen notification binder_features flag Yu-Ting Tseng
8 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-03 17:58 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Frozen processes present a significant challenge in binder transactions.
When a process is frozen, it cannot, by design, accept and/or respond to
binder transactions. As a result, the sender needs to adjust its
behavior, such as postponing transactions until the peer process
unfreezes. However, there is currently no way to subscribe to these
state change events, making it impossible to implement frozen-aware
behaviors efficiently.
Introduce a binder API for subscribing to frozen state change events.
This allows programs to react to changes in peer process state,
mitigating issues related to binder transactions sent to frozen
processes.
Implementation details:
For a given binder_ref, the state of frozen notification can be one of
the followings:
1. Userspace doesn't want a notification. binder_ref->freeze is null.
2. Userspace wants a notification but none is in flight.
list_empty(&binder_ref->freeze->work.entry) = true
3. A notification is in flight and waiting to be read by userspace.
binder_ref_freeze.sent is false.
4. A notification was read by userspace and kernel is waiting for an ack.
binder_ref_freeze.sent is true.
When a notification is in flight, new state change events are coalesced into
the existing binder_ref_freeze struct. If userspace hasn't picked up the
notification yet, the driver simply rewrites the state. Otherwise, the
notification is flagged as requiring a resend, which will be performed
once userspace acks the original notification that's inflight.
See https://r.android.com/3070045 for how userspace is going to use this
feature.
Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
---
drivers/android/binder.c | 284 +++++++++++++++++++++++++++-
drivers/android/binder_internal.h | 21 +-
include/uapi/linux/android/binder.h | 36 ++++
3 files changed, 337 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b21a7b246a0d..77f318127a6e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1295,6 +1295,7 @@ static void binder_free_ref(struct binder_ref *ref)
if (ref->node)
binder_free_node(ref->node);
kfree(ref->death);
+ kfree(ref->freeze);
kfree(ref);
}
@@ -3763,6 +3764,155 @@ static void binder_transaction(struct binder_proc *proc,
}
}
+static int
+binder_request_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+ bool is_frozen;
+
+ freeze = kzalloc(sizeof(*freeze), GFP_KERNEL);
+ if (!freeze)
+ return -ENOMEM;
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_REQUEST_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ kfree(freeze);
+ return -EINVAL;
+ }
+
+ 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");
+ 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);
+
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_clear_freeze_notification(struct binder_proc *proc,
+ struct binder_thread *thread,
+ struct binder_handle_cookie *handle_cookie)
+{
+ struct binder_ref_freeze *freeze;
+ struct binder_ref *ref;
+
+ binder_proc_lock(proc);
+ ref = binder_get_ref_olocked(proc, handle_cookie->handle, false);
+ if (!ref) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION invalid ref %d\n",
+ proc->pid, thread->pid, handle_cookie->handle);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+
+ binder_node_lock(ref->node);
+
+ if (!ref->freeze) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification not active\n",
+ proc->pid, thread->pid);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ freeze = ref->freeze;
+ binder_inner_proc_lock(proc);
+ if (freeze->cookie != handle_cookie->cookie) {
+ binder_user_error("%d:%d BC_CLEAR_FREEZE_NOTIFICATION freeze notification cookie mismatch %016llx != %016llx\n",
+ proc->pid, thread->pid, (u64)freeze->cookie,
+ (u64)handle_cookie->cookie);
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return -EINVAL;
+ }
+ ref->freeze = NULL;
+ /*
+ * Take the existing freeze object and overwrite its work type. There are three cases here:
+ * 1. No pending notification. In this case just add the work to the queue.
+ * 2. A notification was sent and is pending an ack from userspace. Once an ack arrives, we
+ * should resend with the new work type.
+ * 3. A notification is pending to be sent. Since the work is already in the queue, nothing
+ * needs to be done here.
+ */
+ freeze->work.type = BINDER_WORK_CLEAR_FREEZE_NOTIFICATION;
+ if (list_empty(&freeze->work.entry)) {
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ } else if (freeze->sent) {
+ freeze->resend = true;
+ }
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(ref->node);
+ binder_proc_unlock(proc);
+ return 0;
+}
+
+static int
+binder_freeze_notification_done(struct binder_proc *proc,
+ struct binder_thread *thread,
+ binder_uintptr_t cookie)
+{
+ struct binder_ref_freeze *freeze = NULL;
+ struct binder_work *w;
+
+ binder_inner_proc_lock(proc);
+ list_for_each_entry(w, &proc->delivered_freeze, entry) {
+ struct binder_ref_freeze *tmp_freeze =
+ container_of(w, struct binder_ref_freeze, work);
+
+ if (tmp_freeze->cookie == cookie) {
+ freeze = tmp_freeze;
+ break;
+ }
+ }
+ if (!freeze) {
+ binder_user_error("%d:%d BC_FREEZE_NOTIFICATION_DONE %016llx not found\n",
+ proc->pid, thread->pid, (u64)cookie);
+ binder_inner_proc_unlock(proc);
+ return -EINVAL;
+ }
+ binder_dequeue_work_ilocked(&freeze->work);
+ freeze->sent = false;
+ if (freeze->resend) {
+ freeze->resend = false;
+ binder_enqueue_work_ilocked(&freeze->work, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
+ binder_inner_proc_unlock(proc);
+ return 0;
+}
+
/**
* binder_free_buf() - free the specified buffer
* @proc: binder proc that owns buffer
@@ -4246,6 +4396,44 @@ static int binder_thread_write(struct binder_proc *proc,
binder_inner_proc_unlock(proc);
} break;
+ case BC_REQUEST_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_request_freeze_notification(proc, thread,
+ &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_handle_cookie handle_cookie;
+ int error;
+
+ if (copy_from_user(&handle_cookie, ptr, sizeof(handle_cookie)))
+ return -EFAULT;
+ ptr += sizeof(handle_cookie);
+ error = binder_clear_freeze_notification(proc, thread, &handle_cookie);
+ if (error)
+ return error;
+ } break;
+
+ case BC_FREEZE_NOTIFICATION_DONE: {
+ binder_uintptr_t cookie;
+ int error;
+
+ if (get_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+
+ ptr += sizeof(cookie);
+ error = binder_freeze_notification_done(proc, thread, cookie);
+ if (error)
+ return error;
+ } break;
+
default:
pr_err("%d:%d unknown command %u\n",
proc->pid, thread->pid, cmd);
@@ -4635,6 +4823,46 @@ static int binder_thread_read(struct binder_proc *proc,
if (cmd == BR_DEAD_BINDER)
goto done; /* DEAD_BINDER notifications can cause transactions */
} break;
+
+ case BINDER_WORK_FROZEN_BINDER: {
+ struct binder_ref_freeze *freeze;
+ struct binder_frozen_state_info info;
+
+ memset(&info, 0, sizeof(info));
+ freeze = container_of(w, struct binder_ref_freeze, work);
+ info.is_frozen = freeze->is_frozen;
+ info.cookie = freeze->cookie;
+ freeze->sent = true;
+ binder_enqueue_work_ilocked(w, &proc->delivered_freeze);
+ binder_inner_proc_unlock(proc);
+
+ if (put_user(BR_FROZEN_BINDER, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (copy_to_user(ptr, &info, sizeof(info)))
+ return -EFAULT;
+ ptr += sizeof(info);
+ binder_stat_br(proc, thread, BR_FROZEN_BINDER);
+ goto done; /* BR_FROZEN_BINDER notifications can cause transactions */
+ } break;
+
+ case BINDER_WORK_CLEAR_FREEZE_NOTIFICATION: {
+ struct binder_ref_freeze *freeze =
+ container_of(w, struct binder_ref_freeze, work);
+ binder_uintptr_t cookie = freeze->cookie;
+
+ binder_inner_proc_unlock(proc);
+ kfree(freeze);
+ binder_stats_deleted(BINDER_STAT_FREEZE);
+ if (put_user(BR_CLEAR_FREEZE_NOTIFICATION_DONE, (uint32_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(uint32_t);
+ if (put_user(cookie, (binder_uintptr_t __user *)ptr))
+ return -EFAULT;
+ ptr += sizeof(binder_uintptr_t);
+ binder_stat_br(proc, thread, BR_CLEAR_FREEZE_NOTIFICATION_DONE);
+ } break;
+
default:
binder_inner_proc_unlock(proc);
pr_err("%d:%d: bad work type %d\n",
@@ -5242,6 +5470,48 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
return false;
}
+static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
+{
+ struct rb_node *n;
+ struct binder_ref *ref;
+
+ binder_inner_proc_lock(proc);
+ for (n = rb_first(&proc->nodes); n; n = rb_next(n)) {
+ struct binder_node *node;
+
+ node = rb_entry(n, struct binder_node, rb_node);
+ binder_inner_proc_unlock(proc);
+ binder_node_lock(node);
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ /*
+ * Need the node lock to synchronize
+ * with new notification requests and the
+ * inner lock to synchronize with queued
+ * freeze notifications.
+ */
+ binder_inner_proc_lock(ref->proc);
+ if (!ref->freeze) {
+ binder_inner_proc_unlock(ref->proc);
+ continue;
+ }
+ ref->freeze->work.type = BINDER_WORK_FROZEN_BINDER;
+ if (list_empty(&ref->freeze->work.entry)) {
+ ref->freeze->is_frozen = is_frozen;
+ binder_enqueue_work_ilocked(&ref->freeze->work, &ref->proc->todo);
+ binder_wakeup_proc_ilocked(ref->proc);
+ } else {
+ if (ref->freeze->sent && ref->freeze->is_frozen != is_frozen)
+ ref->freeze->resend = true;
+ ref->freeze->is_frozen = is_frozen;
+ }
+ binder_inner_proc_unlock(ref->proc);
+ }
+ binder_node_unlock(node);
+ binder_inner_proc_lock(proc);
+ }
+ binder_inner_proc_unlock(proc);
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -5253,6 +5523,7 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ binder_add_freeze_work(target_proc, false);
return 0;
}
@@ -5285,6 +5556,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info *info,
binder_inner_proc_lock(target_proc);
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
+ } else {
+ binder_add_freeze_work(target_proc, true);
}
return ret;
@@ -5658,6 +5931,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
binder_stats_created(BINDER_STAT_PROC);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
+ INIT_LIST_HEAD(&proc->delivered_freeze);
INIT_LIST_HEAD(&proc->waiting_threads);
filp->private_data = proc;
@@ -6209,7 +6483,9 @@ static const char * const binder_return_strings[] = {
"BR_FAILED_REPLY",
"BR_FROZEN_REPLY",
"BR_ONEWAY_SPAM_SUSPECT",
- "BR_TRANSACTION_PENDING_FROZEN"
+ "BR_TRANSACTION_PENDING_FROZEN",
+ "BR_FROZEN_BINDER",
+ "BR_CLEAR_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_command_strings[] = {
@@ -6232,6 +6508,9 @@ static const char * const binder_command_strings[] = {
"BC_DEAD_BINDER_DONE",
"BC_TRANSACTION_SG",
"BC_REPLY_SG",
+ "BC_REQUEST_FREEZE_NOTIFICATION",
+ "BC_CLEAR_FREEZE_NOTIFICATION",
+ "BC_FREEZE_NOTIFICATION_DONE",
};
static const char * const binder_objstat_strings[] = {
@@ -6241,7 +6520,8 @@ static const char * const binder_objstat_strings[] = {
"ref",
"death",
"transaction",
- "transaction_complete"
+ "transaction_complete",
+ "freeze",
};
static void print_binder_stats(struct seq_file *m, const char *prefix,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 5b7c80b99ae8..3e4b35873c64 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -129,12 +129,13 @@ enum binder_stat_types {
BINDER_STAT_DEATH,
BINDER_STAT_TRANSACTION,
BINDER_STAT_TRANSACTION_COMPLETE,
+ BINDER_STAT_FREEZE,
BINDER_STAT_COUNT
};
struct binder_stats {
- atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING_FROZEN) + 1];
- atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
+ atomic_t br[_IOC_NR(BR_CLEAR_FREEZE_NOTIFICATION_DONE) + 1];
+ atomic_t bc[_IOC_NR(BC_FREEZE_NOTIFICATION_DONE) + 1];
atomic_t obj_created[BINDER_STAT_COUNT];
atomic_t obj_deleted[BINDER_STAT_COUNT];
};
@@ -159,6 +160,8 @@ struct binder_work {
BINDER_WORK_DEAD_BINDER,
BINDER_WORK_DEAD_BINDER_AND_CLEAR,
BINDER_WORK_CLEAR_DEATH_NOTIFICATION,
+ BINDER_WORK_FROZEN_BINDER,
+ BINDER_WORK_CLEAR_FREEZE_NOTIFICATION,
} type;
};
@@ -275,6 +278,14 @@ struct binder_ref_death {
binder_uintptr_t cookie;
};
+struct binder_ref_freeze {
+ struct binder_work work;
+ binder_uintptr_t cookie;
+ bool is_frozen:1;
+ bool sent:1;
+ bool resend:1;
+};
+
/**
* struct binder_ref_data - binder_ref counts and id
* @debug_id: unique ID for the ref
@@ -307,6 +318,8 @@ struct binder_ref_data {
* @node indicates the node must be freed
* @death: pointer to death notification (ref_death) if requested
* (protected by @node->lock)
+ * @freeze: pointer to freeze notification (ref_freeze) if requested
+ * (protected by @node->lock)
*
* Structure to track references from procA to target node (on procB). This
* structure is unsafe to access without holding @proc->outer_lock.
@@ -323,6 +336,7 @@ struct binder_ref {
struct binder_proc *proc;
struct binder_node *node;
struct binder_ref_death *death;
+ struct binder_ref_freeze *freeze;
};
/**
@@ -374,6 +388,8 @@ struct binder_ref {
* (atomics, no lock needed)
* @delivered_death: list of delivered death notification
* (protected by @inner_lock)
+ * @delivered_freeze: list of delivered freeze notification
+ * (protected by @inner_lock)
* @max_threads: cap on number of binder threads
* (protected by @inner_lock)
* @requested_threads: number of binder threads requested but not
@@ -421,6 +437,7 @@ struct binder_proc {
struct list_head todo;
struct binder_stats stats;
struct list_head delivered_death;
+ struct list_head delivered_freeze;
u32 max_threads;
int requested_threads;
int requested_threads_started;
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index d44a8118b2ed..1fd92021a573 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -236,6 +236,12 @@ struct binder_frozen_status_info {
__u32 async_recv;
};
+struct binder_frozen_state_info {
+ binder_uintptr_t cookie;
+ __u32 is_frozen;
+ __u32 reserved;
+};
+
/* struct binder_extened_error - extended error information
* @id: identifier for the failed operation
* @command: command as defined by binder_driver_return_protocol
@@ -467,6 +473,17 @@ enum binder_driver_return_protocol {
/*
* The target of the last async transaction is frozen. No parameters.
*/
+
+ BR_FROZEN_BINDER = _IOR('r', 21, struct binder_frozen_state_info),
+ /*
+ * The cookie and a boolean (is_frozen) that indicates whether the process
+ * transitioned into a frozen or an unfrozen state.
+ */
+
+ BR_CLEAR_FREEZE_NOTIFICATION_DONE = _IOR('r', 22, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
enum binder_driver_command_protocol {
@@ -550,6 +567,25 @@ enum binder_driver_command_protocol {
/*
* binder_transaction_data_sg: the sent command.
*/
+
+ BC_REQUEST_FREEZE_NOTIFICATION =
+ _IOW('c', 19, struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_CLEAR_FREEZE_NOTIFICATION = _IOW('c', 20,
+ struct binder_handle_cookie),
+ /*
+ * int: handle
+ * void *: cookie
+ */
+
+ BC_FREEZE_NOTIFICATION_DONE = _IOW('c', 21, binder_uintptr_t),
+ /*
+ * void *: cookie
+ */
};
#endif /* _UAPI_LINUX_BINDER_H */
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v6 2/2] binder: frozen notification binder_features flag
2024-06-24 15:56 ` Yu-Ting Tseng
` (7 preceding siblings ...)
2024-07-03 17:58 ` [PATCH v6 1/2] " Yu-Ting Tseng
@ 2024-07-03 17:58 ` Yu-Ting Tseng
8 siblings, 0 replies; 20+ messages in thread
From: Yu-Ting Tseng @ 2024-07-03 17:58 UTC (permalink / raw)
To: cmllamas, tkjos, gregkh
Cc: arve, maco, joel, brauner, surenb, aliceryhl, kernel-team,
linux-kernel, Yu-Ting Tseng
Add a flag to binder_features to indicate that the freeze notification
feature is available.
Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
Acked-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binderfs.c | 8 ++++++++
.../selftests/filesystems/binderfs/binderfs_test.c | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 3001d754ac36..ad1fa7abc323 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -58,6 +58,7 @@ enum binderfs_stats_mode {
struct binder_features {
bool oneway_spam_detection;
bool extended_error;
+ bool freeze_notification;
};
static const struct constant_table binderfs_param_stats[] = {
@@ -74,6 +75,7 @@ static const struct fs_parameter_spec binderfs_fs_parameters[] = {
static struct binder_features binder_features = {
.oneway_spam_detection = true,
.extended_error = true,
+ .freeze_notification = true,
};
static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
@@ -608,6 +610,12 @@ static int init_binder_features(struct super_block *sb)
if (IS_ERR(dentry))
return PTR_ERR(dentry);
+ dentry = binderfs_create_file(dir, "freeze_notification",
+ &binder_features_fops,
+ &binder_features.freeze_notification);
+ if (IS_ERR(dentry))
+ return PTR_ERR(dentry);
+
return 0;
}
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 5f362c0fd890..319567f0fae1 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -65,6 +65,7 @@ static int __do_binderfs_test(struct __test_metadata *_metadata)
static const char * const binder_features[] = {
"oneway_spam_detection",
"extended_error",
+ "freeze_notification",
};
change_mountns(_metadata);
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread