linux-kernel - Re: [PATCH] selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"

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: <f0503a61-f605-468e-ae15-c4934faea632@lucifer.local>
Date: Sun, 20 Jul 2025 10:23:20 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Laight <david.laight.linux@...il.com>
Cc: David Hildenbrand <david@...hat.com>, wang lian <lianux.mm@...il.com>,
        akpm@...ux-foundation.org, Liam.Howlett@...cle.com, brauner@...nel.org,
        gkwang@...x-info.com, jannh@...gle.com, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, ludovico.zy.wu@...il.com, p1ucky0923@...il.com,
        ryncsn@...il.com, sj@...nel.org, vbabka@...e.cz,
        zijing.zhang@...ton.me, ziy@...dia.com, shuah@...nel.org,
        broonie@...nel.org
Subject: Re: [PATCH] selftests/mm: reuse FORCE_READ to replace "asm
 volatile("" : "+r" (XXX));"

On Sat, Jul 19, 2025 at 10:27:38AM +0100, David Laight wrote:
> On Thu, 17 Jul 2025 13:43:45 +0200
> David Hildenbrand <david@...hat.com> wrote:
>
> > On 17.07.25 12:48, wang lian wrote:
> > >> On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@...il.com> wrote:
> > >
> > >>> Several mm selftests use the `asm volatile("" : "+r" (variable));`
> > >>> construct to force a read of a variable, preventing the compiler from
> > >>> optimizing away the memory access. This idiom is cryptic and duplicated
> > >>> across multiple test files.
> > >>>
> > >>> Following a suggestion from David[1], this patch refactors this
> > >>> common pattern into a FORCE_READ() macro
> > >>>
> > >>>   tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
> > >>>   tools/testing/selftests/mm/hugetlb-madvise.c  |  5 +---
> > >>>   tools/testing/selftests/mm/migration.c        | 13 ++++----
> > >>>   tools/testing/selftests/mm/pagemap_ioctl.c    |  4 +--
> > >>>   .../selftests/mm/split_huge_page_test.c       |  4 +--
> > >>>   5 files changed, 24 insertions(+), 32 deletions(-)
> > >
> > >> The patch forgot to move the FORCE_READ definition into a header?
> > >
> > > Hi Andrew,
> > > You are absolutely right. My apologies for the inconvenience.
> > > This patch was sent standalone based on a suggestion from David during
> > > the discussion of a previous, larger patch series. In that original series,
> > > I had already moved the FORCE_READ() macro definition into vm_util.h.
> > >
> > > You can find the original patch series and discussion at this link:
> > > https://lore.kernel.org/lkml/20250714130009.14581-1-lianux.mm@gmail.com/
> > > It should also be in your mailing list archive.
> > >
> > > To make this easier to review and apply, I can send a new, small patch series
> > > that first introduces the FORCE_READ() macro in vm_util.h and then applies this refactoring.
> >
> > Please simply perform the move of FORCE_READ() in this very patch where
> > you also use it elswehere.
>
> Why not use READ_ONCE() instead (or even just make all the variables 'volatile char *').
> I had to look up the definition to find the hidden indirection in FORCE_READ().

Honestly this is an incredible level of nitpicking for a _self test_
patch.

I don't think you need to look up definitions to understand that volatile
prevents the compiler from optimising out a read like this. And what
exactly is hidden? We cast to the volatile type of the pointer, then deref
it in a fashion that cannot be optimised out?

But I mean, maybe I'm missing some complexity here? I am always happy to be
corrected :)

The point is to read fault a page for testing purposes.

This is fine, works, and it's _test code_.

Overall though, this discussion is not helpful and this is a moot point,
sorry.

>
> It has to be said that now writes to variables that are READ_ONCE() have to be
> WRITE_ONCE() why not just make the variables 'volatile'.
> That will stop things bleating about missing READ/WRITE_ONCE() wrappers.
> There was a rational for not using volatile, but it is getting to be moot.

I'm struggling to understand what on earth you're talking about here, what
would bleat about self test code missing READ/WRITE_ONCE() wrappers?

And you're suggesting going through and changing all pointers to be
volatile... because why? What? That'd be awful and very very silly.

Note that checkpatch.pl _will_ bleat about this.

TL;DR: No, absolutely not.

Wang - do not do anything like this, please!

>
> 	David
>
>
> >
> > I missed that when skimming over this patch.
> >
>

Let's all have some empathy for this being one of Wang's first patches. I
appreciate this patch and it's a strict improvement on the past situation
AFAIC.

Cheers, Lorenzo