linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Varios NVMe Fixes
@ 2019-07-31 23:35 Logan Gunthorpe
  2019-07-31 23:35 ` [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed Logan Gunthorpe
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2019-07-31 23:35 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	Chaitanya Kulkarni, Max Gurtovoy, Logan Gunthorpe

Hey,

These patches are just a resend of a bunch I've sent that
haven't been picked up yet. I don't want them to get forgotten.

Thanks,

Logan

--

Logan Gunthorpe (4):
  nvmet: Fix use-after-free bug when a port is removed
  nvmet-loop: Flush nvme_delete_wq when removing the port
  nvmet-file: fix nvmet_file_flush() always returning an error
  nvme-core: Fix extra device_put() call on error path

 drivers/nvme/host/core.c       |  2 +-
 drivers/nvme/target/configfs.c |  1 +
 drivers/nvme/target/core.c     | 15 +++++++++++++++
 drivers/nvme/target/loop.c     |  8 ++++++++
 drivers/nvme/target/nvmet.h    |  3 +++
 5 files changed, 28 insertions(+), 1 deletion(-)

--
2.20.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed
  2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
@ 2019-07-31 23:35 ` Logan Gunthorpe
  2019-07-31 23:52   ` Chaitanya Kulkarni
  2019-07-31 23:35 ` [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port Logan Gunthorpe
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Logan Gunthorpe @ 2019-07-31 23:35 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	Chaitanya Kulkarni, Max Gurtovoy, Logan Gunthorpe

When a port is removed through configfs, any connected controllers
are still active and can still send commands. This causes a
use-after-free bug which is detected by KASAN for any admin command
that dereferences req->port (like in nvmet_execute_identify_ctrl).

To fix this, disconnect all active controllers when a subsystem is
removed from a port. This ensures there are no active controllers
when the port is eventually removed.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/target/configfs.c |  1 +
 drivers/nvme/target/core.c     | 12 ++++++++++++
 drivers/nvme/target/nvmet.h    |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index cd52b9f15376..98613a45bd3b 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -675,6 +675,7 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 
 found:
 	list_del(&p->entry);
+	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
 
 	if (list_empty(&port->subsystems))
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index dad0243c7c96..b86a23aa9020 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -280,6 +280,18 @@ void nvmet_unregister_transport(const struct nvmet_fabrics_ops *ops)
 }
 EXPORT_SYMBOL_GPL(nvmet_unregister_transport);
 
