Re: [PATCH net v6 3/3] net/smc: carry oversized SMC-Rv2 LLC messages in the queue entry

[Sidraya Jayagond <sidraya@linux.ibm.com>]

On 12/08/26 4:49 am, Yehyeong Lee wrote:
> smc_llc_rmt_delete_rkey() and smc_llc_save_add_link_rkeys() read the part
> of a v2 message that does not fit into the 44-byte union smc_llc_msg, and
> both bound themselves by the size of the buffer it landed in, not by what
> arrived.  On a link with a shared v2 receive buffer a 44-byte
> DELETE_RKEY_V2 declaring 255 rkeys reaches rkey[9..254] in whatever an
> earlier message left in lgr->wr_rx_buf_v2, and passes each of them to
> smc_rtoken_delete().  One of those 255 matched a registered rtoken and
> deleted it.  An ADD_LINK on such a link installs up to 255 rtokens from
> the same bytes.
> 
> Copy the tail into the queue entry, so its length is the length of the
> message that arrived, and declare the rkeys that fit inline as a member of
> the union instead of reaching them through a cast.  The same
> DELETE_RKEY_V2 now processes the 9 rkeys it carries.  The copy is limited
> to the longest tail the two functions can read, so the peer does not pick
> the size of the entry.
> 
> The bound the previous patch placed on links without a shared v2 receive
> buffer is no longer needed.
> 
> Fixes: 27ef6a9981fe ("net/smc: support SMC-R V2 for rdma devices with max_recv_sge equals to 1")
> Cc: stable@vger.kernel.org
> Suggested-by: D. Wythe <alibuda@linux.alibaba.com>
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
> Changes since v5: added the Fixes: and Cc: stable tags; asserted that the two
> DELETE_RKEY_V2 layouts agree on offsetof(rkey); limited the copied tail to
> what the two readers can use; corrected the comment in smc_wr_init_sge().
> 
> Measured over rxe with KASAN: a DELETE_RKEY_V2 carrying 12 rkeys over a link
> with a shared v2 receive buffer round-trips all 12 values, the last three
> coming from the copied tail; 8, 9 and 10 rkeys and a 44-byte message declaring
> 10 give 8, 9, 10 and 9 processed rkeys respectively.  kmemleak reports nothing
> over the link-addition path, and does report the queue entry when the free
> added by patch 1 is removed again.
> 
> Five runs per cell with and without the new limit: a 44-byte DELETE_RKEY_V2
> declaring 255 rkeys reports 9 processed on a link with and without a shared
> v2 receive buffer, an ADD_LINK v2 extension installs the 6 rtokens the peer
> sent, and no KASAN report appears.  The only message the limit changes in
> that lab is a REQ_ADD_LINK, which copied 16 bytes that have no reader and
> now copies none.  On the unpatched kernel the same DELETE_RKEY_V2 reports
> 254 and 255, and the ADD_LINK installs 255 rtokens per call.
> 

Nit: There are a few extra spaces after sentence ending punctuation in
the commit message. Please remove them.

