linux-kernel - Re: [PATCH 09/13] x86/mm/pae: Use WRITE

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-=whVRHVJAOOc=+1eQMSuWSoRAp+yGkb3Kob2=7r05M+oXw@mail.gmail.com>
Date:   Sat, 22 Oct 2022 10:42:52 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, willy@...radead.org, akpm@...ux-foundation.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        aarcange@...hat.com, kirill.shutemov@...ux.intel.com,
        jroedel@...e.de, ubizjak@...il.com
Subject: Re: [PATCH 09/13] x86/mm/pae: Use WRITE_ONCE()

On Sat, Oct 22, 2022 at 4:48 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
>  static inline void native_set_pte(pte_t *ptep, pte_t pte)
>  {
> -       ptep->pte_high = pte.pte_high;
> +       WRITE_ONCE(ptep->pte_high, pte.pte_high);
>         smp_wmb();
> -       ptep->pte_low = pte.pte_low;
> +       WRITE_ONCE(ptep->pte_low, pte.pte_low);

With this, the smp_wmb() should just go away too. It was really only
ever there as a compiler barrier.

Two WRITE_ONCE() statements are inherently ordered for the compiler
(due to volatile rules), and x86 doesn't re-order writes.

It's not a big deal, since smp_wmb() is just a barrier() on x86-64
anyway, but it might make some improvement to code generation to
remove it, and the smp_wmb() really isn't adding anything.

If somebody likes the smp_wmb() as a comment, I think it would be
better to actually _make_ it a comment, and have these functions turn
into just

  /* Force ordered word-sized writes, set low word with present bit last */
  static inline void native_set_pte(pte_t *ptep, pte_t pte)
  {
        WRITE_ONCE(ptep->pte_high, pte.pte_high);
        WRITE_ONCE(ptep->pte_low, pte.pte_low);
  }

or similar. I think that kind of one-liner comment is much more
informative than a "smp_wmb()".

Or do we already have a comment elsewhere about why the ordering is
important (and how *clearing* clears the low word with the present bit
first, but setting a *new* entry sets the high word first so that the
64-bit entry is complete when the present bit is set?)

                 Linus