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

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

[John Stultz]

From: Ruchi Kandoi <kandoiruchi@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 <micha@cellrox.com>
have been folded into this patch to make it easier to reivew.

Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Oren Laadan <orenl@cellrox.com>
Cc: Ruchi Kandoi <kandoiruchi@google.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: Ruchi Kandoi <kandoiruchi@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 <arjan@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <john.stultz@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
+ * 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:
+		rcu_read_lock();
+		tsk = find_task_by_vpid((pid_t)arg3);
+		if (tsk == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		get_task_struct(tsk);
+		rcu_read_unlock();
+		if (ptrace_may_access(tsk, PTRACE_MODE_ATTACH)) {
+			put_task_struct(tsk);
+			return -EPERM;
+		}
+		if (arg2 <= 0)
+			tsk->timer_slack_ns =
+				tsk->default_timer_slack_ns;
+		else
+			tsk->timer_slack_ns = arg2;
+		put_task_struct(tsk);
+		error = 0;
+		break;
 	case PR_SET_CHILD_SUBREAPER:
 		me->signal->is_child_subreaper = !!arg2;
 		break;
-- 
1.9.1

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

[Kees Cook]

On Fri, Feb 5, 2016 at 10:08 AM, John Stultz <john.stultz@linaro.org> wrote:
> From: Ruchi Kandoi <kandoiruchi@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 <micha@cellrox.com>
> have been folded into this patch to make it easier to reivew.
>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Oren Laadan <orenl@cellrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@google.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Android Kernel Team <kernel-team@android.com>
> Signed-off-by: Ruchi Kandoi <kandoiruchi@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 <arjan@linux.intel.com>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Stultz <john.stultz@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?)

> +               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.

> +               }
> +               get_task_struct(tsk);
> +               rcu_read_unlock();
> +               if (ptrace_may_access(tsk, PTRACE_MODE_ATTACH)) {
> +                       put_task_struct(tsk);
> +                       return -EPERM;
> +               }
> +               if (arg2 <= 0)
> +                       tsk->timer_slack_ns =
> +                               tsk->default_timer_slack_ns;
> +               else
> +                       tsk->timer_slack_ns = arg2;
> +               put_task_struct(tsk);
> +               error = 0;
> +               break;
>         case PR_SET_CHILD_SUBREAPER:
>                 me->signal->is_child_subreaper = !!arg2;
>                 break;
> --
> 1.9.1
>

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

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

[Andrew Morton]

On Fri,  5 Feb 2016 10:08:43 -0800 John Stultz <john.stultz@linaro.org> wrote:

> From: Ruchi Kandoi <kandoiruchi@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 <micha@cellrox.com>
> have been folded into this patch to make it easier to reivew.
> 
> ...
>
> @@ -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:
> +		rcu_read_lock();
> +		tsk = find_task_by_vpid((pid_t)arg3);

hm, as far as I can tell this is the first instance in which prctl() is
used to play with a task other than "current".  Maybe this isn't a good
precedent.

If you look at all the other diddle-other-task functions in
kernel/sys.c, you'll see that they are standalone syscalls.  What
you've done here is just a bit lazy: added what is effectively a new
standalone syscall, only it has been hidden within the prctl() switch
statement.

I don't see a practical problem with this - we could have implemented
all those other syscalls as prctl submodes as well.  But we didn't...

IOW, it would be more consistent to add sys_set_timer_slack()?

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@chromium.org> wrote:
> On Fri, Feb 5, 2016 at 10:08 AM, John Stultz <john.stultz@linaro.org> wrote:
>> From: Ruchi Kandoi <kandoiruchi@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 <micha@cellrox.com>
>> have been folded into this patch to make it easier to reivew.
>>
>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Oren Laadan <orenl@cellrox.com>
>> Cc: Ruchi Kandoi <kandoiruchi@google.com>
>> Cc: Rom Lemarchand <romlem@android.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Android Kernel Team <kernel-team@android.com>
>> Signed-off-by: Ruchi Kandoi <kandoiruchi@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 <arjan@linux.intel.com>
>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: John Stultz <john.stultz@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

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:13 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri,  5 Feb 2016 10:08:43 -0800 John Stultz <john.stultz@linaro.org> wrote:
>> @@ -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:
>> +             rcu_read_lock();
>> +             tsk = find_task_by_vpid((pid_t)arg3);
>
> hm, as far as I can tell this is the first instance in which prctl() is
> used to play with a task other than "current".  Maybe this isn't a good
> precedent.
>
> If you look at all the other diddle-other-task functions in
> kernel/sys.c, you'll see that they are standalone syscalls.  What
> you've done here is just a bit lazy: added what is effectively a new
> standalone syscall, only it has been hidden within the prctl() switch
> statement.
>
> I don't see a practical problem with this - we could have implemented
> all those other syscalls as prctl submodes as well.  But we didn't...
>
> IOW, it would be more consistent to add sys_set_timer_slack()?

