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

[Alexey Dobriyan <adobriyan@gmail.com>]

On Wed, May 06, 2020 at 02:44:50PM +0100, John Garry wrote:
> On 06/05/2020 14:24, Alexey Dobriyan wrote:
> > 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.
> 
> I'd rather hear the maintainer’s opinion before bothering testing this...
> 
> And you have not given any significant justification for your fix over a 
> revert.

It is in the comment: whatever gets written to ->cq_head must be
in [0, ->q_depth) range.

> > --- 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