requester: instance-id: Release read lock on conflict
36af84cdbb66 ("requester: Add new APIs for instance ID allocation and
freeing") introduced the new instance ID allocation APIs, and some
unbalanced locking along with it. When a conflict arose on an instance
ID, the read lock was not released by the non-owning caller.
Release the lock on conflict and on error, and add a test case to
prevent regression.
gitlint-ignore: UC1, B1
Fixes: 36af84cdbb66 ("requester: Add new APIs for instance ID allocation and freeing")
Reported-by: Jerry Chen <jerry_c_chen@wiwynn.com>
Change-Id: Iecd1583c6b8863b458cc4fbf1ac42b20ca2a3433
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f6a904f..3a3c40d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -45,6 +45,10 @@
Anyone left using the deprecated paths can migrate using the coccinelle patch
at `evolutions/current/oem-ibm-header-compat.cocci`.
+### Fixed
+
+1. requester: instance-id: Release read lock on conflict
+
## [0.8.0] - 2024-05-23
### Added
diff --git a/src/requester/instance-id.c b/src/requester/instance-id.c
index 7b6de87..a2700af 100644
--- a/src/requester/instance-id.c
+++ b/src/requester/instance-id.c
@@ -99,20 +99,28 @@
return 0;
}
+static const struct flock pldm_instance_id_cfls = {
+ .l_type = F_RDLCK,
+ .l_whence = SEEK_SET,
+ .l_len = 1,
+};
+
+static const struct flock pldm_instance_id_cflx = {
+ .l_type = F_WRLCK,
+ .l_whence = SEEK_SET,
+ .l_len = 1,
+};
+
+static const struct flock pldm_instance_id_cflu = {
+ .l_type = F_UNLCK,
+ .l_whence = SEEK_SET,
+ .l_len = 1,
+};
+
LIBPLDM_ABI_STABLE
int pldm_instance_id_alloc(struct pldm_instance_db *ctx, pldm_tid_t tid,
pldm_instance_id_t *iid)
{
- static const struct flock cfls = {
- .l_type = F_RDLCK,
- .l_whence = SEEK_SET,
- .l_len = 1,
- };
- static const struct flock cflx = {
- .l_type = F_WRLCK,
- .l_whence = SEEK_SET,
- .l_len = 1,
- };
uint8_t l_iid;
if (!iid) {
@@ -138,7 +146,7 @@
loff = tid * PLDM_INST_ID_MAX + l_iid;
/* Reserving the TID's IID. Done via a shared lock */
- flop = cfls;
+ flop = pldm_instance_id_cfls;
flop.l_start = loff;
rc = fcntl(ctx->lock_db_fd, F_OFD_SETLK, &flop);
if (rc < 0) {
@@ -160,14 +168,16 @@
* practice because this is prevented by the lock database being
* opened O_RDONLY.
*/
- flop = cflx;
+ flop = pldm_instance_id_cflx;
flop.l_start = loff;
rc = fcntl(ctx->lock_db_fd, F_OFD_GETLK, &flop);
if (rc < 0) {
if (errno == EAGAIN || errno == EINTR) {
- return -EAGAIN;
+ rc = -EAGAIN;
+ goto release_cfls;
}
- return -EPROTO;
+ rc = -EPROTO;
+ goto release_cfls;
}
/* F_UNLCK is the type of the lock if we could successfully
@@ -178,9 +188,24 @@
*iid = l_iid;
return 0;
}
+
if (flop.l_type != F_RDLCK) {
+ rc = -EPROTO;
+ }
+
+ release_cfls:
+ flop = pldm_instance_id_cflu;
+ flop.l_start = loff;
+ if (fcntl(ctx->lock_db_fd, F_OFD_SETLK, &flop) < 0) {
+ if (errno == EAGAIN || errno == EINTR) {
+ return -EAGAIN;
+ }
return -EPROTO;
}
+
+ if (rc < 0) {
+ return rc;
+ }
}
/* Failed to allocate an IID after a full loop. Make the caller try
@@ -192,11 +217,6 @@
int pldm_instance_id_free(struct pldm_instance_db *ctx, pldm_tid_t tid,
pldm_instance_id_t iid)
{
- static const struct flock cflu = {
- .l_type = F_UNLCK,
- .l_whence = SEEK_SET,
- .l_len = 1,
- };
struct flock flop;
int rc;
@@ -205,7 +225,7 @@
return -EINVAL;
}
- flop = cflu;
+ flop = pldm_instance_id_cflu;
flop.l_start = tid * PLDM_INST_ID_MAX + iid;
rc = fcntl(ctx->lock_db_fd, F_OFD_SETLK, &flop);
if (rc < 0) {
diff --git a/tests/instance-id.cpp b/tests/instance-id.cpp
index ae4102d..ad290ac 100644
--- a/tests/instance-id.cpp
+++ b/tests/instance-id.cpp
@@ -237,6 +237,84 @@
ASSERT_EQ(pldm_instance_db_destroy(db), 0);
}
+TEST_F(PldmInstanceDbTest, releaseConflictedSameTid)
+{
+ static constexpr pldm_tid_t tid = 1;
+ struct
+ {
+ struct pldm_instance_db* db;
+ pldm_instance_id_t iid;
+ } connections[] = {
+ {nullptr, 0},
+ {nullptr, 0},
+ };
+ pldm_instance_id_t iid;
+
+ /* Allocate IID 0 for the TID to the first connection */
+ ASSERT_EQ(pldm_instance_db_init(&connections[0].db, dbPath.c_str()), 0);
+ EXPECT_EQ(
+ pldm_instance_id_alloc(connections[0].db, tid, &connections[0].iid), 0);
+
+ /*
+ * On the second connection, allocate the first available IID for the TID.
+ * This should generate a conflict on IID 0 (allocated to the first
+ * connection), and result in IID 1 being provided.
+ *
+ * There should now be one read lock held on each of IID 0 and IID 1 for TID
+ * 1 (by the first and second connections respectively).
+ */
+ ASSERT_EQ(pldm_instance_db_init(&connections[1].db, dbPath.c_str()), 0);
+ EXPECT_EQ(
+ pldm_instance_id_alloc(connections[1].db, tid, &connections[1].iid), 0);
+
+ /*
+ * Make sure the implementation hasn't allocated the connections a
+ * conflicting IID for the TID.
+ */
+ EXPECT_NE(connections[0].iid, connections[1].iid);
+
+ /*
+ * Now free the IID allocated to the first connection.
+ *
+ * We should be able to re-acquire this later.
+ */
+ EXPECT_EQ(pldm_instance_id_free(connections[0].db, tid, connections[0].iid),
+ 0);
+
+ /*
+ * Iterate through the IID space on the first connection to wrap it back
+ * around to IID 0.
+ *
+ * Note that:
+ *
+ * 1. The first connection has already allocated (and released) IID 0,
+ * eliminating one iteration
+ *
+ * 2. IID 1 is held by the second connection. This eliminates a second
+ * iteration as it must be skipped to avoid a conflict.
+ */
+ for (int i = 0; i < (pldmMaxInstanceIds - 1 - 1); i++)
+ {
+ EXPECT_EQ(pldm_instance_id_alloc(connections[0].db, tid, &iid), 0);
+ EXPECT_EQ(pldm_instance_id_free(connections[0].db, tid, iid), 0);
+ }
+
+ /*
+ * The next IID allocated to the first connection should be the IID it
+ * allocated initially (which should be 0).
+ */
+ EXPECT_EQ(pldm_instance_id_alloc(connections[0].db, tid, &iid), 0);
+ EXPECT_EQ(iid, connections[0].iid);
+
+ /* Now tidy up */
+ ASSERT_EQ(pldm_instance_id_free(connections[0].db, tid, iid), 0);
+
+ EXPECT_EQ(pldm_instance_id_free(connections[1].db, tid, connections[1].iid),
+ 0);
+ ASSERT_EQ(pldm_instance_db_destroy(connections[1].db), 0);
+ ASSERT_EQ(pldm_instance_db_destroy(connections[0].db), 0);
+}
+
TEST_F(PldmInstanceDbTest, freeUnallocatedInstanceId)
{
struct pldm_instance_db* db = nullptr;