linux-kernel - Re: [PATCH] mm: whack now bogus comment in pmd

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] [day] [month] [year] [list]

Message-ID: <CAGudoHFFRRPau=iCYFjNQV_HSnC582ZZgY=iJ1HrS=sCAQ6mRw@mail.gmail.com>
Date: Wed, 14 Aug 2024 23:28:36 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: npiggin@...il.com, david@...hat.com, willy@...radead.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] mm: whack now bogus comment in pmd_install() concerning a fence

On Wed, Aug 14, 2024 at 11:16 PM Andrew Morton
<akpm@...ux-foundation.org> wrote:
>
> On Wed, 14 Aug 2024 16:52:56 +0200 Mateusz Guzik <mjguzik@...il.com> wrote:
>
> > Commit 362a61ad6119 ("fix SMP data race in pagetable setup vs walking")
> > added the following:
> >
> > +       smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
> > +
> >         spin_lock(&mm->page_table_lock);
> >
> > However, over the years the fence along with the comment got moved
> > around the file, eventually landing in a spot where it is *NOT* followed
> > by a lock acquire (or any other operation which might happen to provide
> > any fence on a given arch), rendering the comment stale.
> >
> > ...
> >
> > I fully concede I could not be arsed to check if the fence is still
> > needed to begin with, I ran into this while looking at something else.
> > The comment puzzled me for a minute suggesting pmd_populate has an
> > immediate lock acquire inside.
> >
> > ...
> >
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -436,7 +436,7 @@ void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
> >                * seen in-order. See the alpha page table accessors for the
> >                * smp_rmb() barriers in page table walking code.
> >                */
> > -             smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
> > +             smp_wmb();
> >               pmd_populate(mm, pmd, *pte);
> >               *pte = NULL;
> >       }
>
> It's best to document all such barriers, so the preferred patch would
> be to fix the comment rather than removing it.
>
> And if the barrier now does nothing then of course removing the thing
> would be best.
>
> So I'd suggest that the wrong comment be left there, if only to tell
> developers why the barrier used to be there!

The comment above it (only partially seen in the context) documents
what the purpose is.

The comment I'm removing merely mentions a no longer applicable
optimization opportunity: it used to be immediately followed by
spin_lock. If the architecture at hand provides a full fence when
acquiring a lock *and* has a costly smp_wmb, then a hypothetical
smp_wmb__before_spin_lock could be used to elide it.

-- 
Mateusz Guzik <mjguzik gmail.com>