All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martijn Coenen <maco@android.com>
To: gregkh@linuxfoundation.org, john.stultz@linaro.org,
	tkjos@google.com, arve@android.com, sherryy@android.com,
	tglx@linutronix.de, peterz@infradead.org, amit.pundir@linaro.org
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	maco@google.com, Martijn Coenen <maco@android.com>
Subject: [PATCH v3 5/6] ANDROID: binder: don't check prio permissions on restore.
Date: Thu, 26 Oct 2017 16:07:49 +0200	[thread overview]
Message-ID: <20171026140750.119265-6-maco@android.com> (raw)
In-Reply-To: <20171026140750.119265-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 5958a0876fe8..af1c3bb3f53e 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,
@@ -3282,7 +3296,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);
@@ -3377,7 +3391,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,
@@ -3963,7 +3977,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.15.0.rc2.357.g7e34df9404-goog

  parent reply	other threads:[~2017-10-26 14:08 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-26 14:07 [PATCH v3 0/6] ANDROID: binder: RT priority inheritance Martijn Coenen
2017-10-26 14:07 ` [PATCH v3 1/6] ANDROID: binder: add support for RT prio inheritance Martijn Coenen
2017-11-15 13:01   ` Peter Zijlstra
2017-11-16  9:18     ` Martijn Coenen
2017-11-16 11:27       ` Peter Zijlstra
2017-11-16 13:03         ` Martijn Coenen
2017-11-16 15:10           ` Peter Zijlstra
2017-11-17 10:23             ` Martijn Coenen
2017-10-26 14:07 ` [PATCH v3 2/6] ANDROID: binder: add min sched_policy to node Martijn Coenen
2017-11-15 13:02   ` Peter Zijlstra
2017-11-16  9:27     ` Martijn Coenen
2017-10-26 14:07 ` [PATCH v3 3/6] ANDROID: binder: improve priority inheritance Martijn Coenen
2017-11-15 13:03   ` Peter Zijlstra
2017-11-16  9:31     ` Martijn Coenen
2017-10-26 14:07 ` [PATCH v3 4/6] ANDROID: binder: add RT inheritance flag to node Martijn Coenen
2017-11-15 13:05   ` Peter Zijlstra
2017-11-16  9:37     ` Martijn Coenen
2017-10-26 14:07 ` Martijn Coenen [this message]
2017-11-15 13:08   ` [PATCH v3 5/6] ANDROID: binder: don't check prio permissions on restore Peter Zijlstra
2017-10-26 14:07 ` [PATCH v3 6/6] 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=20171026140750.119265-6-maco@android.com \
    --to=maco@android.com \
    --cc=amit.pundir@linaro.org \
    --cc=arve@android.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@google.com \
    --cc=peterz@infradead.org \
    --cc=sherryy@android.com \
    --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.