linux-kernel - Re: [PATCH] rdma/siw: avoid smp_store

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: <20190712120328.GB27512@ziepe.ca>
Date:   Fri, 12 Jul 2019 09:03:28 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Bernard Metzler <BMT@...ich.ibm.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Doug Ledford <dledford@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64

On Fri, Jul 12, 2019 at 11:33:46AM +0000, Bernard Metzler wrote:
> >diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> >b/drivers/infiniband/sw/siw/siw_verbs.c
> >index 32dc79d0e898..41c5ab293fe1 100644
> >+++ b/drivers/infiniband/sw/siw/siw_verbs.c
> >@@ -1142,10 +1142,11 @@ int siw_req_notify_cq(struct ib_cq *base_cq,
> >enum ib_cq_notify_flags flags)
> > 
> > 	if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED)
> > 		/* CQ event for next solicited completion */
> >-		smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED);
> >+		WRITE_ONCE(*cq->notify, SIW_NOTIFY_SOLICITED);
> > 	else
> > 		/* CQ event for any signalled completion */
> >-		smp_store_mb(*cq->notify, SIW_NOTIFY_ALL);
> >+		WRITE_ONCE(*cq->notify, SIW_NOTIFY_ALL);
> >+	smp_wmb();
> > 
> > 	if (flags & IB_CQ_REPORT_MISSED_EVENTS)
> > 		return cq->cq_put - cq->cq_get;
> 
> 
> Hi Arnd,
> Many thanks for pointing that out! Indeed, this CQ notification
> mechanism does not take 32 bit architectures into account.
> Since we have only three flags to hold here, it's probably better
> to make it a 32bit value. That would remove the issue w/o
> introducing extra smp_wmb(). 

I also prefer not to see smp_wmb() in drivers..

> I'd prefer smp_store_mb(), since on some architectures it shall be
> more efficient.  That would also make it sufficient to use
> READ_ONCE.

The READ_ONCE is confusing to me too, if you need store_release
semantics then the reader also needs to pair with load_acquite -
otherwise it doesn't work.

Still, we need to do something rapidly to fix the i386 build, please
revise right away..

Jason