From: Francis Deslauriers Date: Thu, 8 Oct 2020 20:59:57 +0000 (-0400) Subject: Cleanup: Move `create_posix_shm()` to common/shm.c X-Git-Tag: v2.13.0-rc1~185 X-Git-Url: https://git.lttng.org/?p=lttng-tools.git;a=commitdiff_plain;h=b7fc068d873bcfc93761f418bfefe8c928c33a59;hp=246611b0dffa58fbc0e2329ddf6f9dc9d9eff7ce Cleanup: Move `create_posix_shm()` to common/shm.c * 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 Signed-off-by: Jérémie Galarneau Change-Id: I690b38801ffb4772e7c5069c4cccf470b0671f63 --- diff --git a/src/bin/lttng-sessiond/Makefile.am b/src/bin/lttng-sessiond/Makefile.am index 1d3741c61..e71c10814 100644 --- a/src/bin/lttng-sessiond/Makefile.am +++ b/src/bin/lttng-sessiond/Makefile.am @@ -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 \ diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index a344cd476..40a418d0c 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -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" diff --git a/src/bin/lttng-sessiond/register.c b/src/bin/lttng-sessiond/register.c index 6381c1179..ac1583dc2 100644 --- a/src/bin/lttng-sessiond/register.c +++ b/src/bin/lttng-sessiond/register.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -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 index 8b7744221..000000000 --- a/src/bin/lttng-sessiond/shm.c +++ /dev/null @@ -1,191 +0,0 @@ -/* - * Copyright (C) 2011 David Goulet - * Copyright (C) 2011 Mathieu Desnoyers - * - * SPDX-License-Identifier: GPL-2.0-only - * - */ - -#define _LGPL_SOURCE -#include -#include -#include -#include -#include -#include -#include -#include - -#include - -#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 index 94d2b72a1..000000000 --- a/src/bin/lttng-sessiond/shm.h +++ /dev/null @@ -1,14 +0,0 @@ -/* - * Copyright (C) 2011 David Goulet - * Copyright (C) 2011 Mathieu Desnoyers - * - * 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 */ diff --git a/src/common/Makefile.am b/src/common/Makefile.am index a56775e31..2dca5a4a3 100644 --- a/src/common/Makefile.am +++ b/src/common/Makefile.am @@ -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 index 000000000..737f73e08 --- /dev/null +++ b/src/common/shm.c @@ -0,0 +1,229 @@ +/* + * Copyright (C) 2011 David Goulet + * Copyright (C) 2011 Mathieu Desnoyers + * + * SPDX-License-Identifier: GPL-2.0-only + * + */ + +#define _LGPL_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#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 index 000000000..d714506b8 --- /dev/null +++ b/src/common/shm.h @@ -0,0 +1,16 @@ +/* + * Copyright (C) 2011 David Goulet + * Copyright (C) 2011 Mathieu Desnoyers + * + * 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 */ diff --git a/src/common/ust-consumer/ust-consumer.c b/src/common/ust-consumer/ust-consumer.c index 68b6cac6c..aede53658 100644 --- a/src/common/ust-consumer/ust-consumer.c +++ b/src/common/ust-consumer/ust-consumer.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #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) { diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index e9142b337..01e7e82c0 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -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) \