LU-17841 kfilnd: Race between hello and tagged RMA

A race exists between processing an incoming hello and initiating the
RMA for bulk operations that can result in RKEY re-use.

Initiator:
Posts tagged receive with RKEY based on peerA::kp_local_session_key X
and tn_mr_key Y
Bulk request (1) sent to target
Some earlier transaction fails:
 - Deletes peerA::kp_local_session_key X
 - Creates peerA::kp_local_session_key Z
 - HELLO request send to peerA

Target:
Processes HELLO request - updates kp_remote_session_key from X to Z.
Handles bulk request (1)
Performs RMA using session key Z and tn_mr_key Y, but completion is
delayed

Initiator:
Bulk request (1) hits timeout
 - Tagged receive canceled, and tn_mr_key Y is released
Posts tagged receive with RKEY based on peerA::kp_local_session_key Z
and tn_mr_key Y
Bulk request (2) sent to target

Target:
RMA for (1) is completed using the RKEY for (2)

The solution is to create a new bulk request message that contains
the session key used to set up the tagged buffer on the initiator.
This is compared against the session key exchanged during hello
handshake prior to initiating the RMA. If there's a mismatch
then the RMA is failed and the transaction is finalized. The session
key stored in the new bulk request is also used to generate the RKEY
rather than using the session key stored in the kfilnd_peer. This is
a protocol change so the KFILND_MSG_VERSION is bumped.

During testing it was found that the kfilnd_msg::version was not
being set correctly for immediate and bulk messages. To allow interop
the kfilnd_msg::version must be set to the handshaked negotiated
version that is stored in kfilnd_peer::kp_version. This has been
fixed. This issue only impacts kfilnd peers with message version > 1,
so backwards compatability between versions 1 and 2 will work
correctly.

The KFILND_TN_DEBUG macro is modified to print additional information
that was useful when debugging this issue.

Lastly, the TN_EVENT_TAG_TX_OK was missing from tn_event_to_str(), so
this is added.

HPE-bug-id: LUS-12317
Test-Parameters: trivial
Signed-off-by: Chris Horn <chris.horn@hpe.com>
Change-Id: I0b52a8367cd45b7587ba9ec3fa5212f548bebb57
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/55072
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Ian Ziemba <ian.ziemba@hpe.com>
Reviewed-by: Ron Gredvig <ron.gredvig@hpe.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lnet/klnds/kfilnd/kfilnd.c b/lnet/klnds/kfilnd/kfilnd.c
index 2066d41..09d2f4b 100644 (file)
--- a/lnet/klnds/kfilnd/kfilnd.c
+++ b/lnet/klnds/kfilnd/kfilnd.c
@@ -328,8 +328,16 @@ static int kfilnd_recv(struct lnet_ni *ni, void *private, struct lnet_msg *msg,
        }
 
        /* Store relevant fields to generate a bulk response. */
-       tn->tn_response_mr_key = rxmsg->proto.bulk_req.key;
-       tn->tn_response_rx = rxmsg->proto.bulk_req.response_rx;
+       if (rxmsg->version == KFILND_MSG_VERSION_1) {
+               tn->tn_response_mr_key = rxmsg->proto.bulk_req.key;
+               tn->tn_response_rx = rxmsg->proto.bulk_req.response_rx;
+               tn->tn_response_session_key = tn->tn_kp->kp_remote_session_key;
+       } else {
+               tn->tn_response_mr_key = rxmsg->proto.bulk_req_v2.kbrm2_key;
+               tn->tn_response_rx = rxmsg->proto.bulk_req_v2.kbrm2_response_rx;
+               tn->tn_response_session_key =
+                               rxmsg->proto.bulk_req_v2.kbrm2_session_key;
+       }
 
 #if 0
        tn->tn_tx_msg.length = kfilnd_init_proto(tn->tn_tx_msg.msg,

diff --git a/lnet/klnds/kfilnd/kfilnd.h b/lnet/klnds/kfilnd/kfilnd.h
index 816337a..82b50e5 100644 (file)
--- a/lnet/klnds/kfilnd/kfilnd.h
+++ b/lnet/klnds/kfilnd/kfilnd.h
@@ -515,6 +515,24 @@ struct kfilnd_bulk_req_msg {
        __u16 key;
 } __packed;
 
+struct kfilnd_bulk_req_msg_v2 {
+       /* Entire LNet header needed by the destination to match incoming
+        * message.
+        */
+       struct lnet_hdr_nid4 kbrm2_hdr;
+
+       /* Specific RX context the target must target to push/pull LNet
+        * payload.
+        */
+       __u32 kbrm2_response_rx;
+
+       /* Memory key needed by the target to push/pull LNet payload. */
+       __u16 kbrm2_key;
+
+       /* Session key used by peer. */
+       __u32 kbrm2_session_key;
+} __packed;
+
 /* Kfilnd message. Includes base transport header plus embedded protocol
  * message.
  */
@@ -548,13 +566,15 @@ struct kfilnd_msg {
                struct kfilnd_immed_msg immed;
                struct kfilnd_bulk_req_msg bulk_req;
                struct kfilnd_hello_msg hello;
+               struct kfilnd_bulk_req_msg_v2 bulk_req_v2;
        } __packed proto;
 } __packed;
 
 #define KFILND_MSG_MAGIC LNET_PROTO_KFI_MAGIC  /* unique magic */
 
 #define KFILND_MSG_VERSION_1   0x1
