Re: [PATCH] fix: infiniband/rxe: check_rkey: fix refcount underflow due to unchecked rxe_get return value

[Leon Romanovsky]

On Sat, Jun 27, 2026 at 07:09:07PM -0700, Zhu Yanjun wrote:
> 
> 在 2026/6/27 4:45, WenTao Liang 写道:
> > 
> > 
> > > 2026年6月27日 09:42，yanjun.zhu <yanjun.zhu@xxxxxxxxx> 写道：
> > > 
> > > On 6/26/26 8:05 AM, WenTao Liang wrote:
> > > > rxe_get is a conditional get (kref_get_unless_zero) that returns 0 when
> > > >   the object's refcount is already zero. In check_rkey, the
> > > > return value of
> > > >   rxe_get(mr) is ignored. If rxe_get fails (returns 0), the code
> > > > continues
> > > >   to use mr without a valid reference, and error paths will call
> > > >   rxe_put(mr) on an unheld reference, causing a refcount underflow.
> > > > Check the return value of rxe_get and bail out with an error
> > > > when it fails.
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: 290c4a902b79 ("RDMA/rxe: Fix \"Replace mr by rkey in
> > > > responder resources\"")
> > > > Signed-off-by: WenTao Liang <vulab@xxxxxxxxxxx>
> > > > ---
> > > >  drivers/infiniband/sw/rxe/rxe_resp.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c
> > > > b/drivers/infiniband/sw/rxe/rxe_resp.c
> > > > index 9cb2f6fbf2dd..0c3f3930b494 100644
> > > > --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> > > > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> > > > @@ -514,7 +514,12 @@ static enum resp_states check_rkey(struct
> > > > rxe_qp *qp,
> > > > if (mw->access & IB_ZERO_BASED)
> > > > qp->resp.offset = mw->addr;
> > > >  -rxe_get(mr);
> > > > +if (!rxe_get(mr)) {
> > > 
> > > Can you reproduce this (rxe_get(mr) = 0)?
> > > 
> > > Thanks a lot.
> > > 
> > > Zhu Yanjun
> > > 
> > > > +rxe_put(mw);
> > > > +mw = NULL;
> > > > +state = get_rkey_violation_state(pkt);
> > > > +goto err;
> > > > +}
> > > > rxe_put(mw);
> > > > mw = NULL;
> > > > } else {
> > 
> > Hi Zhu Yanjun,
> > 
> > Thank you for reviewing the patch.
> > 
> > I haven't been able to reproduce the exact scenario where rxe_get(mr)
> > returns 0 in testing, because the race window is extremely narrow.
> > 
> > However, this patch is a defensive fix based on code analysis:
> > 
> > 1. rxe_get() is a wrapper around kref_get_unless_zero and explicitly
> >    returns a success/failure indication. Ignoring the return value
> >    violates the API contract.
> static inline __must_check
> bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp)
> {
>     int old = refcount_read(r);
> 
>     do {
>         if (!old)
>             break;
>     } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i));
>     if (oldp)
>         *oldp = old;
> 
>     if (unlikely(old < 0 || old + i < 0))
> 
>         refcount_warn_saturate(r, REFCOUNT_ADD_NOT_ZERO_OVF);
> 
>     return old;
> }
> 
> 
> rxe_get will call the above function finally. Only mr->ref_cnt is zero, this
> rxe_get will return false.
> 
> When mr->ref_cnt is zero, mr will be destroyed in rxe_put. But in the
> function
> 
> 
>  444 static enum resp_states check_rkey(struct rxe_qp *qp,
>  445                    struct rxe_pkt_info *pkt)
> ...
>  507         mr = mw->mr;
>  508         if (!mr) {
>  509             rxe_dbg_qp(qp, "MW doesn't have an MR\n");
>  510             state = get_rkey_violation_state(pkt);
>  511             goto err;
>  512         }
>  513
>  514         if (mw->access & IB_ZERO_BASED)
>  515             qp->resp.offset = mw->addr;
>  516
>  517         rxe_get(mr);
>  518         rxe_put(mw);
>  519         mw = NULL;
> ...
> }
> 
> mr is guaranteed to be non-NULL by the checks above, and its reference count
> should be valid at this point.
> 
> It should be impossible for rxe_get() to fail here under normal conditions.

I came to the same conclusion. This rxe_get() is useless.

Thanks