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: <20241106134034.GN458827@nvidia.com>
Date: Wed, 6 Nov 2024 09:40:34 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Uros Bizjak <ubizjak@...il.com>
Cc: Arnd Bergmann <arnd@...db.de>, Joerg Roedel <joro@...tes.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
	Robin Murphy <robin.murphy@....com>, vasant.hegde@....com,
	Kevin Tian <kevin.tian@...el.com>, jon.grimm@....com,
	santosh.shukla@....com, pandoh@...gle.com, kumaranand@...gle.com,
	Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH v9 03/10] asm/rwonce: Introduce [READ|WRITE]_ONCE()
 support for __int128

On Wed, Nov 06, 2024 at 11:01:20AM +0100, Uros Bizjak wrote:
> On Wed, Nov 6, 2024 at 9:55 AM Arnd Bergmann <arnd@...db.de> wrote:
> >
> > On Tue, Nov 5, 2024, at 13:30, Joerg Roedel wrote:
> > > On Fri, Nov 01, 2024 at 04:22:57PM +0000, Suravee Suthikulpanit wrote:
> > >>  include/asm-generic/rwonce.h   | 2 +-
> > >>  include/linux/compiler_types.h | 8 +++++++-
> > >>  2 files changed, 8 insertions(+), 2 deletions(-)
> > >
> > > This patch needs Cc:
> > >
> > >       Arnd Bergmann <arnd@...db.de>
> > >       linux-arch@...r.kernel.org
> > >
> >
> > It also needs an update to the comment about why this is safe:
> >
> > >> +++ b/include/asm-generic/rwonce.h
> > >> @@ -33,7 +33,7 @@
> > >>   * (e.g. a virtual address) and a strong prevailing wind.
> > >>   */
> > >>  #define compiletime_assert_rwonce_type(t)                                   \
> > >> -    compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
> > >> +    compiletime_assert(__native_word(t) || sizeof(t) == sizeof(__dword_type), \
> > >>              "Unsupported access size for {READ,WRITE}_ONCE().")
> >
> > As far as I can tell, 128-but words don't get stored atomically on
> > any architecture, so this seems wrong, because it would remove
> > the assertion on someone incorrectly using WRITE_ONCE() on a
> > 128-bit variable.
> 
> READ_ONCE() and WRITE_ONCE() do not guarantee atomicity for double
> word types. They only guarantee (c.f. include/asm/generic/rwonce.h):
> 
>  * Prevent the compiler from merging or refetching reads or writes. The
>  * compiler is also forbidden from reordering successive instances of
>  * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
>  * particular ordering. ...
> 
> and later:
> 
>  * Yes, this permits 64-bit accesses on 32-bit architectures. These will
>  * actually be atomic in some cases (namely Armv7 + LPAE), but for others we
>  * rely on the access being split into 2x32-bit accesses for a 32-bit quantity
>  * (e.g. a virtual address) and a strong prevailing wind.
> 
> This is the "strong prevailing wind", mentioned in the patch review at [1].
> 
> [1] https://lore.kernel.org/lkml/20241016130819.GJ3559746@nvidia.com/

Yes, there are two common uses for READ_ONCE, actually "read once" and
prevent compiler double read "optimizations". It doesn't matter if
things tear in this case because it would be used with cmpxchg or
something like that.

The other is an atomic relaxed memory order read, which would
have to guarentee non-tearing.

It is unfortunate the kernel has combined these two things, and we
probably have exciting bugs on 32 bit from places using the atomic
read variation on a u64..

Jason

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