Re: [PATCH] prctl: Add PR_SET_TIMERSLACK_PID for setting timer slack of an arbitrary thread.

[John Stultz]

On Fri, Feb 5, 2016 at 12:10 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Fri, Feb 5, 2016 at 10:08 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>> From: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
>>
>> 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 <micha@xxxxxxxxxxx>
>> have been folded into this patch to make it easier to reivew.
>>
>> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Oren Laadan <orenl@xxxxxxxxxxx>
>> Cc: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
>> Cc: Rom Lemarchand <romlem@xxxxxxxxxxx>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Android Kernel Team <kernel-team@xxxxxxxxxxx>
>> Signed-off-by: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
>> [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 <arjan@xxxxxxxxxxxxxxx>
>> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> ---
>>  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