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

[John Garry <john.garry@huawei.com>]

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.

Thanks,
John

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