From: Martijn Coenen <maco@android.com>
To: gregkh@linuxfoundation.org, john.stultz@linaro.org,
tkjos@google.com, arve@android.com, amit.pundir@linaro.org,
tglx@linutronix.de
Cc: peterz@infradead.org, hch@lst.de, linux-kernel@vger.kernel.org,
devel@driverdev.osuosl.org, maco@google.com, malchev@google.com,
ccross@android.com, Martijn Coenen <maco@android.com>
Subject: [PATCH v2 08/13] ANDROID: binder: don't check prio permissions on restore.
Date: Thu, 31 Aug 2017 10:04:25 +0200 [thread overview]
Message-ID: <20170831080430.118765-9-maco@android.com> (raw)
In-Reply-To: <20170831080430.118765-1-maco@android.com>
Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:
1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
for the duration of the transaction
4) When thread A is done with the call from B, we will
try to restore the prio back to 98. But, we fail
because the process doesn't hold CAP_SYS_NICE,
neither is RLIMIT_RT_PRIO set.
While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 017693dd4ec1..0c0ecd78b9a7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1109,9 +1109,10 @@ static int to_kernel_prio(int policy, int user_priority)
}
/**
- * binder_set_priority() - sets the scheduler priority of a task
+ * binder_do_set_priority() - sets the scheduler priority of a task
* @task: task to set priority on
* @desired: desired priority to run at
+ * @verify: verify whether @task is allowed to run at @desired prio
*
* The scheduler policy of tasks is changed explicitly, because we want to
* support a few distinct features:
@@ -1145,8 +1146,9 @@ static int to_kernel_prio(int policy, int user_priority)
* temporarily runs at its original priority.
* 5) rt_mutex does not currently support PI for CFS tasks.
*/
-static void binder_set_priority(struct task_struct *task,
- struct binder_priority desired)
+static void binder_do_set_priority(struct task_struct *task,
+ struct binder_priority desired,
+ bool verify)
{
int priority; /* user-space prio value */
bool has_cap_nice;
@@ -1170,7 +1172,7 @@ static void binder_set_priority(struct task_struct *task,
priority = to_userspace_prio(policy, desired.prio);
- if (is_rt_policy(policy) && !has_cap_nice) {
+ if (verify && is_rt_policy(policy) && !has_cap_nice) {
long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
if (max_rtprio == 0) {
@@ -1182,7 +1184,7 @@ static void binder_set_priority(struct task_struct *task,
}
}
- if (is_fair_policy(policy) && !has_cap_nice) {
+ if (verify && is_fair_policy(policy) && !has_cap_nice) {
long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
if (min_nice > MAX_NICE) {
@@ -1215,6 +1217,18 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
}
+static void binder_set_priority(struct task_struct *task,
+ struct binder_priority desired)
+{
+ binder_do_set_priority(task, desired, /* verify = */ true);
+}
+
+static void binder_restore_priority(struct task_struct *task,
+ struct binder_priority desired)
+{
+ binder_do_set_priority(task, desired, /* verify = */ false);
+}
+
static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
struct binder_priority node_prio,
@@ -3251,7 +3265,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(&t->work, &target_thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(&target_thread->wait);
- binder_set_priority(current, in_reply_to->saved_priority);
+ binder_restore_priority(current, in_reply_to->saved_priority);
binder_free_transaction(in_reply_to);
} else if (!(t->flags & TF_ONE_WAY)) {
BUG_ON(t->buffer->async_transaction != 0);
@@ -3340,7 +3354,7 @@ static void binder_transaction(struct binder_proc *proc,
BUG_ON(thread->return_error.cmd != BR_OK);
if (in_reply_to) {
- binder_set_priority(current, in_reply_to->saved_priority);
+ binder_restore_priority(current, in_reply_to->saved_priority);
thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
binder_enqueue_work(thread->proc,
&thread->return_error.work,
@@ -3940,7 +3954,7 @@ static int binder_thread_read(struct binder_proc *proc,
wait_event_interruptible(binder_user_error_wait,
binder_stop_on_user_error < 2);
}
- binder_set_priority(current, proc->default_priority);
+ binder_restore_priority(current, proc->default_priority);
}
if (non_block) {
--
2.14.1.581.gf28d330327-goog
next prev parent reply other threads:[~2017-08-31 8:09 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-31 8:04 [PATCH v2 00/13] ANDROID: binder: RT priority inheritance and small fixes Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 01/13] ANDROID: binder: remove proc waitqueue Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 02/13] ANDROID: binder: push new transactions to waiting threads Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 03/13] ANDROID: binder: add support for RT prio inheritance Martijn Coenen
2017-08-31 8:18 ` Peter Zijlstra
2017-08-31 8:27 ` Martijn Coenen
2017-08-31 11:32 ` Peter Zijlstra
2017-08-31 12:00 ` Martijn Coenen
2017-08-31 12:21 ` Peter Zijlstra
2017-09-01 7:24 ` Greg KH
2017-10-09 11:21 ` Martijn Coenen
2017-10-09 11:43 ` Greg KH
2017-08-31 8:04 ` [PATCH v2 04/13] ANDROID: binder: add min sched_policy to node Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 05/13] ANDROID: binder: improve priority inheritance Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 06/13] ANDROID: binder: add RT inheritance flag to node Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 07/13] ANDROID: binder: Add BINDER_GET_NODE_DEBUG_INFO ioctl Martijn Coenen
2017-08-31 8:04 ` Martijn Coenen [this message]
2017-08-31 8:04 ` [PATCH v2 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()) Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 10/13] ANDROID: binder: call poll_wait() unconditionally Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 11/13] ANDROID: binder: don't enqueue death notifications to thread todo Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 12/13] ANDROID: binder: don't queue async transactions to thread Martijn Coenen
2017-08-31 8:04 ` [PATCH v2 13/13] ANDROID: binder: Add tracing for binder priority inheritance Martijn Coenen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170831080430.118765-9-maco@android.com \
--to=maco@android.com \
--cc=amit.pundir@linaro.org \
--cc=arve@android.com \
--cc=ccross@android.com \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@google.com \
--cc=malchev@google.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=tkjos@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.