linux-kernel - Re: [PATCH] kernel/hung_task.c: Fix a typo in check_hung

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: <c40abc2d-2343-2eba-a5e4-4309a2577908@windriver.com>
Date:   Sat, 7 Aug 2021 20:21:25 +0800
From:   Jun Miao <jun.miao@...driver.com>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        deller@....de, wei.liu@...nel.org,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel/hung_task.c: Fix a typo in check_hung_task()
 comment


On 8/6/21 10:27 PM, Lukas Bulwahn wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Fri, Aug 6, 2021 at 1:41 PM Jun Miao <jun.miao@...driver.com> wrote:
>> It's "mustn't", not "musn't". Let's fix that.
>>
>> Signed-off-by: Jun Miao <jun.miao@...driver.com>
>> ---
>>   kernel/hung_task.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
>> index 9888e2bc8c76..ea5ba912db06 100644
>> --- a/kernel/hung_task.c
>> +++ b/kernel/hung_task.c
>> @@ -99,7 +99,7 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout)
>>          /*
>>           * When a freshly created task is scheduled once, changes its state to
>>           * TASK_UNINTERRUPTIBLE without having ever been switched out once, it
>> -        * musn't be checked.
>> +        * mustn't be checked.
> I cannot even parse this comment.
>
> Does "When a freshly created task is scheduled once, changes its state
> to TASK_UNINTERRUPTIBLE" mean "When a freshly created task is
> scheduled once and it changes its state to TASK_UNINTERRUPTIBLE"?
>
> Does this "it must not be checked" read as "it shall not be checked"
> (as in "because if you check it, something goes wrong") or "it is not
> required to be checked" (as in "usually, you need to check it
> (otherwise something goes wrong), but here in this case, you do not
> need to check it, because it cannot go wrong in this case")?
>
> Fixing spelling mistakes is okay, but it is even better to check the
> sentence you are correcting and try to comprehend it.

Hi Lukas, thanks for your advice from my heart.

 From the context of code:
---
  90         unsigned long switch_count = t->nvcsw + t->nivcsw;
  91
  ... ...
  99         /*
100          * When a freshly created task is scheduled once, changes 
its state to
101          * TASK_UNINTERRUPTIBLE without having ever been switched 
out once, it
102          * mustn't be checked.
103          */
104         if (unlikely(!switch_count))
105                 return;
---

It should read as "it shall not be checked" (as in "because if you check 
it, something goes wrong")
. When create a task and set it as "TASK_UNINTERRUPTIBLE" we don`t need 
to check "swtich_count=0".
If check will report a false positive.

 From a history commit cf2592f59c0e, there is a detail explain:
     - the task is freshly created and scheduled
     - it puts its state to TASK_UNINTERRUPTIBLE and is not yet switched out
     - check_hung_task() scans this task and will report a false 
positive because
       t->nvcsw + t->nivcsw == t->last_switch_count == 0

     Add a check for such cases.

Thanks
Jun
> Lukas