Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK

[Yang Xu]

on 2019/07/23 15:23, Cyrill Gorcunov wrote:

> On Tue, Jul 23, 2019 at 11:30:53AM +0800, Yang Xu wrote:
> > arg2 will never<  0, for its type is 'unsigned long'. So negative
> > judgment is meaningless.
> >
> > Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxxxxx>
> > ---
> >   kernel/sys.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 2969304c29fe..399457d26bef 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> >   			error = current->timer_slack_ns;
> >   		break;
> >   	case PR_SET_TIMERSLACK:
> > -		if (arg2<= 0)
> > +		if (arg2)
> > +			current->timer_slack_ns = arg2;
> > +		else
> >   			current->timer_slack_ns =
> >   					current->default_timer_slack_ns;
> > -		else
> > -			current->timer_slack_ns = arg2;
> >   		break;
> >   	case PR_MCE_KILL:
> >   		if (arg4 | arg5)
> > From a glance it looks correct to me, but then...
>
> 1) you might simply compare with zero, iow if (arg2 == 0)
>     instead of changing 7 lines
Hi Cyril

Indeed.  simply compare with zero might be better.

> 2) according to man page passing negative value should be acceptable,
>     though it never worked as expected. I've been grepping "git log"
>     for this file and the former API is coming from
>
> commit 6976675d94042fbd446231d1bd8b7de71a980ada
> Author: Arjan van de Ven<arjan@xxxxxxxxxxxxxxx>
> Date:   Mon Sep 1 15:52:40 2008 -0700
>
>      hrtimer: create a "timer_slack" field in the task struct
>
> which is 11 years old by now. Nobody complained so far even when man
> page is saying pretty obviously
>
>         PR_SET_TIMERSLACK (since Linux 2.6.28)
>                Each thread has two associated timer slack values:  a  "default"
>                value, and a "current" value.  This operation sets the "current"
>                timer slack value for the calling  thread.   If  the  nanosecond
>                value  supplied in arg2 is greater than zero, then the "current"
>                value is set to this value.  If arg2 is less than  or  equal  to
>                zero,  the  "current"  timer  slack  is  reset  to  the thread's
>                "default" timer slack value.
>
> So i think to match the man page (and assuming that accepting negative value
> has been supposed) we should rather do
>
> 	if ((long)arg2<  0)
Looks correct. But if we set a ULONG_MAX(PR_GET_TIMERSLACK also limits ULONG_MAX)
value(about 4s) on 32bit machine, this code will think this value is a negative value and use default value.

I guess man page was written as "less than or equal to zero" because of this confusing code(arg2<=0, but arg2
is an unsinged long value).
I think we can change this man page and also add bounds value description.

Also, I found a patch about arg2 is an unsigned long value

commit 7fe5e04292e71af34ae171b88caa2a139e0b6125
Author: Chen Gang<gang.chen@xxxxxxxxxxx>
Date:   Thu Feb 21 16:43:06 2013 -0800

    sys_prctl(): arg2 is unsigned long which is never<  0

    arg2 will never<  0, for its type is 'unsigned long'

    Also, use the provided macros.

What do you think about it ?

> Thoughts?