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

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

On 8/26/26 20:19, Bernard Metzler wrote:
> On 26.08.2026 08:38, Guoqing Jiang wrote:
>> 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;
>>
> That looks good, while I think we should always NULL a pointer to
> something before releasing a hold/reference to it. Putting the reference
> implies a memory barrier and so it's guaranteed others see that NULL 
> as well.

Hmm, good point.

> You are right - there are places where this is not done as it should.
>
>> 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.
>>
> Let's put this in an extra thread. You may have a point ;)

I will dig a little bit more before start a new thread.

Thanks,
Guoqing