I'm fine with moving this way.

Ruchi/Rom: Any objections to that idea?

Thomas/Arjan: Any other functionality we should consider including
when adding a syscall to tweak timer slack?

thanks
-john

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

[Andrew Morton]

On Fri, 5 Feb 2016 12:23:13 -0800 John Stultz <john.stultz@linaro.org> wrote:

> On Fri, Feb 5, 2016 at 12:13 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Fri,  5 Feb 2016 10:08:43 -0800 John Stultz <john.stultz@linaro.org> wrote:
> >> @@ -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:
> >> +             rcu_read_lock();
> >> +             tsk = find_task_by_vpid((pid_t)arg3);
> >
> > hm, as far as I can tell this is the first instance in which prctl() is
> > used to play with a task other than "current".  Maybe this isn't a good
> > precedent.
> >
> > If you look at all the other diddle-other-task functions in
> > kernel/sys.c, you'll see that they are standalone syscalls.  What
> > you've done here is just a bit lazy: added what is effectively a new
> > standalone syscall, only it has been hidden within the prctl() switch
> > statement.
> >
> > I don't see a practical problem with this - we could have implemented
> > all those other syscalls as prctl submodes as well.  But we didn't...
> >
> > IOW, it would be more consistent to add sys_set_timer_slack()?
> 
> I'm fine with moving this way.
> 
> Ruchi/Rom: Any objections to that idea?
> 
> Thomas/Arjan: Any other functionality we should consider including
> when adding a syscall to tweak timer slack?

A syscall is quite a bit more fuss - implement it on x86_64, provide a
no-op default in sys_ni.c, add a test suite into
tools/testing/selftests (mainly for arch maintainers), wait for the
various arch maintainers to wire it up.

Fortunately the build system now emits little messages which tell
maintainers that there's a new syscall which needs looking at.

And a manpage will be needed, but a prctl manpage patch would have been
needed anyway.

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:32 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 5 Feb 2016 12:23:13 -0800 John Stultz <john.stultz@linaro.org> wrote:
>> On Fri, Feb 5, 2016 at 12:13 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>>
>>> IOW, it would be more consistent to add sys_set_timer_slack()?
>>
>> I'm fine with moving this way.
>>
>> Ruchi/Rom: Any objections to that idea?
>>
>> Thomas/Arjan: Any other functionality we should consider including
>> when adding a syscall to tweak timer slack?
>
> A syscall is quite a bit more fuss - implement it on x86_64, provide a
> no-op default in sys_ni.c, add a test suite into
> tools/testing/selftests (mainly for arch maintainers), wait for the
> various arch maintainers to wire it up.

Yea. It is. And I'm not excited to start over on this, but this
functionality has already run into trouble in the Android tree, as the
PR_SET_TIMERSLACK_PID value has hit multiple collisions over time.  So
this functionality upstream would help resolve that pain.

> Fortunately the build system now emits little messages which tell
> maintainers that there's a new syscall which needs looking at.
>
> And a manpage will be needed, but a prctl manpage patch would have been
> needed anyway.

Yea.

thanks
-john

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

[Kees Cook]

