Re: sched: Disallow sched_attr::sched_policy < 0

Re: sched: Disallow sched_attr::sched_policy < 0

[Dave Jones]

On Mon, Jun 02, 2014 at 02:13:19AM +0000, Linux Kernel wrote:
 
 >     sched: Disallow sched_attr::sched_policy < 0
 >     
 >     The scheduler uses policy=-1 to preserve the current policy state to
 >     implement sys_sched_setparam(), this got exposed to userspace by
 >     accident through sys_sched_setattr(), cure this.
 >     
 > ---
 >  kernel/sched/core.c |    3 +++
 >  1 files changed, 3 insertions(+), 0 deletions(-)
 > 
 > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
 > index f2205f0..cdefcf7 100644
 > --- a/kernel/sched/core.c
 > +++ b/kernel/sched/core.c
 > @@ -3662,6 +3662,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 >  	if (retval)
 >  		return retval;
 >  
 > +	if (attr.sched_policy < 0)
 > +		return -EINVAL;
 > +
 >  	rcu_read_lock();
 >  	retval = -ESRCH;
 >  	p = find_process_by_pid(pid);
 
Todays coverity run picked up..

3687
>>>     CID 1219934:  Unsigned compared against 0  (NO_EFFECT)
>>>     This less-than-zero comparison of an unsigned value is never true. "attr.sched_policy < 0U".
3688            if (attr.sched_policy < 0)
3689                    return -EINVAL;

Re: sched: Disallow sched_attr::sched_policy < 0

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Mon, Jun 02, 2014 at 04:22:04PM -0400, Dave Jones wrote:
> On Mon, Jun 02, 2014 at 02:13:19AM +0000, Linux Kernel wrote:
>  
>  >     sched: Disallow sched_attr::sched_policy < 0
>  >     
>  >     The scheduler uses policy=-1 to preserve the current policy state to
>  >     implement sys_sched_setparam(), this got exposed to userspace by
>  >     accident through sys_sched_setattr(), cure this.
>  >     
>  > ---
>  >  kernel/sched/core.c |    3 +++
>  >  1 files changed, 3 insertions(+), 0 deletions(-)
>  > 
>  > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>  > index f2205f0..cdefcf7 100644
>  > --- a/kernel/sched/core.c
>  > +++ b/kernel/sched/core.c
>  > @@ -3662,6 +3662,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
>  >  	if (retval)
>  >  		return retval;
>  >  
>  > +	if (attr.sched_policy < 0)
>  > +		return -EINVAL;
>  > +
>  >  	rcu_read_lock();
>  >  	retval = -ESRCH;
>  >  	p = find_process_by_pid(pid);
>  
> Todays coverity run picked up..
> 
> 3687
> >>>     CID 1219934:  Unsigned compared against 0  (NO_EFFECT)
> >>>     This less-than-zero comparison of an unsigned value is never true. "attr.sched_policy < 0U".
> 3688            if (attr.sched_policy < 0)
> 3689                    return -EINVAL;
> 

