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
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 <[email protected]>
Signed-off-by: Wu Bo <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Wu Bo <[email protected]>
---
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
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 <[email protected]>
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(-)
>
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
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