| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y2wtHBPMyxfkKiSg@google.com>
Date: Wed, 9 Nov 2022 22:43:40 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Li Li <dualli@...omium.org>
Cc: dualli@...gle.com, gregkh@...uxfoundation.org, arve@...roid.com,
tkjos@...roid.com, maco@...roid.com, joel@...lfernandes.org,
brauner@...nel.org, surenb@...gle.com, arnd@...db.de,
masahiroy@...nel.org, devel@...verdev.osuosl.org,
linux-kernel@...r.kernel.org, hridya@...gle.com,
smoreland@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH v1 1/1] binder: return pending info for frozen async txns
On Thu, Nov 03, 2022 at 12:05:49PM -0700, Li Li wrote:
> From: Li Li <dualli@...gle.com>
>
> An async transaction to a frozen process will still be successsfully
nit: sucessfully typo
> put in the queue. But this pending async transaction won't be processed
> until the target process is unfrozen at an unspecified time in the
> future. Pass this important information back to the user space caller
> by returning BR_TRANSACTION_PENDING.
>
> Signed-off-by: Li Li <dualli@...gle.com>
> ---
> drivers/android/binder.c | 23 ++++++++++++++++++++---
> drivers/android/binder_internal.h | 3 ++-
> include/uapi/linux/android/binder.h | 7 ++++++-
> 3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 880224ec6abb..a097b89f6a7a 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2728,7 +2728,10 @@ binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
> *
> * Return: 0 if the transaction was successfully queued
> * BR_DEAD_REPLY if the target process or thread is dead
> - * BR_FROZEN_REPLY if the target process or thread is frozen
> + * BR_FROZEN_REPLY if the target process or thread is frozen and
> + * the sync transaction was rejected
> + * BR_TRANSACTION_PENDING if the target process is frozen and the
> + * async transaction was successfully queued
> */
> static int binder_proc_transaction(struct binder_transaction *t,
> struct binder_proc *proc,
> @@ -2807,6 +2810,9 @@ static int binder_proc_transaction(struct binder_transaction *t,
> binder_stats_deleted(BINDER_STAT_TRANSACTION);
> }
>
> + if (oneway && proc->is_frozen)
Do you need the inner lock here for proc->is_frozen?
> + return BR_TRANSACTION_PENDING;
> +
> return 0;
> }
>
> @@ -3607,9 +3613,16 @@ static void binder_transaction(struct binder_proc *proc,
> } else {
> BUG_ON(target_node == NULL);
> BUG_ON(t->buffer->async_transaction != 1);
> - binder_enqueue_thread_work(thread, tcomplete);
> return_error = binder_proc_transaction(t, target_proc, NULL);
> - if (return_error)
> + /*
> + * Let the caller know its async transaction reaches a frozen
nit: I believe you meant s/its/when or similar?
> + * process and is put in a pending queue, waiting for the target
> + * process to be unfrozen.
> + */
> + if (return_error == BR_TRANSACTION_PENDING)
> + tcomplete->type = BINDER_WORK_TRANSACTION_PENDING;
> + binder_enqueue_thread_work(thread, tcomplete);
I wonder if switching the order of queuing the tcomplete here and waking
up the target thread inside binder_proc_transaction() will have any
performance implications if this task gets scheduled out.
> + if (return_error && return_error != BR_TRANSACTION_PENDING)
> goto err_dead_proc_or_thread;
> }
> if (target_thread)
> @@ -4440,10 +4453,13 @@ static int binder_thread_read(struct binder_proc *proc,
> binder_stat_br(proc, thread, cmd);
> } break;
> case BINDER_WORK_TRANSACTION_COMPLETE:
> + case BINDER_WORK_TRANSACTION_PENDING:
> case BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT: {
> if (proc->oneway_spam_detection_enabled &&
> w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT)
> cmd = BR_ONEWAY_SPAM_SUSPECT;
> + else if (w->type == BINDER_WORK_TRANSACTION_PENDING)
> + cmd = BR_TRANSACTION_PENDING;
> else
> cmd = BR_TRANSACTION_COMPLETE;
> binder_inner_proc_unlock(proc);
> @@ -6170,6 +6186,7 @@ static const char * const binder_return_strings[] = {
> "BR_FAILED_REPLY",
> "BR_FROZEN_REPLY",
> "BR_ONEWAY_SPAM_SUSPECT",
> + "BR_TRANSACTION_PENDING"
> };
>
> static const char * const binder_command_strings[] = {
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index abe19d88c6ec..6c51325a826f 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -133,7 +133,7 @@ enum binder_stat_types {
> };
>
> struct binder_stats {
> - atomic_t br[_IOC_NR(BR_ONEWAY_SPAM_SUSPECT) + 1];
> + atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING) + 1];
> atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> atomic_t obj_created[BINDER_STAT_COUNT];
> atomic_t obj_deleted[BINDER_STAT_COUNT];
> @@ -152,6 +152,7 @@ struct binder_work {
> enum binder_work_type {
> BINDER_WORK_TRANSACTION = 1,
> BINDER_WORK_TRANSACTION_COMPLETE,
> + BINDER_WORK_TRANSACTION_PENDING,
> BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT,
> BINDER_WORK_RETURN_ERROR,
> BINDER_WORK_NODE,
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index e72e4de8f452..c21b3b3eb4e4 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -450,7 +450,7 @@ enum binder_driver_return_protocol {
>
> BR_FROZEN_REPLY = _IO('r', 18),
> /*
> - * The target of the last transaction (either a bcTRANSACTION or
> + * The target of the last sync transaction (either a bcTRANSACTION or
> * a bcATTEMPT_ACQUIRE) is frozen. No parameters.
> */
>
> @@ -460,6 +460,11 @@ enum binder_driver_return_protocol {
> * asynchronous transaction makes the allocated async buffer size exceed
> * detection threshold. No parameters.
> */
> +
> + BR_TRANSACTION_PENDING = _IO('r', 20),
> + /*
> + * The target of the last async transaction is frozen. No parameters.
> + */
> };
>
> enum binder_driver_command_protocol {
> --
> 2.38.1.431.g37b22c650d-goog
>
Powered by blists - more mailing lists