[PATCH v1] qed: Protect iSCSI connection hash operations

[PATCH v1] qed: Protect iSCSI connection hash operations

[Binbin Deng]

The qed iSCSI connection hash is accessed without a common lock.
Operations first look up a qed_hash_iscsi_con wrapper and then keep
dereferencing its embedded connection object while firmware commands are
posted. Connection release removes the wrapper from the hash, releases
the firmware connection, and frees the wrapper. If two endpoints hash to
the same bucket, a release for one handle can race with lookup and object
use for another handle.

The race is reachable from qedi connection setup and teardown work: one
endpoint can call release_conn() while another endpoint concurrently calls
offload_conn(), update_conn(), clear_sq(), destroy_conn(), or change_mac().
The loop in qed_iscsi_get_hash() can also be traversing a bucket while a
node in that bucket is unlinked and freed.

This can cause a KASAN use-after-free, hash-list corruption, a stale
connection pointer being passed to firmware, or a panic. The storage stop
path also needs to see a stable empty/not-empty result before clearing the
started state.

Protect the hash and connection lifetime with a file-local mutex.  Hold it
across each complete connection operation, including firmware submission,
hash insertion or removal, and wrapper free. The mutex also serializes
connection acquisition with storage stop.

Found during a static-analysis-assisted audit: candidate sites were
surfaced by a Coccinelle rule, and the reader/writer paths were
confirmed by LLM-assisted (GLM-5.3) code inspection. Due to a lack
of hardware infrastructure, the issue was not observed at runtime.
It is identified by code inspection only.

Fixes: fc831825f99e ("qed: Add support for hardware offloaded iSCSI.")
Assisted-by: GLM-5.3 coccinelle
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/net/ethernet/qlogic/qed/qed_iscsi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
index 34f1217227bc..f7b97883e4b9 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/log2.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/stddef.h>
@@ -118,6 +119,8 @@ struct qed_iscsi_conn {
 	u8 abortive_dsconnect;
 };
 
+static DEFINE_MUTEX(qed_iscsi_conn_lock);
+
 static int qed_iscsi_async_event(struct qed_hwfn *p_hwfn, u8 fw_event_code,
 				 __le16 echo, union event_ring_data *data,
 				 u8 fw_return_code)
