All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Burton <paul.burton@mips.com>
To: Jonas Gorski <jonas.gorski@gmail.com>,
	Michael Clark <michaeljclark@mac.com>
Cc: RISC-V Patches <patches@groups.riscv.org>,
	Linux RISC-V <linux-riscv@lists.infradead.org>,
	Linux MIPS <linux-mips@linux-mips.org>
Subject: Re: [PATCH 3/3] MIPS: fix truncation in __cmpxchg_small for short values
Date: Mon, 11 Feb 2019 20:20:24 +0000	[thread overview]
Message-ID: <20190211202023.rch4duvcctyspvxe@pburton-laptop> (raw)
In-Reply-To: <CAOiHx==dx10No8AO-5OP8BcuNbfnQdgVG2TiMkjz=Svqd0uKDg@mail.gmail.com>

Hello,

On Mon, Feb 11, 2019 at 01:37:08PM +0100, Jonas Gorski wrote:
> On Mon, 11 Feb 2019 at 05:39, Michael Clark <michaeljclark@mac.com> wrote:
> > diff --git a/arch/mips/kernel/cmpxchg.c b/arch/mips/kernel/cmpxchg.c
> > index 0b9535bc2c53..1169958fd748 100644
> > --- a/arch/mips/kernel/cmpxchg.c
> > +++ b/arch/mips/kernel/cmpxchg.c
> > @@ -57,7 +57,7 @@ unsigned long __cmpxchg_small(volatile void *ptr, unsigned long old,
> >         u32 mask, old32, new32, load32;
> >         volatile u32 *ptr32;
> >         unsigned int shift;
> > -       u8 load;
> > +       u32 load;
> 
> There already is a u32 line above, so maybe move it there.
> 
> Also reading the code to understand this, isn't the old code broken
> for cmpxchg_small calls for 16-bit variables, where old is > 0xff?
> 
> because it does later
> 
>         /*
>          * Ensure the byte we want to exchange matches the expected
>          * old value, and if not then bail.
>          */
>         load = (load32 & mask) >> shift;
>         if (load != old)
>             return load;
> 
> and if load is a u8, it can never be old if old contains a larger
> value than what can fit in a u8.
> 
> After re-reading your commit log, it seems you say something similar,
> but it wasn't quite obvious for me that this means the function is
> basically broken for short values where the old value does not fit in
> u8.
> 
> So this should have an appropriate "Fixes:" tag. And Cc: stable. Seems
> like quite a serious issue to me.

It could be serious if used, though in practice this support was added
to support qrwlock which only really needed 8-bit support at the time.
Since then commit d13316614633 ("locking/qrwlock: Prevent slowpath
writers getting held up by fastpath") removed even that.

But still, yes it's clearly a nasty little bug if anyone does try to use
a 16-bit cmpxchg() & I've marked it for stable backport whilst applying.

Thanks for fixing this Michael :)

Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2019-02-11 20:20 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-11  4:38 [PATCH 0/3] RISC-V: use generic spinlock and rwlock Michael Clark
2019-02-11  4:38 ` [PATCH 1/3] RISC-V: implement xchg_small and cmpxchg_small for char and short Michael Clark
2019-02-12  7:17   ` Christoph Hellwig
2019-02-24 21:03     ` Michael Clark
2019-02-11  4:38 ` [PATCH 2/3] RISC-V: convert custom spinlock/rwlock to generic qspinlock/qrwlock Michael Clark
2019-02-11  4:38 ` [PATCH 3/3] MIPS: fix truncation in __cmpxchg_small for short values Michael Clark
2019-02-11 12:37   ` Jonas Gorski
2019-02-11 20:20     ` Paul Burton [this message]
2019-02-24  0:09       ` Michael Clark
2019-02-11 20:03   ` Paul Burton
2019-02-11 20:03     ` Paul Burton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190211202023.rch4duvcctyspvxe@pburton-laptop \
    --to=paul.burton@mips.com \
    --cc=jonas.gorski@gmail.com \
    --cc=linux-mips@linux-mips.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=michaeljclark@mac.com \
    --cc=patches@groups.riscv.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.