linux-kernel - Re: [PATCH v1] Binder: add TF_UPDATE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YoZnVrU8Dih+urv6@kroah.com>
Date:   Thu, 19 May 2022 17:50:46 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Li Li <dualli@...omium.org>
Cc:     dualli@...gle.com, tkjos@...gle.com, christian@...uner.io,
        arve@...roid.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, maco@...gle.com, hridya@...gle.com,
        surenb@...gle.com, joel@...lfernandes.org, kernel-team@...roid.com
Subject: Re: [PATCH v1] Binder: add TF_UPDATE_TXN

On Wed, May 18, 2022 at 05:06:23PM -0700, Li Li wrote:
> From: Li Li <dualli@...gle.com>

Note, your subject does not say what TF_UPDATE_TXN is, so it's a bit
hard to determine what is happening here.  Can you clean that up a bit
and sumarize what this new addition does?

> 
> When the target process is busy, incoming oneway transactions are
> queued in the async_todo list. If the clients continue sending extra
> oneway transactions while the target process is frozen, this queue can
> become too large to accommodate new transactions. That's why binder
> driver introduced ONEWAY_SPAM_DETECTION to detect this situation. It's
> helpful to debug the async binder buffer exhausting issue, but the
> issue itself isn't solved directly.
> 
> In real cases applications are designed to send oneway transactions
> repeatedly, delivering updated inforamtion to the target process.
> Typical examples are Wi-Fi signal strength and some real time sensor
> data. Even if the apps might only care about the lastet information,
> all outdated oneway transactions are still accumulated there until the
> frozen process is thawed later. For this kind of situations, there's
> no existing method to skip those outdated transactions and deliver the
> latest one only.
> 
> This patch introduces a new transaction flag TF_UPDATE_TXN. To use it,
> use apps can set this new flag along with TF_ONE_WAY. When such an
> oneway transaction is to be queued into the async_todo list of a frozen
> process, binder driver will check if any previous pending transactions
> can be superseded by comparing their code, flags and target node. If
> such an outdated pending transaction is found, the latest transaction
> will supersede that outdated one. This effectively prevents the async
> binder buffer running out and saves unnecessary binder read workloads.
> 
> Signed-off-by: Li Li <dualli@...gle.com>
> ---
>  drivers/android/binder.c            | 90 ++++++++++++++++++++++++++++-
>  drivers/android/binder_trace.h      |  4 ++
>  include/uapi/linux/android/binder.h |  1 +

How was this tested? 

>  3 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f3b639e89dd8..153486a32d69 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2594,6 +2594,60 @@ static int binder_fixup_parent(struct list_head *pf_head,
>  	return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0);
>  }
>  
> +/**
> + * binder_can_update_transaction() - Can a txn be superseded by an updated one?
> + * @t1: the pending async txn in the frozen process
> + * @t2: the new async txn to supersede the outdated pending one
> + *
> + * Return:  true if t2 can supersede t1
> + *          false if t2 can not supersede t1
> + */
> +static bool binder_can_update_transaction(struct binder_transaction *t1,
> +					  struct binder_transaction *t2)
> +{
> +	if ((t1->flags & t2->flags & (TF_ONE_WAY | TF_UPDATE_TXN))
> +			!= (TF_ONE_WAY | TF_UPDATE_TXN)
> +			|| t1->to_proc == NULL || t2->to_proc == NULL)
> +		return false;
> +	if (t1->to_proc->tsk == t2->to_proc->tsk && t1->code == t2->code
> +			&& t1->flags == t2->flags
> +			&& t1->buffer->pid == t2->buffer->pid
> +			&& t1->buffer->target_node->ptr
> +			== t2->buffer->target_node->ptr
> +			&& t1->buffer->target_node->cookie
> +			== t2->buffer->target_node->cookie)

Did checkpatch pass this?  Please always use --strict and fix up all the
issues that it reports as this is not a normal kernel coding style,
sorry.

thanks,

greg k-h