Re: [PATCH] nvme-pci: slimmer CQ head update

[Alexey Dobriyan <adobriyan@gmail.com>]

On Wed, May 06, 2020 at 06:47:01AM -0600, Keith Busch wrote:
> On Wed, May 06, 2020 at 12:03:35PM +0100, John Garry wrote:
> > On 29/02/2020 05:53, Keith Busch wrote:
> > > On Fri, Feb 28, 2020 at 09:45:19PM +0300, Alexey Dobriyan wrote:
> > > >   static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
> > > >   {
> > > > -	if (nvmeq->cq_head == nvmeq->q_depth - 1) {
> > > > +	if (++nvmeq->cq_head == nvmeq->q_depth) {
> > 
> > I figure momentarily nvmeq->cq_head may transition to equal nvmeq->q_depth
> > and then to 0, which causes an out-of-bounds access here:
> > 
> > static inline bool nvme_cqe_pending(struct nvme_queue *nvmeq)
> > {
> > 	return (le16_to_cpu(nvmeq->cqes[nvmeq->cq_head].status) & 1) ==
> > 			nvmeq->cq_phase;
> > }
> 
> Thanks for the notice, your analysis sounds correct to me.
> 
> Ideally we wouldn't let the irq check happen while the threaded
> handler is running, but that is a bit risky to introduce at this
> point. I'm okay with reverting to fix this issue.

Pre-increment is still beneficial, should be done in register.
Please, test.


Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 drivers/nvme/host/pci.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -973,9 +973,16 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 
 static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 {
-	if (++nvmeq->cq_head == nvmeq->q_depth) {
+	/*
+	 * Can't pre-increment ->cq_head directly.
+	 * It must be in [0, ->q_depth) range at all times.
+	 */
+	u16 tmp = READ_ONCE(nvmeq->cq_head);
+	if (++tmp == nvmeq->q_depth) {
 		nvmeq->cq_head = 0;
 		nvmeq->cq_phase ^= 1;
+	} else {
+		nvmeq->cq_head = tmp;
 	}
 }
 

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