* [PATCH] NVMe: init nvme queue before enabling irq @ 2016-01-20 4:57 Wenbo Wang 2016-01-20 9:06 ` Sagi Grimberg 2016-01-20 9:09 ` Johannes Thumshirn 0 siblings, 2 replies; 8+ messages in thread From: Wenbo Wang @ 2016-01-20 4:57 UTC (permalink / raw) During reset process, the nvme_dev->bar (ioremapped) may change, so nvmeq->q_db shall be also updated by nvme_init_queue(). Currently nvmeq irq is enabled before queue init, so a spurious interrupt triggered nvme_process_cq may access nvmeq->q_db just before it is updated, this could cause kernel panic. Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com> Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com> --- drivers/nvme/host/pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index f5c0e26..df55f28 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) if (result < 0) goto release_cq; + /* + * Init queue door bell ioremap address before enabling irq, if not, + * a spurious interrupt triggered nvme_process_cq may access invalid + * address + */ + nvme_init_queue(nvmeq, qid); + result = queue_request_irq(dev, nvmeq, nvmeq->irqname); if (result < 0) goto release_sq; - nvme_init_queue(nvmeq, qid); return result; release_sq: @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev) if (result) goto free_nvmeq; + nvme_init_queue(nvmeq, 0); + nvmeq->cq_vector = 0; result = queue_request_irq(dev, nvmeq, nvmeq->irqname); if (result) { @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work) goto disable; } - nvme_init_queue(dev->queues[0], 0); result = nvme_alloc_admin_tags(dev); if (result) goto disable; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 4:57 [PATCH] NVMe: init nvme queue before enabling irq Wenbo Wang @ 2016-01-20 9:06 ` Sagi Grimberg 2016-01-20 10:52 ` Wenbo Wang 2016-01-20 9:09 ` Johannes Thumshirn 1 sibling, 1 reply; 8+ messages in thread From: Sagi Grimberg @ 2016-01-20 9:06 UTC (permalink / raw) > During reset process, the nvme_dev->bar (ioremapped) may change, > so nvmeq->q_db shall be also updated by nvme_init_queue(). > > Currently nvmeq irq is enabled before queue init, so a spurious > interrupt triggered nvme_process_cq may access nvmeq->q_db just > before it is updated, this could cause kernel panic. > > Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com> > Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com> This patch makes sense to me. > --- > drivers/nvme/host/pci.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index f5c0e26..df55f28 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) > if (result < 0) > goto release_cq; > > + /* > + * Init queue door bell ioremap address before enabling irq, if not, > + * a spurious interrupt triggered nvme_process_cq may access invalid > + * address > + */ Not sure we need an explicit comment on the fix here. But I don't mind keeping it. > + nvme_init_queue(nvmeq, qid); > + I think that this makes the duplication in nvme_alloc_queue redundant. Can you remove it? Other then that, looks good to me, Reviewed-by: Sagi Grimberg <sagig at mellanox.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 9:06 ` Sagi Grimberg @ 2016-01-20 10:52 ` Wenbo Wang 0 siblings, 0 replies; 8+ messages in thread From: Wenbo Wang @ 2016-01-20 10:52 UTC (permalink / raw) > I think that this makes the duplication in nvme_alloc_queue redundant. > Can you remove it? Thanks. Patch v2 has been sent out. -----Original Message----- From: Sagi Grimberg [mailto:sagig@dev.mellanox.co.il] Sent: Wednesday, January 20, 2016 5:06 PM To: Wenbo Wang; keith.busch at intel.com; axboe at fb.com Cc: Wenbo Wang; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq > During reset process, the nvme_dev->bar (ioremapped) may change, so > nvmeq->q_db shall be also updated by nvme_init_queue(). > > Currently nvmeq irq is enabled before queue init, so a spurious > interrupt triggered nvme_process_cq may access nvmeq->q_db just before > it is updated, this could cause kernel panic. > > Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com> > Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com> This patch makes sense to me. > --- > drivers/nvme/host/pci.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index > f5c0e26..df55f28 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) > if (result < 0) > goto release_cq; > > + /* > + * Init queue door bell ioremap address before enabling irq, if not, > + * a spurious interrupt triggered nvme_process_cq may access invalid > + * address > + */ Not sure we need an explicit comment on the fix here. But I don't mind keeping it. > + nvme_init_queue(nvmeq, qid); > + I think that this makes the duplication in nvme_alloc_queue redundant. Can you remove it? Other then that, looks good to me, Reviewed-by: Sagi Grimberg <sagig at mellanox.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 4:57 [PATCH] NVMe: init nvme queue before enabling irq Wenbo Wang 2016-01-20 9:06 ` Sagi Grimberg @ 2016-01-20 9:09 ` Johannes Thumshirn 2016-01-20 9:45 ` Wenbo Wang 1 sibling, 1 reply; 8+ messages in thread From: Johannes Thumshirn @ 2016-01-20 9:09 UTC (permalink / raw) On Tue, Jan 19, 2016@11:57:40PM -0500, Wenbo Wang wrote: > During reset process, the nvme_dev->bar (ioremapped) may change, > so nvmeq->q_db shall be also updated by nvme_init_queue(). > > Currently nvmeq irq is enabled before queue init, so a spurious > interrupt triggered nvme_process_cq may access nvmeq->q_db just > before it is updated, this could cause kernel panic. If it can cause a kernel panic shouldn't it go through stable then as well? > > Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com> > Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com> > --- > drivers/nvme/host/pci.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index f5c0e26..df55f28 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) > if (result < 0) > goto release_cq; > > + /* > + * Init queue door bell ioremap address before enabling irq, if not, > + * a spurious interrupt triggered nvme_process_cq may access invalid > + * address > + */ > + nvme_init_queue(nvmeq, qid); > + > result = queue_request_irq(dev, nvmeq, nvmeq->irqname); > if (result < 0) > goto release_sq; > > - nvme_init_queue(nvmeq, qid); > return result; > > release_sq: > @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev) > if (result) > goto free_nvmeq; > > + nvme_init_queue(nvmeq, 0); > + > nvmeq->cq_vector = 0; > result = queue_request_irq(dev, nvmeq, nvmeq->irqname); > if (result) { > @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work) > goto disable; > } > > - nvme_init_queue(dev->queues[0], 0); > result = nvme_alloc_admin_tags(dev); > if (result) > goto disable; > -- > 1.8.3.1 > > > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme Other than that Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de> -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 9:09 ` Johannes Thumshirn @ 2016-01-20 9:45 ` Wenbo Wang 2016-01-20 9:56 ` Sagi Grimberg 0 siblings, 1 reply; 8+ messages in thread From: Wenbo Wang @ 2016-01-20 9:45 UTC (permalink / raw) > If it can cause a kernel panic shouldn't it go through stable then as well? Sorry, not quite understand this comment. The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition. During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error. During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists. -----Original Message----- From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Johannes Thumshirn Sent: Wednesday, January 20, 2016 5:10 PM To: Wenbo Wang Cc: keith.busch at intel.com; axboe at fb.com; Wenbo Wang; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq On Tue, Jan 19, 2016@11:57:40PM -0500, Wenbo Wang wrote: > During reset process, the nvme_dev->bar (ioremapped) may change, so > nvmeq->q_db shall be also updated by nvme_init_queue(). > > Currently nvmeq irq is enabled before queue init, so a spurious > interrupt triggered nvme_process_cq may access nvmeq->q_db just before > it is updated, this could cause kernel panic. If it can cause a kernel panic shouldn't it go through stable then as well? > > Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com> > Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com> > --- > drivers/nvme/host/pci.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index > f5c0e26..df55f28 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) > if (result < 0) > goto release_cq; > > + /* > + * Init queue door bell ioremap address before enabling irq, if not, > + * a spurious interrupt triggered nvme_process_cq may access invalid > + * address > + */ > + nvme_init_queue(nvmeq, qid); > + > result = queue_request_irq(dev, nvmeq, nvmeq->irqname); > if (result < 0) > goto release_sq; > > - nvme_init_queue(nvmeq, qid); > return result; > > release_sq: > @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev) > if (result) > goto free_nvmeq; > > + nvme_init_queue(nvmeq, 0); > + > nvmeq->cq_vector = 0; > result = queue_request_irq(dev, nvmeq, nvmeq->irqname); > if (result) { > @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work) > goto disable; > } > > - nvme_init_queue(dev->queues[0], 0); > result = nvme_alloc_admin_tags(dev); > if (result) > goto disable; > -- > 1.8.3.1 > > > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme Other than that Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de> -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 9:45 ` Wenbo Wang @ 2016-01-20 9:56 ` Sagi Grimberg 2016-01-20 10:22 ` Johannes Thumshirn 0 siblings, 1 reply; 8+ messages in thread From: Sagi Grimberg @ 2016-01-20 9:56 UTC (permalink / raw) >> If it can cause a kernel panic shouldn't it go through stable then as well? > > Sorry, not quite understand this comment. > The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition. > During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error. > During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists. I think what Johannes meant was that this patch should include a "CC: stable at vger.kernel.org" tag. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 9:56 ` Sagi Grimberg @ 2016-01-20 10:22 ` Johannes Thumshirn 2016-01-20 10:41 ` Wenbo Wang 0 siblings, 1 reply; 8+ messages in thread From: Johannes Thumshirn @ 2016-01-20 10:22 UTC (permalink / raw) On Wed, Jan 20, 2016@11:56:01AM +0200, Sagi Grimberg wrote: > > >>If it can cause a kernel panic shouldn't it go through stable then as well? > > > >Sorry, not quite understand this comment. > >The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition. > >During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error. > >During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists. > > I think what Johannes meant was that this patch should include a > "CC: stable at vger.kernel.org" tag. > Exactly. This makes work for us distribution people a lot easier (i.e. we do not have to manually scan all commits and decide if we need to backport a patch or not) Thanks > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq 2016-01-20 10:22 ` Johannes Thumshirn @ 2016-01-20 10:41 ` Wenbo Wang 0 siblings, 0 replies; 8+ messages in thread From: Wenbo Wang @ 2016-01-20 10:41 UTC (permalink / raw) Thanks for this information. I'll add this. -----Original Message----- From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Johannes Thumshirn Sent: Wednesday, January 20, 2016 6:22 PM To: Sagi Grimberg Cc: Wenbo Wang; Wenbo Wang; keith.busch at intel.com; axboe at fb.com; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq On Wed, Jan 20, 2016@11:56:01AM +0200, Sagi Grimberg wrote: > > >>If it can cause a kernel panic shouldn't it go through stable then as well? > > > >Sorry, not quite understand this comment. > >The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition. > >During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error. > >During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists. > > I think what Johannes meant was that this patch should include a > "CC: stable at vger.kernel.org" tag. > Exactly. This makes work for us distribution people a lot easier (i.e. we do not have to manually scan all commits and decide if we need to backport a patch or not) Thanks > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme -- Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-01-20 10:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-20 4:57 [PATCH] NVMe: init nvme queue before enabling irq Wenbo Wang 2016-01-20 9:06 ` Sagi Grimberg 2016-01-20 10:52 ` Wenbo Wang 2016-01-20 9:09 ` Johannes Thumshirn 2016-01-20 9:45 ` Wenbo Wang 2016-01-20 9:56 ` Sagi Grimberg 2016-01-20 10:22 ` Johannes Thumshirn 2016-01-20 10:41 ` Wenbo Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox