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

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: [email protected]
>> Reviewed-by: Mahanta Jambigi <[email protected]>
>> Signed-off-by: Hidayath Khan <[email protected]>
> Reviewed-by: Breno Leitao <[email protected]>
>
>> ---
>>   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.