On Fri, Feb 5, 2016 at 12:39 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Feb 5, 2016 at 12:32 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Fri, 5 Feb 2016 12:23:13 -0800 John Stultz <john.stultz@linaro.org> wrote:
>>> On Fri, Feb 5, 2016 at 12:13 PM, Andrew Morton
>>> <akpm@linux-foundation.org> wrote:
>>>>
>>>> IOW, it would be more consistent to add sys_set_timer_slack()?
>>>
>>> I'm fine with moving this way.
>>>
>>> Ruchi/Rom: Any objections to that idea?
>>>
>>> Thomas/Arjan: Any other functionality we should consider including
>>> when adding a syscall to tweak timer slack?
>>
>> A syscall is quite a bit more fuss - implement it on x86_64, provide a
>> no-op default in sys_ni.c, add a test suite into
>> tools/testing/selftests (mainly for arch maintainers), wait for the
>> various arch maintainers to wire it up.
>
> Yea. It is. And I'm not excited to start over on this, but this
> functionality has already run into trouble in the Android tree, as the
> PR_SET_TIMERSLACK_PID value has hit multiple collisions over time.  So
> this functionality upstream would help resolve that pain.
>
>> Fortunately the build system now emits little messages which tell
>> maintainers that there's a new syscall which needs looking at.
>>
>> And a manpage will be needed, but a prctl manpage patch would have been
>> needed anyway.
>
> Yea.

Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

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

[Andrew Morton]

On Fri, 5 Feb 2016 12:44:04 -0800 Kees Cook <keescook@chromium.org> wrote:

> On Fri, Feb 5, 2016 at 12:39 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Fri, Feb 5, 2016 at 12:32 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >> On Fri, 5 Feb 2016 12:23:13 -0800 John Stultz <john.stultz@linaro.org> wrote:
> >>> On Fri, Feb 5, 2016 at 12:13 PM, Andrew Morton
> >>> <akpm@linux-foundation.org> wrote:
> >>>>
> >>>> IOW, it would be more consistent to add sys_set_timer_slack()?
> >>>
> >>> I'm fine with moving this way.
> >>>
> >>> Ruchi/Rom: Any objections to that idea?
> >>>
> >>> Thomas/Arjan: Any other functionality we should consider including
> >>> when adding a syscall to tweak timer slack?
> >>
> >> A syscall is quite a bit more fuss - implement it on x86_64, provide a
> >> no-op default in sys_ni.c, add a test suite into
> >> tools/testing/selftests (mainly for arch maintainers), wait for the
> >> various arch maintainers to wire it up.
> >
> > Yea. It is. And I'm not excited to start over on this, but this
> > functionality has already run into trouble in the Android tree, as the
> > PR_SET_TIMERSLACK_PID value has hit multiple collisions over time.  So
> > this functionality upstream would help resolve that pain.
> >
> >> Fortunately the build system now emits little messages which tell
> >> maintainers that there's a new syscall which needs looking at.
> >>
> >> And a manpage will be needed, but a prctl manpage patch would have been
> >> needed anyway.
> >
> > Yea.
> 
> Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?

/proc/<pid>/timer_slack_ns, guarded by ptrace_may_access(), documented
under Documentation/?  Yup, that would work.  It's there for all
architectures from day one and there is precedent.  It's not as nice,
but /proc nasties will always be with us.

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:50 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 5 Feb 2016 12:44:04 -0800 Kees Cook <keescook@chromium.org> wrote:
>> Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?
>
> /proc/<pid>/timer_slack_ns, guarded by ptrace_may_access(), documented
> under Documentation/?  Yup, that would work.  It's there for all
> architectures from day one and there is precedent.  It's not as nice,
> but /proc nasties will always be with us.

Ok. I'll start working on that.

Thanks for the feedback and ideas!
-john

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

[John Stultz]

On Fri, Feb 5, 2016 at 2:35 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Fri, Feb 5, 2016 at 12:50 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Fri, 5 Feb 2016 12:44:04 -0800 Kees Cook <keescook@chromium.org> wrote:
>>> Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?
>>
>> /proc/<pid>/timer_slack_ns, guarded by ptrace_may_access(), documented
>> under Documentation/?  Yup, that would work.  It's there for all
>> architectures from day one and there is precedent.  It's not as nice,
>> but /proc nasties will always be with us.
>
> Ok. I'll start working on that.

Arjan/Thomas:  One curious thing I noticed here while writing some
documentation. The timer_slack_ns value in the task struct is a
unsigned long.

So this means PR_SET_TIMERSLACK limits the maximum slack on 32 bit
machines to ~4 seconds. Where on 64bit machines it can be quite a bit
longer (unreasonably long, really :).