-#define KFILND_MSG_VERSION     KFILND_MSG_VERSION_1
+#define KFILND_MSG_VERSION_2   0x2
+#define KFILND_MSG_VERSION     KFILND_MSG_VERSION_2
 
 /* Get the KFI RX context from a KFI RX address. RX context information is
  * stored in the MSBs of the KFI address.
@@ -575,14 +595,22 @@ struct kfilnd_msg {
        !IS_ERR_OR_NULL((tn)->tn_kp)
 
 #define KFILND_TN_DIR_DEBUG(tn, fmt, dir, ...) \
-       CDEBUG(D_NET, "%s Transaction ID %p: %s:%u %s %s(%p):0x%llx " fmt "\n", \
+       CDEBUG(D_NET, "%s TN %p: %s:%u %s %s(%p):0x%llx lsk %u rsk %u tsk %u trmk %u tmk %u trr %u tta 0x%llx " fmt "\n", \
               msg_type_to_str(tn->msg_type), \
               (tn), \
               libcfs_nidstr(&(tn)->tn_ep->end_dev->kfd_ni->ni_nid), \
-              (tn)->tn_ep->end_context_id, dir, \
-              libcfs_nid2str((tn)->tn_kp->kp_nid), tn->tn_kp, \
-              KFILND_TN_PEER_VALID(tn) ? \
-               KFILND_RX_CONTEXT((tn)->tn_kp->kp_addr) : 0, \
+              (tn)->tn_ep->end_context_id, \
+              dir, \
+              libcfs_nid2str((tn)->tn_kp->kp_nid), \
+              (tn)->tn_kp, \
+              KFILND_RX_CONTEXT((tn)->tn_kp->kp_addr), \
+              (tn)->tn_kp->kp_local_session_key, \
+              (tn)->tn_kp->kp_remote_session_key, \
+              (tn)->tn_response_session_key, \
+              (tn)->tn_response_mr_key, \
+              (tn)->tn_mr_key, \
+              (tn)->tn_response_rx, \
+              (tn)->tn_target_addr, \
               ##__VA_ARGS__)
 
 #define KFILND_TN_DEBUG(tn, fmt, ...) \
@@ -711,6 +739,7 @@ static inline const char *tn_event_to_str(enum tn_events type)
                [TN_EVENT_RX_FAIL] = "TN_EVENT_RX_FAIL",
                [TN_EVENT_INIT_TAG_RMA] = "TN_EVENT_INIT_TAG_RMA",
                [TN_EVENT_SKIP_TAG_RMA] = "TN_EVENT_SKIP_TAG_RMA",
+               [TN_EVENT_TAG_TX_OK] = "TN_EVENT_TAG_TX_OK",
                [TN_EVENT_TAG_TX_FAIL] = "TN_EVENT_TAG_TX_FAIL",
        };
 
@@ -765,6 +794,7 @@ struct kfilnd_transaction {
         */
        u16                     tn_response_mr_key;
        u8                      tn_response_rx;
