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: [email protected]
> Suggested-by: D. Wythe <[email protected]>
> Signed-off-by: Yehyeong Lee <[email protected]>
> ---
> 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 <[email protected]>