While 4 seconds is probably a reasonable interactivity limit, testing
w/ 10 second slack values on a VM showed those timers pushed back to
almost 10 seconds. So it may be useful to have > 4 second slack values
generally. Thus left alone this seems like an unfair disadvantage to
32bit machines.

We can't do too much about the PR_GET_TIMERSLACK/PR_SET_TIMERSLACK
interfaces, since its ABI and specifies a long, so one option there
would be to make sure the value specified is capped to UINT_MAX which
would keep the max value to ~4 seconds on all architectures.

Alternatively, with the /proc/pid/timerslack_ns interface I'm working
on, we can make the backing storage a long long and support 64bits of
nanoseconds on all architectures. (But again, we can't really change
PR_SET/GET_TIMERSLACK, so 32bit systems might see strange values from
that with larger then uint slack values).

Or I can just leave it as ULONG_MAX on all interfaces.

Thoughts or preferences?

thanks
-john

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

[Andrew Morton]

On Fri, 5 Feb 2016 16:51:02 -0800 John Stultz <john.stultz@linaro.org> wrote:

> On Fri, Feb 5, 2016 at 2:35 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Fri, Feb 5, 2016 at 12:50 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >> On Fri, 5 Feb 2016 12:44:04 -0800 Kees Cook <keescook@chromium.org> wrote:
> >>> Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?
> >>
> >> /proc/<pid>/timer_slack_ns, guarded by ptrace_may_access(), documented
> >> under Documentation/?  Yup, that would work.  It's there for all
> >> architectures from day one and there is precedent.  It's not as nice,
> >> but /proc nasties will always be with us.
> >
> > Ok. I'll start working on that.
> 
> Arjan/Thomas:  One curious thing I noticed here while writing some
> documentation. The timer_slack_ns value in the task struct is a
> unsigned long.
> 
> So this means PR_SET_TIMERSLACK limits the maximum slack on 32 bit
> machines to ~4 seconds. Where on 64bit machines it can be quite a bit
> longer (unreasonably long, really :).
> 
> While 4 seconds is probably a reasonable interactivity limit, testing
> w/ 10 second slack values on a VM showed those timers pushed back to
> almost 10 seconds. So it may be useful to have > 4 second slack values
> generally. Thus left alone this seems like an unfair disadvantage to
> 32bit machines.
> 
> We can't do too much about the PR_GET_TIMERSLACK/PR_SET_TIMERSLACK
> interfaces, since its ABI and specifies a long, so one option there
> would be to make sure the value specified is capped to UINT_MAX which
> would keep the max value to ~4 seconds on all architectures.
> 
> Alternatively, with the /proc/pid/timerslack_ns interface I'm working
> on, we can make the backing storage a long long and support 64bits of
> nanoseconds on all architectures. (But again, we can't really change
> PR_SET/GET_TIMERSLACK, so 32bit systems might see strange values from
> that with larger then uint slack values).
> 
> Or I can just leave it as ULONG_MAX on all interfaces.
> 
> Thoughts or preferences?

/proc/<pid>/timer_slack_us?

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

[Arjan van de Ven]

On 2/5/2016 4:51 PM, John Stultz wrote:
> On Fri, Feb 5, 2016 at 2:35 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Fri, Feb 5, 2016 at 12:50 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>> On Fri, 5 Feb 2016 12:44:04 -0800 Kees Cook <keescook@chromium.org> wrote:
>>>> Could this be exposed as a writable /proc entry instead? Like the oom_* stuff?
>>>
>>> /proc/<pid>/timer_slack_ns, guarded by ptrace_may_access(), documented
>>> under Documentation/?  Yup, that would work.  It's there for all
>>> architectures from day one and there is precedent.  It's not as nice,
>>> but /proc nasties will always be with us.
>>
>> Ok. I'll start working on that.
>
> Arjan/Thomas:  One curious thing I noticed here while writing some
> documentation. The timer_slack_ns value in the task struct is a
> unsigned long.
>
> So this means PR_SET_TIMERSLACK limits the maximum slack on 32 bit
> machines to ~4 seconds. Where on 64bit machines it can be quite a bit
> longer (unreasonably long, really :).

originally when we created timerslack, 4 seconds was an eternity and good enough for everyone
by a mile... (assumption was practical upper limit being in the 15 msec range)
and most of the RT guys would only tolerate a little bit of it