+       u32                     tn_response_session_key;
 
        /* Immediate data used to convey transaction state from LNet target to
         * LNet intiator.

diff --git a/lnet/klnds/kfilnd/kfilnd_ep.c b/lnet/klnds/kfilnd/kfilnd_ep.c
index 18f53fc..580ffad 100644 (file)
--- a/lnet/klnds/kfilnd/kfilnd_ep.c
+++ b/lnet/klnds/kfilnd/kfilnd_ep.c
@@ -199,10 +199,21 @@ int kfilnd_ep_gen_fake_err(struct kfilnd_ep *ep,
 
 static uint64_t gen_init_tag_bits(struct kfilnd_transaction *tn)
 {
-       return (tn->tn_kp->kp_remote_session_key << KFILND_EP_KEY_BITS) |
+       return (tn->tn_response_session_key << KFILND_EP_KEY_BITS) |
                tn->tn_response_mr_key;
 }
 
+static bool tn_session_key_is_valid(struct kfilnd_transaction *tn)
+{
+       if (tn->tn_response_session_key == tn->tn_kp->kp_remote_session_key)
+               return true;
+
+       KFILND_TN_DEBUG(tn, "Detected session key mismatch %u != %u\n",
+                       tn->tn_response_session_key,
+                       tn->tn_kp->kp_remote_session_key);
+       return false;
+}
+
 /**
  * kfilnd_ep_post_tagged_send() - Post a tagged send operation.
  * @ep: KFI LND endpoint used to post the tagged receivce operation.
@@ -230,6 +241,9 @@ int kfilnd_ep_post_tagged_send(struct kfilnd_ep *ep,
        if (ep->end_dev->kfd_state != KFILND_STATE_INITIALIZED)
                return -EINVAL;
 
+       if (!tn_session_key_is_valid(tn))
+               return -EINVAL;
+
        /* Progress transaction to failure if send should fail. */
        if (CFS_FAIL_CHECK(CFS_KFI_FAIL_TAGGED_SEND_EVENT)) {
                rc = kfilnd_ep_gen_fake_err(ep, &fake_error);
@@ -241,6 +255,9 @@ int kfilnd_ep_post_tagged_send(struct kfilnd_ep *ep,
                return -EAGAIN;
        }
 
+       KFILND_TN_DEBUG(tn, "tagged_data: %llu tn_status: %d\n",
+                       tn->tagged_data, tn->tn_status);
+
        rc = kfi_tsenddata(ep->end_tx, NULL, 0, NULL, tn->tagged_data,
                           tn->tn_target_addr, gen_init_tag_bits(tn), tn);
        switch (rc) {
@@ -471,6 +488,9 @@ int kfilnd_ep_post_write(struct kfilnd_ep *ep, struct kfilnd_transaction *tn)
        if (ep->end_dev->kfd_state != KFILND_STATE_INITIALIZED)
                return -EINVAL;
 
+       if (!tn_session_key_is_valid(tn))
+               return -EINVAL;
+
        /* Progress transaction to failure if read should fail. */
        if (CFS_FAIL_CHECK(CFS_KFI_FAIL_WRITE_EVENT)) {
                rc = kfilnd_ep_gen_fake_err(ep, &fake_error);
@@ -491,18 +511,19 @@ int kfilnd_ep_post_write(struct kfilnd_ep *ep, struct kfilnd_transaction *tn)
        case 0:
        case -EAGAIN:
                KFILND_EP_DEBUG(ep,
-                               "Transaction ID %p: %s write of %u bytes in %u frags with key 0x%x to peer 0x%llx: rc=%d",
+                               "TN ID %p: %s write of %u bytes in %u frags kp %s(%p) rma_iov.key %llu: rc=%d",
                                tn, rc ? "Failed to post" : "Posted",
                                tn->tn_nob, tn->tn_num_iovec,
-                               tn->tn_response_mr_key, tn->tn_target_addr, rc);
+                               libcfs_nid2str(tn->tn_kp->kp_nid), tn->tn_kp,
+                               rma_iov.key, rc);
                break;
 
        default:
                KFILND_EP_ERROR(ep,
-                               "Transaction ID %p: Failed to post write of %u bytes in %u frags with key 0x%x to peer 0x%llx: rc=%d",
+                               "TN %p: Failed to post write of %u bytes in %u frags kp %s(%p) rma_iov.key %llu: rc=%d",
                                tn, tn->tn_nob, tn->tn_num_iovec,
-                               tn->tn_response_mr_key, tn->tn_target_addr,
-                               rc);
+                               libcfs_nid2str(tn->tn_kp->kp_nid), tn->tn_kp,
+                               rma_iov.key, rc);
        }
 
        return rc;
