linux-kernel - Re: [kernel-sched-cputime] question about probable bug in cputime

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170629125804.Horde.gm62Mi9SrTXYOk8B_t9Srrr@gator4166.hostgator.com>
Date:   Thu, 29 Jun 2017 12:58:04 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Frans Klaver <fransklaver@...il.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [kernel-sched-cputime] question about probable bug in
 cputime_adjust()


Quoting Frans Klaver <fransklaver@...il.com>:

> On 29 June 2017 01:57:19 CEST, "Gustavo A. R. Silva"  
> <garsilva@...eddedor.com> wrote:
>>>>> --- a/kernel/sched/cputime.c
>>>>> +++ b/kernel/sched/cputime.c
>>>>> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>>>>          *            = (rtime_i+1 - rtime_i) + utime_i
>>>>>          *            >= utime_i
>>>>>          */
>>>>> -       if (stime < prev->stime)
>>>>> +       if (stime < prev->stime) {
>>>>>                 stime = prev->stime;
>>>>> -       utime = rtime - stime;
>>>>> +               utime = rtime - stime;
>>>>> +       }
>>>>>
>>>>>
>>>>> If you confirm this, I will send a patch in a full and proper form.
>>>>>
>>>>> I'd really appreciate your comments.
>>>>
>>>> If you do that, how would you meet the guarantee made in line 583?
>>>>
>>
>> You are right, I see now.
>>
>> Then in this case the following patch would be the way to go:
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -615,10 +615,8 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>       * userspace. Once a task gets some ticks, the monotonicy code at
>>          * 'update' will ensure things converge to the observed ratio.
>>          */
>> -       if (stime == 0) {
>> -               utime = rtime;
>> +       if (stime == 0)
>>                 goto update;
>> -       }
>>
>>         if (utime == 0) {
>>                 stime = rtime;
>>
>>
>> but I think this one is even better:
>>
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -615,19 +615,11 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>       * userspace. Once a task gets some ticks, the monotonicy code at
>>          * 'update' will ensure things converge to the observed ratio.
>>          */
>> -       if (stime == 0) {
>> -               utime = rtime;
>> -               goto update;
>> -       }
>> -
>> -       if (utime == 0) {
>> +       if (stime != 0 && utime == 0)
>>                 stime = rtime;
>> -               goto update;
>> -       }
>> -
>> -       stime = scale_stime(stime, rtime, stime + utime);
>> +       else
>> +               stime = scale_stime(stime, rtime, stime + utime);
>
> I don't think it is better. The stime == 0 case is gone now. So  
> scale_time() will be called in that case. This whole if/else block  
> should only be executed if stime != 0.

Oh yeah! something like:

if (stime != 0) {
         if (stime != 0 && utime == 0)
                 stime = rtime;
         else
                 stime = scale_stime(stime, rtime, stime + utime);
}

I'll be right back with the final patch.

Thanks for your time, Frans.
Much appreciated :)
--
Gustavo A. R. Silva