Cleanup: Move `create_posix_shm()` to common/shm.c
authorFrancis Deslauriers <francis.deslauriers@efficios.com>
Thu, 8 Oct 2020 20:59:57 +0000 (16:59 -0400)
committerJérémie Galarneau <jeremie.galarneau@efficios.com>
Wed, 24 Mar 2021 22:54:54 +0000 (18:54 -0400)
* This function will be used for trigger error accounting.

* This function is renamed to `shm_create_anonymous()` and now takes one
  parameter for the owner of the shared memory area. Again this is code
  reuse.

* Remove erroneous comment about the `name` parameter for the
  `shm_open()` function. It's the absence (and not its presence) of the
  '/' at the beginning of the name that makes the behaviour implementation
  defined.

* Move the bin/lttng-sessiond/shm.c to common/shm.c so that all shm
  related functions are in one location.

Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: I690b38801ffb4772e7c5069c4cccf470b0671f63

src/bin/lttng-sessiond/Makefile.am
src/bin/lttng-sessiond/main.c
src/bin/lttng-sessiond/register.c
src/bin/lttng-sessiond/shm.c [deleted file]
src/bin/lttng-sessiond/shm.h [deleted file]
src/common/Makefile.am
src/common/shm.c [new file with mode: 0644]
src/common/shm.h [new file with mode: 0644]
src/common/ust-consumer/ust-consumer.c
tests/unit/Makefile.am

index 1d3741c6129e550c0edc45da929f07e066b703c2..e71c1081443744445bbdeed89895c75c94979593 100644 (file)
@@ -20,7 +20,6 @@ lttng_sessiond_SOURCES = utils.c utils.h \
                        context.c context.h \
                        channel.c channel.h \
                        event.c event.h \
-                       shm.c shm.h \
                        consumer.c consumer.h \
                        session.c session.h \
                        modprobe.c modprobe.h kern-modules.h \
index a344cd4769dbce11493828e2c1f6bbb367f255c3..40a418d0cfd0a79c453a8637df7322e15aca286e 100644 (file)
@@ -51,7 +51,6 @@
 #include "event.h"
 #include "kernel.h"
 #include "kernel-consumer.h"
-#include "shm.h"
 #include "lttng-ust-ctl.h"
 #include "ust-consumer.h"
 #include "utils.h"
index 6381c117950f767ef705d2ccbb66a218d3c1701d..ac1583dc24eb8f3610dfcb481d8cfb19b726d962 100644 (file)
@@ -12,6 +12,7 @@
 #include <urcu.h>
 #include <common/futex.h>
 #include <common/macros.h>
+#include <common/shm.h>
 #include <common/utils.h>
 #include <sys/stat.h>
 
@@ -20,7 +21,6 @@
 #include "testpoint.h"
 #include "health-sessiond.h"
 #include "fd-limit.h"
-#include "shm.h"
 #include "utils.h"
 #include "thread.h"
 
