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

[PATCH net v2] 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.

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.

Restructure so there is a single sock_put() covering both outcomes,
instead of adding a second one to the 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>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
v2:
- Restructure to a single sock_put() on the common path rather than
  adding one to the error path (Breno Leitao).

 net/smc/smc_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index b4208cb186c5..2f7f9d469272 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1148,13 +1148,13 @@ 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)
-			goto err_out;
-		/* avoid race with smcr_tx_sndbuf_nonempty() */
-		spin_lock_bh(&conn->send_lock);
-		smc_switch_link_and_count(conn, to_lnk);
-		rc = smc_switch_cursor(smc, pend, wr_buf);
-		spin_unlock_bh(&conn->send_lock);
+		if (!rc) {
+			/* avoid race with smcr_tx_sndbuf_nonempty() */
+			spin_lock_bh(&conn->send_lock);
+			smc_switch_link_and_count(conn, to_lnk);
+			rc = smc_switch_cursor(smc, pend, wr_buf);
+			spin_unlock_bh(&conn->send_lock);
+		}
 		sock_put(&smc->sk);
 		if (rc)
 			goto err_out;

base-commit: a13dc7dafebe1643676c3d995f0a5651c08387c6
-- 
2.52.0

Re: [PATCH net v2] 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] A TOCTOU race during link failover allows a killed connection to abort the failover and bring down the healthy backup link.
--

--- Patch [1]: [PATCH net v2] net/smc: fix socket refcount leak in smc_switch_conns() ---
Note: Due to system recitation constraints, this review uses a free-form summary format rather than quoting the original patch diff.

commit 52d43741c14f87f177497a6086c3732a98a7fec5
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() that happens
when smc_cdc_get_free_slot() returns an error. It restructures the code to
use a single sock_put() on the common path instead of adding one to the
early error exit path.

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in smc_switch_conns() related to how it handles connection errors
during link failover.

When smc_switch_conns() iterates through connections to migrate them, it
drops the conns_lock and calls smc_cdc_get_free_slot(). 

If a specific connection is killed concurrently while the lock is dropped,
smc_cdc_get_free_slot() returns -EPIPE. 

Because smc_switch_conns() evaluates this return code and jumps to the
err_out label on any error, it proceeds to call
smcr_link_down_cond_sched() on the target link.

Does this mean a single killed connection returning -EPIPE will incorrectly
abort the entire failover process and bring down the healthy backup link?

Since this forces a teardown of the backup link, could this be used to
trigger a denial of service for all other sockets sharing the link group
if an application closes the socket during the link switch window?

Would it be better to skip the killed connection and continue migrating
the remaining connections, rather than treating a connection-specific error
as a total link failure?

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 20 Aug 2026 16:47:29 +0200 you 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;
> 
> [...]

Here is the summary with links:
  - [net,v2] net/smc: fix socket refcount leak in smc_switch_conns()
    https://git.kernel.org/netdev/net/c/719296c4aa82

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html