@@ -547,6 +568,9 @@ int kfilnd_ep_post_read(struct kfilnd_ep *ep, struct kfilnd_transaction *tn)
        if (ep->end_dev->kfd_state != KFILND_STATE_INITIALIZED)
                return -EINVAL;
 
+       if (!tn_session_key_is_valid(tn))
+               return -EINVAL;
+
        /* Progress transaction to failure if read should fail. */
        if (CFS_FAIL_CHECK(CFS_KFI_FAIL_READ_EVENT)) {
                rc = kfilnd_ep_gen_fake_err(ep, &fake_error);
@@ -567,17 +591,19 @@ int kfilnd_ep_post_read(struct kfilnd_ep *ep, struct kfilnd_transaction *tn)
        case 0:
        case -EAGAIN:
                KFILND_EP_DEBUG(ep,
-                               "Transaction ID %p: %s read of %u bytes in %u frags with key 0x%x to peer 0x%llx: rc=%d",
+                               "TN %p: %s read of %u bytes in %u frags kp %s(%p) rma_iov.key %llu: rc=%d",
                                tn, rc ? "Failed to post" : "Posted",
                                tn->tn_nob, tn->tn_num_iovec,
-                               tn->tn_response_mr_key, tn->tn_target_addr, rc);
+                               libcfs_nid2str(tn->tn_kp->kp_nid), tn->tn_kp,
+                               rma_iov.key, rc);
                break;
 
        default:
                KFILND_EP_ERROR(ep,
-                               "Transaction ID %p: Failed to post read of %u bytes in %u frags with key 0x%x to peer 0x%llx: rc=%d",
+                               "TN %p: Failed to post read of %u bytes in %u frags kp %s(%p) rma_iov.key %llu: rc=%d",
                                tn, tn->tn_nob, tn->tn_num_iovec,
-                               tn->tn_response_mr_key, tn->tn_target_addr, rc);
+                               libcfs_nid2str(tn->tn_kp->kp_nid), tn->tn_kp,
+                               rma_iov.key, rc);
        }
 
        return rc;

diff --git a/lnet/klnds/kfilnd/kfilnd_tn.c b/lnet/klnds/kfilnd/kfilnd_tn.c
index 2ab71e8..a812097 100644 (file)
--- a/lnet/klnds/kfilnd/kfilnd_tn.c
+++ b/lnet/klnds/kfilnd/kfilnd_tn.c
@@ -46,18 +46,21 @@ static __sum16 kfilnd_tn_cksum(void *ptr, int nob)
        return NO_CHECKSUM;
 }
 
-static int kfilnd_tn_msgtype2size(enum kfilnd_msg_type type)
+static int kfilnd_tn_msgtype2size(struct kfilnd_msg *msg)
 {
        const int hdr_size = offsetof(struct kfilnd_msg, proto);
 
-       switch (type) {
+       switch (msg->type) {
        case KFILND_MSG_IMMEDIATE:
                return offsetof(struct kfilnd_msg, proto.immed.payload[0]);
 
        case KFILND_MSG_BULK_PUT_REQ:
        case KFILND_MSG_BULK_GET_REQ:
-               return hdr_size + sizeof(struct kfilnd_bulk_req_msg);
-
+               if (msg->version == KFILND_MSG_VERSION_1)
+                       return hdr_size + sizeof(struct kfilnd_bulk_req_msg);
+               else if (msg->version == KFILND_MSG_VERSION_2)
+                       return hdr_size + sizeof(struct kfilnd_bulk_req_msg_v2);
+               fallthrough;
        default:
                return -1;
        }
@@ -127,21 +130,36 @@ static void kfilnd_tn_pack_bulk_req(struct kfilnd_transaction *tn)
 {
        struct kfilnd_msg *msg = tn->tn_tx_msg.msg;
 
-       /* Pack the protocol header and payload. */
-       lnet_hdr_to_nid4(&tn->tn_lntmsg->msg_hdr, &msg->proto.bulk_req.hdr);
-       msg->proto.bulk_req.key = tn->tn_mr_key;
-       msg->proto.bulk_req.response_rx = tn->tn_response_rx;
-
        /* Pack the transport header. */
        msg->magic = KFILND_MSG_MAGIC;
-       msg->version = KFILND_MSG_VERSION;
+       msg->version = tn->tn_kp->kp_version;
        msg->type = tn->msg_type;
-       msg->nob = sizeof(struct kfilnd_bulk_req_msg) +
-               offsetof(struct kfilnd_msg, proto);
        msg->cksum = NO_CHECKSUM;
        msg->srcnid = lnet_nid_to_nid4(&tn->tn_ep->end_dev->kfd_ni->ni_nid);
        msg->dstnid = tn->tn_kp->kp_nid;
 
+       if (msg->version == KFILND_MSG_VERSION_1) {
+               msg->nob = sizeof(struct kfilnd_bulk_req_msg) +
+                       offsetof(struct kfilnd_msg, proto);
+
+               /* Pack the protocol header and payload. */
+               lnet_hdr_to_nid4(&tn->tn_lntmsg->msg_hdr,
+                                &msg->proto.bulk_req.hdr);
+               msg->proto.bulk_req.key = tn->tn_mr_key;
+               msg->proto.bulk_req.response_rx = tn->tn_response_rx;
+       } else {
+               msg->nob = sizeof(struct kfilnd_bulk_req_msg_v2) +
+                       offsetof(struct kfilnd_msg, proto);
+
+               /* Pack the protocol header and payload. */
+               lnet_hdr_to_nid4(&tn->tn_lntmsg->msg_hdr,
+                                &msg->proto.bulk_req_v2.kbrm2_hdr);
+               msg->proto.bulk_req_v2.kbrm2_key = tn->tn_mr_key;
+               msg->proto.bulk_req_v2.kbrm2_response_rx = tn->tn_response_rx;
+               msg->proto.bulk_req_v2.kbrm2_session_key =
+                                               tn->tn_kp->kp_local_session_key;
+       }
+
        /* Checksum entire message. */
        msg->cksum = kfilnd_tn_cksum(msg, msg->nob);
 
@@ -164,7 +182,7 @@ static void kfilnd_tn_pack_immed_msg(struct kfilnd_transaction *tn)
 
        /* Pack the transport header. */
        msg->magic = KFILND_MSG_MAGIC;
-       msg->version = KFILND_MSG_VERSION;
+       msg->version = tn->tn_kp->kp_version;
        msg->type = tn->msg_type;
        msg->nob = offsetof(struct kfilnd_msg, proto.immed.payload[tn->tn_nob]);
        msg->cksum = NO_CHECKSUM;
@@ -223,10 +241,10 @@ static int kfilnd_tn_unpack_msg(struct kfilnd_ep *ep, struct kfilnd_msg *msg,
                return -EPROTO;
        }
 
-       if (msg->nob < kfilnd_tn_msgtype2size(msg->type)) {
+       if (msg->nob < kfilnd_tn_msgtype2size(msg)) {
                KFILND_EP_ERROR(ep, "Short %s: %d(%d)\n",
                                msg_type_to_str(msg->type),
-                               msg->nob, kfilnd_tn_msgtype2size(msg->type));
+                               msg->nob, kfilnd_tn_msgtype2size(msg));
                return -EPROTO;
        }
 
@@ -824,7 +842,10 @@ static int kfilnd_tn_state_idle(struct kfilnd_transaction *tn,
                        rc = lnet_parse(tn->tn_ep->end_dev->kfd_ni,
                                        &hdr, &srcnid, tn, 0);
                } else {
-                       lnet_hdr_from_nid4(&hdr, &msg->proto.bulk_req.hdr);
+                       if (msg->version == KFILND_MSG_VERSION_1)
+                               lnet_hdr_from_nid4(&hdr, &msg->proto.bulk_req.hdr);
+                       else
+                               lnet_hdr_from_nid4(&hdr, &msg->proto.bulk_req_v2.kbrm2_hdr);
                        rc = lnet_parse(tn->tn_ep->end_dev->kfd_ni,
                                        &hdr, &srcnid, tn, 1);
                }