linux-kernel - Re: [GIT pull] x86/urgent for v6.11-rc3

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: <CAHk-=wjasZbABR14SFRgkL8MZxP+X4qBUOUM3c97cL5gXuwrUQ@mail.gmail.com>
Date: Sun, 11 Aug 2024 11:07:29 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [GIT pull] x86/urgent for v6.11-rc3

On Sun, 11 Aug 2024 at 06:58, Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Chen Yu (1):
>       x86/paravirt: Fix incorrect virt spinlock setting on bare metal

Oh, and while looking at this pull some more, and agreeing violently
with this change, I did notice that the code that *uses* the
virt_spin_lock_key is a bit odd:

  static inline bool virt_spin_lock(struct qspinlock *lock)

does this:

        if (!static_branch_likely(&virt_spin_lock_key))
                return false;

and this looks really odd to me.

Our static key code is pretty confusing, but we basically have

 - virt_spin_lock_key is now a "struct static_key_false", which means
that we consider the virt case the unlikely case.

   I agree whole-heartedly, because it's going to be the slow case
anyway, so this is good.

 - that means that 'static_branch_likely()' will generate a branch
(because the key is marked unlikely0

Isn't this wrong? So instead of falling through to the native
qspinlock case, we will branch to it, and we fall through to the
virt-spinlock case?

So i think that static_branch_likely() should have been changed to a
static_branch_unlikely() too, but it's possible that I've just
confused myself.

Anyway, somebody should double-check me.

I doubt it actually matters, since I think this all is fundamentally
just in the slow-path, so the "do a branch or a no-op" is likely
entirely in the noise even if I followed the code right. But it looked
off to me.

               Linus