* [PATCH v3] nvmet: force reconnect when number of queue changes
@ 2022-10-07 7:29 Daniel Wagner
2022-10-12 6:42 ` Sagi Grimberg
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-10-07 7:29 UTC (permalink / raw)
To: linux-nvme
Cc: Sagi Grimberg, Frederick Knight, Hannes Reinecke, James Smart,
John Meneghini, Daniel Wagner, Shinichiro Kawasaki
In order to test queue number changes we need to make sure that the
host reconnects. Because only when the host disconnects from the
target the number of queues are allowed to change according the spec.
The initial idea was to disable and re-enable the ports and have the
host wait until the KATO timer expires, triggering error
recovery. Though the host would see a DNR reply when trying to
reconnect. Because of the DNR bit the connection is dropped
completely. There is no point in trying to reconnect with the same
parameters according the spec.
We can force to reconnect the host is by deleting all controllers. The
host will observe any newly posted request to fail and thus starts the
error recovery but this time without the DNR bit set.
Also, the item passed into nvmet_subsys_attr_qid_max_show is not a
member of struct nvmet_port, it is part of nvmet_subsys. Hence,
don't try to dereference it as struct nvme_ctrl pointer.
Fixes: 3e980f5995e0 ("nvmet: Expose max queues to configfs")
Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Link: https://lore.kernel.org/r/20220913064203.133536-1-dwagner@suse.de
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
v3:
- Updated Fixes tag
- Updated commit message with some additional information
from the v2 discussion
v2:
- instead preventing changes, force reconnect by delete ctrls
- renamed patch
- https://lore.kernel.org/linux-nvme/20220927143157.3659-1-dwagner@suse.de/
v1:
- initial verison
- https://lore.kernel.org/linux-nvme/20220913064203.133536-1-dwagner@suse.de/
drivers/nvme/target/configfs.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e34a2896fedb..051a420d818e 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1290,12 +1290,10 @@ static ssize_t nvmet_subsys_attr_qid_max_show(struct config_item *item,
static ssize_t nvmet_subsys_attr_qid_max_store(struct config_item *item,
const char *page, size_t cnt)
{
- struct nvmet_port *port = to_nvmet_port(item);
+ struct nvmet_subsys *subsys = to_subsys(item);
+ struct nvmet_ctrl *ctrl;
u16 qid_max;
- if (nvmet_is_port_enabled(port, __func__))
- return -EACCES;
-
if (sscanf(page, "%hu\n", &qid_max) != 1)
return -EINVAL;
@@ -1303,8 +1301,13 @@ static ssize_t nvmet_subsys_attr_qid_max_store(struct config_item *item,
return -EINVAL;
down_write(&nvmet_config_sem);
- to_subsys(item)->max_qid = qid_max;
+ subsys->max_qid = qid_max;
+
+ /* Force reconnect */
+ list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+ ctrl->ops->delete_ctrl(ctrl);
up_write(&nvmet_config_sem);
+
return cnt;
}
CONFIGFS_ATTR(nvmet_subsys_, attr_qid_max);
--
2.37.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-07 7:29 [PATCH v3] nvmet: force reconnect when number of queue changes Daniel Wagner
@ 2022-10-12 6:42 ` Sagi Grimberg
2022-10-12 6:57 ` Hannes Reinecke
2022-10-17 17:38 ` Daniel Wagner
2 siblings, 0 replies; 7+ messages in thread
From: Sagi Grimberg @ 2022-10-12 6:42 UTC (permalink / raw)
To: Daniel Wagner, linux-nvme
Cc: Frederick Knight, Hannes Reinecke, James Smart, John Meneghini,
Shinichiro Kawasaki
Acked-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-07 7:29 [PATCH v3] nvmet: force reconnect when number of queue changes Daniel Wagner
2022-10-12 6:42 ` Sagi Grimberg
@ 2022-10-12 6:57 ` Hannes Reinecke
2022-10-17 17:38 ` Daniel Wagner
2 siblings, 0 replies; 7+ messages in thread
From: Hannes Reinecke @ 2022-10-12 6:57 UTC (permalink / raw)
To: Daniel Wagner, linux-nvme
Cc: Sagi Grimberg, Frederick Knight, James Smart, John Meneghini,
Shinichiro Kawasaki
On 10/7/22 09:29, Daniel Wagner wrote:
> In order to test queue number changes we need to make sure that the
> host reconnects. Because only when the host disconnects from the
> target the number of queues are allowed to change according the spec.
>
> The initial idea was to disable and re-enable the ports and have the
> host wait until the KATO timer expires, triggering error
> recovery. Though the host would see a DNR reply when trying to
> reconnect. Because of the DNR bit the connection is dropped
> completely. There is no point in trying to reconnect with the same
> parameters according the spec.
>
> We can force to reconnect the host is by deleting all controllers. The
> host will observe any newly posted request to fail and thus starts the
> error recovery but this time without the DNR bit set.
>
> Also, the item passed into nvmet_subsys_attr_qid_max_show is not a
> member of struct nvmet_port, it is part of nvmet_subsys. Hence,
> don't try to dereference it as struct nvme_ctrl pointer.
>
> Fixes: 3e980f5995e0 ("nvmet: Expose max queues to configfs")
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Link: https://lore.kernel.org/r/20220913064203.133536-1-dwagner@suse.de
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> v3:
> - Updated Fixes tag
> - Updated commit message with some additional information
> from the v2 discussion
>
> v2:
> - instead preventing changes, force reconnect by delete ctrls
> - renamed patch
> - https://lore.kernel.org/linux-nvme/20220927143157.3659-1-dwagner@suse.de/
>
> v1:
> - initial verison
> - https://lore.kernel.org/linux-nvme/20220913064203.133536-1-dwagner@suse.de/
>
> drivers/nvme/target/configfs.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-07 7:29 [PATCH v3] nvmet: force reconnect when number of queue changes Daniel Wagner
2022-10-12 6:42 ` Sagi Grimberg
2022-10-12 6:57 ` Hannes Reinecke
@ 2022-10-17 17:38 ` Daniel Wagner
2022-10-18 15:06 ` Christoph Hellwig
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2022-10-17 17:38 UTC (permalink / raw)
To: hch
Cc: Sagi Grimberg, Frederick Knight, Hannes Reinecke, James Smart,
John Meneghini, Shinichiro Kawasaki, linux-nvme
Hi Christoph,
Please consider to add the patch for the current rc phase as it fixes an
invalid memory access and I would like to avoid that the code fuzzer
army is going after us :)
Thanks,
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-17 17:38 ` Daniel Wagner
@ 2022-10-18 15:06 ` Christoph Hellwig
2022-10-18 15:15 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2022-10-18 15:06 UTC (permalink / raw)
To: Daniel Wagner
Cc: hch, Sagi Grimberg, Frederick Knight, Hannes Reinecke,
James Smart, John Meneghini, Shinichiro Kawasaki, linux-nvme
On Mon, Oct 17, 2022 at 07:38:17PM +0200, Daniel Wagner wrote:
> Hi Christoph,
>
> Please consider to add the patch for the current rc phase as it fixes an
> invalid memory access and I would like to avoid that the code fuzzer
> army is going after us :)
I filed this under enhancement and planned to add it to 6.2. Canyou
explain the invalid memory access a bit more?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-18 15:06 ` Christoph Hellwig
@ 2022-10-18 15:15 ` Christoph Hellwig
2022-10-19 6:42 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2022-10-18 15:15 UTC (permalink / raw)
To: Daniel Wagner
Cc: hch, Sagi Grimberg, Frederick Knight, Hannes Reinecke,
James Smart, John Meneghini, Shinichiro Kawasaki, linux-nvme
On Tue, Oct 18, 2022 at 08:06:24AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 17, 2022 at 07:38:17PM +0200, Daniel Wagner wrote:
> > Hi Christoph,
> >
> > Please consider to add the patch for the current rc phase as it fixes an
> > invalid memory access and I would like to avoid that the code fuzzer
> > army is going after us :)
>
> I filed this under enhancement and planned to add it to 6.2. Canyou
> explain the invalid memory access a bit more?
I guess this would the minimal memory access fix?
---
From c881123281d429da61ebd049c0867f8520dbfcb3 Mon Sep 17 00:00:00 2001
From: Daniel Wagner <dwagner@suse.de>
Date: Fri, 7 Oct 2022 09:29:34 +0200
Subject: nvmet: fix invalid memory reference in nvmet_subsys_attr_qid_max_show
The item passed into nvmet_subsys_attr_qid_max_show is not a member of
struct nvmet_port, it is part of nvmet_subsys. Hence, don't try to
dereference it as struct nvme_ctrl pointer.
Fixes: 3e980f5995e0 ("nvmet: Expose max queues to configfs")
Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Link: https://lore.kernel.org/r/20220913064203.133536-1-dwagner@suse.de
Signed-off-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Acked-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/target/configfs.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e34a2896fedb2..9443ee1d4ae3d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1290,12 +1290,8 @@ static ssize_t nvmet_subsys_attr_qid_max_show(struct config_item *item,
static ssize_t nvmet_subsys_attr_qid_max_store(struct config_item *item,
const char *page, size_t cnt)
{
- struct nvmet_port *port = to_nvmet_port(item);
u16 qid_max;
- if (nvmet_is_port_enabled(port, __func__))
- return -EACCES;
-
if (sscanf(page, "%hu\n", &qid_max) != 1)
return -EINVAL;
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] nvmet: force reconnect when number of queue changes
2022-10-18 15:15 ` Christoph Hellwig
@ 2022-10-19 6:42 ` Daniel Wagner
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-10-19 6:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sagi Grimberg, Frederick Knight, Hannes Reinecke, James Smart,
John Meneghini, Shinichiro Kawasaki, linux-nvme
On Tue, Oct 18, 2022 at 08:15:44AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 08:06:24AM -0700, Christoph Hellwig wrote:
> > On Mon, Oct 17, 2022 at 07:38:17PM +0200, Daniel Wagner wrote:
> > > Hi Christoph,
> > >
> > > Please consider to add the patch for the current rc phase as it fixes an
> > > invalid memory access and I would like to avoid that the code fuzzer
> > > army is going after us :)
> >
> > I filed this under enhancement and planned to add it to 6.2. Canyou
> > explain the invalid memory access a bit more?
>
> I guess this would the minimal memory access fix?
Yes, this is the correct fix. Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-19 6:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-07 7:29 [PATCH v3] nvmet: force reconnect when number of queue changes Daniel Wagner
2022-10-12 6:42 ` Sagi Grimberg
2022-10-12 6:57 ` Hannes Reinecke
2022-10-17 17:38 ` Daniel Wagner
2022-10-18 15:06 ` Christoph Hellwig
2022-10-18 15:15 ` Christoph Hellwig
2022-10-19 6:42 ` Daniel Wagner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).