diff --git a/src/bin/lttng-sessiond/shm.c b/src/bin/lttng-sessiond/shm.c
deleted file mode 100644 (file)
index 8b77442..0000000
+++ /dev/null
@@ -1,191 +0,0 @@
-/*
- * Copyright (C) 2011 David Goulet <david.goulet@polymtl.ca>
- * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
- *
- * SPDX-License-Identifier: GPL-2.0-only
- *
- */
-
-#define _LGPL_SOURCE
-#include <fcntl.h>
-#include <limits.h>
-#include <sys/mman.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <sys/wait.h>
-#include <unistd.h>
-#include <urcu.h>
-
-#include <common/error.h>
-
-#include "shm.h"
-
-/*
- * Using fork to set umask in the child process (not multi-thread safe). We
- * deal with the shm_open vs ftruncate race (happening when the sessiond owns
- * the shm and does not let everybody modify it, to ensure safety against
- * shm_unlink) by simply letting the mmap fail and retrying after a few
- * seconds. For global shm, everybody has rw access to it until the sessiond
- * starts.
- */
-static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
-{
-       int wait_shm_fd, ret;
-       mode_t mode;
-
-       assert(shm_path);
-
-       /* Default permissions */
-       mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP;
-
-       /*
-        * Change owner of the shm path.
-        */
-       if (global) {
-               /*
-                * If global session daemon, any application can
-                * register. Make it initially writeable so applications
-                * registering concurrently can do ftruncate() by
-                * themselves.
-                */
-               mode |= S_IROTH | S_IWOTH;
-       }
-
-       /*
-        * We're alone in a child process, so we can modify the process-wide
-        * umask.
-        */
-       umask(~mode);
-
-       /*
-        * Try creating shm (or get rw access). We don't do an exclusive open,
-        * because we allow other processes to create+ftruncate it concurrently.
-        *
-        * A sysctl, fs.protected_regular may prevent the session daemon from
-        * opening a previously created shm when the O_CREAT flag is provided.
-        * Systemd enables this ABI-breaking change by default since v241.
-        *
-        * First, attempt to use the create-or-open semantic that is
-        * desired here. If this fails with EACCES, work around this broken
-        * behaviour and attempt to open the shm without the O_CREAT flag.
-        *
-        * The two attempts are made in this order since applications are
-        * expected to race with the session daemon to create this shm.
-        * Attempting an shm_open() without the O_CREAT flag first could fail
-        * because the file doesn't exist. It could then be created by an
-        * application, which would cause a second try with the O_CREAT flag to
-        * fail with EACCES.
-        *
-        * Note that this introduces a new failure mode where a user could
-        * launch an application (creating the shm) and unlink the shm while
-        * the session daemon is launching, causing the second attempt
-        * to fail. This is not recovered-from as unlinking the shm will
-        * prevent userspace tracing from succeeding anyhow: the sessiond would
-        * use a now-unlinked shm, while the next application would create
-        * a new named shm.
-        */
-       wait_shm_fd = shm_open(shm_path, O_RDWR | O_CREAT, mode);
-       if (wait_shm_fd < 0) {
-               if (errno == EACCES) {
-                       /* Work around sysctl fs.protected_regular. */
-                       DBG("shm_open of %s returned EACCES, this may be caused "
-                                       "by the fs.protected_regular sysctl. "
-                                       "Attempting to open the shm without "
-                                       "creating it.", shm_path);
-                       wait_shm_fd = shm_open(shm_path, O_RDWR, mode);
-               }
-               if (wait_shm_fd < 0) {
-                       PERROR("Failed to open wait shm at %s", shm_path);
-                       goto error;
-               }
-       }
-
-       ret = ftruncate(wait_shm_fd, mmap_size);
-       if (ret < 0) {
-               PERROR("ftruncate wait shm");
-               exit(EXIT_FAILURE);
-       }
-
-       if (global) {
-               ret = fchown(wait_shm_fd, 0, 0);
-               if (ret < 0) {
-                       PERROR("fchown");
-                       exit(EXIT_FAILURE);
-               }
-               /*
-                * If global session daemon, any application can
-                * register so the shm needs to be set in read-only mode
-                * for others.
-                */
-               mode &= ~S_IWOTH;
-               ret = fchmod(wait_shm_fd, mode);
-               if (ret < 0) {
-                       PERROR("fchmod");
-                       exit(EXIT_FAILURE);
-               }
-       } else {
-               ret = fchown(wait_shm_fd, getuid(), getgid());
-               if (ret < 0) {
-                       PERROR("fchown");
-                       exit(EXIT_FAILURE);
-               }
-       }
-
-       DBG("Got the wait shm fd %d", wait_shm_fd);
-
-       return wait_shm_fd;
-
-error:
-       DBG("Failing to get the wait shm fd");
-
-       return -1;
-}
-
-/*
- * Return the wait shm mmap for UST application notification. The global
- * variable is used to indicate if the the session daemon is global
- * (root:tracing) or running with an unprivileged user.
- *
- * This returned value is used by futex_wait_update() in futex.c to WAKE all
- * waiters which are UST application waiting for a session daemon.
- */
-char *shm_ust_get_mmap(char *shm_path, int global)
-{
-       size_t mmap_size;
-       int wait_shm_fd, ret;
-       char *wait_shm_mmap;
-       long sys_page_size;
-
-       assert(shm_path);
-
-       sys_page_size = sysconf(_SC_PAGE_SIZE);
-       if (sys_page_size < 0) {
-               PERROR("sysconf PAGE_SIZE");
-               goto error;
-       }
-       mmap_size = sys_page_size;
-
-       wait_shm_fd = get_wait_shm(shm_path, mmap_size, global);
-       if (wait_shm_fd < 0) {
-               goto error;
-       }
-
-       wait_shm_mmap = mmap(NULL, mmap_size, PROT_WRITE | PROT_READ,
-                       MAP_SHARED, wait_shm_fd, 0);
-
-       /* close shm fd immediately after taking the mmap reference */
-       ret = close(wait_shm_fd);
-       if (ret) {
-               PERROR("Error closing fd");
-       }
-
-       if (wait_shm_mmap == MAP_FAILED) {
-               DBG("mmap error (can be caused by race with ust).");
-               goto error;
-       }
-
-       return wait_shm_mmap;
-
-error:
-       return NULL;
-}
diff --git a/src/bin/lttng-sessiond/shm.h b/src/bin/lttng-sessiond/shm.h
deleted file mode 100644 (file)
index 94d2b72..0000000
+++ /dev/null
@@ -1,14 +0,0 @@
-/*
- * Copyright (C) 2011 David Goulet <david.goulet@polymtl.ca>
- * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
- *
- * SPDX-License-Identifier: GPL-2.0-only
- *
- */
-
-#ifndef _LTT_SHM_H
-#define _LTT_SHM_H
-
-char *shm_ust_get_mmap(char *shm_path, int global);
-
-#endif /* _LTT_SHM_H */
index a56775e310a3b24ee35a880cff2e73707a4652b1..2dca5a4a33f3b311819ce83ce9680851f399f786 100644 (file)
@@ -82,6 +82,7 @@ libcommon_la_SOURCES = \
        pipe.c pipe.h \
        readwrite.c readwrite.h \
        runas.c runas.h \
