Re: [PATCH net] net/smc: fix TOCTOU race between smc_listen_out() and listener close

On 05/08/26 12:51 pm, Dust Li wrote:
> On 2026-08-04 12:52:12, Sidraya Jayagond wrote:
>>
>>
>> On 03/08/26 6:24 pm, Breno Leitao wrote:
>>>> @@ -1931,11 +1931,12 @@ static void smc_listen_out(struct smc_sock *new_smc)
>>>>  		atomic_dec(&lsmc->queued_smc_hs);
>>>>  
>>>>  	release_sock(newsmcsk); /* lock in smc_listen_work() */
>>>> +	lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
>>>>  	if (lsmc->sk.sk_state == SMC_LISTEN) {
>>>> -		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
>>>>  		smc_accept_enqueue(&lsmc->sk, newsmcsk);
>>>>  		release_sock(&lsmc->sk);
>>>>  	} else { /* no longer listening */
>>>> +		release_sock(&lsmc->sk);
>>>>  		smc_close_non_accepted(newsmcsk);
>>>>  	}
>>>
>>> Do you need to call smc_close_non_accepted() without the lock? otherwise
>>> you can have the lock around the whole if/else clause.
>>
>> Yes, smc_close_non_accepted() calls __smc_release() in turn calls
>> smc_close_active(), which in the SMC_ACTIVE case hits
>> smc_close_stream_wait() a blocking wait that can sleep up to
>> SMC_MAX_STREAM_WAIT_TIMEOUT (2 minutes). Holding the listener lock
>> across that would stall any concurrent smc_accept(), smc_listen_out(),
>> or smc_release() on the listener for the same duration.
>>
>> The release_sock() is intentionally placed before
>> smc_close_non_accepted() to keep the critical section minimal:
>> lock
>>  check state
>>    enqueue or not
>>      unlock
>>        then do the slow close work without the listener lock held.
> 
> 
> That patch looks good to me.
> 
> Reviewed-by: Dust Li <[email protected]>
> 
> 
> Two corrections to the justification though:
> 
> 1. This newsmcsk sock have never been given to userspace, so
>    smc_close_stream_wait() should return immediately. I think the stronger
>    argument for keeping smc_close_non_accepted() outside the listener
>    lock is what follows in smc_close_active()/__smc_release():
>    smc_close_final() can wait up to SMC_WR_TX_WAIT_FREE_SLOT_TIME (10s)
>    for a WR slot, and smc_clcsock_release() tears down the TCP
>    socket.
> 
> 2. SMC_MAX_STREAM_WAIT_TIMEOUT is 2 * HZ, i.e. 2 seconds, not 2 minutes.
> 

Thank you for the corrections,Dust. Both points are valid.
> 
> Best regards,
> Dust
>