On Fri, Feb 5, 2016 at 12:10 PM, Kees Cook <keesc...@chromium.org> wrote:
> On Fri, Feb 5, 2016 at 10:08 AM, John Stultz <john.stu...@linaro.org> wrote:
>> From: Ruchi Kandoi <kandoiru...@google.com>
>>
>> This allows power/performance management software to set timer
>> slack for other threads according to its policy for the thread
>> (such as when the thread is designated foreground vs. background
>> activity)
>>
>> Second argument is similar to PR_SET_TIMERSLACK, if non-zero
>> then the slack is set to that value otherwise sets it to the
>> default for the thread.
>>
>> Takes PID of the thread as the third argument.
>>
>> This interface checks that the calling task has permissions to
>> to use PTRACE_MODE_ATTACH on the target task, so that we can
>> ensure arbitrary apps do not change the timer slack for other
>> apps.
>>
>> Additional fixes from Ruchi and Micha Kalfon <mi...@cellrox.com>
>> have been folded into this patch to make it easier to reivew.
>>
>> Cc: Arjan van de Ven <ar...@linux.intel.com>
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Cc: Oren Laadan <or...@cellrox.com>
>> Cc: Ruchi Kandoi <kandoiru...@google.com>
>> Cc: Rom Lemarchand <rom...@android.com>
>> Cc: Kees Cook <keesc...@chromium.org>
>> Cc: Andrew Morton <a...@linux-foundation.org>
>> Cc: Android Kernel Team <kernel-t...@android.com>
>> Signed-off-by: Ruchi Kandoi <kandoiru...@google.com>
>> [jstultz:
>> * Folded in CAP_SYS_NICE check from Ruchi.
>> * Folded in fix misplaced PR_SET_TIMERSLACK_PID case fix from
>> Micha.
>> * Folded in make PR_SET_TIMERSLACK_PID pid namespace aware fix
>> from Micha.
>> * Changed PR_SET_TIMERSLACK_PID so it didn't collide with
>> already upstream prctrl values.
>> * Reworked commit message.
>> * Moved from CAP_SYS_NICE to PTRACE_MODE_ATTACH for permissions
>> checks]
>> Acked-by: Arjan van de Ven <ar...@linux.intel.com>
>> Reviewed-by: Thomas Gleixner <t...@linutronix.de>
>> Signed-off-by: John Stultz <john.stu...@linaro.org>
>> ---
>> include/uapi/linux/prctl.h | 7 +++++++
>> kernel/sys.c | 25 +++++++++++++++++++++++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>> index a8d0759..1a13c2b 100644
>> --- a/include/uapi/linux/prctl.h
>> +++ b/include/uapi/linux/prctl.h
>> @@ -187,6 +187,13 @@ struct prctl_mm_map {
>>
>> #define PR_SET_FP_MODE 45
>> #define PR_GET_FP_MODE 46
>> +
>> +/* Sets the timerslack for arbitrary threads
>
> Multiline comments should start with a blank:
>
> /*
> * Sets the timerslack...
> * ...
> */
>
>> + * arg2 slack value, 0 means "use default"
>> + * arg3 pid of the thread whose timer slack needs to be set
>> + */
>> +#define PR_SET_TIMERSLACK_PID 47
>> +
>> # define PR_FP_MODE_FR (1 << 0) /* 64b FP registers */
>> # define PR_FP_MODE_FRE (1 << 1) /* 32b compatibility
>> */
>>
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 78947de..5189378 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -41,6 +41,9 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/version.h>
>> #include <linux/ctype.h>
>> +#include <linux/mm.h>
>> +#include <linux/mempolicy.h>
>> +#include <linux/sched.h>
>>
>> #include <linux/compat.h>
>> #include <linux/syscalls.h>
>> @@ -2076,6 +2079,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
>> arg2, unsigned long, arg3,
>> unsigned long, arg4, unsigned long, arg5)
>> {
>> struct task_struct *me = current;
>> + struct task_struct *tsk;
>> unsigned char comm[sizeof(me->comm)];
>> long error;
>>
>> @@ -2218,6 +2222,27 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
>> arg2, unsigned long, arg3,
>> case PR_GET_TID_ADDRESS:
>> error = prctl_get_tid_address(me, (int __user **)arg2);
>> break;
>> + case PR_SET_TIMERSLACK_PID:
>
> You should reject unused params that are non-zero to allow future
> extensions. (Which begs the question, are there other timer-related
> things that might need setting too?)
Ok. Will look into this. As for other timer related things, I don't
think we have any userspace reachable interfaces. There was some
discussion around infinitely deferrable timers (so timers that only
fire if we're already expiring timers, but wouldn't ever cause a hw
event on their own), but that was likely to be done w/ a per-timer
specified flags, and stalled out awhile back. However, I suspect folks
might eventually want a similar ability to stop a specific task from
causing any wakeups if that functionality ever did land.
>> + rcu_read_lock();
>> + tsk = find_task_by_vpid((pid_t)arg3);
>> + if (tsk == NULL) {
>> + rcu_read_unlock();
>> + return -EINVAL;
>
> Traditionally, unable to look up a pid results in ESRCH.
Sounds good. Will fix.
Thanks for the feedback!
-john
