linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
@ 2022-03-09 19:44 ` Mike Christie
  2022-03-10  1:57 ` [PATCH v3 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn Wenchao Hao
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Mike Christie @ 2022-03-09 19:44 UTC (permalink / raw)
  To: Wenchao Hao, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong

On 3/9/22 7:57 PM, Wenchao Hao wrote:
> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
> 
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
> 
> This implement makes it impossible to initialize resources of device
> before add it to sysfs during setup.
> 
> So this patchset splict both the setup and teradown of iscsi_cls_conn to
> 2 steps.
> 
> For setup flow, we should call iscsi_alloc_conn() and initialize some
> resources, then call iscsi_add_conn().
> 
> For teradown flow, we should call iscsi_remove_conn() to remove device
> and free resources which related to iscsi_cls_conn, then call
> iscsi_put_conn() to free iscsi_cls_conn.
> 
> V2 -> V3:
>   * Fix some bugs and optimization the code implement.
> 
> V1 -> V2:
>   * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
>   * change the teardown flow of iscsi_cls_conn
> 
> Wenchao Hao (3):
>   scsi: iscsi: Add helper functions to manage iscsi_cls_conn
>   scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
>   scsi:libiscsi: teradown iscsi_cls_conn gracefully
> 
>  drivers/scsi/libiscsi.c             | 23 +++++---
>  drivers/scsi/scsi_transport_iscsi.c | 90 +++++++++++++++--------------
>  include/scsi/scsi_transport_iscsi.h |  5 +-
>  3 files changed, 66 insertions(+), 52 deletions(-)
> 

Nice. Thanks.

Reviewed-by: Mike Christie <michael.christie@oracle.com>

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

* [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly
@ 2022-03-10  1:57 Wenchao Hao
  2022-03-09 19:44 ` Mike Christie
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Wenchao Hao @ 2022-03-10  1:57 UTC (permalink / raw)
  To: Mike Christie, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong, Wenchao Hao

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

The origin implement do device setup in iscsi_create_conn() which
bind the alloc/init and add in one function; do device teardown in
iscsi_destroy_conn() which bind remove and free in one function.

This implement makes it impossible to initialize resources of device
before add it to sysfs during setup.

So this patchset splict both the setup and teradown of iscsi_cls_conn to
2 steps.

For setup flow, we should call iscsi_alloc_conn() and initialize some
resources, then call iscsi_add_conn().

For teradown flow, we should call iscsi_remove_conn() to remove device
and free resources which related to iscsi_cls_conn, then call
iscsi_put_conn() to free iscsi_cls_conn.

V2 -> V3:
  * Fix some bugs and optimization the code implement.

V1 -> V2:
  * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
  * change the teardown flow of iscsi_cls_conn

Wenchao Hao (3):
  scsi: iscsi: Add helper functions to manage iscsi_cls_conn
  scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
  scsi:libiscsi: teradown iscsi_cls_conn gracefully

 drivers/scsi/libiscsi.c             | 23 +++++---
 drivers/scsi/scsi_transport_iscsi.c | 90 +++++++++++++++--------------
 include/scsi/scsi_transport_iscsi.h |  5 +-
 3 files changed, 66 insertions(+), 52 deletions(-)

-- 
2.32.0


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

* [PATCH v3 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
  2022-03-09 19:44 ` Mike Christie
@ 2022-03-10  1:57 ` Wenchao Hao
  2022-03-10  1:57 ` [PATCH v3 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized Wenchao Hao
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Wenchao Hao @ 2022-03-10  1:57 UTC (permalink / raw)
  To: Mike Christie, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong, Wenchao Hao

iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn
iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs.
iscsi_remove_conn(): remove iscsi_cls_conn from sysfs

Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
Signed-off-by: Wu Bo <wubo40@huawei.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 97 +++++++++++++++++++++++++++++
 include/scsi/scsi_transport_iscsi.h |  4 ++
 2 files changed, 101 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..65117ed5626e 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2340,6 +2340,103 @@ void iscsi_free_session(struct iscsi_cls_session *session)
 }
 EXPORT_SYMBOL_GPL(iscsi_free_session);
 
+/**
+ * iscsi_alloc_conn - alloc iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ */
+struct iscsi_cls_conn *
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+	struct iscsi_transport *transport = session->transport;
+	struct iscsi_cls_conn *conn;
+
+	conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
+	if (!conn)
+		return NULL;
+	if (dd_size)
+		conn->dd_data = &conn[1];
+
+	mutex_init(&conn->ep_mutex);
+	INIT_LIST_HEAD(&conn->conn_list);
+	INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
+	conn->transport = transport;
+	conn->cid = cid;
+	conn->state = ISCSI_CONN_DOWN;
+
+	/* this is released in the dev's release function */
+	if (!get_device(&session->dev))
+		goto free_conn;
+
+	dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
+	device_initialize(&conn->dev);
+	conn->dev.parent = &session->dev;
+	conn->dev.release = iscsi_conn_release;
+
+	return conn;
+
+free_conn:
+	kfree(conn);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+	int err;
+	unsigned long flags;
+	struct iscsi_cls_session *session = iscsi_dev_to_session(conn->dev.parent);
+
+	err = device_add(&conn->dev);
+	if (err) {
+		iscsi_cls_session_printk(KERN_ERR, session,
+					 "could not register connection's dev\n");
+		return err;
+	}
+	err = transport_register_device(&conn->dev);
+	if (err) {
+		iscsi_cls_session_printk(KERN_ERR, session,
+					 "could not register transport's dev\n");
+		device_del(&conn->dev);
+		return err;
+	}
+
+	spin_lock_irqsave(&connlock, flags);
+	list_add(&conn->conn_list, &connlist);
+	spin_unlock_irqrestore(&connlock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iscsi_add_conn);
+
+/**
+ * iscsi_remove_conn - remove iscsi class connection from sysfs
+ * @conn: iscsi cls connection
+ *
+ * this would remove iscsi_cls_conn from sysfs, and wait for previous
+ * read/write of iscsi_cls_conn's attributes in sysfs finishing
+ */
+void iscsi_remove_conn(struct iscsi_cls_conn *conn)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&connlock, flags);
+	list_del(&conn->conn_list);
+	spin_unlock_irqrestore(&connlock, flags);
+
+	transport_unregister_device(&conn->dev);
+	device_del(&conn->dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_remove_conn);
+
 /**
  * iscsi_create_conn - create iscsi class connection
  * @session: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..ae686addde0c 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,10 @@ extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
 						unsigned int target_id);
 extern void iscsi_remove_session(struct iscsi_cls_session *session);
 extern void iscsi_free_session(struct iscsi_cls_session *session);
+extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
+						int dd_size, uint32_t cid);
+extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
+extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
 extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
 						int dd_size, uint32_t cid);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
-- 
2.32.0


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

* [PATCH v3 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
  2022-03-09 19:44 ` Mike Christie
  2022-03-10  1:57 ` [PATCH v3 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn Wenchao Hao
@ 2022-03-10  1:57 ` Wenchao Hao
  2022-03-10  1:57 ` [PATCH v3 3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully Wenchao Hao
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Wenchao Hao @ 2022-03-10  1:57 UTC (permalink / raw)
  To: Mike Christie, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong, Wenchao Hao

iscsi_create_conn() would expose iscsi_cls_conn to sysfs, while the
initialization of iscsi_conn's dd_data is not ready now. When userspace
try to access an attribute such as connect's address, it might cause
a NULL pointer dereference.

So we should add iscsi_cls_conn to sysfs until it has been initialized.
And remove iscsi_create_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
Signed-off-by: Wu Bo <wubo40@huawei.com>
---
 drivers/scsi/libiscsi.c             | 13 ++++-
 drivers/scsi/scsi_transport_iscsi.c | 76 -----------------------------
 include/scsi/scsi_transport_iscsi.h |  2 -
 3 files changed, 11 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..38e68b449c08 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3040,8 +3040,9 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
 	struct iscsi_conn *conn;
 	struct iscsi_cls_conn *cls_conn;
 	char *data;
+	int err;
 
-	cls_conn = iscsi_create_conn(cls_session, sizeof(*conn) + dd_size,
+	cls_conn = iscsi_alloc_conn(cls_session, sizeof(*conn) + dd_size,
 				     conn_idx);
 	if (!cls_conn)
 		return NULL;
@@ -3078,13 +3079,21 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
 		goto login_task_data_alloc_fail;
 	conn->login_task->data = conn->data = data;
 
+	err = iscsi_add_conn(cls_conn);
+	if (err)
+		goto login_task_add_dev_fail;
+
 	return cls_conn;
 
+login_task_add_dev_fail:
+	free_pages((unsigned long) conn->data,
+		   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+
 login_task_data_alloc_fail:
 	kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
 		    sizeof(void*));
 login_task_alloc_fail:
-	iscsi_destroy_conn(cls_conn);
+	iscsi_put_conn(cls_conn);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_setup);
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 65117ed5626e..fc33000a6af9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2437,82 +2437,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_remove_conn);
 
-/**
- * iscsi_create_conn - create iscsi class connection
- * @session: iscsi cls session
- * @dd_size: private driver data size
- * @cid: connection id
- *
- * This can be called from a LLD or iscsi_transport. The connection
- * is child of the session so cid must be unique for all connections
- * on the session.
- *
- * Since we do not support MCS, cid will normally be zero. In some cases
- * for software iscsi we could be trying to preallocate a connection struct
- * in which case there could be two connection structs and cid would be
- * non-zero.
- */
-struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
-{
-	struct iscsi_transport *transport = session->transport;
-	struct iscsi_cls_conn *conn;
-	unsigned long flags;
-	int err;
-
-	conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
-	if (!conn)
-		return NULL;
-	if (dd_size)
-		conn->dd_data = &conn[1];
-
-	mutex_init(&conn->ep_mutex);
-	INIT_LIST_HEAD(&conn->conn_list);
-	INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
-	conn->transport = transport;
-	conn->cid = cid;
-	conn->state = ISCSI_CONN_DOWN;
-
-	/* this is released in the dev's release function */
-	if (!get_device(&session->dev))
-		goto free_conn;
-
-	dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
-	conn->dev.parent = &session->dev;
-	conn->dev.release = iscsi_conn_release;
-	err = device_register(&conn->dev);
-	if (err) {
-		iscsi_cls_session_printk(KERN_ERR, session, "could not "
-					 "register connection's dev\n");
-		goto release_parent_ref;
-	}
-	err = transport_register_device(&conn->dev);
-	if (err) {
-		iscsi_cls_session_printk(KERN_ERR, session, "could not "
-					 "register transport's dev\n");
-		goto release_conn_ref;
-	}
-
-	spin_lock_irqsave(&connlock, flags);
-	list_add(&conn->conn_list, &connlist);
-	spin_unlock_irqrestore(&connlock, flags);
-
-	ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
-	return conn;
-
-release_conn_ref:
-	device_unregister(&conn->dev);
-	put_device(&session->dev);
-	return NULL;
-release_parent_ref:
-	put_device(&session->dev);
-free_conn:
-	kfree(conn);
-	return NULL;
-}
-
-EXPORT_SYMBOL_GPL(iscsi_create_conn);
-
 /**
  * iscsi_destroy_conn - destroy iscsi class connection
  * @conn: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index ae686addde0c..4af6768e8195 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -445,8 +445,6 @@ extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
 						int dd_size, uint32_t cid);
 extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
-extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
-						int dd_size, uint32_t cid);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
 extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
-- 
2.32.0


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

* [PATCH v3 3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
                   ` (2 preceding siblings ...)
  2022-03-10  1:57 ` [PATCH v3 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized Wenchao Hao
@ 2022-03-10  1:57 ` Wenchao Hao
  2022-03-11  8:42 ` [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Wenchao Hao @ 2022-03-10  1:57 UTC (permalink / raw)
  To: Mike Christie, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong, Wenchao Hao

commit 1b8d0300a3e9 ("scsi: libiscsi: Fix UAF in
iscsi_conn_get_param()/iscsi_conn_teardown()") fixed an UAF in
iscsi_conn_get_param() and introduced 2 tmp_xxx varibles, the
implement looks ugly.

We can fix this UAF with the help of device_del() gracefully.
Call iscsi_remove_conn() at the beginning of iscsi_conn_teardown would
make userspace unable to see iscsi_cls_conn any more, then we can free
memory safely.
And remove iscsi_destroy_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
Signed-off-by: Wu Bo <wubo40@huawei.com>
---
 drivers/scsi/libiscsi.c             | 10 +++++-----
 drivers/scsi/scsi_transport_iscsi.c | 27 +++++----------------------
 include/scsi/scsi_transport_iscsi.h |  1 -
 3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 38e68b449c08..da583a39b307 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3109,8 +3109,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
 	struct iscsi_session *session = conn->session;
-	char *tmp_persistent_address = conn->persistent_address;
-	char *tmp_local_ipaddr = conn->local_ipaddr;
+
+	iscsi_remove_conn(cls_conn);
 
 	del_timer_sync(&conn->transport_timer);
 
@@ -3132,6 +3132,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 	spin_lock_bh(&session->frwd_lock);
 	free_pages((unsigned long) conn->data,
 		   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+	kfree(conn->persistent_address);
+	kfree(conn->local_ipaddr);
 	/* regular RX path uses back_lock */
 	spin_lock_bh(&session->back_lock);
 	kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
@@ -3142,9 +3144,7 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 	spin_unlock_bh(&session->frwd_lock);
 	mutex_unlock(&session->eh_mutex);
 
-	iscsi_destroy_conn(cls_conn);
-	kfree(tmp_persistent_address);
-	kfree(tmp_local_ipaddr);
+	iscsi_put_conn(cls_conn);
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_teardown);
 
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index fc33000a6af9..d0f996d14782 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2165,7 +2165,11 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
 {
 	if (!iscsi_is_conn_dev(dev))
 		return 0;
-	return iscsi_destroy_conn(iscsi_dev_to_conn(dev));
+
+	iscsi_remove_conn(iscsi_dev_to_conn(dev));
+	iscsi_put_conn(iscsi_dev_to_conn(dev));
+
+	return 0;
 }
 
 void iscsi_remove_session(struct iscsi_cls_session *session)
