linux-kernel - Re: [PATCH] locking/atomic: scripts: Fix type error in macro try

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: <CAFULd4aUoQf5CmUVEg5A7vKMz0CjrgSyZj=GyuhLkFviOPbGpg@mail.gmail.com>
Date: Sat, 20 Jul 2024 10:35:52 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Bibo Mao <maobibo@...ngson.cn>, Will Deacon <will@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	"Paul E . McKenney" <paulmck@...nel.org>, Carlos Llamas <cmllamas@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg

On Fri, Jul 19, 2024 at 6:18 PM Boqun Feng <boqun.feng@...il.com> wrote:
>
> On Fri, Jul 19, 2024 at 12:15:28PM +0200, Uros Bizjak wrote:
> > On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@...ngson.cn> wrote:
> > >
> > > When porting pv spinlock function on LoongArch system, there is
> > > compiling error such as:
> > >                  from linux/include/linux/smp.h:13,
> > >                  from linux/kernel/locking/qspinlock.c:16:
> > > linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> > > linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> > >   242 |         typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> > >       |                                  ^
> > > linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> > >  4908 |         raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > >       |         ^~~~~~~~~~~~~~~~~~~~~~~
> > > linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> > >   377 |         if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
> >
> > This points to the mismatch between "pn->state" and "old" variable.
> > The correct fix is:
> >
> > --cut here--
> > diff --git a/kernel/locking/qspinlock_paravirt.h
> > b/kernel/locking/qspinlock_paravirt.h
> > index f5a36e67b593..ac2e22502741 100644
> > --- a/kernel/locking/qspinlock_paravirt.h
> > +++ b/kernel/locking/qspinlock_paravirt.h
> > @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
> > *node, struct mcs_spinlock *prev)
> > static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> > {
> >        struct pv_node *pn = (struct pv_node *)node;
> > -       enum vcpu_state old = vcpu_halted;
> > +       u8 old = vcpu_halted;
> >        /*
>
> Looks reasonable to me, we should also add static_assert() for
> try_cmpxhg_*() to make sure the old has the same size of the cmpxchged
> field.

This is what -Wincompatible-pointer-types should detect, and it does
for the LoongArch target. Apparently, x86_64 targets use
-fshort-enums, so the mismatch was not detected on this particular
target.

Bibo Mao, does the proposed change works for you, so I can propose a
formal patch submission with the fix?

Thanks,
Uros.