Re: [PATCH] RDMA/siw: Clear association under lock if siw_qp_modify fails in siw_accept

[Guoqing Jiang <guoqing.jiang@linux.dev>]

Hi Bernard,

On 8/25/26 21:53, Bernard Metzler wrote:
> On 25.08.2026 15:09, Guoqing Jiang wrote:
>> We need to clear qp and cep before release state_lock as 
>> siw_qp_llp_closeo
>> and siw_qp_modify->siw_qp_llp_close did.
>>
>> Otherwise if siw_qp_modify() fails in siw_accept(), the QP's state_lock
>> is released before the error path cleanup. A concurrent ibv_modify_qp()
>> transitioning the QP to ERROR can race in this window:
>>
>>    siw_accept()                       ibv_modify_qp(ERROR)
>>    ----------------------             ----------------------
>>    siw_qp_modify() fails
>>    up_write(&qp->state_lock)
>> down_write(&qp->state_lock)
>>                                       nextstate_from_idle():
>>                      if (qp->cep)
>>                                         siw_cep_put(qp->cep) <- frees 
>> cep
>>                                         qp->cep = NULL
>>    goto error
>>      cep->qp = NULL                   <- UAF
>>
>> Clear qp->cep and cep->qp, and drop the association reference taken by
>> siw_cep_get(), all under the write lock held from the initial
>> down_write(&qp->state_lock). Thread B therefore sees qp->cep == NULL,
>> skips its own put, and cannot free the cep before siw_accept() is done
>> with it.
>>
>> Reported-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
>> Link: 
>> https://lore.kernel.org/linux-rdma/d6fbe475-a5c2-f975-99b0-a0bd6b6d10e8@linux.dev/T/#m5876c1ff2de8686a9a1173b8f1aa0ff5363a785c
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/sw/siw/siw_cm.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_cm.c 
>> b/drivers/infiniband/sw/siw/siw_cm.c
>> index 0245b25e7271..1573f2e888b2 100644
>> --- a/drivers/infiniband/sw/siw/siw_cm.c
>> +++ b/drivers/infiniband/sw/siw/siw_cm.c
>> @@ -1719,9 +1719,13 @@ int siw_accept(struct iw_cm_id *id, struct 
>> iw_cm_conn_param *params)
>>                  SIW_QP_ATTR_STATE | SIW_QP_ATTR_LLP_HANDLE |
>>                      SIW_QP_ATTR_ORD | SIW_QP_ATTR_IRD |
>>                      SIW_QP_ATTR_MPA);
>> +    if (rv) {
>> +        cep->qp = NULL;
> This can better be omitted since done in error_unlock path
> anyway and is redundant otherwise.

Right, thanks for the review! And probably call siw_cep_put before set 
qp->cep to
NULL as other places did.

> It would also better retain the logic of calling siw_qp_put(qp) right 
> after
> clearing cep's reference to the qp, as done in error path.

I suppose this change could be okay.

@@ -1719,9 +1719,12 @@ int siw_accept(struct iw_cm_id *id, struct 
iw_cm_conn_param *params)
                            SIW_QP_ATTR_STATE | SIW_QP_ATTR_LLP_HANDLE |
                                    SIW_QP_ATTR_ORD | SIW_QP_ATTR_IRD |
                                    SIW_QP_ATTR_MPA);
+       if (rv) {
+               siw_cep_put(cep);
+               qp->cep = NULL;
+               goto error_unlock;
+       }
         up_write(&qp->state_lock);
-       if (rv)
-               goto error;

I am wondering if there is another use-after-free case in 
siw_qp_cm_drop. Because
the beginning of siw_qp_cm_drop borrows the qp->cep pointer without 
holding a
reference. If another thread put the last reference of cep, then 
siw_qp_cm_drop
wakes and deferences cep which has been freed.

>
> Thank you!
> Bernard.> +        qp->cep = NULL;
>> +        siw_cep_put(cep);
>> +        goto error_unlock;
>> +    }
>>       up_write(&qp->state_lock);
>> -    if (rv)
>> -        goto error;
>>         siw_dbg_cep(cep, "[QP %u]: send mpa reply, %d byte pdata\n",
>>               qp_id(qp), params->private_data_len);

Thanks,
Guoqing