@@ -2437,27 +2441,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_remove_conn);
 
-/**
- * iscsi_destroy_conn - destroy iscsi class connection
- * @conn: iscsi cls session
- *
- * This can be called from a LLD or iscsi_transport.
- */
-int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&connlock, flags);
-	list_del(&conn->conn_list);
-	spin_unlock_irqrestore(&connlock, flags);
-
-	transport_unregister_device(&conn->dev);
-	ISCSI_DBG_TRANS_CONN(conn, "Completing conn destruction\n");
-	device_unregister(&conn->dev);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
-
 void iscsi_put_conn(struct iscsi_cls_conn *conn)
 {
 	put_device(&conn->dev);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 4af6768e8195..aedbc4488149 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -447,7 +447,6 @@ extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
-extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_unblock_session(struct iscsi_cls_session *session);
 extern void iscsi_block_session(struct iscsi_cls_session *session);
 extern int iscsi_scan_finished(struct Scsi_Host *shost, unsigned long time);
-- 
2.32.0


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

* Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
                   ` (3 preceding siblings ...)
  2022-03-10  1:57 ` [PATCH v3 3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully Wenchao Hao
@ 2022-03-11  8:42 ` Wenchao Hao
  2022-03-15  4:21 ` Martin K. Petersen
  2022-03-19  3:56 ` Martin K. Petersen
  6 siblings, 0 replies; 8+ messages in thread
From: Wenchao Hao @ 2022-03-11  8:42 UTC (permalink / raw)
  To: Lee Duncan, Chris Leech, open-iscsi, linux-kernel
  Cc: Wu Bo, Zhiqiang Liu, linfeilong

On 2022/3/10 9:57, Wenchao Hao wrote:
> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
> 
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
> 
> This implement makes it impossible to initialize resources of device
> before add it to sysfs during setup.
> 
> So this patchset splict both the setup and teradown of iscsi_cls_conn to
> 2 steps.
> 
> For setup flow, we should call iscsi_alloc_conn() and initialize some
> resources, then call iscsi_add_conn().
> 
> For teradown flow, we should call iscsi_remove_conn() to remove device
> and free resources which related to iscsi_cls_conn, then call
> iscsi_put_conn() to free iscsi_cls_conn.
> 

Friendly ping...

> V2 -> V3:
>    * Fix some bugs and optimization the code implement.
> 
> V1 -> V2:
>    * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
>    * change the teardown flow of iscsi_cls_conn
> 
> Wenchao Hao (3):
>    scsi: iscsi: Add helper functions to manage iscsi_cls_conn
>    scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
>    scsi:libiscsi: teradown iscsi_cls_conn gracefully
> 
>   drivers/scsi/libiscsi.c             | 23 +++++---
>   drivers/scsi/scsi_transport_iscsi.c | 90 +++++++++++++++--------------
>   include/scsi/scsi_transport_iscsi.h |  5 +-
>   3 files changed, 66 insertions(+), 52 deletions(-)
> 


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

* Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
                   ` (4 preceding siblings ...)
  2022-03-11  8:42 ` [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
@ 2022-03-15  4:21 ` Martin K. Petersen
  2022-03-19  3:56 ` Martin K. Petersen
  6 siblings, 0 replies; 8+ messages in thread
From: Martin K. Petersen @ 2022-03-15  4:21 UTC (permalink / raw)
  To: Wenchao Hao
  Cc: Mike Christie, Lee Duncan, Chris Leech, James E . J . Bottomley,
	Martin K . Petersen, open-iscsi, linux-scsi, linux-kernel, Wu Bo,
	Zhiqiang Liu, linfeilong


Wenchao,

> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.

Applied to 5.18/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

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

* Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly
  2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
                   ` (5 preceding siblings ...)
  2022-03-15  4:21 ` Martin K. Petersen
@ 2022-03-19  3:56 ` Martin K. Petersen
  6 siblings, 0 replies; 8+ messages in thread
From: Martin K. Petersen @ 2022-03-19  3:56 UTC (permalink / raw)
  To: open-iscsi, linux-kernel, Wenchao Hao, linux-scsi, Chris Leech,
	Lee Duncan, James E . J . Bottomley, Mike Christie
  Cc: Martin K . Petersen

On Wed, 9 Mar 2022 20:57:56 -0500, Wenchao Hao wrote:

> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
> 
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
> 
> [...]

Applied to 5.18/scsi-queue, thanks!

[1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn
      https://git.kernel.org/mkp/scsi/c/ad515cada7da
[2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
      https://git.kernel.org/mkp/scsi/c/7dae459f5e56
[3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully
      https://git.kernel.org/mkp/scsi/c/8709c323091b

-- 
Martin K. Petersen	Oracle Linux Engineering

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

end of thread, other threads:[~2022-03-19  3:57 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-10  1:57 [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
2022-03-09 19:44 ` Mike Christie
2022-03-10  1:57 ` [PATCH v3 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn Wenchao Hao
2022-03-10  1:57 ` [PATCH v3 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized Wenchao Hao
2022-03-10  1:57 ` [PATCH v3 3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully Wenchao Hao
2022-03-11  8:42 ` [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly Wenchao Hao
2022-03-15  4:21 ` Martin K. Petersen
2022-03-19  3:56 ` Martin K. Petersen

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