[PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach

[PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach

[Fan Wu]

An active-open connection may keep retry_timer armed because
cxgbi_sock_closed() returns early in CTP_ACTIVE_OPEN state.  On adapter
detach, the timer callback can then dereference csk->cdev after cdev has
been freed.

Set CTPF_OFFLOAD_DOWN under csk->lock during portmap cleanup, reject new
retry_timer arming, and make an already queued callback a no-op while
offload is down.  A connection still in CTP_ACTIVE_OPEN is pushed through
cxgbi_sock_fail_act_open() so that its ATID, L2T entry, port mapping, and
final reference are released while cdev is still valid, rather than left
for the now-disabled retry timer.  timer_delete_sync() then drains a
pending or running callback before the final cxgbi_sock_put().

This issue was found by an in-house static analysis tool.

Fixes: 7b36b6e03b0d ("[SCSI] cxgb4i v5: iscsi driver")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes in v2:
 - drop the lldi/t4 deref-move in csk_act_open_retry_timer(): csk->cdev is
   always valid when the callback runs (timer_delete_sync drains before the
   cdev free), per Varun Prakash.

 drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 8 +++++++-
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 9 ++++++++-
 drivers/scsi/cxgbi/libcxgbi.c      | 6 +++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
index 69de965..f61f280 100644
--- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
+++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
@@ -556,6 +556,11 @@ static void act_open_retry_timer(struct timer_list *t)
 
 	cxgbi_sock_get(csk);
 	spin_lock_bh(&csk->lock);
+	if (cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN)) {
+		spin_unlock_bh(&csk->lock);
+		cxgbi_sock_put(csk);
+		return;
+	}
 	skb = alloc_wr(sizeof(struct cpl_act_open_req), 0, GFP_ATOMIC);
 	if (!skb)
 		cxgbi_sock_fail_act_open(csk, -ENOMEM);
@@ -585,7 +590,8 @@ static int do_act_open_rpl(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
 
 	cxgbi_sock_get(csk);
 	spin_lock_bh(&csk->lock);
-	if (rpl->status == CPL_ERR_CONN_EXIST &&
+	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
+	    rpl->status == CPL_ERR_CONN_EXIST &&
 	    csk->retry_timer.function != act_open_retry_timer) {
 		csk->retry_timer.function = act_open_retry_timer;
 		mod_timer(&csk->retry_timer, jiffies + HZ / 2);
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 4267662..7c9b465 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1001,6 +1001,12 @@ static void csk_act_open_retry_timer(struct timer_list *t)
 	cxgbi_sock_get(csk);
 	spin_lock_bh(&csk->lock);
 
+	if (cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN)) {
+		spin_unlock_bh(&csk->lock);
+		cxgbi_sock_put(csk);
+		return;
+	}
+
 	if (t4) {
 		size = sizeof(struct cpl_act_open_req);
 		size6 = sizeof(struct cpl_act_open_req6);
@@ -1075,7 +1081,8 @@ static void do_act_open_rpl(struct cxgbi_device *cdev, struct sk_buff *skb)
 	cxgbi_sock_get(csk);
 	spin_lock_bh(&csk->lock);
 
-	if (status == CPL_ERR_CONN_EXIST &&
+	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
+	    status == CPL_ERR_CONN_EXIST &&
 	    csk->retry_timer.function != csk_act_open_retry_timer) {
 		csk->retry_timer.function = csk_act_open_retry_timer;
 		mod_timer(&csk->retry_timer, jiffies + HZ / 2);
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index ea9631b..131d4f0 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -107,8 +107,12 @@ void cxgbi_device_portmap_cleanup(struct cxgbi_device *cdev)
 				csk, cdev);
 			spin_lock_bh(&csk->lock);
 			cxgbi_sock_set_flag(csk, CTPF_OFFLOAD_DOWN);
-			cxgbi_sock_closed(csk);
+			if (csk->state == CTP_ACTIVE_OPEN)
+				cxgbi_sock_fail_act_open(csk, -ENODEV);
+			else
+				cxgbi_sock_closed(csk);
 			spin_unlock_bh(&csk->lock);
+			timer_delete_sync(&csk->retry_timer);
 			cxgbi_sock_put(csk);
 		}
 	}
-- 
2.34.1

Re: [PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach

[sashiko-bot]

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

New issues:
- [Critical] Use-After-Free and Double Free for CTP_ACTIVE_OPEN connections during adapter detach
- [Critical] Race Condition leading to NULL Pointer Dereference in do_act_open_rpl

Pre-existing issues:
- [High] Memory leak and un-cancelled timer on user-initiated disconnect
--

--- Patch [1]: [PATCH v2] scsi: cxgbi: fix retry_timer use-after-free on adapter detach ---
commit 5cacab0f814551fa10e52ff589125571018e3cee
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: cxgbi: fix retry_timer use-after-free on adapter detach

This commit attempts to fix a use-after-free on the retry_timer by setting
a new flag CTPF_OFFLOAD_DOWN during portmap cleanup. It rejects new timer
arming and forces connections in CTP_ACTIVE_OPEN to be pushed through
cxgbi_sock_fail_act_open() to release resources.

> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> index 0e2bee937fe81..207c8ffb5f188 100644
> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
[ ... ]
> @@ -584,7 +589,8 @@ static int do_act_open_rpl(struct t3cdev *tdev, struct sk_buff *skb, void *ctx)
>  
>  	cxgbi_sock_get(csk);
>  	spin_lock_bh(&csk->lock);
> -	if (rpl->status == CPL_ERR_CONN_EXIST &&
> +	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
> +	    rpl->status == CPL_ERR_CONN_EXIST &&
>  	    csk->retry_timer.function != act_open_retry_timer) {
>  		csk->retry_timer.function = act_open_retry_timer;
>  		mod_timer(&csk->retry_timer, jiffies + HZ / 2);

[Severity: Critical]
If CTPF_OFFLOAD_DOWN is set by a concurrent adapter detach, will this logic
fall through to the else branch and call cxgbi_sock_fail_act_open()?

When cxgbi_device_portmap_cleanup() already called cxgbi_sock_fail_act_open(),
csk->cdev gets cleared:

cxgbi_sock_closed()
   csk->cdev->csk_release_offload_resources(csk); 

If do_act_open_rpl() then calls cxgbi_sock_fail_act_open() again, it will set
the state to CTP_CONNECTING and call cxgbi_sock_closed().

Could this result in a NULL pointer dereference when cxgbi_sock_closed()
tries to access csk->cdev again?


> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> index 804806e1cbb4b..2e2a10117afcd 100644
> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
[ ... ]
> @@ -892,7 +898,8 @@ static void do_act_open_rpl(struct cxgbi_device *cdev, struct sk_buff *skb)
>  	cxgbi_sock_get(csk);
>  	spin_lock_bh(&csk->lock);
>  
> -	if (status == CPL_ERR_CONN_EXIST &&
> +	if (!cxgbi_sock_flag(csk, CTPF_OFFLOAD_DOWN) &&
> +	    status == CPL_ERR_CONN_EXIST &&
>  	    csk->retry_timer.function != csk_act_open_retry_timer) {
>  		csk->retry_timer.function = csk_act_open_retry_timer;
>  		mod_timer(&csk->retry_timer, jiffies + HZ / 2);

[Severity: Critical]
Is this susceptible to the same concurrent execution issue as cxgb3i above?

If do_act_open_rpl() executes concurrently with cxgbi_device_portmap_cleanup()
and falls into the else branch, it seems we might trigger a redundant teardown
and hit a NULL pointer dereference on csk->cdev.


> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index f3bb7af4e984e..7b44d47c03946 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
[ ... ]
> @@ -99,8 +99,12 @@ void cxgbi_device_portmap_cleanup(struct cxgbi_device *cdev)
>  				csk, cdev);
>  			spin_lock_bh(&csk->lock);
>  			cxgbi_sock_set_flag(csk, CTPF_OFFLOAD_DOWN);
> -			cxgbi_sock_closed(csk);
> +			if (csk->state == CTP_ACTIVE_OPEN)
> +				cxgbi_sock_fail_act_open(csk, -ENODEV);
> +			else
> +				cxgbi_sock_closed(csk);

[Severity: High]
This isn't a bug introduced by this patch, but does the primary user-initiated
disconnect path also need to be fixed for active-open connections?

When the iSCSI daemon initiates a disconnect while the connection is still in
CTP_ACTIVE_OPEN, cxgbi_ep_disconnect() calls cxgbi_sock_closed():

cxgbi_ep_disconnect()
    if (likely(csk->state >= CTP_ESTABLISHED))
        need_active_close(csk);
    else
        cxgbi_sock_closed(csk);

Because the state is CTP_ACTIVE_OPEN, cxgbi_sock_closed() returns early
without doing anything:

cxgbi_sock_closed()
    if (csk->state == CTP_ACTIVE_OPEN || csk->state == CTP_CLOSED)
        return;

Does this leave the retry_timer armed and offload resources unreleased,
causing a memory leak and a background timer loop?

[Severity: Critical]
Could calling cxgbi_sock_fail_act_open() here drop too many references and
lead to a use-after-free?

cxgbi_sock_fail_act_open() sets the state to CTP_CONNECTING and calls
cxgbi_sock_closed(). Because the state is no longer CTP_ACTIVE_OPEN, it
bypasses the early return in cxgbi_sock_closed().

This causes cxgbi_sock_closed() to drop references for the port, the offload
resources, and call cxgbi_sock_put(csk).

If the iSCSI daemon already disconnected, the socket's refcount could reach
0 inside cxgbi_sock_closed(), freeing the memory.


>  			spin_unlock_bh(&csk->lock);
> +			timer_delete_sync(&csk->retry_timer);
>  			cxgbi_sock_put(csk);
>  		}
>  	}

[Severity: Critical]
Are we potentially accessing freed memory here?

If csk was already freed during the cxgbi_sock_fail_act_open() call above,
would spin_unlock_bh(), timer_delete_sync(), and cxgbi_sock_put() operate
on freed memory?

Also, doesn't cxgbi_sock_fail_act_open() steal the endpoint's reference by
bypassing the early return in cxgbi_sock_closed(), resulting in a double-free
when this cxgbi_sock_put(csk) is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814020948.197655-1-fanwu01@zju.edu.cn?part=1