| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 7 Mar 2022 10:05:15 -0600
From: Mike Christie <michael.christie@...cle.com>
To: Wenchao Hao <haowenchao@...wei.com>, Lee Duncan <lduncan@...e.com>,
Chris Leech <cleech@...hat.com>,
"James E . J . Bottomley" <jejb@...ux.ibm.com>,
"Martin K . Petersen" <martin.petersen@...cle.com>,
open-iscsi@...glegroups.com, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Wu Bo <wubo40@...wei.com>, Zhiqiang Liu <liuzhiqiang26@...wei.com>
Subject: Re: [PATCH 1/2] scsi: iscsi: Add helper functions to alloc and add
iscsi_cls_conn
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 <haowenchao@...wei.com>
> Signed-off-by: Wu Bo <wubo40@...wei.com>
> ---
> 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);
Powered by blists - more mailing lists