transport: free un-wanted responses in pldm_transport_send_recv_msg()
mctp-demux broadcasts the PLDM response messages to all of mctp
socket instances. That means the GetSensorReading response from one
instance (pldmd) can be received by the other instance (pldmtool)
which is waiting for the response of GetPDR request message. When the
gap time between the two requests of pldmtool is long enough the
number of GetSensorReading response messages can be more than 32. That
means the instance ID of new GetPDR command in pldmtool can equal with
the used and free one in GetSensorReading. The
`pldm_transport_send_recv_msg()` will detect the reponse of
GetSensorReading as the response of the GetPDR request.
To prevent this unexpected behavior the `pldm_transport_send_recv_msg()`
should free the un-wanted responses before sending new PLDM request. The
serialised message to be sent must already encode an allocated instance
ID, which by the specification must not be reused before expiry.
Therefore after the socket is drained, any subsequent response
containing the request's instance ID must be the response matching the
request.
Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>
[AJ: Add a test, massage the commit message]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I6c684bdaea2ba5d96b24ecd3c72e846c644fb16d
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 068c8a1..265ce67 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,7 @@
1. pdr: Allow record_handle to be NULL for pldm_pdr_add_check()
2. transport: pldm_transport_poll(): Adjust return value semantics
+3. transport: free un-wanted responses in pldm_transport_send_recv_msg()
### Deprecated
diff --git a/include/libpldm/requester/pldm.h b/include/libpldm/requester/pldm.h
index d7e2a0e..6b84acb 100644
--- a/include/libpldm/requester/pldm.h
+++ b/include/libpldm/requester/pldm.h
@@ -25,6 +25,7 @@
PLDM_REQUESTER_SETUP_FAIL = -10,
PLDM_REQUESTER_INVALID_SETUP = -11,
PLDM_REQUESTER_POLL_FAIL = -12,
+ PLDM_REQUESTER_TRANSPORT_BUSY = -13,
} pldm_requester_rc_t;
/* ------ Old API ---- deprecated */
diff --git a/src/transport/test.c b/src/transport/test.c
index 3d92111..0f35ffc 100644
--- a/src/transport/test.c
+++ b/src/transport/test.c
@@ -49,20 +49,33 @@
desc = &test->seq[test->cursor];
- if (desc->type != PLDM_TRANSPORT_TEST_ELEMENT_LATENCY) {
- return PLDM_REQUESTER_POLL_FAIL;
- }
+ if (desc->type == PLDM_TRANSPORT_TEST_ELEMENT_LATENCY) {
+ rc = timerfd_settime(test->timerfd, 0, &desc->latency, NULL);
+ if (rc < 0) {
+ return PLDM_REQUESTER_POLL_FAIL;
+ }
- rc = timerfd_settime(test->timerfd, 0, &desc->latency, NULL);
- if (rc < 0) {
+ /* This was an explicit latency element, so now move beyond it for recv */
+ test->cursor++;
+ } else if (desc->type == PLDM_TRANSPORT_TEST_ELEMENT_MSG_RECV) {
+ /* Expire the timer immediately so it appears ready */
+ static const struct itimerspec ready = {
+ .it_value = { 0, 1 },
+ .it_interval = { 0, 0 },
+ };
+ rc = timerfd_settime(test->timerfd, 0, &ready, NULL);
+ if (rc < 0) {
+ return PLDM_REQUESTER_POLL_FAIL;
+ }
+
+ /* Don't increment test->cursor as recv needs to consume the current test element */
+ } else {
return PLDM_REQUESTER_POLL_FAIL;
}
pollfd->fd = test->timerfd;
pollfd->events = POLLIN;
- test->cursor++;
-
return 0;
}
#endif
diff --git a/src/transport/transport.c b/src/transport/transport.c
index 1eeffb6..b860e2c 100644
--- a/src/transport/transport.c
+++ b/src/transport/transport.c
@@ -162,6 +162,7 @@
struct timeval now;
struct timeval end;
int ret;
+ int cnt;
if (req_msg_len < sizeof(*req_hdr) || !resp_msg_len) {
return PLDM_REQUESTER_INVALID_SETUP;
@@ -169,6 +170,20 @@
req_hdr = pldm_req_msg;
+ for (cnt = 0; cnt <= (PLDM_INSTANCE_MAX + 1) * PLDM_MAX_TIDS &&
+ pldm_transport_poll(transport, 0) == 1;
+ cnt++) {
+ rc = pldm_transport_recv_msg(transport, tid, pldm_resp_msg,
+ resp_msg_len);
+ if (rc == PLDM_REQUESTER_SUCCESS) {
+ /* This isn't the message we wanted */
+ free(*pldm_resp_msg);
+ }
+ }
+ if (cnt == (PLDM_INSTANCE_MAX + 1) * PLDM_MAX_TIDS) {
+ return PLDM_REQUESTER_TRANSPORT_BUSY;
+ }
+
rc = pldm_transport_send_msg(transport, tid, pldm_req_msg, req_msg_len);
if (rc != PLDM_REQUESTER_SUCCESS) {
return rc;
diff --git a/tests/transport.cpp b/tests/transport.cpp
index 9f00986..fcf2d0a 100644
--- a/tests/transport.cpp
+++ b/tests/transport.cpp
@@ -216,3 +216,55 @@
pldm_transport_test_destroy(test);
}
#endif
+
+#ifdef LIBPLDM_API_TESTING
+TEST(Transport, send_recv_drain_one_unwanted)
+{
+ uint8_t unwanted[] = {0x01, 0x00, 0x01, 0x01};
+ uint8_t req[] = {0x81, 0x00, 0x01, 0x01};
+ uint8_t resp[] = {0x01, 0x00, 0x01, 0x00};
+ const struct pldm_transport_test_descriptor seq[] = {
+ {
+ .type = PLDM_TRANSPORT_TEST_ELEMENT_MSG_RECV,
+ .recv_msg =
+ {
+ .src = 2,
+ .msg = unwanted,
+ .len = sizeof(unwanted),
+ },
+ },
+ {
+ .type = PLDM_TRANSPORT_TEST_ELEMENT_MSG_SEND,
+ .send_msg =
+ {
+ .dst = 1,
+ .msg = req,
+ .len = sizeof(req),
+ },
+ },
+ {
+ .type = PLDM_TRANSPORT_TEST_ELEMENT_MSG_RECV,
+ .recv_msg =
+ {
+ .src = 1,
+ .msg = resp,
+ .len = sizeof(resp),
+ },
+ },
+ };
+ struct pldm_transport_test* test = NULL;
+ struct pldm_transport* ctx;
+ size_t len;
+ void* msg;
+ int rc;
+
+ EXPECT_EQ(pldm_transport_test_init(&test, seq, ARRAY_SIZE(seq)), 0);
+ ctx = pldm_transport_test_core(test);
+ rc = pldm_transport_send_recv_msg(ctx, 1, req, sizeof(req), &msg, &len);
+ EXPECT_EQ(rc, PLDM_SUCCESS);
+ EXPECT_NE(memcmp(msg, unwanted, len), 0);
+ EXPECT_EQ(memcmp(msg, resp, len), 0);
+ free(msg);
+ pldm_transport_test_destroy(test);
+}
+#endif