>  net/smc/smc_llc.c | 125 ++++++++++++++++++++++++++++++++--------------
>  net/smc/smc_wr.c  |   6 +--
>  2 files changed, 91 insertions(+), 40 deletions(-)
> 
> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 748d65186f68..393aa0af18d1 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -157,6 +157,7 @@ struct smc_llc_msg_confirm_rkey {	/* type 0x06 */
>  };
>  
>  #define SMC_LLC_DEL_RKEY_MAX	8
> +#define SMC_LLC_DEL_RKEY_V2_INLINE	9
>  #define SMC_LLC_FLAG_RKEY_RETRY	0x10
>  #define SMC_LLC_FLAG_RKEY_NEG	0x20
>  
> @@ -177,6 +178,15 @@ struct smc_llc_msg_delete_rkey_v2 {	/* type 0x29 */
>  	__be32 rkey[];
>  };
>  
> +/* the leading rkeys of a DELETE_RKEY_V2 fit into union smc_llc_msg */
> +struct smc_llc_msg_delete_rkey_v2_inline {	/* type 0x29 */
> +	struct smc_llc_hdr hd;
> +	u8 num_rkeys;
> +	u8 num_inval_rkeys;
> +	u8 reserved[2];
> +	__be32 rkey[SMC_LLC_DEL_RKEY_V2_INLINE];
> +};
> +
>  union smc_llc_msg {
>  	struct smc_llc_msg_confirm_link confirm_link;
>  	struct smc_llc_msg_add_link add_link;
> @@ -186,6 +196,7 @@ union smc_llc_msg {
>  
>  	struct smc_llc_msg_confirm_rkey confirm_rkey;
>  	struct smc_llc_msg_delete_rkey delete_rkey;
> +	struct smc_llc_msg_delete_rkey_v2_inline delete_rkey_v2;
>  
>  	struct smc_llc_msg_test_link test_link;
>  	struct {
> @@ -194,15 +205,25 @@ union smc_llc_msg {
>  	} raw;
>  };
>  
> +static_assert(SMC_LLC_DEL_RKEY_V2_INLINE ==
> +	      (sizeof(union smc_llc_msg) -
> +	       offsetof(struct smc_llc_msg_delete_rkey_v2, rkey)) /
> +	      sizeof(__be32));
> +static_assert(offsetof(struct smc_llc_msg_delete_rkey_v2_inline, rkey) ==
> +	      offsetof(struct smc_llc_msg_delete_rkey_v2, rkey));
> +
>  #define SMC_LLC_FLAG_RESP		0x80
>  
>  struct smc_llc_qentry {
>  	struct list_head list;
>  	struct smc_link *link;
> +	u16 body_len;
>  	union smc_llc_msg msg;
> +	u8 body[] __counted_by(body_len);
>  };
>  
> -static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc);
> +static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc,
> +			    u32 byte_len);
>  
>  struct smc_llc_qentry *smc_llc_flow_qentry_clr(struct smc_llc_flow *flow)
>  {
> @@ -998,22 +1019,19 @@ static int smc_llc_cli_conf_link(struct smc_link *link,
>  
>  static void smc_llc_save_add_link_rkeys(struct smc_link *link,
>  					struct smc_link *link_new,
> -					u8 *llc_msg)
> +					struct smc_llc_qentry *qentry)
>  {
>  	const u32 rt_off = offsetof(struct smc_llc_msg_add_link_v2_ext, rt);
>  	struct smc_llc_msg_add_link_v2_ext *ext;
>  	struct smc_link_group *lgr = link->lgr;
>  	int max, i;
>  
> -	/* Without a shared v2 receive buffer the extension is not copied
> -	 * into the queue entry, so not even ext->num_rkeys is there.
> -	 */
> -	if (!smc_link_shared_v2_rxbuf(link))
> +	/* the rkey count itself is only there if enough bytes arrived */
> +	if (qentry->body_len < rt_off)
>  		return;
> -	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
> -						     SMC_WR_TX_SIZE);
> +	ext = (struct smc_llc_msg_add_link_v2_ext *)qentry->body;
>  	max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> -	max = min_t(u32, max, (SMC_WR_BUF_V2_SIZE - SMC_WR_TX_SIZE - rt_off) /
> +	max = min_t(u32, max, (qentry->body_len - rt_off) /
>  			      sizeof(ext->rt[0]));
>  	down_write(&lgr->rmbs_lock);
>  	for (i = 0; i < max; i++) {
> @@ -1107,9 +1125,7 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
>  	if (rc)
>  		goto out_clear_lnk;
>  	if (lgr->smc_version == SMC_V2) {
> -		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
> -			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)llc;
> -		smc_llc_save_add_link_rkeys(link, lnk_new, llc_msg);
> +		smc_llc_save_add_link_rkeys(link, lnk_new, qentry);
>  	} else {
>  		rc = smc_llc_cli_rkey_exchange(link, lnk_new);
>  		if (rc) {
> @@ -1510,9 +1526,7 @@ int smc_llc_srv_add_link(struct smc_link *link,
>  	if (rc)
>  		goto out_err;
>  	if (lgr->smc_version == SMC_V2) {
> -		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
> -			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
> -		smc_llc_save_add_link_rkeys(link, link_new, llc_msg);
> +		smc_llc_save_add_link_rkeys(link, link_new, qentry);
>  	} else {
>  		rc = smc_llc_srv_rkey_exchange(link, link_new);
>  		if (rc)
> @@ -1561,7 +1575,8 @@ void smc_llc_add_link_local(struct smc_link *link)
>  	add_llc.hd.common.llc_type = SMC_LLC_ADD_LINK;
>  	smc_llc_init_msg_hdr(&add_llc.hd, link->lgr, sizeof(add_llc));
>  	/* no dev and port needed */
> -	smc_llc_enqueue(link, (union smc_llc_msg *)&add_llc);
> +	smc_llc_enqueue(link, (union smc_llc_msg *)&add_llc,
> +			sizeof(union smc_llc_msg));
>  }
>  
>  /* worker to process an add link message */
> @@ -1597,7 +1612,8 @@ void smc_llc_srv_delete_link_local(struct smc_link *link, u8 del_link_id)
>  	del_llc.link_num = del_link_id;
>  	del_llc.reason = htonl(SMC_LLC_DEL_LOST_PATH);
>  	del_llc.hd.flags |= SMC_LLC_FLAG_DEL_LINK_ORDERLY;
> -	smc_llc_enqueue(link, (union smc_llc_msg *)&del_llc);
> +	smc_llc_enqueue(link, (union smc_llc_msg *)&del_llc,
> +			sizeof(union smc_llc_msg));
>  }
>  
>  static void smc_llc_process_cli_delete_link(struct smc_link_group *lgr)
> @@ -1819,27 +1835,28 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>  	link = qentry->link;
>  
>  	if (lgr->smc_version == SMC_V2) {
> -		const u32 rkey_off =
> -			offsetof(struct smc_llc_msg_delete_rkey_v2, rkey);
> -		struct smc_llc_msg_delete_rkey_v2 *llcv2;
> -		u32 buf_len;
> -
> -		if (smc_link_shared_v2_rxbuf(link)) {
> -			memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));
> -			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
> -			buf_len = SMC_WR_BUF_V2_SIZE;
> -		} else {
> -			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
> -			buf_len = sizeof(qentry->msg);
> -		}
> +		struct smc_llc_msg_delete_rkey_v2_inline *llcv2;
> +
> +		/* The leading SMC_LLC_DEL_RKEY_V2_INLINE rkeys are declared in
> +		 * the message itself, any further ones were received into
> +		 * qentry->body.
> +		 */
> +		llcv2 = &qentry->msg.delete_rkey_v2;
>  		llcv2->num_inval_rkeys = 0;
>  
>  		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> -		/* bound by the buffer llcv2 points at */
> -		max = min_t(u32, max, (buf_len - rkey_off) /
> -				      sizeof(llcv2->rkey[0]));
> +		max = min_t(u32, max, SMC_LLC_DEL_RKEY_V2_INLINE +
> +				      qentry->body_len / sizeof(__be32));
>  		for (i = 0; i < max; i++) {
> -			if (smc_rtoken_delete(link, llcv2->rkey[i]))
> +			__be32 rkey;
> +
> +			if (i < SMC_LLC_DEL_RKEY_V2_INLINE)
> +				rkey = llcv2->rkey[i];
> +			else
> +				memcpy(&rkey, qentry->body +
> +					      (i - SMC_LLC_DEL_RKEY_V2_INLINE) *
> +					      sizeof(rkey), sizeof(rkey));
> +			if (smc_rtoken_delete(link, rkey))
>  				llcv2->num_inval_rkeys++;
>  		}
>  		memset(&llc->rkey[0], 0, sizeof(llc->rkey));
> @@ -2080,18 +2097,52 @@ static void smc_llc_rx_response(struct smc_link *link,
>  	wake_up(&link->lgr->llc_msg_waiter);
>  }
>  
> -static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc)
> +/* the longest tail either reader of qentry->body can use */
> +static u32 smc_llc_max_body_len(union smc_llc_msg *llc)
> +{
> +	switch (llc->raw.hdr.common.llc_type) {
> +	case SMC_LLC_ADD_LINK:
> +		return offsetof(struct smc_llc_msg_add_link_v2_ext, rt) +
> +		       SMC_LLC_RKEYS_PER_MSG_V2 *
> +		       sizeof(struct smc_llc_msg_add_link_cont_rt);
> +	case SMC_LLC_DELETE_RKEY:
> +		return (SMC_LLC_RKEYS_PER_MSG_V2 -
> +			SMC_LLC_DEL_RKEY_V2_INLINE) * sizeof(__be32);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc,
> +			    u32 byte_len)
>  {
>  	struct smc_link_group *lgr = link->lgr;
>  	struct smc_llc_qentry *qentry;
>  	unsigned long flags;
> +	u16 body_len = 0;
> +
> +	/* V2 messages can be longer than the inline union smc_llc_msg. Carry
> +	 * the remainder in the qentry itself, so that its lifetime and its
> +	 * length match the message the peer actually sent.
> +	 */
> +	if (lgr->smc_version == SMC_V2 && byte_len > SMC_WR_TX_SIZE)
> +		body_len = min_t(u32, byte_len, SMC_WR_BUF_V2_SIZE) -
> +			   SMC_WR_TX_SIZE;
> +	body_len = min_t(u32, body_len, smc_llc_max_body_len(llc));
>  
> -	qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
> +	qentry = kmalloc_flex(*qentry, body, body_len, GFP_ATOMIC);
>  	if (!qentry)
>  		return;
> +	qentry->body_len = body_len;
>  	qentry->link = link;
>  	INIT_LIST_HEAD(&qentry->list);
>  	memcpy(&qentry->msg, llc, sizeof(union smc_llc_msg));
> +	if (body_len) {
> +		u8 *src = smc_link_shared_v2_rxbuf(link) ?
> +			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)llc;
> +
> +		memcpy(qentry->body, src + SMC_WR_TX_SIZE, body_len);
> +	}
>  
>  	/* process responses immediately */
>  	if ((llc->raw.hdr.flags & SMC_LLC_FLAG_RESP) &&
> @@ -2123,7 +2174,7 @@ static void smc_llc_rx_handler(struct ib_wc *wc, void *buf)
>  			return; /* invalid message */
>  	}
>  
> -	smc_llc_enqueue(link, llc);
> +	smc_llc_enqueue(link, llc, wc->byte_len);
>  }
>  
>  /***************************** worker, utils *********************************/
> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 59c92b46945c..97ba46893b17 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
> @@ -602,9 +602,9 @@ static void smc_wr_init_sge(struct smc_link *lnk)
>  
>  	/* With SMC-Rv2 there can be messages larger than SMC_WR_TX_SIZE.
>  	 * Each ib_recv_wr gets 2 sges, the second one is a spillover buffer
> -	 * and the same buffer for all sges. When a larger message arrived then
> -	 * the content of the first small sge is copied to the beginning of
> -	 * the larger spillover buffer, allowing easy data mapping.
> +	 * and the same buffer for all sges. The spillover sge starts at
> +	 * SMC_WR_TX_SIZE, so the leading bytes of that buffer are never
> +	 * written.
>  	 */
>  	for (i = 0; i < lnk->wr_rx_cnt; i++) {
>  		int x = i * lnk->wr_rx_sge_cnt;
Patch looks good to me.

Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>