From 55fdb17b181bb4cd387469f5171632b8a76e4928 Mon Sep 17 00:00:00 2001 From: Matthew Rosato Date: Wed, 25 Oct 2023 11:55:49 -0400 Subject: [PATCH] ap_tools/ap-check: handle get-attributes between pre and post event Since mdevctl commit acf78c1ff6c9 it is now possible for the get-attributes event to occur between a pre-define and post-define. This is done in order to obtain the active attributes for the device before writing them to the config file, and implies that the get-attributes cannot re-obtain the file lock. For other cases where mdevctl calls get-attributes, the file lock is not already held and must be obtained by ap-check before reading attributes from active devices. To solve this, let's use the knowledge that mdevctl is a single-threaded tool and add a test to detect this scenario. If the file lock is already held by the parent during a get-attributes, don't attempt to re-acquire it. Reported-by: Boris Fiuczynski Reviewed-by: Boris Fiuczynski Signed-off-by: Matthew Rosato Signed-off-by: Steffen Eiden --- ap_tools/ap-check.c | 32 +++++++++++++++++++++++++++----- include/lib/ap.h | 1 + libap/ap.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/ap_tools/ap-check.c b/ap_tools/ap-check.c index 40fedaf9..a60a4be3 100644 --- a/ap_tools/ap-check.c +++ b/ap_tools/ap-check.c @@ -798,12 +798,34 @@ static int ap_check_handle_get_attributes(struct ap_check_anchor *anc) FILE *f; int rc; - rc = ap_get_lock_callout(); - if (rc) { - fprintf(stderr, "Failed to acquire configuration lock %d\n", rc); - return -1; + /* + * For the get-attributes callout, we are typically called without the + * callout lock held. However, there is a particular scenario (define + * of an active mdev) where we may or may not be called with the lock + * already held on behalf of mdevctl, depending on the mdevctl version. + * Let's test for lock ownership first and, if already owned by the + * parent (mdevctl) proceed rather than waiting on the file lock. + */ + rc = ap_try_lock_callout(); + switch (rc) { + case 0: + /* Lock acquired */ + anc->cleanup_lock = true; + break; + case 1: + /* Lock held by parent -- trust the lock will remain held */ + break; + default: + /* Lock not acquired or held by parent -- do a normal obtain */ + rc = ap_get_lock_callout(); + if (rc) { + fprintf(stderr, + "Failed to acquire configuration lock %d\n", + rc); + return -1; + } + anc->cleanup_lock = true; } - anc->cleanup_lock = true; /* * Read the 'matrix' and 'control_domains' attributes to get the diff --git a/include/lib/ap.h b/include/lib/ap.h index 2646ad58..58baeeb5 100644 --- a/include/lib/ap.h +++ b/include/lib/ap.h @@ -89,6 +89,7 @@ void ap_list_remove_all(struct util_list *list); /* Lock Functions */ int ap_get_lock(void); int ap_get_lock_callout(void); +int ap_try_lock_callout(void); int ap_release_lock(void); int ap_release_lock_callout(void); diff --git a/libap/ap.c b/libap/ap.c index 5e7798c7..e683b291 100644 --- a/libap/ap.c +++ b/libap/ap.c @@ -722,6 +722,34 @@ int ap_get_lock_callout(void) return util_lockfile_parent_lock(AP_LOCKFILE, AP_LOCK_RETRIES); } +/** + * Attempt to acquire the ap config lock using the Parent Process ID without + * waiting/retries. Detect if the attempt was rejected because the lock is + * already held by the Parent Process ID. + * + * @retval 0 Lock acquired on behalf of parent process + * @retval 1 Lock not obtained, already held by parent + * @retval != 0 Lock was not obtained, other error + */ +int ap_try_lock_callout(void) +{ + int pid, ppid, rc; + + if (util_lockfile_parent_lock(AP_LOCKFILE, 0)) { + /* Lock is already held, let's peek at the owner */ + ppid = getppid(); + rc = util_lockfile_peek_owner(AP_LOCKFILE, &pid); + if (rc || pid != ppid) { + /* We didn't get the lock, unknown or other owner */ + return 2; + } + /* Signify that the lock is already held by the caller */ + return 1; + } + + return 0; +} + /** * Release the ap config lock *