linux-kernel - Re: [PATCH v4 02/13] riscv: Do not fail to build on byte/halfword operations with Zawrs

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: <CAHVXubhUVi=Ryc4fF+7Z12h7p1aQtVwHfD6YGfw676aijjw5Rg@mail.gmail.com>
Date: Thu, 1 Aug 2024 08:30:57 +0200
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: Andrew Jones <ajones@...tanamicro.com>
Cc: Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Andrea Parri <parri.andrea@...il.com>, 
	Nathan Chancellor <nathan@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>, 
	Arnd Bergmann <arnd@...db.de>, Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>, 
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-arch@...r.kernel.org
Subject: Re: [PATCH v4 02/13] riscv: Do not fail to build on byte/halfword
 operations with Zawrs

On Wed, Jul 31, 2024 at 6:14 PM Andrew Jones <ajones@...tanamicro.com> wrote:
>
> On Wed, Jul 31, 2024 at 05:52:46PM GMT, Alexandre Ghiti wrote:
> > Hi Drew,
> >
> > On Wed, Jul 31, 2024 at 4:10 PM Andrew Jones <ajones@...tanamicro.com> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 09:23:54AM GMT, Alexandre Ghiti wrote:
> > > > riscv does not have lr instructions on byte and halfword but the
> > > > qspinlock implementation actually uses such atomics provided by the
> > > > Zabha extension, so those sizes are legitimate.
> > >
> > > We currently always come to __cmpwait() through smp_cond_load_relaxed()
> > > and queued_spin_lock_slowpath() adds another invocation.
> >
> > atomic_cond_read_relaxed() and smp_cond_load_acquire() also call
> > smp_cond_load_relaxed()
> >
> > And here https://elixir.bootlin.com/linux/v6.11-rc1/source/kernel/locking/qspinlock.c#L380,
> > the size passed is 1.
>
> Oh, I see.
>
> >
> > > However, isn't
> > > the reason we're hitting the BUILD_BUG() because the switch fails to find
> > > a case for 16, not because it fails to find cases for 1 or 2? The new
> > > invocation passes a pointer to a struct mcs_spinlock, which looks like
> > > it has size 16. We need to ensure that when ptr points to a pointer that
> > > we pass the size of uintptr_t.
> >
> > I guess you're refering to this call here
> > https://elixir.bootlin.com/linux/v6.11-rc1/source/kernel/locking/qspinlock.c#L551,
> > but it's a pointer to a pointer, which will then pass a size 8.
>
> Ah, missed that '&'...
>
> >
> > And the build error that I get is the following:
> >
> > In function '__cmpwait',
> >     inlined from 'queued_spin_lock_slowpath' at
> > ../kernel/locking/qspinlock.c:380:3:
> > ./../include/linux/compiler_types.h:510:45: error: call to
> > '__compiletime_assert_2' declared with attribute error: BUILD_BUG
> > failed
> >   510 |         _compiletime_assert(condition, msg,
> > __compiletime_assert_, __COUNTER__)
> >       |                                             ^
> > ./../include/linux/compiler_types.h:491:25: note: in definition of
> > macro '__compiletime_assert'
> >   491 |                         prefix ## suffix();
> >          \
> >       |                         ^~~~~~
> > ./../include/linux/compiler_types.h:510:9: note: in expansion of macro
> > '_compiletime_assert'
> >   510 |         _compiletime_assert(condition, msg,
> > __compiletime_assert_, __COUNTER__)
> >       |         ^~~~~~~~~~~~~~~~~~~
> > ../include/linux/build_bug.h:39:37: note: in expansion of macro
> > 'compiletime_assert'
> >    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> >       |                                     ^~~~~~~~~~~~~~~~~~
> > ../include/linux/build_bug.h:59:21: note: in expansion of macro
> > 'BUILD_BUG_ON_MSG'
> >    59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> >       |                     ^~~~~~~~~~~~~~~~
> > ../arch/riscv/include/asm/cmpxchg.h:376:17: note: in expansion of
> > macro 'BUILD_BUG'
> >   376 |                 BUILD_BUG();
> >
> > which points to the first smp_cond_load_relaxed() I mentioned above.
> >
>
> OK, you've got me straightened out now, but can we only add the fallback
> for sizes 1 and 2 and leave the default as a BUILD_BUG()?

Yes, sure, I'll do that.

Thanks,

Alex

>
> Thanks,
> drew