[PATCH net] net/smc: fix socket refcount leak in smc_switch_conns()

[PATCH net] net/smc: fix socket refcount leak in smc_switch_conns()

[Hidayath Khan]

smc_switch_conns() takes a reference on the SMC socket before dropping
lgr->conns_lock, so the connection stays alive while the CDC slot is
fetched:

        sock_hold(&smc->sk);
        read_unlock_bh(&lgr->conns_lock);
        /* pre-fetch buffer outside of send_lock, might sleep */
        rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
        if (rc)
                goto err_out;

The err_out label only drops the wr_tx link reference, so this early exit
returns without the matching sock_put().  The second error exit is not
affected because sock_put() has already run by then:

        rc = smc_switch_cursor(smc, pend, wr_buf);
        spin_unlock_bh(&conn->send_lock);
        sock_put(&smc->sk);
        if (rc)
                goto err_out;

A leaked sk_refcnt means the smc_sock is never destroyed.  Its send and
receive buffers stay allocated, and for a user socket the reference held
on the network namespace is never released, so the netns can no longer be
torn down.

smc_cdc_get_free_slot() fails when the target link goes down or when the
connection has been killed while the switch is in progress.  Both are
reachable during the link failover this function implements, so the leak
is triggered by the same hardware events that make smc_switch_conns() run
in the first place.

Drop the reference on the early error path.

Fixes: 95f7f3e7dc6b ("net/smc: improved fix wait on already cleared link")
Cc: stable@vger.kernel.org
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index b4208cb186c5..c0027d2fe4e8 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1148,8 +1148,10 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
 		read_unlock_bh(&lgr->conns_lock);
 		/* pre-fetch buffer outside of send_lock, might sleep */
 		rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
-		if (rc)
+		if (rc) {
+			sock_put(&smc->sk);
 			goto err_out;
+		}
 		/* avoid race with smcr_tx_sndbuf_nonempty() */
 		spin_lock_bh(&conn->send_lock);
 		smc_switch_link_and_count(conn, to_lnk);

base-commit: e18c8f801a5aa6c1d26af3b359234f11c13ef2c0
-- 
2.52.0

Re: [PATCH net] net/smc: fix socket refcount leak in smc_switch_conns()

[Breno Leitao]

On Tue, Aug 04, 2026 at 10:28:00AM +0200, Hidayath Khan wrote:
> smc_switch_conns() takes a reference on the SMC socket before dropping
> lgr->conns_lock, so the connection stays alive while the CDC slot is
> fetched:
> 
>         sock_hold(&smc->sk);
>         read_unlock_bh(&lgr->conns_lock);
>         /* pre-fetch buffer outside of send_lock, might sleep */
>         rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
>         if (rc)
>                 goto err_out;
> 
> The err_out label only drops the wr_tx link reference, so this early exit
> returns without the matching sock_put().  The second error exit is not
> affected because sock_put() has already run by then:
> 
>         rc = smc_switch_cursor(smc, pend, wr_buf);
>         spin_unlock_bh(&conn->send_lock);
>         sock_put(&smc->sk);
>         if (rc)
>                 goto err_out;
> 
> A leaked sk_refcnt means the smc_sock is never destroyed.  Its send and
> receive buffers stay allocated, and for a user socket the reference held
> on the network namespace is never released, so the netns can no longer be
> torn down.
> 
> smc_cdc_get_free_slot() fails when the target link goes down or when the
> connection has been killed while the switch is in progress.  Both are
> reachable during the link failover this function implements, so the leak
> is triggered by the same hardware events that make smc_switch_conns() run
> in the first place.
> 
> Drop the reference on the early error path.
> 
> Fixes: 95f7f3e7dc6b ("net/smc: improved fix wait on already cleared link")
> Cc: stable@vger.kernel.org
> Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

> ---
>  net/smc/smc_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5..c0027d2fe4e8 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1148,8 +1148,10 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
>  		read_unlock_bh(&lgr->conns_lock);
>  		/* pre-fetch buffer outside of send_lock, might sleep */
>  		rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);

Do you need sock_hold(smc->sk) to call smc_cdc_get_free_slot ? Otherwise
you can move the sock_hold() after the exit.

Re: [PATCH net] net/smc: fix socket refcount leak in smc_switch_conns()