+       shm.c shm.h \
        session-descriptor.c \
        snapshot.c snapshot.h \
        spawn-viewer.c spawn-viewer.h \
diff --git a/src/common/shm.c b/src/common/shm.c
new file mode 100644 (file)
index 0000000..737f73e
--- /dev/null
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2011 David Goulet <david.goulet@polymtl.ca>
+ * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ */
+
+#define _LGPL_SOURCE
+#include <fcntl.h>
+#include <limits.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <urcu.h>
+
+#include <common/error.h>
+
+#include "shm.h"
+
+/*
+ * Using fork to set umask in the child process (not multi-thread safe). We
+ * deal with the shm_open vs ftruncate race (happening when the sessiond owns
+ * the shm and does not let everybody modify it, to ensure safety against
+ * shm_unlink) by simply letting the mmap fail and retrying after a few
+ * seconds. For global shm, everybody has rw access to it until the sessiond
+ * starts.
+ */
+static int get_wait_shm(char *shm_path, size_t mmap_size, int global)
+{
+       int wait_shm_fd, ret;
+       mode_t mode;
+
+       assert(shm_path);
+
+       /* Default permissions */
+       mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP;
+
+       /*
+        * Change owner of the shm path.
+        */
+       if (global) {
+               /*
+                * If global session daemon, any application can
+                * register. Make it initially writeable so applications
+                * registering concurrently can do ftruncate() by
+                * themselves.
+                */
+               mode |= S_IROTH | S_IWOTH;
+       }
+
+       /*
+        * We're alone in a child process, so we can modify the process-wide
+        * umask.
+        */
+       umask(~mode);
+
+       /*
+        * Try creating shm (or get rw access). We don't do an exclusive open,
+        * because we allow other processes to create+ftruncate it concurrently.
+        *
+        * A sysctl, fs.protected_regular may prevent the session daemon from
+        * opening a previously created shm when the O_CREAT flag is provided.
+        * Systemd enables this ABI-breaking change by default since v241.
+        *
+        * First, attempt to use the create-or-open semantic that is
+        * desired here. If this fails with EACCES, work around this broken
+        * behaviour and attempt to open the shm without the O_CREAT flag.
+        *
+        * The two attempts are made in this order since applications are
+        * expected to race with the session daemon to create this shm.
+        * Attempting an shm_open() without the O_CREAT flag first could fail
+        * because the file doesn't exist. It could then be created by an
+        * application, which would cause a second try with the O_CREAT flag to
+        * fail with EACCES.
+        *
+        * Note that this introduces a new failure mode where a user could
+        * launch an application (creating the shm) and unlink the shm while
+        * the session daemon is launching, causing the second attempt
+        * to fail. This is not recovered-from as unlinking the shm will
+        * prevent userspace tracing from succeeding anyhow: the sessiond would
+        * use a now-unlinked shm, while the next application would create
+        * a new named shm.
+        */
+       wait_shm_fd = shm_open(shm_path, O_RDWR | O_CREAT, mode);
+       if (wait_shm_fd < 0) {
+               if (errno == EACCES) {
+                       /* Work around sysctl fs.protected_regular. */
+                       DBG("shm_open of %s returned EACCES, this may be caused "
+                                       "by the fs.protected_regular sysctl. "
+                                       "Attempting to open the shm without "
+                                       "creating it.", shm_path);
+                       wait_shm_fd = shm_open(shm_path, O_RDWR, mode);
+               }
+               if (wait_shm_fd < 0) {
+                       PERROR("Failed to open wait shm at %s", shm_path);
+                       goto error;
+               }
+       }
+
+       ret = ftruncate(wait_shm_fd, mmap_size);
+       if (ret < 0) {
+               PERROR("ftruncate wait shm");
+               exit(EXIT_FAILURE);
+       }
+
+       if (global) {
+               ret = fchown(wait_shm_fd, 0, 0);
+               if (ret < 0) {
+                       PERROR("fchown");
+                       exit(EXIT_FAILURE);
+               }
+               /*
+                * If global session daemon, any application can
+                * register so the shm needs to be set in read-only mode
+                * for others.
+                */
+               mode &= ~S_IWOTH;
+               ret = fchmod(wait_shm_fd, mode);
+               if (ret < 0) {
+                       PERROR("fchmod");
+                       exit(EXIT_FAILURE);
+               }
+       } else {
+               ret = fchown(wait_shm_fd, getuid(), getgid());
+               if (ret < 0) {
+                       PERROR("fchown");
+                       exit(EXIT_FAILURE);
+               }
+       }
+
+       DBG("Got the wait shm fd %d", wait_shm_fd);
+
+       return wait_shm_fd;
+
+error:
+       DBG("Failing to get the wait shm fd");
+
+       return -1;
+}
+
+/*
+ * Return the wait shm mmap for UST application notification. The global
+ * variable is used to indicate if the the session daemon is global
+ * (root:tracing) or running with an unprivileged user.
+ *
+ * This returned value is used by futex_wait_update() in futex.c to WAKE all
+ * waiters which are UST application waiting for a session daemon.
+ */
+char *shm_ust_get_mmap(char *shm_path, int global)
+{
+       size_t mmap_size;
+       int wait_shm_fd, ret;
+       char *wait_shm_mmap;
+       long sys_page_size;
+
+       assert(shm_path);
+
+       sys_page_size = sysconf(_SC_PAGE_SIZE);
+       if (sys_page_size < 0) {
+               PERROR("sysconf PAGE_SIZE");
+               goto error;
+       }
+       mmap_size = sys_page_size;
+
+       wait_shm_fd = get_wait_shm(shm_path, mmap_size, global);
+       if (wait_shm_fd < 0) {
+               goto error;
+       }
+
+       wait_shm_mmap = mmap(NULL, mmap_size, PROT_WRITE | PROT_READ,
+                       MAP_SHARED, wait_shm_fd, 0);
+
+       /* close shm fd immediately after taking the mmap reference */
+       ret = close(wait_shm_fd);
+       if (ret) {
+               PERROR("Error closing fd");
+       }
+
+       if (wait_shm_mmap == MAP_FAILED) {
+               DBG("mmap error (can be caused by race with ust).");
+               goto error;
+       }
+
+       return wait_shm_mmap;
+
+error:
+       return NULL;
+}
+
+/*
+ * shm_create_anonymous is never called concurrently within a process.
+ */
+int shm_create_anonymous(const char *owner_name)
+{
+       char tmp_name[NAME_MAX];
+       int shmfd, ret;
+
+       ret = snprintf(tmp_name, NAME_MAX, "/shm-%s-%d", owner_name, getpid());
+       if (ret < 0) {
+               PERROR("snprintf");
+               return -1;
+       }
+       /*
+        * Allocate shm, and immediately unlink its shm oject, keeping only the
+        * file descriptor as a reference to the object.
+        */
+       shmfd = shm_open(tmp_name, O_CREAT | O_EXCL | O_RDWR, 0700);
+       if (shmfd < 0) {
+               PERROR("shm_open");
+               goto error_shm_open;
+       }
+       ret = shm_unlink(tmp_name);
+       if (ret < 0 && errno != ENOENT) {
+               PERROR("shm_unlink");
+               goto error_shm_release;
+       }
+       return shmfd;
+
+error_shm_release:
+       ret = close(shmfd);
+       if (ret) {
+               PERROR("close");
+       }
+error_shm_open:
+       return -1;
+}
diff --git a/src/common/shm.h b/src/common/shm.h
new file mode 100644 (file)
index 0000000..d714506
--- /dev/null
@@ -0,0 +1,16 @@
+/*
+ * Copyright (C) 2011 David Goulet <david.goulet@polymtl.ca>
+ * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-only
+ *
+ */
+
+#ifndef _LTT_SHM_H
+#define _LTT_SHM_H
+
+char *shm_ust_get_mmap(char *shm_path, int global);
+
+int shm_create_anonymous(const char *owner_name);
+
+#endif /* _LTT_SHM_H */
index 68b6cac6c9e459a3b1aaed8434930e3e1287a62c..aede53658736e2037d8f90bb53b050c4bb2c2997 100644 (file)
@@ -37,6 +37,7 @@
 #include <common/utils.h>
 #include <common/index/index.h>
 #include <common/consumer/consumer.h>