is there any real/practial use of going longer than 4 seconds? if there
is then yeah fixing it makes sense.
if it's just theoretical... shrug... 32 bit systems have a bunch of
other limits/differences a well.

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

[John Stultz]

On Fri, Feb 5, 2016 at 5:56 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 5 Feb 2016 16:51:02 -0800 John Stultz <john.stultz@linaro.org> wrote:
>> Alternatively, with the /proc/pid/timerslack_ns interface I'm working
>> on, we can make the backing storage a long long and support 64bits of
>> nanoseconds on all architectures. (But again, we can't really change
>> PR_SET/GET_TIMERSLACK, so 32bit systems might see strange values from
>> that with larger then uint slack values).
>>
>> Or I can just leave it as ULONG_MAX on all interfaces.
>>
>> Thoughts or preferences?
>
> /proc/<pid>/timer_slack_us?

So the issue isn't so much in the new interface (we can have it take a
long long), but really in the existing PR_GET/SET_TIMERSLACK. I'm just
trying to figure out if following the existing oddness is the best
approach, or if we should make the new interface do a more consistent
thing, but with the result that the PR_GET_TIMERSLACK interface might
return "incorrect" values (just the lower 32bits).

thanks
-john

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

[John Stultz]

On Fri, Feb 5, 2016 at 6:15 PM, Arjan van de Ven <arjan@linux.intel.com> wrote:
> On 2/5/2016 4:51 PM, John Stultz wrote:
>>
>> Arjan/Thomas:  One curious thing I noticed here while writing some
>> documentation. The timer_slack_ns value in the task struct is a
>> unsigned long.
>>
>> So this means PR_SET_TIMERSLACK limits the maximum slack on 32 bit
>> machines to ~4 seconds. Where on 64bit machines it can be quite a bit
>> longer (unreasonably long, really :).
>
>
> originally when we created timerslack, 4 seconds was an eternity and good
> enough for everyone
> by a mile... (assumption was practical upper limit being in the 15 msec
> range)
> and most of the RT guys would only tolerate a little bit of it
>
> is there any real/practial use of going longer than 4 seconds? if there
> is then yeah fixing it makes sense.
> if it's just theoretical... shrug... 32 bit systems have a bunch of
> other limits/differences a well.

So I'd think it would be mostly theoretical, but in my testing on a
VM, setting the timerslack for bash to 10 secs made time sleep 1 take
~10.5 seconds. So its apparently not too hard to coalesce fairly far
out (I need to spend a bit more time to verify that events really
weren't happening during that time and we're not just doing
unnecessary delays with the extra slack).

But yea. My main concern is that if we do a consistent 64bit interface
for all arches in the /proc/<pid>/timerslack_ns interface, it will
make PR_GET_TIMERSLACK return incorrect results on 32bit systems when
the slack is >= 2^32.

I've got a first pass of the patch done which just uses ULONG_MAX on
the respective arch, but I've got to close up for the day, so I'll see
about doing a follow on patch that makes the task timer_slack_ns value
be a u64 and extends the interface to use that as well on all arches,
and send out both on monday so folks can see which they prefer.

thanks
-john

thanks
-john

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

[Arjan van de Ven]

>> and most of the RT guys would only tolerate a little bit of it
>>
>> is there any real/practial use of going longer than 4 seconds? if there
>> is then yeah fixing it makes sense.
>> if it's just theoretical... shrug... 32 bit systems have a bunch of
>> other limits/differences a well.
>
> So I'd think it would be mostly theoretical, but in my testing on a
> VM, setting the timerslack for bash to 10 secs made time sleep 1 take
> ~10.5 seconds. So its apparently not too hard to coalesce fairly far
> out (I need to spend a bit more time to verify that events really
> weren't happening during that time and we're not just doing
> unnecessary delays with the extra slack).

99% sure you're hitting something else;
we look pretty much only 1 ahead in the queue for timers to run to see if
they can be run, once we hit a timer that's not ready yet we stop.
your 10 second ahead is behind a whole bunch of other not-ready ones
so won't even be looked at until its close


> But yea. My main concern is that if we do a consistent 64bit interface
> for all arches in the /proc/<pid>/timerslack_ns interface, it will
> make PR_GET_TIMERSLACK return incorrect results on 32bit systems when
> the slack is >= 2^32.

or we return UINT_MAX for that case. not too hard.