+void nvmet_port_del_ctrls(struct nvmet_port *port, struct nvmet_subsys *subsys)
+{
+	struct nvmet_ctrl *ctrl;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		if (ctrl->port == port)
+			ctrl->ops->delete_ctrl(ctrl);
+	}
+	mutex_unlock(&subsys->lock);
+}
+
 int nvmet_enable_port(struct nvmet_port *port)
 {
 	const struct nvmet_fabrics_ops *ops;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6ee66c610739..c51f8dd01dc4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -418,6 +418,9 @@ void nvmet_port_send_ana_event(struct nvmet_port *port);
 int nvmet_register_transport(const struct nvmet_fabrics_ops *ops);
 void nvmet_unregister_transport(const struct nvmet_fabrics_ops *ops);
 
+void nvmet_port_del_ctrls(struct nvmet_port *port,
+			  struct nvmet_subsys *subsys);
+
 int nvmet_enable_port(struct nvmet_port *port);
 void nvmet_disable_port(struct nvmet_port *port);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port
  2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
  2019-07-31 23:35 ` [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed Logan Gunthorpe
@ 2019-07-31 23:35 ` Logan Gunthorpe
  2019-07-31 23:53   ` Chaitanya Kulkarni
  2019-07-31 23:35 ` [PATCH v3 3/4] nvmet-file: fix nvmet_file_flush() always returning an error Logan Gunthorpe
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Logan Gunthorpe @ 2019-07-31 23:35 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	Chaitanya Kulkarni, Max Gurtovoy, Logan Gunthorpe

After calling nvme_loop_delete_ctrl(), the controllers will not
yet be deleted because nvme_delete_ctrl() only schedules work
to do the delete.

This means a race can occur if a port is removed but there
are still active controllers trying to access that memory.

To fix this, flush the nvme_delete_wq before returning from
nvme_loop_remove_port() so that any controllers that might
be in the process of being deleted won't access a freed port.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/target/loop.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index b16dc3981c69..0940c5024a34 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -654,6 +654,14 @@ static void nvme_loop_remove_port(struct nvmet_port *port)
 	mutex_lock(&nvme_loop_ports_mutex);
 	list_del_init(&port->entry);
 	mutex_unlock(&nvme_loop_ports_mutex);
+
+	/*
+	 * Ensure any ctrls that are in the process of being
+	 * deleted are in fact deleted before we return
+	 * and free the port. This is to prevent active
+	 * ctrls from using a port after it's freed.
+	 */
+	flush_workqueue(nvme_delete_wq);
 }
 
 static const struct nvmet_fabrics_ops nvme_loop_ops = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/4] nvmet-file: fix nvmet_file_flush() always returning an error
  2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
  2019-07-31 23:35 ` [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed Logan Gunthorpe
  2019-07-31 23:35 ` [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port Logan Gunthorpe
@ 2019-07-31 23:35 ` Logan Gunthorpe
  2019-07-31 23:35 ` [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path Logan Gunthorpe
  2019-08-01  1:02 ` [PATCH v3 0/4] Varios NVMe Fixes Sagi Grimberg
  4 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2019-07-31 23:35 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	Chaitanya Kulkarni, Max Gurtovoy, Logan Gunthorpe

Presently, nvmet_file_flush() always returns a call to
errno_to_nvme_status() but that helper doesn't take into account the
case when errno=0. So nvmet_file_flush() always returns an error code.

All other callers of errno_to_nvme_status() check for success before
calling it.

To fix this, ensure errno_to_nvme_status() returns success if the
errno is zero. This should prevent future mistakes like this from
happening.

Fixes: c6aa3542e010 ("nvmet: add error log support for file backend")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b86a23aa9020..3a67e244e568 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -46,6 +46,9 @@ inline u16 errno_to_nvme_status(struct nvmet_req *req, int errno)
 	u16 status;
 
 	switch (errno) {
+	case 0:
+		status = NVME_SC_SUCCESS;
+		break;
 	case -ENOSPC:
 		req->error_loc = offsetof(struct nvme_rw_command, length);
 		status = NVME_SC_CAP_EXCEEDED | NVME_SC_DNR;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path
  2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
                   ` (2 preceding siblings ...)
  2019-07-31 23:35 ` [PATCH v3 3/4] nvmet-file: fix nvmet_file_flush() always returning an error Logan Gunthorpe
@ 2019-07-31 23:35 ` Logan Gunthorpe
  2019-07-31 23:55   ` Chaitanya Kulkarni
  2019-08-01  1:02 ` [PATCH v3 0/4] Varios NVMe Fixes Sagi Grimberg
  4 siblings, 1 reply; 9+ messages in thread
From: Logan Gunthorpe @ 2019-07-31 23:35 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	Chaitanya Kulkarni, Max Gurtovoy, Logan Gunthorpe

In the error path for nvme_init_subsystem(), nvme_put_subsystem()
will call device_put(), but it will get called again after the
mutex_unlock().

The device_put() only needs to be called if device_add() fails.

This bug caused a KASAN use-after-free error when adding and
removing subsytems in a loop:

  BUG: KASAN: use-after-free in device_del+0x8d9/0x9a0
  Read of size 8 at addr ffff8883cdaf7120 by task multipathd/329

  CPU: 0 PID: 329 Comm: multipathd Not tainted 5.2.0-rc6-vmlocalyes-00019-g70a2b39005fd-dirty #314
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
  Call Trace:
   dump_stack+0x7b/0xb5
   print_address_description+0x6f/0x280
   ? device_del+0x8d9/0x9a0
   __kasan_report+0x148/0x199
   ? device_del+0x8d9/0x9a0
   ? class_release+0x100/0x130
   ? device_del+0x8d9/0x9a0
   kasan_report+0x12/0x20
   __asan_report_load8_noabort+0x14/0x20
   device_del+0x8d9/0x9a0
   ? device_platform_notify+0x70/0x70
   nvme_destroy_subsystem+0xf9/0x150
   nvme_free_ctrl+0x280/0x3a0
   device_release+0x72/0x1d0
   kobject_put+0x144/0x410
   put_device+0x13/0x20
   nvme_free_ns+0xc4/0x100
   nvme_release+0xb3/0xe0
   __blkdev_put+0x549/0x6e0
   ? kasan_check_write+0x14/0x20
   ? bd_set_size+0xb0/0xb0
   ? kasan_check_write+0x14/0x20
   ? mutex_lock+0x8f/0xe0
   ? __mutex_lock_slowpath+0x20/0x20
   ? locks_remove_file+0x239/0x370
   blkdev_put+0x72/0x2c0
   blkdev_close+0x8d/0xd0
   __fput+0x256/0x770
   ? _raw_read_lock_irq+0x40/0x40
   ____fput+0xe/0x10
   task_work_run+0x10c/0x180
   ? filp_close+0xf7/0x140
   exit_to_usermode_loop+0x151/0x170
   do_syscall_64+0x240/0x2e0
   ? prepare_exit_to_usermode+0xd5/0x190
   entry_SYSCALL_64_after_hwframe+0x44/0xa9
  RIP: 0033:0x7f5a79af05d7
  Code: 00 00 0f 05 48 3d 00 f0 ff ff 77 3f c3 66 0f 1f 44 00 00 53 89 fb 48 83 ec 10 e8 c4 fb ff ff 89 df 89 c2 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 2b 89 d7 89 44 24 0c e8 06 fc ff ff 8b 44 24
  RSP: 002b:00007f5a7799c810 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
  RAX: 0000000000000000 RBX: 0000000000000008 RCX: 00007f5a79af05d7
  RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000008
  RBP: 00007f5a58000f98 R08: 0000000000000002 R09: 00007f5a7935ee80
  R10: 0000000000000000 R11: 0000000000000293 R12: 000055e432447240
  R13: 0000000000000000 R14: 0000000000000001 R15: 000055e4324a9cf0

  Allocated by task 1236:
   save_stack+0x21/0x80
   __kasan_kmalloc.constprop.6+0xab/0xe0
   kasan_kmalloc+0x9/0x10
   kmem_cache_alloc_trace+0x102/0x210
   nvme_init_identify+0x13c3/0x3820
   nvme_loop_configure_admin_queue+0x4fa/0x5e0
   nvme_loop_create_ctrl+0x469/0xf40
   nvmf_dev_write+0x19a3/0x21ab
   __vfs_write+0x66/0x120
   vfs_write+0x154/0x490
   ksys_write+0x104/0x240
   __x64_sys_write+0x73/0xb0
   do_syscall_64+0xa5/0x2e0
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

  Freed by task 329:
   save_stack+0x21/0x80
   __kasan_slab_free+0x129/0x190
   kasan_slab_free+0xe/0x10
   kfree+0xa7/0x200
   nvme_release_subsystem+0x49/0x60
   device_release+0x72/0x1d0
   kobject_put+0x144/0x410
   put_device+0x13/0x20
   klist_class_dev_put+0x31/0x40
   klist_put+0x8f/0xf0
   klist_del+0xe/0x10
   device_del+0x3a7/0x9a0
   nvme_destroy_subsystem+0xf9/0x150
   nvme_free_ctrl+0x280/0x3a0
   device_release+0x72/0x1d0
   kobject_put+0x144/0x410
   put_device+0x13/0x20
   nvme_free_ns+0xc4/0x100
   nvme_release+0xb3/0xe0
   __blkdev_put+0x549/0x6e0
   blkdev_put+0x72/0x2c0
   blkdev_close+0x8d/0xd0
   __fput+0x256/0x770
   ____fput+0xe/0x10
   task_work_run+0x10c/0x180
   exit_to_usermode_loop+0x151/0x170
   do_syscall_64+0x240/0x2e0
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: 32fd90c40768 ("nvme: change locking for the per-subsystem controller list")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 80c7a7ee240b..e35f16b60fc9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2488,6 +2488,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		if (ret) {
 			dev_err(ctrl->device,
 				"failed to register subsystem device.\n");
+			put_device(&subsys->dev);
 			goto out_unlock;
 		}
 		ida_init(&subsys->ns_ida);
@@ -2510,7 +2511,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	nvme_put_subsystem(subsys);
 out_unlock:
 	mutex_unlock(&nvme_subsystems_lock);
-	put_device(&subsys->dev);
 	return ret;
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed
  2019-07-31 23:35 ` [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed Logan Gunthorpe
@ 2019-07-31 23:52   ` Chaitanya Kulkarni
  0 siblings, 0 replies; 9+ messages in thread
From: Chaitanya Kulkarni @ 2019-07-31 23:52 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, Max Gurtovoy

Looks good.

Reviewed-by : Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 07/31/2019 04:36 PM, Logan Gunthorpe wrote:
> When a port is removed through configfs, any connected controllers
> are still active and can still send commands. This causes a
> use-after-free bug which is detected by KASAN for any admin command
> that dereferences req->port (like in nvmet_execute_identify_ctrl).
>
> To fix this, disconnect all active controllers when a subsystem is
> removed from a port. This ensures there are no active controllers
> when the port is eventually removed.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>   drivers/nvme/target/configfs.c |  1 +
>   drivers/nvme/target/core.c     | 12 ++++++++++++
>   drivers/nvme/target/nvmet.h    |  3 +++
>   3 files changed, 16 insertions(+)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index cd52b9f15376..98613a45bd3b 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -675,6 +675,7 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
>
>   found:
>   	list_del(&p->entry);
> +	nvmet_port_del_ctrls(port, subsys);
>   	nvmet_port_disc_changed(port, subsys);
>
>   	if (list_empty(&port->subsystems))
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index dad0243c7c96..b86a23aa9020 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -280,6 +280,18 @@ void nvmet_unregister_transport(const struct nvmet_fabrics_ops *ops)
>   }
>   EXPORT_SYMBOL_GPL(nvmet_unregister_transport);
>
> +void nvmet_port_del_ctrls(struct nvmet_port *port, struct nvmet_subsys *subsys)
> +{
> +	struct nvmet_ctrl *ctrl;
> +
> +	mutex_lock(&subsys->lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		if (ctrl->port == port)
> +			ctrl->ops->delete_ctrl(ctrl);
> +	}
> +	mutex_unlock(&subsys->lock);
> +}
> +
>   int nvmet_enable_port(struct nvmet_port *port)
>   {
>   	const struct nvmet_fabrics_ops *ops;
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 6ee66c610739..c51f8dd01dc4 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -418,6 +418,9 @@ void nvmet_port_send_ana_event(struct nvmet_port *port);
>   int nvmet_register_transport(const struct nvmet_fabrics_ops *ops);
>   void nvmet_unregister_transport(const struct nvmet_fabrics_ops *ops);
>
> +void nvmet_port_del_ctrls(struct nvmet_port *port,
> +			  struct nvmet_subsys *subsys);
> +
>   int nvmet_enable_port(struct nvmet_port *port);
>   void nvmet_disable_port(struct nvmet_port *port);
>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port
  2019-07-31 23:35 ` [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port Logan Gunthorpe
@ 2019-07-31 23:53   ` Chaitanya Kulkarni
  0 siblings, 0 replies; 9+ messages in thread
From: Chaitanya Kulkarni @ 2019-07-31 23:53 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, Max Gurtovoy

Looks good.

Reviewed-by : Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 07/31/2019 04:35 PM, Logan Gunthorpe wrote:
> After calling nvme_loop_delete_ctrl(), the controllers will not
> yet be deleted because nvme_delete_ctrl() only schedules work
> to do the delete.
>
> This means a race can occur if a port is removed but there
> are still active controllers trying to access that memory.
>
> To fix this, flush the nvme_delete_wq before returning from
> nvme_loop_remove_port() so that any controllers that might
> be in the process of being deleted won't access a freed port.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>   drivers/nvme/target/loop.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index b16dc3981c69..0940c5024a34 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -654,6 +654,14 @@ static void nvme_loop_remove_port(struct nvmet_port *port)
>   	mutex_lock(&nvme_loop_ports_mutex);
>   	list_del_init(&port->entry);
>   	mutex_unlock(&nvme_loop_ports_mutex);
> +
> +	/*
> +	 * Ensure any ctrls that are in the process of being
> +	 * deleted are in fact deleted before we return
> +	 * and free the port. This is to prevent active
> +	 * ctrls from using a port after it's freed.
> +	 */
> +	flush_workqueue(nvme_delete_wq);
>   }
>
>   static const struct nvmet_fabrics_ops nvme_loop_ops = {
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path
  2019-07-31 23:35 ` [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path Logan Gunthorpe
@ 2019-07-31 23:55   ` Chaitanya Kulkarni
  0 siblings, 0 replies; 9+ messages in thread
From: Chaitanya Kulkarni @ 2019-07-31 23:55 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg, Max Gurtovoy

Looks good.

Reviewed-by : Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 07/31/2019 04:36 PM, Logan Gunthorpe wrote:
> In the error path for nvme_init_subsystem(), nvme_put_subsystem()
> will call device_put(), but it will get called again after the
> mutex_unlock().
>
> The device_put() only needs to be called if device_add() fails.
>
> This bug caused a KASAN use-after-free error when adding and
> removing subsytems in a loop:
>
>    BUG: KASAN: use-after-free in device_del+0x8d9/0x9a0
>    Read of size 8 at addr ffff8883cdaf7120 by task multipathd/329
>
>    CPU: 0 PID: 329 Comm: multipathd Not tainted 5.2.0-rc6-vmlocalyes-00019-g70a2b39005fd-dirty #314
>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
>    Call Trace:
>     dump_stack+0x7b/0xb5
>     print_address_description+0x6f/0x280
>     ? device_del+0x8d9/0x9a0
>     __kasan_report+0x148/0x199
>     ? device_del+0x8d9/0x9a0
>     ? class_release+0x100/0x130
>     ? device_del+0x8d9/0x9a0
>     kasan_report+0x12/0x20
>     __asan_report_load8_noabort+0x14/0x20
>     device_del+0x8d9/0x9a0
>     ? device_platform_notify+0x70/0x70
>     nvme_destroy_subsystem+0xf9/0x150
>     nvme_free_ctrl+0x280/0x3a0
>     device_release+0x72/0x1d0
>     kobject_put+0x144/0x410
>     put_device+0x13/0x20
>     nvme_free_ns+0xc4/0x100
>     nvme_release+0xb3/0xe0
>     __blkdev_put+0x549/0x6e0
>     ? kasan_check_write+0x14/0x20
>     ? bd_set_size+0xb0/0xb0
>     ? kasan_check_write+0x14/0x20
>     ? mutex_lock+0x8f/0xe0
>     ? __mutex_lock_slowpath+0x20/0x20
>     ? locks_remove_file+0x239/0x370
>     blkdev_put+0x72/0x2c0
>     blkdev_close+0x8d/0xd0
>     __fput+0x256/0x770
>     ? _raw_read_lock_irq+0x40/0x40
>     ____fput+0xe/0x10
>     task_work_run+0x10c/0x180
>     ? filp_close+0xf7/0x140
>     exit_to_usermode_loop+0x151/0x170
>     do_syscall_64+0x240/0x2e0
>     ? prepare_exit_to_usermode+0xd5/0x190
>     entry_SYSCALL_64_after_hwframe+0x44/0xa9
>    RIP: 0033:0x7f5a79af05d7
>    Code: 00 00 0f 05 48 3d 00 f0 ff ff 77 3f c3 66 0f 1f 44 00 00 53 89 fb 48 83 ec 10 e8 c4 fb ff ff 89 df 89 c2 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 2b 89 d7 89 44 24 0c e8 06 fc ff ff 8b 44 24
>    RSP: 002b:00007f5a7799c810 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
>    RAX: 0000000000000000 RBX: 0000000000000008 RCX: 00007f5a79af05d7
>    RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000008
>    RBP: 00007f5a58000f98 R08: 0000000000000002 R09: 00007f5a7935ee80
>    R10: 0000000000000000 R11: 0000000000000293 R12: 000055e432447240
>    R13: 0000000000000000 R14: 0000000000000001 R15: 000055e4324a9cf0
>
>    Allocated by task 1236:
>     save_stack+0x21/0x80
>     __kasan_kmalloc.constprop.6+0xab/0xe0
>     kasan_kmalloc+0x9/0x10
>     kmem_cache_alloc_trace+0x102/0x210
>     nvme_init_identify+0x13c3/0x3820
>     nvme_loop_configure_admin_queue+0x4fa/0x5e0
>     nvme_loop_create_ctrl+0x469/0xf40
>     nvmf_dev_write+0x19a3/0x21ab
>     __vfs_write+0x66/0x120
>     vfs_write+0x154/0x490
>     ksys_write+0x104/0x240
>     __x64_sys_write+0x73/0xb0
>     do_syscall_64+0xa5/0x2e0
>     entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>    Freed by task 329:
>     save_stack+0x21/0x80
>     __kasan_slab_free+0x129/0x190
>     kasan_slab_free+0xe/0x10
>     kfree+0xa7/0x200
>     nvme_release_subsystem+0x49/0x60
>     device_release+0x72/0x1d0
>     kobject_put+0x144/0x410
>     put_device+0x13/0x20
>     klist_class_dev_put+0x31/0x40
>     klist_put+0x8f/0xf0
>     klist_del+0xe/0x10
>     device_del+0x3a7/0x9a0
>     nvme_destroy_subsystem+0xf9/0x150
>     nvme_free_ctrl+0x280/0x3a0
>     device_release+0x72/0x1d0
>     kobject_put+0x144/0x410
>     put_device+0x13/0x20
>     nvme_free_ns+0xc4/0x100
>     nvme_release+0xb3/0xe0
>     __blkdev_put+0x549/0x6e0
>     blkdev_put+0x72/0x2c0
>     blkdev_close+0x8d/0xd0
>     __fput+0x256/0x770
>     ____fput+0xe/0x10
>     task_work_run+0x10c/0x180
>     exit_to_usermode_loop+0x151/0x170
>     do_syscall_64+0x240/0x2e0
>     entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Fixes: 32fd90c40768 ("nvme: change locking for the per-subsystem controller list")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 80c7a7ee240b..e35f16b60fc9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2488,6 +2488,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   		if (ret) {
>   			dev_err(ctrl->device,
>   				"failed to register subsystem device.\n");
> +			put_device(&subsys->dev);
>   			goto out_unlock;
>   		}
>   		ida_init(&subsys->ns_ida);
> @@ -2510,7 +2511,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	nvme_put_subsystem(subsys);
>   out_unlock:
>   	mutex_unlock(&nvme_subsystems_lock);
> -	put_device(&subsys->dev);
>   	return ret;
>   }
>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/4] Varios NVMe Fixes
  2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
                   ` (3 preceding siblings ...)
  2019-07-31 23:35 ` [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path Logan Gunthorpe
@ 2019-08-01  1:02 ` Sagi Grimberg
  4 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2019-08-01  1:02 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Chaitanya Kulkarni,
	Max Gurtovoy

Thanks,

applied to nvme-5.3-rc

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-08-01  1:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 23:35 [PATCH v3 0/4] Varios NVMe Fixes Logan Gunthorpe
2019-07-31 23:35 ` [PATCH v3 1/4] nvmet: Fix use-after-free bug when a port is removed Logan Gunthorpe
2019-07-31 23:52   ` Chaitanya Kulkarni
2019-07-31 23:35 ` [PATCH v3 2/4] nvmet-loop: Flush nvme_delete_wq when removing the port Logan Gunthorpe
2019-07-31 23:53   ` Chaitanya Kulkarni
2019-07-31 23:35 ` [PATCH v3 3/4] nvmet-file: fix nvmet_file_flush() always returning an error Logan Gunthorpe
2019-07-31 23:35 ` [PATCH v3 4/4] nvme-core: Fix extra device_put() call on error path Logan Gunthorpe
2019-07-31 23:55   ` Chaitanya Kulkarni
2019-08-01  1:02 ` [PATCH v3 0/4] Varios NVMe Fixes Sagi Grimberg

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