linux-kernel - Re: [GIT PULL] sched/urgent for 5.18-rc4

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: <CAKfTPtD2QEyZ6ADd5WrwETMOX0XOwJGnVddt7VHgfURdqgOS-Q@mail.gmail.com>
Date:   Sun, 24 Apr 2022 21:31:42 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Borislav Petkov <bp@...e.de>, kuyo chang <kuyo.chang@...iatek.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] sched/urgent for 5.18-rc4

On Sun, 24 Apr 2022 at 21:00, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Sun, Apr 24, 2022 at 2:55 AM Borislav Petkov <bp@...e.de> wrote:
> >
> > - Fix a corner case when calculating sched runqueue variables
>
> This worries me.
>
> It now does:
>
> +       if (se_weight(se) < se->avg.load_sum)
> +               se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
>
> and at no point does it check if se_weight(se) is zero.
>
> It *used* to check for that divide-by-zero issue, so from what I can
> tell, a zero se_weight() is actually possible.
>
> Now, it's entirely possible that no, se_weight() can never go down to
> zero. But it's not obvious,. and the commit message doesn't mention
> this change at all.
>
> So I pulled, but then after looking at it I unpulled again in the
> hopes that somebody will clarify the issue for me.
>
> And scale_load_down() (in se_weight()) does try to make the result be
> at least 2 on 64-bit, but only if the original wasn't zero. Very
> confusing.
>
> So can somebody please tell me why se_weight() cannot be 0, and why we
> _used_ to check for zero? Because that commit sure as heck doesn't
> explain it.

For task, weight can't be null
For task group, weight is initialized to nice 0 in init_tg_cfs_entry()
and then it's clamp in calc_group_shares in order to not be null
Then since 26cf52229efc ("sched: Avoid scale real weight down to
zero"), scale_load_down can't return null value.

In fact, the condition if (se_weight(se)) was not needed any more and
should have been removed with commit 26cf52229efc

>
> And - as usual with the -tip tree - the "Link:" thing is almost
> entirely pointless. It doesn't actually point to any discussion of the
> problems, it only points to the patch submission.
>
> I realize that is convenient for automation, but it's really not
> generally a very useful link. It would be much more useful to link to
> whatever problem report that actually *causes* the submission, not to
> the submission itself. We already see the end result in the commit,
> it's the "how did we get here" that is the most interesting part.
>
> And no, I don't see any explanation for "why se_weight() cannot be
> zero" in that submission thread either.
>
> Somebody please hit me over the head with a clue bat.
>
>                  Linus