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

Yang Xu
2019-07-23T08:11:20Z

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@cn.fujitsu.com>
>> ---
>> 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@linux.intel.com>
> 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@asianux.com>
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?
>
>
>