From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A901D36CDE0; Sun, 5 Jul 2026 12:26:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783254390; cv=none; b=jN7oJkzkrxjsJFkcO3yW43ZTLeco9tQCdcXAWvEh+oB8Av7r4okRFA1XLapZXvkz9xuHd8YKHTk7Vza03aqW4jNMs5YvyedVy9D69vtJp7pEoIFAih8YtQobsJzubJ5ViaUgQd4gsMTTbt7yljX3jM53Lmop58ZN+VG8w0Hts50= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783254390; c=relaxed/simple; bh=aB3Z9WGuFaC2BC1xe0HIlkuoBOZ55BrZBf5t7bPZfjY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TNJtVjhnBmCoJoV1/MV3l8OWvz/NT4WHVO+V40+KfVQ0OnuXDF3UBJYNQAO2zAO2IDi49CBO5kTltP+Hmi4+0CoFDMxfmqSSA0tOVKP1PTFRU4f8PvSWeSqRK1ktgO3Q7FMP2Jay3eOr6DHuocAYb7Yf4mLO/J3wsF3umZcVyrA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=JpXn5PS3; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JpXn5PS3" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 47A651F000E9; Sun, 5 Jul 2026 12:26:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783254389; bh=KzFnOOOKHluNEjhqZvTI1UdcNCzvMLYH9AIe8qq+Gzc=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=JpXn5PS3TBYSjnkFHLsi1wWWXKWOQ1inO4WtwzNg99riF44E+1BA9AmcP69uTQ5DB 135k2sUEqTDbszGyyGzXFByzhtUZxNnR8r2D/5TdQSGDxKzx4tk8pklt7Emnr/k1AW 80a4/CgBCTVIfVdb95HERzIaPPfzF6CXzy7KR4eBUG9KLmBtxBCOv2vd7YSNbmWmeB TzX+EJDIkZg9kwzbZJfq0P8Qv14moC1v1yejElKe4FMPyeXdGHG5HPkFdr+bys4uZG 5ac22Vyub9kLjWxyqxgxgAP/5GULjodurNbTCa7QlxbNsjl0f+xb52HP6KiRDtKOTC qnDbhJPHQ7sbQ== Date: Sun, 5 Jul 2026 15:26:23 +0300 From: Leon Romanovsky To: Zhu Yanjun Cc: WenTao Liang , Jason Gunthorpe , linux-rdma@vger.kernel.org, stable@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] fix: infiniband/rxe: check_rkey: fix refcount underflow due to unchecked rxe_get return value Message-ID: <20260705122623.GE15188@unreal> References: <20260626150511.50084-1-vulab@iscas.ac.cn> <6597C9CE-970B-4650-97BD-651F8226E3D9@iscas.ac.cn> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 写道： > > > > > > 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@vger.kernel.org > > > > Fixes: 290c4a902b79 ("RDMA/rxe: Fix \"Replace mr by rkey in > > > > responder resources\"") > > > > Signed-off-by: WenTao Liang > > > > --- > > > >  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