Once upon a time GCC also did warns like that, but my compiler is silent
:-(

Yes, that needs fixing..

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: sched: Disallow sched_attr::sched_policy < 0

[Richard Weinberger]

On Tue, Jun 3, 2014 at 10:08 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jun 02, 2014 at 04:22:04PM -0400, Dave Jones wrote:
>> On Mon, Jun 02, 2014 at 02:13:19AM +0000, Linux Kernel wrote:
>>
>>  >     sched: Disallow sched_attr::sched_policy < 0
>>  >
>>  >     The scheduler uses policy=-1 to preserve the current policy state to
>>  >     implement sys_sched_setparam(), this got exposed to userspace by
>>  >     accident through sys_sched_setattr(), cure this.
>>  >
>>  > ---
>>  >  kernel/sched/core.c |    3 +++
>>  >  1 files changed, 3 insertions(+), 0 deletions(-)
>>  >
>>  > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>  > index f2205f0..cdefcf7 100644
>>  > --- a/kernel/sched/core.c
>>  > +++ b/kernel/sched/core.c
>>  > @@ -3662,6 +3662,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
>>  >    if (retval)
>>  >            return retval;
>>  >
>>  > +  if (attr.sched_policy < 0)
>>  > +          return -EINVAL;
>>  > +
>>  >    rcu_read_lock();
>>  >    retval = -ESRCH;
>>  >    p = find_process_by_pid(pid);
>>
>> Todays coverity run picked up..
>>
>> 3687
>> >>>     CID 1219934:  Unsigned compared against 0  (NO_EFFECT)
>> >>>     This less-than-zero comparison of an unsigned value is never true. "attr.sched_policy < 0U".
>> 3688            if (attr.sched_policy < 0)
>> 3689                    return -EINVAL;
>>
>
> Once upon a time GCC also did warns like that, but my compiler is silent
> :-(
>
> Yes, that needs fixing..

I sent already a patch for that...

-- 
Thanks,
//richard

Re: sched: Disallow sched_attr::sched_policy < 0

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

On Tue, Jun 03, 2014 at 10:15:44AM +0200, Richard Weinberger wrote:
> > Once upon a time GCC also did warns like that, but my compiler is silent
> > :-(
> >
> > Yes, that needs fixing..
> 
> I sent already a patch for that...

I found it.. thanks again :-)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: sched: Disallow sched_attr::sched_policy < 0

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Am 03.06.2014 10:32, schrieb Peter Zijlstra:
> On Tue, Jun 03, 2014 at 10:15:44AM +0200, Richard Weinberger wrote:
>>> Once upon a time GCC also did warns like that, but my compiler is silent
>>> :-(

-Wtype-limits is what you're looking for.

/me currently builds some kernel configs to find out how much noise it triggers...

>>> Yes, that needs fixing..
>>
>> I sent already a patch for that...
> 
> I found it.. thanks again :-)
> 

:-)

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: sched: Disallow sched_attr::sched_policy < 0

[Dave Jones]

On Tue, Jun 03, 2014 at 10:47:18AM +0200, Richard Weinberger wrote:
 > Am 03.06.2014 10:32, schrieb Peter Zijlstra:
 > > On Tue, Jun 03, 2014 at 10:15:44AM +0200, Richard Weinberger wrote:
 > >>> Once upon a time GCC also did warns like that, but my compiler is silent
 > >>> :-(
 > 
 > -Wtype-limits is what you're looking for.
 > 
 > /me currently builds some kernel configs to find out how much noise it triggers...
 
Probably quite a bit I'd bet. There's a load of similar bugs in
coverity's db. Some of them look benign, and are probably there just in
case someone ever changes the type of a var, but it's non-obvious
sometimes if the values a function receives can ever actually be < 0

	Dave

Re: sched: Disallow sched_attr::sched_policy < 0

[Linus Torvalds]

On Tue, Jun 3, 2014 at 1:08 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Once upon a time GCC also did warns like that, but my compiler is silent
> :-(

You should be happy. The gcc warnings were shit.

Iirc, gcc literally at one point warned about things like

   unsigned int i;

   if (i < 5)

because that's comparing an unsigned type ("i") with an expression
having a signed type ("5"). Yes, technically true, but it's not
actually a useful warning.

That got fixed pretty quickly, but I think gcc *still* warns about things like

    unsigned int i;

    if (i >= 0 && i <= 6)
        ...

which is actually a very valid thing to do, and is commonly the result
of using a range-checking macro, or in general writing code so that it
is robust and doesn't care about the actual underlying type.

Warnings about robust code are f*cking broken, and easily worse than
not having the warning at all. Because it results in people removing
the range check.

Btw, -Wsign-compare still complains about

   int i;

   if (i < 0 || i > sizeof(i))
       return error;

which is another example of a f*cking broken warning. There is no way
to avoid that warning without making the code worse. That code is
_correct_, dammit, and anybody who thinks it should warn (or the
programmer should cast the sizeof to "int") is a tool and a moron.

End result: disabling "-Wsign-compare" is thus the only correct thing
to do. Sadly compiler writers don't seem to care too deeply about the
sanity of their warnings.

              Linus