[PATCH] scsi: iscsi: publish endpoints after transport setup

[PATCH] scsi: iscsi: publish endpoints after transport setup

[Ruoyu Wang]

iscsi_create_endpoint() inserts a new endpoint into iscsi_ep_idr before
transport drivers have initialized their endpoint-private data. The
endpoint handle is returned only after ep_connect() completes, but handles
are allocated from a predictable IDR and iscsi_lookup_endpoint() looks
them up directly.

Reserve the endpoint ID with a NULL IDR entry, add
iscsi_register_endpoint() for the publish step, and call it from the
in-tree transport drivers after private endpoint setup has completed.
Until registration, endpoint lookup keeps returning NULL for the reserved
handle.

Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/scsi/be2iscsi/be_iscsi.c    |  6 ++++++
 drivers/scsi/bnx2i/bnx2i_iscsi.c    |  4 ++++
 drivers/scsi/cxgbi/libcxgbi.c       |  6 ++++++
 drivers/scsi/qedi/qedi_iscsi.c      |  4 ++++
 drivers/scsi/qla4xxx/ql4_os.c       |  6 ++++++
 drivers/scsi/scsi_transport_iscsi.c | 24 +++++++++++++++++++++++-
 include/scsi/scsi_transport_iscsi.h |  1 +
 7 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 8d374ae863ba2..1abb2bc695923 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -1188,6 +1188,12 @@ beiscsi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
 		goto free_ep;
 	}
 
+	ret = iscsi_register_endpoint(ep);
+	if (ret) {
+		beiscsi_ep_disconnect(ep);
+		return ERR_PTR(ret);
+	}
+
 	return ep;
 
 free_ep:
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 6c80e5b514fda..2c84b4864e240 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1914,6 +1914,10 @@ static struct iscsi_endpoint *bnx2i_ep_connect(struct Scsi_Host *shost,
 	if (rc)
 		goto del_active_ep;
 
+	rc = iscsi_register_endpoint(ep);
+	if (rc)
+		goto del_active_ep;
+
 	mutex_unlock(&hba->net_dev_lock);
 	return ep;
 
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index ea9631bfe2e23..c1b2a401ddca3 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -2937,11 +2937,17 @@ check_route:
 	cep->csk = csk;
 	cep->chba = hba;
 
+	err = iscsi_register_endpoint(ep);
+	if (err)
+		goto release_ep;
+
 	log_debug(1 << CXGBI_DBG_ISCSI | 1 << CXGBI_DBG_SOCK,
 		"ep 0x%p, cep 0x%p, csk 0x%p, hba 0x%p,%s.\n",
 		ep, cep, csk, hba, hba->ndev->name);
 	return ep;
 
+release_ep:
+	iscsi_destroy_endpoint(ep);
 release_conn:
 	cxgbi_sock_put(csk);
 	cxgbi_sock_closed(csk);
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 6ab3a989d2817..60c37566f768c 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -1014,6 +1014,10 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
 		goto ep_rel_conn;
 	}
 
+	ret = iscsi_register_endpoint(ep);
+	if (ret)
+		goto ep_rel_conn;
+
 	atomic_inc(&qedi->num_offloads);
 	return ep;
 
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index d598ab4126f80..8b30620d0e8dc 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -1749,6 +1749,12 @@ qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
 
 	qla_ep->host = shost;
 
+	ret = iscsi_register_endpoint(ep);
+	if (ret) {
+		iscsi_destroy_endpoint(ep);
+		return ERR_PTR(ret);
+	}
+
 	return ep;
 }
 
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 8aa76f813bcdb..a8c2a32c0f134 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -217,7 +217,7 @@ iscsi_create_endpoint(int dd_size)
 	 * First endpoint id should be 1 to comply with user space
 	 * applications (iscsid).
 	 */
-	id = idr_alloc(&iscsi_ep_idr, ep, 1, -1, GFP_NOIO);
+	id = idr_alloc(&iscsi_ep_idr, NULL, 1, -1, GFP_NOIO);
 	if (id < 0) {
 		mutex_unlock(&iscsi_ep_idr_mutex);
 		printk(KERN_ERR "Could not allocate endpoint ID. Error %d.\n",
@@ -257,6 +257,28 @@ free_ep:
 }
 EXPORT_SYMBOL_GPL(iscsi_create_endpoint);
 
+int iscsi_register_endpoint(struct iscsi_endpoint *ep)
+{
+	void *old;
+	int err = 0;
+
+	mutex_lock(&iscsi_ep_idr_mutex);
+	old = idr_find(&iscsi_ep_idr, ep->id);
+	if (old) {
+		err = -EBUSY;
+		goto unlock;
+	}
+
+	old = idr_replace(&iscsi_ep_idr, ep, ep->id);
+	if (IS_ERR(old))
+		err = PTR_ERR(old);
+unlock:
+	mutex_unlock(&iscsi_ep_idr_mutex);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(iscsi_register_endpoint);
+
 void iscsi_destroy_endpoint(struct iscsi_endpoint *ep)
 {
 	sysfs_remove_group(&ep->dev.kobj, &iscsi_endpoint_group);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 76de2b662f4fb..96299d25e0f73 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -459,6 +459,7 @@ extern void iscsi_get_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 struct iscsi_endpoint *iscsi_create_endpoint(int dd_size);
+int iscsi_register_endpoint(struct iscsi_endpoint *ep);
 extern void iscsi_destroy_endpoint(struct iscsi_endpoint *ep);
 extern struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle);
 extern void iscsi_put_endpoint(struct iscsi_endpoint *ep);
-- 
2.51.0

Re: [PATCH] scsi: iscsi: publish endpoints after transport setup

[Mike Christie]

On 6/17/26 1:21 PM, Ruoyu Wang wrote:
> iscsi_create_endpoint() inserts a new endpoint into iscsi_ep_idr before
> transport drivers have initialized their endpoint-private data. The

What is the bug you are fixing with this patch?

We should normally be doing iscsi_lookup_endpoint calls from interface
calls done under the rx_mutex. The ep creation is also done under the
mutex so we should never see a partially setup endpoint.

Is there an async error path where we do a lookup from?


> endpoint handle is returned only after ep_connect() completes, but handles
> are allocated from a predictable IDR and iscsi_lookup_endpoint() looks
> them up directly.
> 
> Reserve the endpoint ID with a NULL IDR entry, add
> iscsi_register_endpoint() for the publish step, and call it from the
> in-tree transport drivers after private endpoint setup has completed.
> Until registration, endpoint lookup keeps returning NULL for the reserved
> handle.
>