@@ -1080,6 +1083,7 @@ static int qed_iscsi_stop(struct qed_dev *cdev)
 {
 	int rc;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	if (!(cdev->flags & QED_FLAG_STORAGE_STARTED)) {
 		DP_NOTICE(cdev, "iscsi already stopped\n");
 		return 0;
@@ -1158,6 +1162,7 @@ static int qed_iscsi_acquire_conn(struct qed_dev *cdev,
 	struct qed_hash_iscsi_con *hash_con;
 	int rc;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	/* Allocate a hashed connection */
 	hash_con = kzalloc_obj(*hash_con, GFP_ATOMIC);
 	if (!hash_con)
@@ -1188,6 +1193,7 @@ static int qed_iscsi_release_conn(struct qed_dev *cdev, u32 handle)
 {
 	struct qed_hash_iscsi_con *hash_con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
@@ -1209,6 +1215,7 @@ static int qed_iscsi_offload_conn(struct qed_dev *cdev,
 	struct qed_hash_iscsi_con *hash_con;
 	struct qed_iscsi_conn *con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
@@ -1284,6 +1291,7 @@ static int qed_iscsi_update_conn(struct qed_dev *cdev,
 	struct qed_hash_iscsi_con *hash_con;
 	struct qed_iscsi_conn *con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
@@ -1308,6 +1316,7 @@ static int qed_iscsi_clear_conn_sq(struct qed_dev *cdev, u32 handle)
 {
 	struct qed_hash_iscsi_con *hash_con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
@@ -1324,6 +1333,7 @@ static int qed_iscsi_destroy_conn(struct qed_dev *cdev,
 {
 	struct qed_hash_iscsi_con *hash_con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
@@ -1354,6 +1364,7 @@ static int qed_iscsi_change_mac(struct qed_dev *cdev,
 {
 	struct qed_hash_iscsi_con *hash_con;
 
+	guard(mutex)(&qed_iscsi_conn_lock);
 	hash_con = qed_iscsi_get_hash(cdev, handle);
 	if (!hash_con) {
 		DP_NOTICE(cdev, "Failed to find connection for handle %d\n",
-- 
2.43.0

Re: [PATCH v1] qed: Protect iSCSI connection hash operations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds one global lock, `static
  DEFINE_MUTEX(qed_iscsi_conn_lock)` (qed_iscsi.c:122), but the data it…
- [Medium] The commit says the mutex "serializes connection acquisition
  with storage stop", but qed_iscsi_acquire_conn() never checks…
- [Low] qed_iscsi_start() still writes state the new lock is meant to
  protect, without taking qed_iscsi_conn_lock: `cdev->flags |=…

Pre-existing issues:
- [Low] Peer code: qed_fcoe.c and qed_nvmetcp.c use the same unlocked
  lookup/use/hlist_del+kfree pattern on cdev->connections, and the new…

--

qed: Protect iSCSI connection hash operations

This adds a static mutex to qed_iscsi.c. The mutex is taken with guard()
in qed_iscsi_stop() and in each iSCSI connection operation (acquire,
release, offload, update, clear_sq, destroy, change_mac). Hash lookup,
firmware submission and wrapper free are then serialized against
connection release.

> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
> index 34f1217227bc..f7b97883e4b9 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c

[ ... ]

> @@ -118,6 +119,8 @@ struct qed_iscsi_conn {
>  	u8 abortive_dsconnect;
>  };
>  
> +static DEFINE_MUTEX(qed_iscsi_conn_lock);
> +

[Severity: Medium]
This is one global lock, but the state it protects belongs to each
device. cdev->connections is declared in struct qed_dev as
DECLARE_HASHTABLE(connections, 10), and QED_FLAG_STORAGE_STARTED lives in
cdev->flags.

The lock is also held across blocking QED_SPQ_MODE_EBLOCK ramrods: conn
offload, update, clear_sq, terminate, mac update and FUNC_STOP. For
example:

qed_iscsi_offload_conn()
    guard(mutex)(&qed_iscsi_conn_lock);
    ...
    qed_sp_iscsi_conn_offload(..., QED_SPQ_MODE_EBLOCK, NULL)
        qed_spq_post()
            qed_spq_block()

qed_spq_block() polls with 1000 x msleep(5), drains the MCP, then polls
again before it declares the ramrod stuck. One stalled ramrod can
therefore hold the mutex for about 10 seconds or more.

Doesn't this serialize iSCSI connection setup, teardown and
qed_iscsi_stop() across every qed adapter in the system? If one PF's
firmware stalls, these would all wait behind it on unrelated adapters:

- qedi offload work
- ep_disconnect driven by cleanup work
- __qedi_remove()->qedi_ops->stop()

The netlink-driven paths are already serialized by rx_queue_mutex in
scsi_transport_iscsi.c. The extra cost therefore lands mostly on offload
work, cleanup-work disconnects and stop.

The commit message calls this a "file-local mutex", but it doesn't
mention the coupling across devices. Would a lock per device, in struct
qed_dev or struct qed_iscsi_info, close the same race without it?

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch.
qed_fcoe.c and qed_nvmetcp.c use the same unlocked pattern on
cdev->connections: lookup, use, then hlist_del() and kfree(). That
covers qed_fcoe_get_hash(), qed_fcoe_acquire_conn(),
qed_fcoe_release_conn(), qed_nvmetcp_get_hash() and
qed_nvmetcp_acquire_conn(). The new mutex does not cover any of them.

No in-tree race was found there. qedf only reaches the FCoE connection
ops from qedf_rport_event_handler(). libfc runs that handler on the
ordered rport_event_queue created in fc_setup_rport(). The NVMe-TCP ops
from qed_get_nvmetcp_ops() have no in-tree users.

>  static int qed_iscsi_async_event(struct qed_hwfn *p_hwfn, u8 fw_event_code,
>  				 __le16 echo, union event_ring_data *data,
>  				 u8 fw_return_code)
> @@ -1080,6 +1083,7 @@ static int qed_iscsi_stop(struct qed_dev *cdev)
>  {
>  	int rc;
>  
> +	guard(mutex)(&qed_iscsi_conn_lock);
>  	if (!(cdev->flags & QED_FLAG_STORAGE_STARTED)) {
>  		DP_NOTICE(cdev, "iscsi already stopped\n");
>  		return 0;

[Severity: Low]
qed_iscsi_stop(), the qed_iscsi_get_hash() callers, acquire and release
now all hold qed_iscsi_conn_lock. qed_iscsi_start() still writes the
same state without the lock, and it sets the flag before it initializes
the table:

qed_iscsi_start() {
    ...
	cdev->flags |= QED_FLAG_STORAGE_STARTED;
	hash_init(cdev->connections);
    ...
}

In a normal probe this can't overlap any connection op, because
ops->start runs before iscsi_host_add().

Recovery is different. qedi_recovery_handler() calls
__qedi_probe(QEDI_MODE_RECOVERY), which assigns a new qedi->cdev and
calls qedi_ops->start while the iSCSI host is still registered.

A qedi_ep_connect() that passed its QEDI_IN_RECOVERY check earlier could
call acquire_conn() on the new cdev at the same moment. hash_init() would
then drop that entry. Can this leak the wrapper and the CID?

Should qed_iscsi_start() take the lock, and set QED_FLAG_STORAGE_STARTED
only after hash_init()? This matters mostly together with a STARTED
check in qed_iscsi_acquire_conn() (see below).

The error paths in qed_iscsi_start() call qed_iscsi_stop(), which now
takes this mutex, and the mutex is not recursive. Would that need a
__qed_iscsi_stop() helper that runs with the lock already held?

[ ... ]

> @@ -1158,6 +1162,7 @@ static int qed_iscsi_acquire_conn(struct qed_dev *cdev,
>  	struct qed_hash_iscsi_con *hash_con;
>  	int rc;
>  
> +	guard(mutex)(&qed_iscsi_conn_lock);
>  	/* Allocate a hashed connection */
>  	hash_con = kzalloc_obj(*hash_con, GFP_ATOMIC);
>  	if (!hash_con)

[Severity: Medium]
The commit message says "The mutex also serializes connection
acquisition with storage stop". However, qed_iscsi_acquire_conn() never
checks QED_FLAG_STORAGE_STARTED. What happens if qed_iscsi_stop() gets
the mutex first?

CPU1 (recovery)
__qedi_remove(QEDI_MODE_RECOVERY)
    qedi_ops->stop()
        qed_iscsi_stop()
            guard(mutex)(&qed_iscsi_conn_lock);
            hash_empty() is true
            qed_sp_iscsi_func_stop()
            cdev->flags &= ~QED_FLAG_STORAGE_STARTED;

CPU2, waiting on the mutex in this path:
qedi_ep_connect()
    qedi_ops->acquire_conn()
        qed_iscsi_acquire_conn()
            kzalloc_obj()
            qed_iscsi_acquire_connection()
            hash_add()

Once CPU2 has inserted the entry, qed_iscsi_get_hash() returns NULL for
it because the started flag is clear:

	if (!(cdev->flags & QED_FLAG_STORAGE_STARTED))
		return NULL;

Every operation on that handle then fails with "Failed to find
connection": offload, update, clear_sq, destroy, change_mac and
release_conn. qedi only logs the release_conn failure.
qed_iscsi_free() only frees free_list entries.

Doesn't this leak the hash wrapper, the CID, and the connection's chains
and DMA buffers? Should qed_iscsi_acquire_conn() return an error while
holding the lock if the function isn't started?

This ordering is possible because qedi_ep_connect() checks
QEDI_IN_RECOVERY only once. That check can run before
qedi_schedule_recovery_handler() sets the bit.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020064701.535531-1-18983559317%40163.com