[Hidayathulla Khan I]

On 04/08/26 2:36 pm, Breno Leitao wrote:
> On Tue, Aug 04, 2026 at 10:28:00AM +0200, Hidayath Khan wrote:
>> smc_switch_conns() takes a reference on the SMC socket before dropping
>> lgr->conns_lock, so the connection stays alive while the CDC slot is
>> fetched:
>>
>>          sock_hold(&smc->sk);
>>          read_unlock_bh(&lgr->conns_lock);
>>          /* pre-fetch buffer outside of send_lock, might sleep */
>>          rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
>>          if (rc)
>>                  goto err_out;
>>
>> The err_out label only drops the wr_tx link reference, so this early exit
>> returns without the matching sock_put().  The second error exit is not
>> affected because sock_put() has already run by then:
>>
>>          rc = smc_switch_cursor(smc, pend, wr_buf);
>>          spin_unlock_bh(&conn->send_lock);
>>          sock_put(&smc->sk);
>>          if (rc)
>>                  goto err_out;
>>
>> A leaked sk_refcnt means the smc_sock is never destroyed.  Its send and
>> receive buffers stay allocated, and for a user socket the reference held
>> on the network namespace is never released, so the netns can no longer be
>> torn down.
>>
>> smc_cdc_get_free_slot() fails when the target link goes down or when the
>> connection has been killed while the switch is in progress.  Both are
>> reachable during the link failover this function implements, so the leak
>> is triggered by the same hardware events that make smc_switch_conns() run
>> in the first place.
>>
>> Drop the reference on the early error path.
>>
>> Fixes: 95f7f3e7dc6b ("net/smc: improved fix wait on already cleared link")
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
> Reviewed-by: Breno Leitao <leitao@debian.org>
>
>> ---
>>   net/smc/smc_core.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index b4208cb186c5..c0027d2fe4e8 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1148,8 +1148,10 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
>>   		read_unlock_bh(&lgr->conns_lock);
>>   		/* pre-fetch buffer outside of send_lock, might sleep */
>>   		rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
> Do you need sock_hold(smc->sk) to call smc_cdc_get_free_slot ? Otherwise
> you can move the sock_hold() after the exit.

Thanks for the review.

Yes.  conns_lock is what pins the socket. The reference is taken in
smc_lgr_register_conn() and dropped in __smc_lgr_unregister_conn(), both
under that lock.  After read_unlock_bh() a concurrent close can free it,
and smc_cdc_get_free_slot() reads conn->killed after a sleeping
wait_event_interruptible_timeout().  Moving the hold later would turn the
leak into a use-after-free.

Re: [PATCH net] net/smc: fix socket refcount leak in smc_switch_conns()

[sashiko-bot]

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

Pre-existing issues:
- [High] Failover is aborted and the backup link is unnecessarily torn down if a single connection is closed or aborted during the process.
--

--- Patch [1]: [PATCH net] net/smc: fix socket refcount leak in smc_switch_conns() ---
commit 92ea50109006ec40a22d7ea0ca6e43139666208a
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: fix socket refcount leak in smc_switch_conns()

This commit fixes a socket refcount leak in smc_switch_conns(). It adds a
missing sock_put() call on an early error exit path where
smc_cdc_get_free_slot() fails.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1148,8 +1148,10 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
>  		read_unlock_bh(&lgr->conns_lock);
>  		/* pre-fetch buffer outside of send_lock, might sleep */
>  		rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
> -		if (rc)
> +		if (rc) {
> +			sock_put(&smc->sk);
>  			goto err_out;
> +		}

[Severity: High]
This is a pre-existing issue, but could this early exit path unintentionally
disrupt all other active connections on the link group?

In smc_switch_conns(), the code iterates through connections to migrate them
to the backup link. If a single connection is aborted concurrently (e.g.,
by remote input causing smc_conn_abort_work to run), smc_cdc_get_free_slot()
detects that the connection was killed and returns -EPIPE.

By jumping directly to err_out, the code calls smcr_link_down_cond_sched()
and tears down the entire backup link. Shouldn't the loop skip the single
dead connection and continue migrating the remaining healthy connections,
rather than aborting the failover for the whole link group?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804082800.498672-1-hidayath@linux.ibm.com?part=1