We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.
iscsi_create_conn() expose iscsi_cls_conn to sysfs while the related
resources are not initialized. So we should delay the calling of
device_add() until these resources has been initialized.
This patchset solve this issue by changing iscsi_conn_setup() and works
well for iscsi_tcp.
Wenchao Hao (2):
scsi: iscsi: Add helper functions to alloc and add iscsi_cls_conn
scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
drivers/scsi/libiscsi.c | 13 ++++-
drivers/scsi/scsi_transport_iscsi.c | 85 +++++++++++++++++++++++------
include/scsi/scsi_transport_iscsi.h | 3 +
3 files changed, 83 insertions(+), 18 deletions(-)
--
2.32.0
iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
not expose it to userspace.
iscsi_add_conn() would expose it to userspace.
LLDs should split the alloc and register to 2 steps.
And simplify iscsi_create_conn() with these helper functions.
Signed-off-by: Wenchao Hao <[email protected]>
Signed-off-by: Wu Bo <[email protected]>
---
drivers/scsi/scsi_transport_iscsi.c | 85 +++++++++++++++++++++++------
include/scsi/scsi_transport_iscsi.h | 3 +
2 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..092d4429bb1d 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2341,7 +2341,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
EXPORT_SYMBOL_GPL(iscsi_free_session);
/**
- * iscsi_create_conn - create iscsi class connection
+ * iscsi_alloc_conn - alloc iscsi class connection
* @session: iscsi cls session
* @dd_size: private driver data size
* @cid: connection id
@@ -2356,12 +2356,10 @@ EXPORT_SYMBOL_GPL(iscsi_free_session);
* non-zero.
*/
struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+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;
- unsigned long flags;
- int err;
conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
if (!conn)
@@ -2383,35 +2381,90 @@ iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
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_register(&conn->dev);
if (err) {
iscsi_cls_session_printk(KERN_ERR, session, "could not "
"register connection's dev\n");
- goto release_parent_ref;
+ put_device(&session->dev);
+ return err;
}
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;
+ device_unregister(&conn->dev);
+ put_device(&session->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_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.
+ *
+ * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
+ * means attributes of iscsi_cls_conn are accessible to userspace. So the
+ * caller must make sure everything related these sysfs attributes are
+ * already initialized.
+ */
+struct iscsi_cls_conn *
+iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+ struct iscsi_cls_conn *conn;
+ int err;
+
+ conn = iscsi_alloc_conn(session, dd_size, cid);
+ if (!conn)
+ return NULL;
+
+ err = iscsi_add_conn(conn);
+ if (err) {
+ kfree(conn);
+ return NULL;
+ }
+
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);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..fd9ce99c2186 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,9 @@ 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 int iscsi_add_conn(struct iscsi_cls_conn *conn);
+extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
+ int dd_size, uint32_t cid);
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
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.
Signed-off-by: Wenchao Hao <[email protected]>
Signed-off-by: Wu Bo <[email protected]>
---
drivers/scsi/libiscsi.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..1cf25f4acb71 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);
+ kfree(cls_conn);
return NULL;
}
EXPORT_SYMBOL_GPL(iscsi_conn_setup);
--
2.32.0
On 3/7/22 6:56 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.
>
> iscsi_create_conn() expose iscsi_cls_conn to sysfs while the related
> resources are not initialized. So we should delay the calling of
> device_add() until these resources has been initialized.
>
> This patchset solve this issue by changing iscsi_conn_setup() and works
> well for iscsi_tcp.
>
Overall I think you need to also fix up the drivers. It just makes it a
nicer driver API where the LLDs don't know about sysfs and doesn't have
to worry about it.
Let's start with just this first piece where we handle sysfs in the lib
and class like you are doing in this patchset. We can do the LLDs
interaction with the lib in a second patchset to make this easier and fix
the initial bug and cleanup some code.
In a separate patchset, we can then go deeper and maybe just merge/kill some
of the lib/class interface since every driver except qla4xxx hooks into the
lib. So we have this distinction just for that one driver's session mode
and that doesn't make a lot of sense.
On 3/7/22 6:56 PM, Wenchao Hao wrote:
> iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
> not expose it to userspace.
> iscsi_add_conn() would expose it to userspace.
>
> LLDs should split the alloc and register to 2 steps.
>
> And simplify iscsi_create_conn() with these helper functions.
>
> Signed-off-by: Wenchao Hao <[email protected]>
> Signed-off-by: Wu Bo <[email protected]>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 85 +++++++++++++++++++++++------
> include/scsi/scsi_transport_iscsi.h | 3 +
> 2 files changed, 72 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 554b6f784223..092d4429bb1d 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2341,7 +2341,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
> EXPORT_SYMBOL_GPL(iscsi_free_session);
>
> /**
> - * iscsi_create_conn - create iscsi class connection
> + * iscsi_alloc_conn - alloc iscsi class connection
> * @session: iscsi cls session
> * @dd_size: private driver data size
> * @cid: connection id
> @@ -2356,12 +2356,10 @@ EXPORT_SYMBOL_GPL(iscsi_free_session);
> * non-zero.
> */
> struct iscsi_cls_conn *
> -iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
> +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;
> - unsigned long flags;
> - int err;
>
> conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
> if (!conn)
> @@ -2383,35 +2381,90 @@ iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
> dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
> 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_register(&conn->dev);
You should use device_initialize in iscsi_alloc_conn. Here use device_add.
Then make a iscsi_remove_conn function which does device_del. The iscsi_free_conn
does a device_put.
Patch2 should not be doing kfree on the cls_conn because it has no idea what
the iscsi class needs to do to unravel things. For example, your patch leaks the
parent on failures because it doesn't do a put on the parent/session to handle
the get that's done in iscsi_alloc_conn.
In a patch3 you can then fix up iscsi_conn_teardown to remove the tmp_* kfree
code. So at the beginning of iscsi_conn_teardown do the iscsi_remove_conn.
Then after we have cleaned everything up in there do the iscsi_free_conn.
We then don't need the tmp_* code. We can just do:
kfree(conn->persistent_address);
kfree(conn->local_ipaddr);
anytime after the iscsi_remove_conn call.
> if (err) {
> iscsi_cls_session_printk(KERN_ERR, session, "could not "
> "register connection's dev\n");
> - goto release_parent_ref;
> + put_device(&session->dev);
> + return err;
> }
> 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;
> + device_unregister(&conn->dev);
> + put_device(&session->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_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.
> + *
> + * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
> + * means attributes of iscsi_cls_conn are accessible to userspace. So the
> + * caller must make sure everything related these sysfs attributes are
> + * already initialized.
> + */
> +struct iscsi_cls_conn *
> +iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
You should kill this since it's not used anymore.
> +{
> + struct iscsi_cls_conn *conn;
> + int err;
> +
> + conn = iscsi_alloc_conn(session, dd_size, cid);
> + if (!conn)
> + return NULL;
> +
> + err = iscsi_add_conn(conn);
> + if (err) {
> + kfree(conn);
> + return NULL;
> + }
> +
> 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);
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index c5d7810fd792..fd9ce99c2186 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -441,6 +441,9 @@ 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 int iscsi_add_conn(struct iscsi_cls_conn *conn);
> +extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
> + int dd_size, uint32_t cid);
> 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);