+#include <common/shm.h>
 #include <common/optional.h>
 
 #include "ust-consumer.h"
@@ -352,48 +353,6 @@ error_alloc:
        return ret;
 }
 
-/*
- * create_posix_shm is never called concurrently within a process.
- */
-static
-int create_posix_shm(void)
-{
-       char tmp_name[NAME_MAX];
-       int shmfd, ret;
-
-       ret = snprintf(tmp_name, NAME_MAX, "/ust-shm-consumer-%d", getpid());
-       if (ret < 0) {
-               PERROR("snprintf");
-               return -1;
-       }
-       /*
-        * Allocate shm, and immediately unlink its shm oject, keeping
-        * only the file descriptor as a reference to the object.
-        * We specifically do _not_ use the / at the beginning of the
-        * pathname so that some OS implementations can keep it local to
-        * the process (POSIX leaves this implementation-defined).
-        */
-       shmfd = shm_open(tmp_name, O_CREAT | O_EXCL | O_RDWR, 0700);
-       if (shmfd < 0) {
-               PERROR("shm_open");
-               goto error_shm_open;
-       }
-       ret = shm_unlink(tmp_name);
-       if (ret < 0 && errno != ENOENT) {
-               PERROR("shm_unlink");
-               goto error_shm_release;
-       }
-       return shmfd;
-
-error_shm_release:
-       ret = close(shmfd);
-       if (ret) {
-               PERROR("close");
-       }
-error_shm_open:
-       return -1;
-}
-
 static int open_ust_stream_fd(struct lttng_consumer_channel *channel, int cpu,
                const struct lttng_credentials *session_credentials)
 {
@@ -401,7 +360,7 @@ static int open_ust_stream_fd(struct lttng_consumer_channel *channel, int cpu,
        int ret;
 
        if (!channel->shm_path[0]) {
-               return create_posix_shm();
+               return shm_create_anonymous("ust-consumer");
        }
        ret = get_stream_shm_path(shm_path, channel->shm_path, cpu);
        if (ret) {
index e9142b3376b282199b7b0a62aabab56ce9cab905..01e7e82c0fbed2cee4cdcf4e38e6d103ba7a17e4 100644 (file)
@@ -82,7 +82,6 @@ SESSIOND_OBJS = $(top_builddir)/src/bin/lttng-sessiond/buffer-registry.$(OBJEXT)
         $(top_builddir)/src/bin/lttng-sessiond/condition-internal.$(OBJEXT) \
         $(top_builddir)/src/bin/lttng-sessiond/save.$(OBJEXT) \
         $(top_builddir)/src/bin/lttng-sessiond/notification-thread-commands.$(OBJEXT) \
-        $(top_builddir)/src/bin/lttng-sessiond/shm.$(OBJEXT) \
         $(top_builddir)/src/bin/lttng-sessiond/kernel.$(OBJEXT) \
         $(top_builddir)/src/bin/lttng-sessiond/ht-cleanup.$(OBJEXT) \
         $(top_builddir)/src/bin/lttng-sessiond/notification-thread.$(OBJEXT) \
This page took 0.033743 seconds and 4 git commands to generate.