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

[Leon Romanovsky <leon@kernel.org>]

On Thu, Aug 27, 2026 at 03:28:20PM +0200, Bernard Metzler wrote:
> On 27.08.2026 14:55, Guoqing Jiang wrote:
> > We need to clear cep before release state_lock as siw_qp_llp_close 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 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>
> > ---
> > V2: remove redundant code per Bernard's review
> > 
> >   drivers/infiniband/sw/siw/siw_cm.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
> > index 0245b25e7271..ed49818793dd 100644
> > --- a/drivers/infiniband/sw/siw/siw_cm.c
> > +++ b/drivers/infiniband/sw/siw/siw_cm.c
> > @@ -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) {
> > +		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);
> 
> Looks good. Thank you, Guoqing.
> 
> Acked-by: Bernard Metzler <bernard.metzler@linux.dev>

Bernard,

I will take this patch. However, siw_accept() looks wrong to me. It
mixes locking, qp->cep handling, and siw_cep_put() in a seemingly
inconsistent manner.

For example, here you clear qp->cep and call siw_cep_put() while holding
the lock, whereas in the error unwind path you do exactly the opposite.

Be aware that AI-generated suggestions for SIW are complete garbage. If
you do not clean up these code paths, your driver will soon end up in a
broken and unmaintainable state. The same applies to RXE.

Thanks