Re: [PATCH v2 1/1] binder: return pending info for frozen async txns
On Wed, Nov 23, 2022 at 10:58 AM Carlos Llamas wrote: > > This looks good. Could you rename the new op to signify the frozen > scenario though? We might have some other instances of pending > transactions in the future so might as well be specific here. Done [1]. v3: rename BR_TRANSACTION_PENDING to BR_TRANSACTION_PENDING_FROZEN to signify the frozen scenario and avoid potential confusion [1] https://lkml.org/lkml/2022/11/23/1472 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/1] binder: return pending info for frozen async txns
From: Li Li
An async transaction to a frozen process will still be successfully
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_FROZEN.
Signed-off-by: Li Li
---
drivers/android/binder.c| 32 +++--
drivers/android/binder_internal.h | 3 ++-
include/uapi/linux/android/binder.h | 7 ++-
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 880224ec6abb..acd53147d5d1 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_FROZEN 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,
@@ -2738,6 +2741,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
struct binder_transaction *t_outdated = NULL;
+ bool frozen = false;
BUG_ON(!node);
binder_node_lock(node);
@@ -2751,15 +2755,16 @@ static int binder_proc_transaction(struct
binder_transaction *t,
binder_inner_proc_lock(proc);
if (proc->is_frozen) {
+ frozen = true;
proc->sync_recv |= !oneway;
proc->async_recv |= oneway;
}
- if ((proc->is_frozen && !oneway) || proc->is_dead ||
+ if ((frozen && !oneway) || proc->is_dead ||
(thread && thread->is_dead)) {
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
+ return frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
}
if (!thread && !pending_async)
@@ -2770,7 +2775,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
} else if (!pending_async) {
binder_enqueue_work_ilocked(>work, >todo);
} else {
- if ((t->flags & TF_UPDATE_TXN) && proc->is_frozen) {
+ if ((t->flags & TF_UPDATE_TXN) && frozen) {
t_outdated = binder_find_outdated_transaction_ilocked(t,
>async_todo);
if (t_outdated) {
@@ -2807,6 +2812,9 @@ static int binder_proc_transaction(struct
binder_transaction *t,
binder_stats_deleted(BINDER_STAT_TRANSACTION);
}
+ if (oneway && frozen)
+ return BR_TRANSACTION_PENDING_FROZEN;
+
return 0;
}
@@ -3607,9 +3615,17 @@ 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 when async transaction reaches a frozen
+* process and is put in a pending queue, waiting for the target
+* process to be unfrozen.
+*/
+ if (return_error == BR_TRANSACTION_PENDING_FROZEN)
+ tcomplete->type = BINDER_WORK_TRANSACTION_PENDING;
+ binder_enqueue_thread_work(thread, tcomplete);
+ if (return_error &&
+ return_error != BR_TRANSACTION_PENDING_FROZEN)
goto err_dead_proc_or_thread;
}
if (target_thread)
@@ -4440,10 +4456,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_SU
[PATCH v3 0/1] binder: return pending info for frozen async txns
From: Li Li User applications need to know if their binder transactions reach a frozen process or not. For sync binder calls, Linux kernel already has a dedicated return value BR_FROZEN_REPLY, indicating this sync binder transaction will be rejected (similar to BR_DEAD_REPLY) as the target process is frozen. But for async binder calls, the user space application doesn't have a way to know if the target process is frozen. This patch adds a new return value, BR_TRANSACTION_PENDING_FROZEN, to fix this issue. Similar to BR_TRANSACTION_COMPLETE, it means the async binder transaction has been put in the queue of the target process, but it's waiting for the target process to be unfrozen. v1: checkpatch.pl --strict passed v2: protect proc->is_frozen with lock, fix typo in comments v3: rename BR_TRANSACTION_PENDING to BR_TRANSACTION_PENDING_FROZEN to signify the frozen scenario and avoid potential confusion Li Li (1): binder: return pending info for frozen async txns drivers/android/binder.c| 32 +++-- drivers/android/binder_internal.h | 3 ++- include/uapi/linux/android/binder.h | 7 ++- 3 files changed, 34 insertions(+), 8 deletions(-) -- 2.38.1.584.g0f3c55d4c2-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/1] binder: return pending info for frozen async txns
From: Li Li
An async transaction to a frozen process will still be successfully
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
---
drivers/android/binder.c| 31 +++--
drivers/android/binder_internal.h | 3 ++-
include/uapi/linux/android/binder.h | 7 ++-
3 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 880224ec6abb..a798f6661488 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,
@@ -2738,6 +2741,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
struct binder_transaction *t_outdated = NULL;
+ bool frozen = false;
BUG_ON(!node);
binder_node_lock(node);
@@ -2751,15 +2755,16 @@ static int binder_proc_transaction(struct
binder_transaction *t,
binder_inner_proc_lock(proc);
if (proc->is_frozen) {
+ frozen = true;
proc->sync_recv |= !oneway;
proc->async_recv |= oneway;
}
- if ((proc->is_frozen && !oneway) || proc->is_dead ||
+ if ((frozen && !oneway) || proc->is_dead ||
(thread && thread->is_dead)) {
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
+ return frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
}
if (!thread && !pending_async)
@@ -2770,7 +2775,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
} else if (!pending_async) {
binder_enqueue_work_ilocked(>work, >todo);
} else {
- if ((t->flags & TF_UPDATE_TXN) && proc->is_frozen) {
+ if ((t->flags & TF_UPDATE_TXN) && frozen) {
t_outdated = binder_find_outdated_transaction_ilocked(t,
>async_todo);
if (t_outdated) {
@@ -2807,6 +2812,9 @@ static int binder_proc_transaction(struct
binder_transaction *t,
binder_stats_deleted(BINDER_STAT_TRANSACTION);
}
+ if (oneway && frozen)
+ return BR_TRANSACTION_PENDING;
+
return 0;
}
@@ -3607,9 +3615,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 when async transaction reaches a frozen
+* 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);
+ if (return_error && return_error != BR_TRANSACTION_PENDING)
goto err_dead_proc_or_thread;
}
if (target_thread)
@@ -4440,10 +4455,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)
[PATCH v2 0/1] binder: return pending info for frozen async txns
From: Li Li User applications need to know if their binder transactions reach a frozen process or not. For sync binder calls, Linux kernel already has a dedicated return value BR_FROZEN_REPLY, indicating this sync binder transaction will be rejected (similar to BR_DEAD_REPLY) as the target process is frozen. But for async binder calls, the user space application doesn't have a way to know if the target process is frozen. This patch adds a new return value, BR_TRANSACTION_PENDING, to fix this issue. Similar to BR_TRANSACTION_COMPLETE, it means the async binder transaction has been put in the queue of the target process, but it's waiting for the target process to be unfrozen. v1: checkpatch.pl --strict passed v2: protect proc->is_frozen with lock, fix typo in comments Li Li (1): binder: return pending info for frozen async txns drivers/android/binder.c| 31 +++-- drivers/android/binder_internal.h | 3 ++- include/uapi/linux/android/binder.h | 7 ++- 3 files changed, 33 insertions(+), 8 deletions(-) -- 2.38.1.431.g37b22c650d-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/1] binder: return pending info for frozen async txns
On Wed, Nov 9, 2022 at 2:43 PM Carlos Llamas wrote:
>
> On Thu, Nov 03, 2022 at 12:05:49PM -0700, Li Li wrote:
> > From: Li Li
> >
> > An async transaction to a frozen process will still be successsfully
>
> nit: sucessfully typo
Nice catch! Will remove the extra 's' in v2.
>
> > 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
> > ---
> > 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?
You're right. I'll add a new local variable and set it between the existing
binder_inner_proc_lock() and binder_inner_proc_unlock().
>
> > + 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?
Will change it in v2. Thanks!
>
> > + * 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.
Eventually the sender has to wait this whole function finish and then
call another
ioctl to actually read tcomplete back. If this task gets cheduled out,
it won't have
a chance to perform that reading operation even without this change. So there's
no difference.
>
> > + 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;
>
[PATCH v1 1/1] binder: return pending info for frozen async txns
From: Li Li
An async transaction to a frozen process will still be successsfully
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
---
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)
+ 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
+* 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);
+ 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/inclu
[PATCH v1 0/1] binder: return pending info for frozen async txns
From: Li Li User applications need to know if their binder transactions reach a frozen process or not. For sync binder calls, Linux kernel already has a dedicated return value BR_FROZEN_REPLY, indicating this sync binder transaction will be rejected (similar to BR_DEAD_REPLY) as the target process is frozen. But for async binder calls, the user space application doesn't have a way to know if the target process is frozen. This patch add a new return value, BR_TRANSACTION_PENDING, to fix this issue. Similar to BR_TRANSACTION_COMPLETE, it means the async binder transaction has been put in the queue of the target process, but it's waiting for the target process to be unfrozen. v1: checkpatch.pl --strict passed Li Li (1): binder: return pending info for frozen async txns 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(-) -- 2.38.1.431.g37b22c650d-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
On Thu, May 26, 2022 at 10:50 PM Greg KH wrote: > > On Thu, May 26, 2022 at 03:00:17PM -0700, Li Li wrote: > > From: Li Li > > > > Resend [Patch v3] with cover letter in case my previous email failed > > to reach the maillist (no comments for 2 weeks). > > > > The previous comments of the old patch can be found at the following link: > > https://lore.kernel.org/lkml/canbpypjknwso94nug1tkr1dgk2w2kbxijtriyvb7s3czhtz...@mail.gmail.com/ > > > > I copy and paste the key information here for your convenience. > > > > * Question #1 > > > > 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? > > How was this tested? > > > > * Answer #1 === > > > > A more descriptive summary has been added to the new version of patch. > > > > * Question #2 > > > > How was this tested? > > > > * Answer #2 > > > > Old kernel: without this TF_UPDATE_TXN patch > > New kernel: with this TF_UPDATE_TXN patch > > Old apps: without setting TF_UPDATE_TXN > > New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN; > > > > 1. Compatibility: New kernel + Old apps, to verify the original > > behavior doesn't change; > > > > 2. Compatibility: Old kernel + New apps, to verify the original > > behavior doesn't change; > > > > 3. Unit test: New kernel + New apps, to verify the outdated oneway > > binder transaction is actually superseded by the latest one (by > > enabling BINDER_DEBUG logs); > > > > 4. Stress test: New kernel + New apps sending oneway binder > > transactions repeatedly, to verify the size of the available async > > binder buffer over time, and if the transactions fail as before > > (due to async buffer running out). > > > > * Question #3 > > > > 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. > > > > * Answer #3 > > > > Yes, the latest version has passed "./scripts/checkpatch.pl --strict" > > > > * Changelog > > > > v3: > > - Add this changelog required by "The canonical patch format" > > v2: > > - Fix alignment warnings reported by checkpatch --strict > > - Add descriptive summary in patch subject > > > > Li Li (1): > > Binder: add TF_UPDATE_TXN to replace outdated txn > > > > drivers/android/binder.c| 85 - > > drivers/android/binder_trace.h | 4 ++ > > include/uapi/linux/android/binder.h | 1 + > > 3 files changed, 87 insertions(+), 3 deletions(-) > > > > -- > > 2.36.1.124.g0e6072fb45-goog > > > > ___ > > devel mailing list > > de...@linuxdriverproject.org > > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel > > > Hi, > > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman. > You have sent him a patch that has triggered this response. > > Right now, the development tree you have sent a patch for is "closed" > due to the timing of the merge window. Don't worry, the patch(es) you > have sent are not lost, and will be looked at after the merge window is > over (after the -rc1 kernel is released by Linus). > > So thank you for your patience and your patches will be reviewed at this > later time, you do not have to do anything further, this is just a short > note to let you know the patch status and so you don't worry they didn't > make it through. Hi Greg and all reviewers, The rc-1 has been released for some days. Do I need to resend the patch v3 [1] again to the maillist? Please let me know what I should do next to have it reviewed. Thanks! [1]: [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn https://lore.kernel.org/lkml/20220526220018.3334775-1-dua...@chromium.org/ > > thanks, > > greg k-h's patch email bot ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RESEND PATCH v3 1/1] Binder: add TF_UPDATE_TXN to replace outdated txn
From: Li Li
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
---
v3:
- Add this changelog required by "The canonical patch format"
v2:
- Fix alignment warnings reported by checkpatch --strict
- Add descriptive summary in patch subject
drivers/android/binder.c| 85 -
drivers/android/binder_trace.h | 4 ++
include/uapi/linux/android/binder.h | 1 +
3 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f3b639e89dd8..bb968cf2f9ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2594,6 +2594,56 @@ 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 || !t2->to_proc)
+ 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)
+ return true;
+ return false;
+}
+
+/**
+ * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
+ * @t: new async transaction
+ * @target_list: list to find outdated transaction
+ *
+ * Return: the outdated transaction if found
+ * NULL if no outdated transacton can be found
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static struct binder_transaction *
+binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
+struct list_head *target_list)
+{
+ struct binder_work *w;
+
+ list_for_each_entry(w, target_list, entry) {
+ struct binder_transaction *t_queued;
+
+ if (w->type != BINDER_WORK_TRANSACTION)
+ continue;
+ t_queued = container_of(w, struct binder_transaction, work);
+ if (binder_can_update_transaction(t_queued, t))
+ return t_queued;
+ }
+ return NULL;
+}
+
/**
* binder_proc_transaction() - sends a transaction to a process and wakes it up
* @t: transaction to send
@@ -2619,6 +2669,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
struct binder_node *node = t->buffer->target_node;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
+ struct binder_transaction *t_outdated = NULL;
BUG_ON(!node);
binder_node_lock(node);
@@ -2646,12 +2697,24 @@ static int binder_proc_transaction(struct
binder_transaction *t,
if (!thread && !pending_asyn
[RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
From: Li Li Resend [Patch v3] with cover letter in case my previous email failed to reach the maillist (no comments for 2 weeks). The previous comments of the old patch can be found at the following link: https://lore.kernel.org/lkml/canbpypjknwso94nug1tkr1dgk2w2kbxijtriyvb7s3czhtz...@mail.gmail.com/ I copy and paste the key information here for your convenience. * Question #1 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? How was this tested? * Answer #1 === A more descriptive summary has been added to the new version of patch. * Question #2 How was this tested? * Answer #2 Old kernel: without this TF_UPDATE_TXN patch New kernel: with this TF_UPDATE_TXN patch Old apps: without setting TF_UPDATE_TXN New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN; 1. Compatibility: New kernel + Old apps, to verify the original behavior doesn't change; 2. Compatibility: Old kernel + New apps, to verify the original behavior doesn't change; 3. Unit test: New kernel + New apps, to verify the outdated oneway binder transaction is actually superseded by the latest one (by enabling BINDER_DEBUG logs); 4. Stress test: New kernel + New apps sending oneway binder transactions repeatedly, to verify the size of the available async binder buffer over time, and if the transactions fail as before (due to async buffer running out). * Question #3 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. * Answer #3 Yes, the latest version has passed "./scripts/checkpatch.pl --strict" * Changelog v3: - Add this changelog required by "The canonical patch format" v2: - Fix alignment warnings reported by checkpatch --strict - Add descriptive summary in patch subject Li Li (1): Binder: add TF_UPDATE_TXN to replace outdated txn drivers/android/binder.c| 85 - drivers/android/binder_trace.h | 4 ++ include/uapi/linux/android/binder.h | 1 + 3 files changed, 87 insertions(+), 3 deletions(-) -- 2.36.1.124.g0e6072fb45-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] Binder: add TF_UPDATE_TXN to replace outdated txn
From: Li Li
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
---
v3:
- Add this changelog required by "The canonical patch format"
v2:
- Fix alignment warnings reported by checkpatch --strict
- Add descriptive summary in patch subject
drivers/android/binder.c| 85 -
drivers/android/binder_trace.h | 4 ++
include/uapi/linux/android/binder.h | 1 +
3 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f3b639e89dd8..bb968cf2f9ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2594,6 +2594,56 @@ 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 || !t2->to_proc)
+ 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)
+ return true;
+ return false;
+}
+
+/**
+ * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
+ * @t: new async transaction
+ * @target_list: list to find outdated transaction
+ *
+ * Return: the outdated transaction if found
+ * NULL if no outdated transacton can be found
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static struct binder_transaction *
+binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
+struct list_head *target_list)
+{
+ struct binder_work *w;
+
+ list_for_each_entry(w, target_list, entry) {
+ struct binder_transaction *t_queued;
+
+ if (w->type != BINDER_WORK_TRANSACTION)
+ continue;
+ t_queued = container_of(w, struct binder_transaction, work);
+ if (binder_can_update_transaction(t_queued, t))
+ return t_queued;
+ }
+ return NULL;
+}
+
/**
* binder_proc_transaction() - sends a transaction to a process and wakes it up
* @t: transaction to send
@@ -2619,6 +2669,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
struct binder_node *node = t->buffer->target_node;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
+ struct binder_transaction *t_outdated = NULL;
BUG_ON(!node);
binder_node_lock(node);
@@ -2646,12 +2697,24 @@ static int binder_proc_transaction(struct
binder_transaction *t,
if (!thread && !pending_asyn
[PATCH v2] Binder: add TF_UPDATE_TXN to replace outdated txn
From: Li Li
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
---
drivers/android/binder.c| 85 -
drivers/android/binder_trace.h | 4 ++
include/uapi/linux/android/binder.h | 1 +
3 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f3b639e89dd8..bb968cf2f9ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2594,6 +2594,56 @@ 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 || !t2->to_proc)
+ 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)
+ return true;
+ return false;
+}
+
+/**
+ * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
+ * @t: new async transaction
+ * @target_list: list to find outdated transaction
+ *
+ * Return: the outdated transaction if found
+ * NULL if no outdated transacton can be found
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static struct binder_transaction *
+binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
+struct list_head *target_list)
+{
+ struct binder_work *w;
+
+ list_for_each_entry(w, target_list, entry) {
+ struct binder_transaction *t_queued;
+
+ if (w->type != BINDER_WORK_TRANSACTION)
+ continue;
+ t_queued = container_of(w, struct binder_transaction, work);
+ if (binder_can_update_transaction(t_queued, t))
+ return t_queued;
+ }
+ return NULL;
+}
+
/**
* binder_proc_transaction() - sends a transaction to a process and wakes it up
* @t: transaction to send
@@ -2619,6 +2669,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
struct binder_node *node = t->buffer->target_node;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
+ struct binder_transaction *t_outdated = NULL;
BUG_ON(!node);
binder_node_lock(node);
@@ -2646,12 +2697,24 @@ static int binder_proc_transaction(struct
binder_transaction *t,
if (!thread && !pending_async)
thread = binder_select_thread_ilocked(proc);
- if (thread)
+ if (thread) {
binder_enqueue_thread_work_ilocked(thread, >work);
- else if
Re: [PATCH v1] Binder: add TF_UPDATE_TXN
On Thu, May 19, 2022 at 8:50 AM Greg KH wrote:
>
> On Wed, May 18, 2022 at 05:06:23PM -0700, Li Li wrote:
> > From: Li Li
>
> 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?
Sure, I'll add a brief summary there.
>
> >
> > 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
> > ---
> > drivers/android/binder.c| 90 -
> > drivers/android/binder_trace.h | 4 ++
> > include/uapi/linux/android/binder.h | 1 +
>
> How was this tested?
Old kernel: without this TF_UPDATE_TXN patch
New kernel: with this TF_UPDATE_TXN patch
Old apps: without setting TF_UPDATE_TXN
New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN;
1. Compatibility: New kernel + Old apps, to verify the original
behavior doesn't change;
2. Compatibility: Old kernel + New apps, to verify the original
behavior doesn't change;
3. Unit test: New kernel + New apps, to verify the outdated oneway
binder transaction is actually superseded by the latest one (by
enabling BINDER_DEBUG logs);
4. Stress test: New kernel + New apps sending oneway binder
transactions repeatedly, to verify the size of the available async
binder buffer over time, and if the transactions fail as before
(due to async buffer running out).
>
> > 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
> > + &&
[PATCH v1] Binder: add TF_UPDATE_TXN
From: Li Li
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
---
drivers/android/binder.c| 90 -
drivers/android/binder_trace.h | 4 ++
include/uapi/linux/android/binder.h | 1 +
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)
+ return true;
+ return false;
+}
+
+/**
+ * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
+ * @t: new async transaction
+ * @target_list: list to find outdated transaction
+ *
+ * Return: the outdated transaction if found
+ * NULL if no outdated transacton can be found
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static struct binder_transaction *
+binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
+struct list_head *target_list)
+{
+ struct binder_work *w;
+
+ list_for_each_entry(w, target_list, entry) {
+ struct binder_transaction *t_queued;
+
+ if (w->type != BINDER_WORK_TRANSACTION)
+ continue;
+ t_queued = container_of(w, struct binder_transaction, work);
+ if (binder_can_update_transaction(t_queued, t))
+ return t_queued;
+ }
+ return NULL;
+}
+
/**
* binder_proc_transaction() - sends a transaction to a process and wakes it up
* @t: transaction to send
@@ -2619,6 +2673,7 @@ static int binder_proc_transaction(struct
binder_transaction *t,
struct binder_node *node = t->buffer->target_node;
bool oneway = !!(t->flags & TF_ONE_WAY);
bool pending_async = false;
+ struct binder_transaction *t_outdated = NULL;
BUG_ON(!node);
binder_node_lock(node);
@@ -2646,12 +2701,25 @@ static int binder_proc_transaction(struct
binder_transaction *t,
if (!thread && !pending_async)
thread
Re: [PATCH v2 1/1] binder: fix freeze race
On Fri, Sep 10, 2021 at 12:15 AM Greg KH wrote:
>
> On Thu, Sep 09, 2021 at 11:17:42PM -0700, Li Li wrote:
> > On Thu, Sep 9, 2021 at 10:38 PM Greg KH wrote:
> > >
> > > On Thu, Sep 09, 2021 at 08:53:16PM -0700, Li Li wrote:
> > > > struct binder_frozen_status_info {
> > > > __u32pid;
> > > > +
> > > > + /* process received sync transactions since last frozen
> > > > + * bit 0: received sync transaction after being frozen
> > > > + * bit 1: new pending sync transaction during freezing
> > > > + */
> > > > __u32sync_recv;
> > >
> > > You just changed a user/kernel api here, did you just break existing
> > > userspace applications? If not, how did that not happen?
> > >
> >
> > That's a good question. This design does keep backward compatibility.
> >
> > The existing userspace applications call ioctl(BINDER_GET_FROZEN_INFO)
> > to check if there's sync or async binder transactions sent to a frozen
> > process.
> >
> > If the existing userspace app runs on a new kernel, a sync binder call
> > still sets bit 1 of sync_recv (as it's a bool variable) so the ioctl
> > will return the expected value (TRUE). The app just doesn't check bit
> > 1 intentionally so it doesn't have the ability to tell if there's a
> > race - this behavior is aligned with what happens on an old kernel as
> > the old kernel doesn't have bit 1 set at all.
> >
> > The bit 1 of sync_recv enables new userspace app the ability to tell
> > 1) there's a sync binder transaction happened when being frozen - same
> > as before; and 2) if that sync binder transaction happens exactly when
> > there's a race - a new information for rollback decision.
>
> Ah, can you add that to the changelog text to make it more obvious?
>
Sure, added that to V3, plus other minor improvements listed in the cover
letter. Please let me know if there's anything else I should continue
to improve.
https://lore.kernel.org/lkml/20210910164210.2282716-1-dua...@chromium.org/
BTW, I had a stress test running, repeatedly freezing and unfreezing a
couple apps every second, which at the same time initiates new binder
transactions in a loop. The overnight stress test during the past
weekend showed positive results. Without this kernel patch, the reply
transaction will fail in tens of iterations. With this kernel patch
and the corresponding user space fix (rescheduling the freeze op to
next second in case race happens), the stress test runs for 24hrs
without a single failure.
Thanks,
Li
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/1] binder: fix freeze race
From: Li Li
Currently cgroup freezer is used to freeze the application threads, and
BINDER_FREEZE is used to freeze the corresponding binder interface.
There's already a mechanism in ioctl(BINDER_FREEZE) to wait for any
existing transactions to drain out before actually freezing the binder
interface.
But freezing an app requires 2 steps, freezing the binder interface with
ioctl(BINDER_FREEZE) and then freezing the application main threads with
cgroupfs. This is not an atomic operation. The following race issue
might happen.
1) Binder interface is frozen by ioctl(BINDER_FREEZE);
2) Main thread A initiates a new sync binder transaction to process B;
3) Main thread A is frozen by "echo 1 > cgroup.freeze";
4) The response from process B reaches the frozen thread, which will
unexpectedly fail.
This patch provides a mechanism to check if there's any new pending
transaction happening between ioctl(BINDER_FREEZE) and freezing the
main thread. If there's any, the main thread freezing operation can
be rolled back to finish the pending transaction.
Furthermore, the response might reach the binder driver before the
rollback actually happens. That will still cause failed transaction.
As the other process doesn't wait for another response of the response,
the response transaction failure can be fixed by treating the response
transaction like an oneway/async one, allowing it to reach the frozen
thread. And it will be consumed when the thread gets unfrozen later.
NOTE: This patch reuses the existing definition of struct
binder_frozen_status_info but expands the bit assignments of __u32
member sync_recv.
To ensure backward compatibility, bit 0 of sync_recv still indicates
there's an outstanding sync binder transaction. This patch adds new
information to bit 1 of sync_recv, indicating the binder transaction
happens exactly when there's a race.
If an existing userspace app runs on a new kernel, a sync binder call
will set bit 0 of sync_recv so ioctl(BINDER_GET_FROZEN_INFO) still
return the expected value (true). The app just doesn't check bit 1
intentionally so it doesn't have the ability to tell if there's a race.
This behavior is aligned with what happens on an old kernel which
doesn't set bit 1 at all.
A new userspace app can 1) check bit 0 to know if there's a sync binder
transaction happened when being frozen - same as before; and 2) check
bit 1 to know if that sync binder transaction happened exactly when
there's a race - a new information for rollback decision.
Fixes: 432ff1e91694 ("binder: BINDER_FREEZE ioctl")
Test: stress test with apps being frozen and initiating binder calls at
the same time, confirmed the pending transactions succeeded.
Signed-off-by: Li Li
---
drivers/android/binder.c| 35 -
drivers/android/binder_internal.h | 2 ++
include/uapi/linux/android/binder.h | 7 ++
3 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d9030cb6b1e4..1a68c2f590cf 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3038,9 +3038,8 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead || target_proc->is_frozen) {
- return_error = target_thread->is_dead ?
- BR_DEAD_REPLY : BR_FROZEN_REPLY;
+ if (target_thread->is_dead) {
+ return_error = BR_DEAD_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
@@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
binder_proc *proc,
return 0;
}
+static bool binder_txns_pending_ilocked(struct binder_proc *proc)
+{
+ struct rb_node *n;
+ struct binder_thread *thread;
+
+ if (proc->outstanding_txns > 0)
+ return true;
+
+ for (n = rb_first(>threads); n; n = rb_next(n)) {
+ thread = rb_entry(n, struct binder_thread, rb_node);
+ if (thread->transaction_stack)
+ return true;
+ }
+ return false;
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -4679,8 +4694,13 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
(!target_proc->outstanding_txns),
msecs_to_jiffies(info->timeout_ms));
- if (!ret && target_proc->outstanding_txns)
- ret = -EAGAIN;
+ /* Check pending transactions that wait for reply */
+ if (ret >= 0) {
+ binder_inner_proc_lock(target_proc);
+ if (binder_txns_
[PATCH v3 0/1] binder: fix freeze race
From: Li Li As there isn't an atomic operation to freeze the main thread and binder interface together, it's possible the main thread initiates a new binder transaction while the binder interfaces are already frozen. This race issue will result in failed binder transaction and unexpectedly crash the app. This patch allows a post-froze rollback mechanism by checking if there's any new pending binder transaction waiting for response. At the same time, it treats the response transaction like an oneway transaction so that the response can successfully reach the frozen process. Changes in v2: 1. Improve commit msg, adding "Fixes"; 2. Adding missing "_ilocked" suffix to binder_txns_pending(); 3. Document bit assignment of struct binder_frozen_status_info in binder.h. Changes in v3: 1. Make function binder_txns_pending() bool; 2. Remove redundant outstanding_txns check in binder_ioctl_freeze; 3. Change local variable txns_pending from int to __u32 for alignment; 4. Clarify uapi backward compatibility in commit msg. Li Li (1): binder: fix freeze race drivers/android/binder.c| 35 - drivers/android/binder_internal.h | 2 ++ include/uapi/linux/android/binder.h | 7 ++ 3 files changed, 38 insertions(+), 6 deletions(-) -- 2.33.0.309.g3052b89438-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/1] binder: fix freeze race
On Thu, Sep 9, 2021 at 11:03 PM Dan Carpenter wrote:
>
> On Thu, Sep 09, 2021 at 04:21:41PM -0700, Li Li wrote:
> > @@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
> > binder_proc *proc,
> > return 0;
> > }
> >
> > +static int binder_txns_pending(struct binder_proc *proc)
> > +{
> > + struct rb_node *n;
> > + struct binder_thread *thread;
> > +
> > + if (proc->outstanding_txns > 0)
> > + return 1;
>
> Make this function bool.
Will include the change (as well as the extra ->outstanding_txns
check) in the next revision.
>
> > +
> > + for (n = rb_first(>threads); n; n = rb_next(n)) {
> > + thread = rb_entry(n, struct binder_thread, rb_node);
> > + if (thread->transaction_stack)
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > static int binder_ioctl_freeze(struct binder_freeze_info *info,
> > struct binder_proc *target_proc)
> > {
> > @@ -4682,6 +4697,14 @@ static int binder_ioctl_freeze(struct
> > binder_freeze_info *info,
> > if (!ret && target_proc->outstanding_txns)
> > ret = -EAGAIN;
>
> These two lines can be deleted now because binder_txns_pending() checks
> ->outstanding_txns.
>
Thanks,
Li
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/1] binder: fix freeze race
On Thu, Sep 9, 2021 at 10:38 PM Greg KH wrote:
>
> On Thu, Sep 09, 2021 at 08:53:16PM -0700, Li Li wrote:
> > struct binder_frozen_status_info {
> > __u32pid;
> > +
> > + /* process received sync transactions since last frozen
> > + * bit 0: received sync transaction after being frozen
> > + * bit 1: new pending sync transaction during freezing
> > + */
> > __u32sync_recv;
>
> You just changed a user/kernel api here, did you just break existing
> userspace applications? If not, how did that not happen?
>
That's a good question. This design does keep backward compatibility.
The existing userspace applications call ioctl(BINDER_GET_FROZEN_INFO)
to check if there's sync or async binder transactions sent to a frozen
process.
If the existing userspace app runs on a new kernel, a sync binder call
still sets bit 1 of sync_recv (as it's a bool variable) so the ioctl
will return the expected value (TRUE). The app just doesn't check bit
1 intentionally so it doesn't have the ability to tell if there's a
race - this behavior is aligned with what happens on an old kernel as
the old kernel doesn't have bit 1 set at all.
The bit 1 of sync_recv enables new userspace app the ability to tell
1) there's a sync binder transaction happened when being frozen - same
as before; and 2) if that sync binder transaction happens exactly when
there's a race - a new information for rollback decision.
> thanks,
>
> greg k-h
Thanks,
Li
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/1] binder: fix freeze race
From: Li Li
Currently cgroup freezer is used to freeze the application threads, and
BINDER_FREEZE is used to freeze the corresponding binder interface.
There's already a mechanism in ioctl(BINDER_FREEZE) to wait for any
existing transactions to drain out before actually freezing the binder
interface.
But freezing an app requires 2 steps, freezing the binder interface with
ioctl(BINDER_FREEZE) and then freezing the application main threads with
cgroupfs. This is not an atomic operation. The following race issue
might happen.
1) Binder interface is frozen by ioctl(BINDER_FREEZE);
2) Main thread A initiates a new sync binder transaction to process B;
3) Main thread A is frozen by "echo 1 > cgroup.freeze";
4) The response from process B reaches the frozen thread, which will
unexpectedly fail.
This patch provides a mechanism to check if there's any new pending
transaction happening between ioctl(BINDER_FREEZE) and freezing the
main thread. If there's any, the main thread freezing operation can
be rolled back to finish the pending transaction.
Furthermore, the response might reach the binder driver before the
rollback actually happens. That will still cause failed transaction.
As the other process doesn't wait for another response of the response,
the response transaction failure can be fixed by treating the response
transaction like an oneway/async one, allowing it to reach the frozen
thread. And it will be consumed when the thread gets unfrozen later.
Fixes: 432ff1e91694 ("binder: BINDER_FREEZE ioctl")
Signed-off-by: Li Li
---
drivers/android/binder.c| 34 +
drivers/android/binder_internal.h | 2 ++
include/uapi/linux/android/binder.h | 7 ++
3 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d9030cb6b1e4..eaffdf5f692c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3038,9 +3038,8 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead || target_proc->is_frozen) {
- return_error = target_thread->is_dead ?
- BR_DEAD_REPLY : BR_FROZEN_REPLY;
+ if (target_thread->is_dead) {
+ return_error = BR_DEAD_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
@@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
binder_proc *proc,
return 0;
}
+static int binder_txns_pending_ilocked(struct binder_proc *proc)
+{
+ struct rb_node *n;
+ struct binder_thread *thread;
+
+ if (proc->outstanding_txns > 0)
+ return 1;
+
+ for (n = rb_first(>threads); n; n = rb_next(n)) {
+ thread = rb_entry(n, struct binder_thread, rb_node);
+ if (thread->transaction_stack)
+ return 1;
+ }
+ return 0;
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -4682,6 +4697,14 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
if (!ret && target_proc->outstanding_txns)
ret = -EAGAIN;
+ /* Also check pending transactions that wait for reply */
+ if (ret >= 0) {
+ binder_inner_proc_lock(target_proc);
+ if (binder_txns_pending_ilocked(target_proc))
+ ret = -EAGAIN;
+ binder_inner_proc_unlock(target_proc);
+ }
+
if (ret < 0) {
binder_inner_proc_lock(target_proc);
target_proc->is_frozen = false;
@@ -4696,6 +4719,7 @@ static int binder_ioctl_get_freezer_info(
{
struct binder_proc *target_proc;
bool found = false;
+ int txns_pending;
info->sync_recv = 0;
info->async_recv = 0;
@@ -4705,7 +4729,9 @@ static int binder_ioctl_get_freezer_info(
if (target_proc->pid == info->pid) {
found = true;
binder_inner_proc_lock(target_proc);
- info->sync_recv |= target_proc->sync_recv;
+ txns_pending = binder_txns_pending_ilocked(target_proc);
+ info->sync_recv |= target_proc->sync_recv |
+ (txns_pending << 1);
info->async_recv |= target_proc->async_recv;
binder_inner_proc_unlock(target_proc);
}
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 810c0b84d3f8..402c4d4362a8 100644
---
[PATCH v2 0/1] binder: fix freeze race
From: Li Li As there isn't an atomic operation to freeze the main thread and binder interface together, it's possible the main thread initiates a new binder transaction while the binder interfaces are already frozen. This race issue will result in failed binder transaction and unexpectedly crash the app. This patch allows a post-froze rollback mechanism by checking if there's any new pending binder transaction waiting for response. At the same time, it treats the response transaction like an oneway transaction so that the response can successfully reach the frozen process. Changes in v2: 1. Improve commit msg, adding "Fixes" 2. Adding missing "_ilocked" suffix to binder_txns_pending() 3. Document bit assignment of struct binder_frozen_status_info in binder.h Li Li (1): binder: fix freeze race drivers/android/binder.c| 34 + drivers/android/binder_internal.h | 2 ++ include/uapi/linux/android/binder.h | 7 ++ 3 files changed, 39 insertions(+), 4 deletions(-) -- 2.33.0.309.g3052b89438-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/1] binder: fix freeze race
Hi Todd,
Thanks for reviewing the patch! Please see my reply below.
And I'll send out v2 soon addressing your concerns.
On Thu, Sep 9, 2021 at 4:54 PM Todd Kjos wrote:
>
> On Thu, Sep 9, 2021 at 4:21 PM Li Li wrote:
> >
> > From: Li Li
> >
> > Currently cgroup freezer is used to freeze the application threads, and
> > BINDER_FREEZE is used to freeze binder interface. There's already a
> > mechanism for BINDER_FREEZE to wait for any existing transactions to
> > drain out before actually freezing the binder interface.
> >
> > But freezing an app requires 2 steps, freezing the binder interface with
> > BINDER_FREEZEwhich and then freezing the application main threads with
> > cgroupfs. This is not an atomic operation. The following race issue
> > might happen.
> >
> > 1) Binder interface is frozen by ioctl(BINDER_FREEZE);
> > 2) Main thread initiates a new sync binder transaction;
> > 3) Main thread is frozen by "echo 1 > cgroup.freeze";
> > 4) The response reaches the frozen thread, which will unexpectedly fail.
> >
> > This patch provides a mechanism for user space freezer manager to check
> > if there's any new pending transaction happening between BINDER_FREEZE
> > and freezing main thread. If there's any, the main thread freezing
> > operation can be rolled back.
> >
> > Furthermore, the response might reach the binder driver before the
> > rollback actually happens. That will also cause failed transaction.
> >
> > As the other process doesn't wait for another response of the response,
> > the failed response transaction can be fixed by treating the response
> > transaction like an oneway/async one, allowing it to reach the frozen
> > thread. And it will be consumed when the thread gets unfrozen later.
> >
> > Bug: 198493121
>
> Please remove "Bug: " tag. Replace it with a "Fixes:" tag that cites
> the patch that introduced the race.
>
Done in v2.
> > Signed-off-by: Li Li
> > ---
> > drivers/android/binder.c | 32 +++
> > drivers/android/binder_internal.h | 2 ++
> > 2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index d9030cb6b1e4..f9713a97578c 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -3038,9 +3038,8 @@ static void binder_transaction(struct binder_proc
> > *proc,
> > if (reply) {
> > binder_enqueue_thread_work(thread, tcomplete);
> > binder_inner_proc_lock(target_proc);
> > - if (target_thread->is_dead || target_proc->is_frozen) {
> > - return_error = target_thread->is_dead ?
> > - BR_DEAD_REPLY : BR_FROZEN_REPLY;
> > + if (target_thread->is_dead) {
> > + return_error = BR_DEAD_REPLY;
> > binder_inner_proc_unlock(target_proc);
> > goto err_dead_proc_or_thread;
> > }
> > @@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
> > binder_proc *proc,
> > return 0;
> > }
> >
> > +static int binder_txns_pending(struct binder_proc *proc)
> > +{
> > + struct rb_node *n;
> > + struct binder_thread *thread;
> > +
> > + if (proc->outstanding_txns > 0)
> > + return 1;
> > +
> > + for (n = rb_first(>threads); n; n = rb_next(n)) {
> > + thread = rb_entry(n, struct binder_thread, rb_node);
> > + if (thread->transaction_stack)
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > static int binder_ioctl_freeze(struct binder_freeze_info *info,
> >struct binder_proc *target_proc)
> > {
> > @@ -4682,6 +4697,14 @@ static int binder_ioctl_freeze(struct
> > binder_freeze_info *info,
> > if (!ret && target_proc->outstanding_txns)
> > ret = -EAGAIN;
> >
> > + /* Also check pending transactions that wait for reply */
> > + if (ret >= 0) {
> > + binder_inner_proc_lock(target_proc);
> > + if (binder_txns_pending(target_proc))
>
> The convention in the binder driver is to append "_ilocked" to the
> function name if the function expects the inner proc lock to be held
> (so "binder_txns_pending_i
[PATCH v1 1/1] binder: fix freeze race
From: Li Li
Currently cgroup freezer is used to freeze the application threads, and
BINDER_FREEZE is used to freeze binder interface. There's already a
mechanism for BINDER_FREEZE to wait for any existing transactions to
drain out before actually freezing the binder interface.
But freezing an app requires 2 steps, freezing the binder interface with
BINDER_FREEZEwhich and then freezing the application main threads with
cgroupfs. This is not an atomic operation. The following race issue
might happen.
1) Binder interface is frozen by ioctl(BINDER_FREEZE);
2) Main thread initiates a new sync binder transaction;
3) Main thread is frozen by "echo 1 > cgroup.freeze";
4) The response reaches the frozen thread, which will unexpectedly fail.
This patch provides a mechanism for user space freezer manager to check
if there's any new pending transaction happening between BINDER_FREEZE
and freezing main thread. If there's any, the main thread freezing
operation can be rolled back.
Furthermore, the response might reach the binder driver before the
rollback actually happens. That will also cause failed transaction.
As the other process doesn't wait for another response of the response,
the failed response transaction can be fixed by treating the response
transaction like an oneway/async one, allowing it to reach the frozen
thread. And it will be consumed when the thread gets unfrozen later.
Bug: 198493121
Signed-off-by: Li Li
---
drivers/android/binder.c | 32 +++
drivers/android/binder_internal.h | 2 ++
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d9030cb6b1e4..f9713a97578c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3038,9 +3038,8 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead || target_proc->is_frozen) {
- return_error = target_thread->is_dead ?
- BR_DEAD_REPLY : BR_FROZEN_REPLY;
+ if (target_thread->is_dead) {
+ return_error = BR_DEAD_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
@@ -4648,6 +4647,22 @@ static int binder_ioctl_get_node_debug_info(struct
binder_proc *proc,
return 0;
}
+static int binder_txns_pending(struct binder_proc *proc)
+{
+ struct rb_node *n;
+ struct binder_thread *thread;
+
+ if (proc->outstanding_txns > 0)
+ return 1;
+
+ for (n = rb_first(>threads); n; n = rb_next(n)) {
+ thread = rb_entry(n, struct binder_thread, rb_node);
+ if (thread->transaction_stack)
+ return 1;
+ }
+ return 0;
+}
+
static int binder_ioctl_freeze(struct binder_freeze_info *info,
struct binder_proc *target_proc)
{
@@ -4682,6 +4697,14 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
if (!ret && target_proc->outstanding_txns)
ret = -EAGAIN;
+ /* Also check pending transactions that wait for reply */
+ if (ret >= 0) {
+ binder_inner_proc_lock(target_proc);
+ if (binder_txns_pending(target_proc))
+ ret = -EAGAIN;
+ binder_inner_proc_unlock(target_proc);
+ }
+
if (ret < 0) {
binder_inner_proc_lock(target_proc);
target_proc->is_frozen = false;
@@ -4705,7 +4728,8 @@ static int binder_ioctl_get_freezer_info(
if (target_proc->pid == info->pid) {
found = true;
binder_inner_proc_lock(target_proc);
- info->sync_recv |= target_proc->sync_recv;
+ info->sync_recv |= target_proc->sync_recv |
+ (binder_txns_pending(target_proc) << 1);
info->async_recv |= target_proc->async_recv;
binder_inner_proc_unlock(target_proc);
}
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 810c0b84d3f8..402c4d4362a8 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -378,6 +378,8 @@ struct binder_ref {
*binder transactions
*(protected by @inner_lock)
* @sync_recv:process received sync transactions since last frozen
+ *bit 0: received sync transaction after being frozen
+ *bit 1: new pending sync transaction during freezing
*
[PATCH v1 0/1] binder: fix freeze race
From: Li Li As there isn't an atomic operation to freeze the main thread and binder interface together, it's possible the main thread initiates a new binder transaction while the binder interfaces are already frozen. This race issue will result in failed binder transaction and unexpectedly crash the app. This patch allows a post-froze rollback mechanism by checking if there's any new pending binder transaction waiting for response. At the same time, it treats the response transaction like an oneway transaction so that the response can successfully reach the frozen process. Li Li (1): binder: fix freeze race drivers/android/binder.c | 32 +++ drivers/android/binder_internal.h | 2 ++ 2 files changed, 30 insertions(+), 4 deletions(-) -- 2.33.0.309.g3052b89438-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability
On Thu, Mar 18, 2021 at 9:20 AM Todd Kjos wrote: > > On Wed, Mar 17, 2021 at 1:17 PM Jann Horn wrote: > > > > On Wed, Mar 17, 2021 at 7:00 PM Christian Brauner > > wrote: > > > On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote: > > > > To improve the user experience when switching between recently used > > > > applications, the background applications which are not currently needed > > > > are cached in the memory. Normally, a well designed application will not > > > > consume valuable CPU resources in the background. However, it's possible > > > > some applications are not able or willing to behave as expected, wasting > > > > energy even after being cached. > > > > > > > > It is a good idea to freeze those applications when they're only being > > > > kept alive for the sake of faster startup and energy saving. These > > > > kernel > > > > patches will provide the necessary infrastructure for user space > > > > framework > > > > to freeze and thaw a cached process, check the current freezing status > > > > and > > > > correctly deal with outstanding binder transactions to frozen processes. > > > > I just have some comments on the overall design: > > > > This seems a bit convoluted to me; and I'm not sure whether this is > > really something the kernel should get involved in, or whether this > > patchset is operating at the right layer. > > The issue is that there is lot's of per-process state in the binder > driver that needs to be quiesced prior to freezing the process (using > the standard freeze mechanism of > Documentation/admin-guide/cgroup-v1/freezer-subsystem.rst). That's all > this series does... quiesces binder state prior to freeze and then > re-enable transactions when the process is thawed. > > > > > If there are non-binder threads that are misbehaving, could you > > instead stop all those threads in pure userspace code (e.g. by sending > > a thread-directed signal to all of them and letting the signal handler > > sleep on a futex); and if the binder thread receives a transaction > > that should be handled, wake up those threads again? > > It is not an issue of stopping threads. It's an issue of quiescing > binder for a process so clients aren't blocked waiting for a response > from a frozen process that may never handle the transaction. This > series causes the soon-to-be-frozen process to reject new transactions > and allows user-space to detect when the transactions have drained > from the queues prior to freezing the process. > > > > > Or alternatively you could detect that the application is being woken > > up frequently even though it's supposed to be idle (e.g. using > > information from procfs), and kill it since you consider it to be > > misbehaving? > > > > Or if there are specific usage patterns you see frequently that you > > consider to be wasting CPU resources (e.g. setting an interval timer > > that fires in short intervals), you could try to delay such timers. > > > > > > With your current approach, you're baking the assumption that all IPC > > goes through binder into the kernel API; things like passing a file > > descriptor to a pipe through binder or using shared futexes are no > > No, we're dealing with an issue that is particular to binder IPC when > freezing a process. I suspect that other IPC mechanisms do not have > this issue -- and if any do for Android, then they would need > equivalent pre-freeze/post-freeze mechanisms. So far in the testing of > freezing in Android R, there haven't been issues with pipes or futexes > that required this kind of explicit quiescing (at least none that I > know of -- dualli@, please comment if there have been these kinds of > issues). Correct, currently there's no evidence the similar things should be applied to other IPC mechanisms. But we'll keep an eye on this. > > > longer usable for cross-process communication without making more > > kernel changes. I'm not sure whether that's a good idea. On top of > > that, if you freeze a process while it is in the middle of some > > operation, resources associated with the operation will probably stay > > in use for quite some time; for example, if an app is in the middle of > > downloading some data over HTTP, and you freeze it, this may cause the > > TCP connection to remain active and consume resources for send/receive > > buffers on both the device and the server. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 3/3] binder: BINDER_GET_FROZEN_INFO ioctl
From: Marco Ballesio
User space needs to know if binder transactions occurred to frozen
processes. Introduce a new BINDER_GET_FROZEN ioctl and keep track of
transactions occurring to frozen proceses.
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c| 55 +
drivers/android/binder_internal.h | 6
include/uapi/linux/android/binder.h | 7
3 files changed, 68 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index fe16c455a76e..e1a484ab0366 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2360,6 +2360,10 @@ static int binder_proc_transaction(struct
binder_transaction *t,
}
binder_inner_proc_lock(proc);
+ if (proc->is_frozen) {
+ proc->sync_recv |= !oneway;
+ proc->async_recv |= oneway;
+ }
if ((proc->is_frozen && !oneway) || proc->is_dead ||
(thread && thread->is_dead)) {
@@ -4634,6 +4638,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
if (!info->enable) {
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
return 0;
@@ -4645,6 +4651,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
* for transactions to drain.
*/
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = true;
binder_inner_proc_unlock(target_proc);
@@ -4666,6 +4674,33 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
return ret;
}
+static int binder_ioctl_get_freezer_info(
+ struct binder_frozen_status_info *info)
+{
+ struct binder_proc *target_proc;
+ bool found = false;
+
+ info->sync_recv = 0;
+ info->async_recv = 0;
+
+ mutex_lock(_procs_lock);
+ hlist_for_each_entry(target_proc, _procs, proc_node) {
+ if (target_proc->pid == info->pid) {
+ found = true;
+ binder_inner_proc_lock(target_proc);
+ info->sync_recv |= target_proc->sync_recv;
+ info->async_recv |= target_proc->async_recv;
+ binder_inner_proc_unlock(target_proc);
+ }
+ }
+ mutex_unlock(_procs_lock);
+
+ if (!found)
+ return -EINVAL;
+
+ return 0;
+}
+
static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
{
int ret;
@@ -4844,6 +4879,24 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
goto err;
break;
}
+ case BINDER_GET_FROZEN_INFO: {
+ struct binder_frozen_status_info info;
+
+ if (copy_from_user(, ubuf, sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = binder_ioctl_get_freezer_info();
+ if (ret < 0)
+ goto err;
+
+ if (copy_to_user(ubuf, , sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+ break;
+ }
default:
ret = -EINVAL;
goto err;
@@ -5154,6 +5207,8 @@ static void binder_deferred_release(struct binder_proc
*proc)
proc->is_dead = true;
proc->is_frozen = false;
+ proc->sync_recv = false;
+ proc->async_recv = false;
threads = 0;
active_transactions = 0;
while ((n = rb_first(>threads))) {
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index e6a53e98c6da..2872a7de68e1 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -376,6 +376,10 @@ struct binder_ref {
* @is_frozen:process is frozen and unable to service
*binder transactions
*(protected by @inner_lock)
+ * @sync_recv:process received sync transactions since last frozen
+ *(protected by @inner_lock)
+ * @async_recv: process received async transactions since last frozen
+ *(protected by @inner_lock)
* @freeze_wait: waitqueue of processes waiting for all outstanding
*transactions to be processed
*(protected by @inner_lock)
@@ -422,6 +426,8 @@ struct binder_proc {
int outstanding_txns;
bool is_dead;
[PATCH v3 2/3] binder: use EINTR for interrupted wait for work
From: Marco Ballesio
when interrupted by a signal, binder_wait_for_work currently returns
-ERESTARTSYS. This error code isn't propagated to user space, but a way
to handle interruption due to signals must be provided to code using
this API.
Replace this instance of -ERESTARTSYS with -EINTR, which is propagated
to user space.
Test: built, booted, interrupted a worker thread within
binder_wait_for_work
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b93ca53bb90f..fe16c455a76e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3710,7 +3710,7 @@ static int binder_wait_for_work(struct binder_thread
*thread,
binder_inner_proc_lock(proc);
list_del_init(>waiting_thread_node);
if (signal_pending(current)) {
- ret = -ERESTARTSYS;
+ ret = -EINTR;
break;
}
}
@@ -4853,7 +4853,7 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
if (thread)
thread->looper_need_return = false;
wait_event_interruptible(binder_user_error_wait,
binder_stop_on_user_error < 2);
- if (ret && ret != -ERESTARTSYS)
+ if (ret && ret != -EINTR)
pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid,
current->pid, cmd, arg, ret);
err_unlocked:
trace_binder_ioctl_done(ret);
--
2.31.0.rc2.261.g7f71774620-goog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/3] binder: BINDER_FREEZE ioctl
From: Marco Ballesio
Frozen tasks can't process binder transactions, so a way is required to
inform transmitting ends of communication failures due to the frozen
state of their receiving counterparts. Additionally, races are possible
between transitions to frozen state and binder transactions enqueued to
a specific process.
Implement BINDER_FREEZE ioctl for user space to inform the binder driver
about the intention to freeze or unfreeze a process. When the ioctl is
called, block the caller until any pending binder transactions toward
the target process are flushed. Return an error to transactions to
processes marked as frozen.
Signed-off-by: Marco Ballesio
Co-developed-by: Todd Kjos
Signed-off-by: Todd Kjos
Signed-off-by: Li Li
---
drivers/android/binder.c| 139 ++--
drivers/android/binder_internal.h | 12 +++
include/uapi/linux/android/binder.h | 13 +++
3 files changed, 154 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..b93ca53bb90f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct
binder_transaction *t)
if (target_proc) {
binder_inner_proc_lock(target_proc);
+ target_proc->outstanding_txns--;
+ if (target_proc->outstanding_txns < 0)
+ pr_warn("%s: Unexpected outstanding_txns %d\n",
+ __func__, target_proc->outstanding_txns);
+ if (!target_proc->outstanding_txns && target_proc->is_frozen)
+ wake_up_interruptible_all(_proc->freeze_wait);
if (t->buffer)
t->buffer->transaction = NULL;
binder_inner_proc_unlock(target_proc);
@@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct
binder_transaction *t,
* If the @thread parameter is not NULL, the transaction is always queued
* to the waitlist of that specific thread.
*
- * Return: true if the transactions was successfully queued
- * false if the target process or thread is dead
+ * 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
*/
-static bool binder_proc_transaction(struct binder_transaction *t,
+static int binder_proc_transaction(struct binder_transaction *t,
struct binder_proc *proc,
struct binder_thread *thread)
{
@@ -2354,10 +2361,11 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
binder_inner_proc_lock(proc);
- if (proc->is_dead || (thread && thread->is_dead)) {
+ if ((proc->is_frozen && !oneway) || proc->is_dead ||
+ (thread && thread->is_dead)) {
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return false;
+ return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
}
if (!thread && !pending_async)
@@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
if (!pending_async)
binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
+ proc->outstanding_txns++;
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return true;
+ return 0;
}
/**
@@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead) {
+ if (target_thread->is_dead || target_proc->is_frozen) {
+ return_error = target_thread->is_dead ?
+ BR_DEAD_REPLY : BR_FROZEN_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
BUG_ON(t->buffer->async_transaction != 0);
binder_pop_transaction_ilocked(target_thread, in_reply_to);
binder_enqueue_thread_work_ilocked(target_thread, >work);
+ target_proc->outstanding_txns++;
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
binder_free_transaction(in_reply_to);
@@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
t->from_parent = thread->transaction_stack;
thread->transaction_stack = t;
binder_inner_proc_unlock(proc)
[PATCH v3 0/3] Binder: Enable App Freezing Capability
From: Li Li To improve the user experience when switching between recently used applications, the background applications which are not currently needed are cached in the memory. Normally, a well designed application will not consume valuable CPU resources in the background. However, it's possible some applications are not able or willing to behave as expected, wasting energy even after being cached. It is a good idea to freeze those applications when they're only being kept alive for the sake of faster startup and energy saving. These kernel patches will provide the necessary infrastructure for user space framework to freeze and thaw a cached process, check the current freezing status and correctly deal with outstanding binder transactions to frozen processes. Changes in v2: avoid panic by using pr_warn for unexpected cases. Changes in v3: improved errcode logic in binder_proc_transaction(). Marco Ballesio (3): binder: BINDER_FREEZE ioctl binder: use EINTR for interrupted wait for work binder: BINDER_GET_FROZEN_INFO ioctl drivers/android/binder.c| 198 ++-- drivers/android/binder_internal.h | 18 +++ include/uapi/linux/android/binder.h | 20 +++ 3 files changed, 224 insertions(+), 12 deletions(-) -- 2.31.0.rc2.261.g7f71774620-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/3] binder: BINDER_FREEZE ioctl
On Fri, Mar 12, 2021 at 4:00 PM Todd Kjos wrote:
>
> On Thu, Mar 11, 2021 at 10:46 AM Li Li wrote:
> >
> > From: Marco Ballesio
> >
> > Frozen tasks can't process binder transactions, so a way is required to
> > inform transmitting ends of communication failures due to the frozen
> > state of their receiving counterparts. Additionally, races are possible
> > between transitions to frozen state and binder transactions enqueued to
> > a specific process.
> >
> > Implement BINDER_FREEZE ioctl for user space to inform the binder driver
> > about the intention to freeze or unfreeze a process. When the ioctl is
> > called, block the caller until any pending binder transactions toward
> > the target process are flushed. Return an error to transactions to
> > processes marked as frozen.
> >
> > Signed-off-by: Marco Ballesio
> > Co-developed-by: Todd Kjos
> > Signed-off-by: Todd Kjos
> > Signed-off-by: Li Li
> > ---
> > drivers/android/binder.c| 141 ++--
> > drivers/android/binder_internal.h | 12 +++
> > include/uapi/linux/android/binder.h | 13 +++
> > 3 files changed, 156 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index c119736ca56a..76ea558df03e 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct
> > binder_transaction *t)
> >
> > if (target_proc) {
> > binder_inner_proc_lock(target_proc);
> > + target_proc->outstanding_txns--;
> > + if (target_proc->outstanding_txns < 0)
> > + pr_warn("%s: Unexpected outstanding_txns %d\n",
> > + __func__, target_proc->outstanding_txns);
>
> Shouldn't this be something like "outstanding_txns is negative"? If we
> ever see one of these, is this enough information to be useful? Should
> we at least print the target proc's pid so someone can figure out what
> process had the messed up count?
>
The negative case should never happen. But if it really happens, that
means we found a
fundamental logic error within the binder driver. In such a case, more
DEBUG enums are
required to debug the issue, especially BINDER_DEBUG_TRANSACTION. So it's not
necessary to print the pid again.
>
> > + if (!target_proc->outstanding_txns &&
> > target_proc->is_frozen)
> > +
> > wake_up_interruptible_all(_proc->freeze_wait);
> > if (t->buffer)
> > t->buffer->transaction = NULL;
> > binder_inner_proc_unlock(target_proc);
> > @@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct
> > binder_transaction *t,
> > * If the @thread parameter is not NULL, the transaction is always queued
> > * to the waitlist of that specific thread.
> > *
> > - * Return: true if the transactions was successfully queued
> > - * false if the target process or thread is dead
> > + * 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
> > */
> > -static bool binder_proc_transaction(struct binder_transaction *t,
> > +static int binder_proc_transaction(struct binder_transaction *t,
> > struct binder_proc *proc,
> > struct binder_thread *thread)
> > {
> > @@ -2354,10 +2361,13 @@ static bool binder_proc_transaction(struct
> > binder_transaction *t,
> >
> > binder_inner_proc_lock(proc);
> >
> > - if (proc->is_dead || (thread && thread->is_dead)) {
> > + if ((proc->is_frozen && !oneway) || proc->is_dead ||
> > + (thread && thread->is_dead)) {
> > + bool proc_is_dead = proc->is_dead
> > + || (thread && thread->is_dead);
> > binder_inner_proc_unlock(proc);
> > binder_node_unlock(node);
> > - return false;
> > + return proc_is_dead ? BR_DEAD_REPLY : BR_FROZEN_REPLY;
>
> Do we need the proc_is_dead local? This could be:
> return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY
You're right. The improved code is already in v3, which will be s
[PATCH v2 3/3] binder: BINDER_GET_FROZEN_INFO ioctl
From: Marco Ballesio
User space needs to know if binder transactions occurred to frozen
processes. Introduce a new BINDER_GET_FROZEN ioctl and keep track of
transactions occurring to frozen proceses.
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c| 55 +
drivers/android/binder_internal.h | 6
include/uapi/linux/android/binder.h | 7
3 files changed, 68 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 38bbf9a4ce99..b4999ed04b2e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2360,6 +2360,10 @@ static int binder_proc_transaction(struct
binder_transaction *t,
}
binder_inner_proc_lock(proc);
+ if (proc->is_frozen) {
+ proc->sync_recv |= !oneway;
+ proc->async_recv |= oneway;
+ }
if ((proc->is_frozen && !oneway) || proc->is_dead ||
(thread && thread->is_dead)) {
@@ -4636,6 +4640,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
if (!info->enable) {
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
return 0;
@@ -4647,6 +4653,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
* for transactions to drain.
*/
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = true;
binder_inner_proc_unlock(target_proc);
@@ -4668,6 +4676,33 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
return ret;
}
+static int binder_ioctl_get_freezer_info(
+ struct binder_frozen_status_info *info)
+{
+ struct binder_proc *target_proc;
+ bool found = false;
+
+ info->sync_recv = 0;
+ info->async_recv = 0;
+
+ mutex_lock(_procs_lock);
+ hlist_for_each_entry(target_proc, _procs, proc_node) {
+ if (target_proc->pid == info->pid) {
+ found = true;
+ binder_inner_proc_lock(target_proc);
+ info->sync_recv |= target_proc->sync_recv;
+ info->async_recv |= target_proc->async_recv;
+ binder_inner_proc_unlock(target_proc);
+ }
+ }
+ mutex_unlock(_procs_lock);
+
+ if (!found)
+ return -EINVAL;
+
+ return 0;
+}
+
static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
{
int ret;
@@ -4846,6 +4881,24 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
goto err;
break;
}
+ case BINDER_GET_FROZEN_INFO: {
+ struct binder_frozen_status_info info;
+
+ if (copy_from_user(, ubuf, sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = binder_ioctl_get_freezer_info();
+ if (ret < 0)
+ goto err;
+
+ if (copy_to_user(ubuf, , sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+ break;
+ }
default:
ret = -EINVAL;
goto err;
@@ -5156,6 +5209,8 @@ static void binder_deferred_release(struct binder_proc
*proc)
proc->is_dead = true;
proc->is_frozen = false;
+ proc->sync_recv = false;
+ proc->async_recv = false;
threads = 0;
active_transactions = 0;
while ((n = rb_first(>threads))) {
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index e6a53e98c6da..2872a7de68e1 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -376,6 +376,10 @@ struct binder_ref {
* @is_frozen:process is frozen and unable to service
*binder transactions
*(protected by @inner_lock)
+ * @sync_recv:process received sync transactions since last frozen
+ *(protected by @inner_lock)
+ * @async_recv: process received async transactions since last frozen
+ *(protected by @inner_lock)
* @freeze_wait: waitqueue of processes waiting for all outstanding
*transactions to be processed
*(protected by @inner_lock)
@@ -422,6 +426,8 @@ struct binder_proc {
int outstanding_txns;
bool is_dead;
[PATCH v2 1/3] binder: BINDER_FREEZE ioctl
From: Marco Ballesio
Frozen tasks can't process binder transactions, so a way is required to
inform transmitting ends of communication failures due to the frozen
state of their receiving counterparts. Additionally, races are possible
between transitions to frozen state and binder transactions enqueued to
a specific process.
Implement BINDER_FREEZE ioctl for user space to inform the binder driver
about the intention to freeze or unfreeze a process. When the ioctl is
called, block the caller until any pending binder transactions toward
the target process are flushed. Return an error to transactions to
processes marked as frozen.
Signed-off-by: Marco Ballesio
Co-developed-by: Todd Kjos
Signed-off-by: Todd Kjos
Signed-off-by: Li Li
---
drivers/android/binder.c| 141 ++--
drivers/android/binder_internal.h | 12 +++
include/uapi/linux/android/binder.h | 13 +++
3 files changed, 156 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..76ea558df03e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct
binder_transaction *t)
if (target_proc) {
binder_inner_proc_lock(target_proc);
+ target_proc->outstanding_txns--;
+ if (target_proc->outstanding_txns < 0)
+ pr_warn("%s: Unexpected outstanding_txns %d\n",
+ __func__, target_proc->outstanding_txns);
+ if (!target_proc->outstanding_txns && target_proc->is_frozen)
+ wake_up_interruptible_all(_proc->freeze_wait);
if (t->buffer)
t->buffer->transaction = NULL;
binder_inner_proc_unlock(target_proc);
@@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct
binder_transaction *t,
* If the @thread parameter is not NULL, the transaction is always queued
* to the waitlist of that specific thread.
*
- * Return: true if the transactions was successfully queued
- * false if the target process or thread is dead
+ * 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
*/
-static bool binder_proc_transaction(struct binder_transaction *t,
+static int binder_proc_transaction(struct binder_transaction *t,
struct binder_proc *proc,
struct binder_thread *thread)
{
@@ -2354,10 +2361,13 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
binder_inner_proc_lock(proc);
- if (proc->is_dead || (thread && thread->is_dead)) {
+ if ((proc->is_frozen && !oneway) || proc->is_dead ||
+ (thread && thread->is_dead)) {
+ bool proc_is_dead = proc->is_dead
+ || (thread && thread->is_dead);
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return false;
+ return proc_is_dead ? BR_DEAD_REPLY : BR_FROZEN_REPLY;
}
if (!thread && !pending_async)
@@ -2373,10 +2383,11 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
if (!pending_async)
binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
+ proc->outstanding_txns++;
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return true;
+ return 0;
}
/**
@@ -3013,13 +3024,16 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead) {
+ if (target_thread->is_dead || target_proc->is_frozen) {
+ return_error = target_thread->is_dead ?
+ BR_DEAD_REPLY : BR_FROZEN_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
BUG_ON(t->buffer->async_transaction != 0);
binder_pop_transaction_ilocked(target_thread, in_reply_to);
binder_enqueue_thread_work_ilocked(target_thread, >work);
+ target_proc->outstanding_txns++;
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
binder_free_transaction(in_reply_to);
@@ -3038,7 +3052,9 @@ static void binder_transaction(struct binder_proc *proc,
t->from_parent = thread->transaction_stack;
[PATCH v2 2/3] binder: use EINTR for interrupted wait for work
From: Marco Ballesio
when interrupted by a signal, binder_wait_for_work currently returns
-ERESTARTSYS. This error code isn't propagated to user space, but a way
to handle interruption due to signals must be provided to code using
this API.
Replace this instance of -ERESTARTSYS with -EINTR, which is propagated
to user space.
Test: built, booted, interrupted a worker thread within
binder_wait_for_work
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 76ea558df03e..38bbf9a4ce99 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3712,7 +3712,7 @@ static int binder_wait_for_work(struct binder_thread
*thread,
binder_inner_proc_lock(proc);
list_del_init(>waiting_thread_node);
if (signal_pending(current)) {
- ret = -ERESTARTSYS;
+ ret = -EINTR;
break;
}
}
@@ -4855,7 +4855,7 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
if (thread)
thread->looper_need_return = false;
wait_event_interruptible(binder_user_error_wait,
binder_stop_on_user_error < 2);
- if (ret && ret != -ERESTARTSYS)
+ if (ret && ret != -EINTR)
pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid,
current->pid, cmd, arg, ret);
err_unlocked:
trace_binder_ioctl_done(ret);
--
2.31.0.rc2.261.g7f71774620-goog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/3] Binder: Enable App Freezing Capability
From: Li Li To improve the user experience when switching between recently used applications, the background applications which are not currently needed are cached in the memory. Normally, a well designed application will not consume valuable CPU resources in the background. However, it's possible some applications are not able or willing to behave as expected, wasting energy even after being cached. It is a good idea to freeze those applications when they're only being kept alive for the sake of faster startup and energy saving. These kernel patches will provide the necessary infrastructure for user space framework to freeze and thaw a cached process, check the current freezing status and correctly deal with outstanding binder transactions to frozen processes. Changes in v2: avoid panic by using pr_warn for unexpected cases. Marco Ballesio (3): binder: BINDER_FREEZE ioctl binder: use EINTR for interrupted wait for work binder: BINDER_GET_FROZEN_INFO ioctl drivers/android/binder.c| 200 ++-- drivers/android/binder_internal.h | 18 +++ include/uapi/linux/android/binder.h | 20 +++ 3 files changed, 226 insertions(+), 12 deletions(-) -- 2.31.0.rc2.261.g7f71774620-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/3] binder: BINDER_FREEZE ioctl
On Wed, Mar 10, 2021 at 11:33 PM Greg KH wrote:
>
> On Wed, Mar 10, 2021 at 02:52:49PM -0800, Li Li wrote:
> > if (target_proc) {
> > binder_inner_proc_lock(target_proc);
> > + target_proc->outstanding_txns--;
> > + WARN_ON(target_proc->outstanding_txns < 0);
>
> WARN_* is a huge crutch, please just handle stuff like this properly and
> if you really need to, warn userspace (but what can they do about it?)
>
> You also just rebooted all systems that have panic-on-warn set, so if
> this can be triggered by userspace, you caused a DoS of things :(
>
> So please remove all of the WARN_ON() you add in this patch series to
> properly handle the error conditions and deal with them correctly.
>
> And if these were here just for debugging, hopefully the code works
> properly now and you do not need debugging anymore so they can all just
> be dropped.
When the target_proc is freed, there's no outstanding transactions already.
The FREEZE ioctl from userspace won't trigger this. It's for debugging.
And I'll remove it in v2. Thanks for the suggestion!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 2/3] binder: use EINTR for interrupted wait for work
From: Marco Ballesio
when interrupted by a signal, binder_wait_for_work currently returns
-ERESTARTSYS. This error code isn't propagated to user space, but a way
to handle interruption due to signals must be provided to code using
this API.
Replace this instance of -ERESTARTSYS with -EINTR, which is propagated
to user space.
Test: built, booted, interrupted a worker thread within
binder_wait_for_work
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9ec3ba038652..34c3e430a270 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3710,7 +3710,7 @@ static int binder_wait_for_work(struct binder_thread
*thread,
binder_inner_proc_lock(proc);
list_del_init(>waiting_thread_node);
if (signal_pending(current)) {
- ret = -ERESTARTSYS;
+ ret = -EINTR;
break;
}
}
@@ -4851,7 +4851,7 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
if (thread)
thread->looper_need_return = false;
wait_event_interruptible(binder_user_error_wait,
binder_stop_on_user_error < 2);
- if (ret && ret != -ERESTARTSYS)
+ if (ret && ret != -EINTR)
pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid,
current->pid, cmd, arg, ret);
err_unlocked:
trace_binder_ioctl_done(ret);
--
2.31.0.rc1.246.gcd05c9c855-goog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 3/3] binder: BINDER_GET_FROZEN_INFO ioctl
From: Marco Ballesio
User space needs to know if binder transactions occurred to frozen
processes. Introduce a new BINDER_GET_FROZEN ioctl and keep track of
transactions occurring to frozen proceses.
Signed-off-by: Marco Ballesio
Signed-off-by: Li Li
---
drivers/android/binder.c| 55 +
drivers/android/binder_internal.h | 6
include/uapi/linux/android/binder.h | 7
3 files changed, 68 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 34c3e430a270..00c68b7eb553 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2358,6 +2358,10 @@ static int binder_proc_transaction(struct
binder_transaction *t,
}
binder_inner_proc_lock(proc);
+ if (proc->is_frozen) {
+ proc->sync_recv |= !oneway;
+ proc->async_recv |= oneway;
+ }
if ((proc->is_frozen && !oneway) || proc->is_dead ||
(thread && thread->is_dead)) {
@@ -4632,6 +4636,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
if (!info->enable) {
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = false;
binder_inner_proc_unlock(target_proc);
return 0;
@@ -4643,6 +4649,8 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
* for transactions to drain.
*/
binder_inner_proc_lock(target_proc);
+ target_proc->sync_recv = false;
+ target_proc->async_recv = false;
target_proc->is_frozen = true;
binder_inner_proc_unlock(target_proc);
@@ -4664,6 +4672,33 @@ static int binder_ioctl_freeze(struct binder_freeze_info
*info,
return ret;
}
+static int binder_ioctl_get_freezer_info(
+ struct binder_frozen_status_info *info)
+{
+ struct binder_proc *target_proc;
+ bool found = false;
+
+ info->sync_recv = 0;
+ info->async_recv = 0;
+
+ mutex_lock(_procs_lock);
+ hlist_for_each_entry(target_proc, _procs, proc_node) {
+ if (target_proc->pid == info->pid) {
+ found = true;
+ binder_inner_proc_lock(target_proc);
+ info->sync_recv |= target_proc->sync_recv;
+ info->async_recv |= target_proc->async_recv;
+ binder_inner_proc_unlock(target_proc);
+ }
+ }
+ mutex_unlock(_procs_lock);
+
+ if (!found)
+ return -EINVAL;
+
+ return 0;
+}
+
static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long
arg)
{
int ret;
@@ -4842,6 +4877,24 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
goto err;
break;
}
+ case BINDER_GET_FROZEN_INFO: {
+ struct binder_frozen_status_info info;
+
+ if (copy_from_user(, ubuf, sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = binder_ioctl_get_freezer_info();
+ if (ret < 0)
+ goto err;
+
+ if (copy_to_user(ubuf, , sizeof(info))) {
+ ret = -EFAULT;
+ goto err;
+ }
+ break;
+ }
default:
ret = -EINVAL;
goto err;
@@ -5152,6 +5205,8 @@ static void binder_deferred_release(struct binder_proc
*proc)
proc->is_dead = true;
proc->is_frozen = false;
+ proc->sync_recv = false;
+ proc->async_recv = false;
threads = 0;
active_transactions = 0;
while ((n = rb_first(>threads))) {
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index e6a53e98c6da..2872a7de68e1 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -376,6 +376,10 @@ struct binder_ref {
* @is_frozen:process is frozen and unable to service
*binder transactions
*(protected by @inner_lock)
+ * @sync_recv:process received sync transactions since last frozen
+ *(protected by @inner_lock)
+ * @async_recv: process received async transactions since last frozen
+ *(protected by @inner_lock)
* @freeze_wait: waitqueue of processes waiting for all outstanding
*transactions to be processed
*(protected by @inner_lock)
@@ -422,6 +426,8 @@ struct binder_proc {
int outstanding_txns;
bool is_dead;
[PATCH v1 1/3] binder: BINDER_FREEZE ioctl
From: Marco Ballesio
Frozen tasks can't process binder transactions, so a way is required to
inform transmitting ends of communication failures due to the frozen
state of their receiving counterparts. Additionally, races are possible
between transitions to frozen state and binder transactions enqueued to
a specific process.
Implement BINDER_FREEZE ioctl for user space to inform the binder driver
about the intention to freeze or unfreeze a process. When the ioctl is
called, block the caller until any pending binder transactions toward
the target process are flushed. Return an error to transactions to
processes marked as frozen.
Signed-off-by: Marco Ballesio
Co-developed-by: Todd Kjos
Signed-off-by: Todd Kjos
Signed-off-by: Li Li
---
drivers/android/binder.c| 137 ++--
drivers/android/binder_internal.h | 12 +++
include/uapi/linux/android/binder.h | 13 +++
3 files changed, 152 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..9ec3ba038652 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1506,6 +1506,10 @@ static void binder_free_transaction(struct
binder_transaction *t)
if (target_proc) {
binder_inner_proc_lock(target_proc);
+ target_proc->outstanding_txns--;
+ WARN_ON(target_proc->outstanding_txns < 0);
+ if (!target_proc->outstanding_txns && target_proc->is_frozen)
+ wake_up_interruptible_all(_proc->freeze_wait);
if (t->buffer)
t->buffer->transaction = NULL;
binder_inner_proc_unlock(target_proc);
@@ -2331,10 +2335,11 @@ static int binder_fixup_parent(struct
binder_transaction *t,
* If the @thread parameter is not NULL, the transaction is always queued
* to the waitlist of that specific thread.
*
- * Return: true if the transactions was successfully queued
- * false if the target process or thread is dead
+ * 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
*/
-static bool binder_proc_transaction(struct binder_transaction *t,
+static int binder_proc_transaction(struct binder_transaction *t,
struct binder_proc *proc,
struct binder_thread *thread)
{
@@ -2354,10 +2359,13 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
binder_inner_proc_lock(proc);
- if (proc->is_dead || (thread && thread->is_dead)) {
+ if ((proc->is_frozen && !oneway) || proc->is_dead ||
+ (thread && thread->is_dead)) {
+ bool proc_is_dead = proc->is_dead
+ || (thread && thread->is_dead);
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return false;
+ return proc_is_dead ? BR_DEAD_REPLY : BR_FROZEN_REPLY;
}
if (!thread && !pending_async)
@@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct
binder_transaction *t,
if (!pending_async)
binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
+ proc->outstanding_txns++;
binder_inner_proc_unlock(proc);
binder_node_unlock(node);
- return true;
+ return 0;
}
/**
@@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc *proc,
if (reply) {
binder_enqueue_thread_work(thread, tcomplete);
binder_inner_proc_lock(target_proc);
- if (target_thread->is_dead) {
+ if (target_thread->is_dead || target_proc->is_frozen) {
+ return_error = target_thread->is_dead ?
+ BR_DEAD_REPLY : BR_FROZEN_REPLY;
binder_inner_proc_unlock(target_proc);
goto err_dead_proc_or_thread;
}
BUG_ON(t->buffer->async_transaction != 0);
binder_pop_transaction_ilocked(target_thread, in_reply_to);
binder_enqueue_thread_work_ilocked(target_thread, >work);
+ target_proc->outstanding_txns++;
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(_thread->wait);
binder_free_transaction(in_reply_to);
@@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
t->from_parent = thread->transaction_stack;
thread->transaction_stack = t;
binder_inner_proc_unlock(proc);
- if (!binder_proc_transaction(t, target_proc,
[PATCH v1 0/3] Binder: Enable App Freezing Capability
From: Li Li To improve the user experience when switching between recently used applications, the background applications which are not currently needed are cached in the memory. Normally, a well designed application will not consume valuable CPU resources in the background. However, it's possible some applications are not able or willing to behave as expected, wasting energy even after being cached. It is a good idea to freeze those applications when they're only being kept alive for the sake of faster startup and energy saving. These kernel patches will provide the necessary infrastructure for user space framework to freeze and thaw a cached process, check the current freezing status and correctly deal with outstanding binder transactions to frozen processes. Marco Ballesio (3): binder: BINDER_FREEZE ioctl binder: use EINTR for interrupted wait for work binder: BINDER_GET_FROZEN_INFO ioctl drivers/android/binder.c| 196 ++-- drivers/android/binder_internal.h | 18 +++ include/uapi/linux/android/binder.h | 20 +++ 3 files changed, 222 insertions(+), 12 deletions(-) -- 2.31.0.rc1.246.gcd05c9c855-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
