Re: [PATCH v3] libsemanage: block on store lock by default

On Wed, Jul 22, 2026 at 10:49 AM Stephen Smalley
<[email protected]> wrote:
>
> semanage_get_lock() polls the store lock once per second and gives up
> after sh->timeout seconds, which is hard-coded to 5 with no
> configuration setting to control it. A single semodule transaction on
> a real policy routinely takes more than 5 seconds, so any overlap
> makes the second caller fail. This makes rpm scriptlets, ansible,
> or other scripted usage of semodule fragile.
>
> Change the default timeout to -1 (wait indefinitely) and in that case
> use a blocking flock() instead of polling. Try a non-blocking flock()
> first for the common case where the lock is not held and if it is
> held, inform the user so they know why it may be taking longer than
> normal. Retain the timeout logic in case we decide to re-introduce
> other configurable settings in the future.
>
> Several other minor cleanups are included as well, e.g. there is no
> need to pass O_TRUNC for the lock file, and there is no need to set
> O_CLOEXEC a second time via fcntl() since open() already passed it.
>
> Fixes: https://github.com/SELinuxProject/selinux/issues/181
> Signed-off-by: Stephen Smalley <[email protected]>
> ---
> v3 fixes the description (s/requires/makes/).

Merged.

>
>  libsemanage/src/handle.c         |  2 +-
>  libsemanage/src/semanage_store.c | 81 +++++++++++++++-----------------
>  2 files changed, 39 insertions(+), 44 deletions(-)
>
> diff --git a/libsemanage/src/handle.c b/libsemanage/src/handle.c
> index 77d0ed8a..1ff86e60 100644
> --- a/libsemanage/src/handle.c
> +++ b/libsemanage/src/handle.c
> @@ -37,7 +37,7 @@
>  #include "semanage_conf.h"
>  #include "semanage_store.h"
>
> -#define SEMANAGE_COMMIT_READ_WAIT 5
> +#define SEMANAGE_COMMIT_READ_WAIT -1
>
>  static char *private_semanage_root = NULL;
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index f14ebe97..f6ca1afb 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -50,6 +50,7 @@ typedef struct dbase_policydb dbase_t;
>  #include <stdio_ext.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <time.h>
>  #include <unistd.h>
>  #include <sys/file.h>
>  #include <sys/stat.h>
> @@ -1944,61 +1945,55 @@ static int semanage_get_lock(semanage_handle_t *sh, const char *lock_name,
>                              const char *lock_file)
>  {
>         int fd;
> -       struct timeval origtime, curtime;
> -       int got_lock = 0;
> +       int left;
>
> -       if ((fd = open(lock_file, O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC,
> +       if ((fd = open(lock_file, O_RDWR | O_CREAT | O_CLOEXEC,
>                        S_IRUSR | S_IWUSR)) == -1) {
>                 ERR(sh, "Could not open direct %s at %s.", lock_name,
>                     lock_file);
>                 return -1;
>         }
> -       if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> -               ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
> -                   lock_file);
> -               close(fd);
> -               return -1;
> -       }
> +
> +       if (flock(fd, LOCK_EX | LOCK_NB) == 0)
> +               return fd;
> +       if (errno != EAGAIN && errno != EWOULDBLOCK)
> +               goto err;
>
>         if (sh->timeout == 0) {
> -               /* return immediately */
> -               origtime.tv_sec = 0;
> -       } else {
> -               origtime.tv_sec = sh->timeout;
> -       }
> -       origtime.tv_usec = 0;
> -       do {
> -               curtime.tv_sec = 1;
> -               curtime.tv_usec = 0;
> -               if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
> -                       got_lock = 1;
> -                       break;
> -               } else if (errno != EAGAIN) {
> -                       ERR(sh, "Error obtaining direct %s at %s.", lock_name,
> -                           lock_file);
> -                       close(fd);
> -                       return -1;
> -               }
> -               if (origtime.tv_sec > 0 || sh->timeout == -1) {
> -                       if (select(0, NULL, NULL, NULL, &curtime) == -1) {
> -                               if (errno == EINTR) {
> -                                       continue;
> -                               }
> -                               ERR(sh,
> -                                   "Error while waiting to get direct %s at %s.",
> -                                   lock_name, lock_file);
> -                               close(fd);
> -                               return -1;
> -                       }
> -                       origtime.tv_sec--;
> -               }
> -       } while (origtime.tv_sec > 0 || sh->timeout == -1);
> -       if (!got_lock) {
>                 ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
>                 close(fd);
>                 return -1;
>         }
> -       return fd;
> +
> +       INFO(sh, "Waiting for semanage %s at %s (held by another process).",
> +            lock_name, lock_file);
> +
> +       if (sh->timeout < 0) {
> +               while (flock(fd, LOCK_EX) != 0) {
> +                       if (errno != EINTR)
> +                               goto err;
> +               }
> +               return fd;
> +       }
> +
> +       for (left = sh->timeout; left > 0; left--) {
> +               const struct timespec ts = { 1, 0 };
> +
> +               nanosleep(&ts, NULL);
> +               if (flock(fd, LOCK_EX | LOCK_NB) == 0)
> +                       return fd;
> +               if (errno != EAGAIN && errno != EWOULDBLOCK)
> +                       goto err;
> +       }
> +
> +       ERR(sh, "Could not get direct %s at %s after %d seconds.", lock_name,
> +           lock_file, sh->timeout);
> +       close(fd);
> +       return -1;
> +err:
> +       ERR(sh, "Error obtaining direct %s at %s.", lock_name, lock_file);
> +       close(fd);
> +       return -1;
>  }
>
>  /* Locking for the module store for transactions.  This is very basic
> --
> 2.55.0
>