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

[Bernard Metzler <bernard.metzler@linux.dev>]

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.
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 ;)

Thanks,
Bernard.
>>
>> 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