The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxcfs/pull/391
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === So we don't keep holding it throughout the expensive the fork/clone/send cycle of figuring out a namespace's init pid. Also change some functions to pass the init namespace inode instead of the process' struct stat, so that it is obvious that we don't use other parts of the stat info. The reason is that we previously acquired the store lock before doing the call to `stat()` and therefore it may not be immediately obvious that things such as the timestamps found inside the `struct stat` are not contributing to any race between the multiple acquisitions of the store lock. Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com>
From fcdedd16a5b6e769c9172b1fe8e1268ce04a1ddf Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <w.bumil...@proxmox.com> Date: Wed, 15 Apr 2020 10:41:01 +0200 Subject: [PATCH] relax init pid store locking a bit So we don't keep holding it throughout the expensive the fork/clone/send cycle of figuring out a namespace's init pid. Also change some functions to pass the init namespace inode instead of the process' struct stat, so that it is obvious that we don't use other parts of the stat info. The reason is that we previously acquired the store lock before doing the call to `stat()` and therefore it may not be immediately obvious that things such as the timestamps found inside the `struct stat` are not contributing to any race between the multiple acquisitions of the store lock. Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com> --- src/bindings.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/bindings.c b/src/bindings.c index 6c3c818..81bebdc 100644 --- a/src/bindings.c +++ b/src/bindings.c @@ -118,14 +118,17 @@ static void unlock_mutex(pthread_mutex_t *l) log_exit("%s - returned %d\n", strerror(ret), ret); } -static void store_lock(void) +static inline void unlock_mutex_function(pthread_mutex_t **mutex) { - lock_mutex(&pidns_store_mutex); + if (*mutex) + unlock_mutex(*mutex); } +#define __do_unlock call_cleaner(unlock_mutex) -static void store_unlock(void) +static pthread_mutex_t* __attribute__((warn_unused_result)) store_lock(void) { - unlock_mutex(&pidns_store_mutex); + lock_mutex(&pidns_store_mutex); + return &pidns_store_mutex; } /* /proc/ = 6 @@ -252,7 +255,7 @@ static void prune_initpid_store(void) } /* Must be called under store_lock */ -static void save_initpid(struct stat *sb, pid_t pid) +static void save_initpid(ino_t pidns_inode, pid_t pid) { __do_free struct pidns_init_store *entry = NULL; __do_close int pidfd = -EBADF; @@ -277,7 +280,7 @@ static void save_initpid(struct stat *sb, pid_t pid) ino_hash = HASH(entry->ino); *entry = (struct pidns_init_store){ - .ino = sb->st_ino, + .ino = pidns_inode, .initpid = pid, .ctime = st.st_ctime, .next = pidns_hash_table[ino_hash], @@ -296,12 +299,12 @@ static void save_initpid(struct stat *sb, pid_t pid) * otherwise. * Must be called under store_lock */ -static struct pidns_init_store *lookup_verify_initpid(struct stat *sb) +static struct pidns_init_store *lookup_verify_initpid(ino_t pidns_inode) { - struct pidns_init_store *entry = pidns_hash_table[HASH(sb->st_ino)]; + struct pidns_init_store *entry = pidns_hash_table[HASH(pidns_inode)]; while (entry) { - if (entry->ino == sb->st_ino) { + if (entry->ino == pidns_inode) { if (initpid_still_valid(entry)) { entry->lastcheck = time(NULL); return entry; @@ -428,6 +431,7 @@ static pid_t get_init_pid_for_task(pid_t task) pid_t lookup_initpid_in_store(pid_t pid) { + __do_unlock pthread_mutex_t *store_mutex = NULL; pid_t answer = 0; char path[LXCFS_PROC_PID_NS_LEN]; struct stat st; @@ -435,19 +439,24 @@ pid_t lookup_initpid_in_store(pid_t pid) snprintf(path, sizeof(path), "/proc/%d/ns/pid", pid); - store_lock(); if (stat(path, &st)) goto out; - entry = lookup_verify_initpid(&st); + store_mutex = store_lock(); + + entry = lookup_verify_initpid(st.st_ino); if (entry) { answer = entry->initpid; goto out; } + /* release the mutex as the following call is expensive */ + unlock_mutex(move_ptr(store_mutex)); answer = get_init_pid_for_task(pid); + store_mutex = store_lock(); + if (answer > 0) - save_initpid(&st, answer); + save_initpid(st.st_ino, answer); out: /* @@ -456,8 +465,6 @@ pid_t lookup_initpid_in_store(pid_t pid) */ prune_initpid_store(); - store_unlock(); - return answer; }
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel