All of lore.kernel.org
 help / color / mirror / Atom feed
From: Josh Kunz <jkz@google.com>
To: qemu-devel@nongnu.org
Cc: riku.voipio@iki.fi, laurent@vivier.eu, alex.bennee@linaro.org,
	 Josh Kunz <jkz@google.com>
Subject: [PATCH 2/5] linux-user: Make fd_trans task-specific.
Date: Thu, 11 Jun 2020 18:46:03 -0700	[thread overview]
Message-ID: <20200612014606.147691-3-jkz@google.com> (raw)
In-Reply-To: <20200612014606.147691-1-jkz@google.com>

The file-descriptor translation subsystem used by QEMU uses some global
variables to track file descriptors an their associated state. In the
future (when clone is implemented) it may be possible to have two
processes that share memory, but have a unique set of file descriptors.
This change associates the file-descriptor translation table with the
per-task TaskState structure. Since many tasks will share file
descriptors (e.g., threads), a structure similar to the existing structure is
used. Each task has a pointer to a global table. That table can be
shared by multiple tasks, or changed if a task needs to use a different
FD table.

Signed-off-by: Josh Kunz <jkz@google.com>
---
 linux-user/Makefile.objs   |  2 +-
 linux-user/fd-trans-tbl.c  | 13 +++++++
 linux-user/fd-trans-type.h | 17 +++++++++
 linux-user/fd-trans.c      |  3 --
 linux-user/fd-trans.h      | 75 ++++++++++++++++++++++++--------------
 linux-user/main.c          |  1 +
 linux-user/qemu.h          | 24 ++++++++++++
 linux-user/syscall.c       | 12 ++++++
 8 files changed, 115 insertions(+), 32 deletions(-)
 create mode 100644 linux-user/fd-trans-tbl.c
 create mode 100644 linux-user/fd-trans-type.h

diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
index d6788f012c..d19102e244 100644
--- a/linux-user/Makefile.objs
+++ b/linux-user/Makefile.objs
@@ -1,7 +1,7 @@
 obj-y = main.o syscall.o strace.o mmap.o signal.o \
 	elfload.o linuxload.o uaccess.o uname.o \
 	safe-syscall.o $(TARGET_ABI_DIR)/signal.o \
-        $(TARGET_ABI_DIR)/cpu_loop.o exit.o fd-trans.o clone.o
+        $(TARGET_ABI_DIR)/cpu_loop.o exit.o fd-trans.o clone.o fd-trans-tbl.o
 
 obj-$(TARGET_HAS_BFLT) += flatload.o
 obj-$(TARGET_I386) += vm86.o
diff --git a/linux-user/fd-trans-tbl.c b/linux-user/fd-trans-tbl.c
new file mode 100644
index 0000000000..6afe91096e
--- /dev/null
+++ b/linux-user/fd-trans-tbl.c
@@ -0,0 +1,13 @@
+#include "qemu/osdep.h"
+#include "fd-trans.h"
+
+struct fd_trans_table *fd_trans_table_clone(struct fd_trans_table *tbl)
+{
+    struct fd_trans_table *new_tbl = g_new0(struct fd_trans_table, 1);
+    new_tbl->fd_max = tbl->fd_max;
+    new_tbl->entries = g_new0(TargetFdTrans*, tbl->fd_max);
+    memcpy(new_tbl->entries,
+           tbl->entries,
+           sizeof(*new_tbl->entries) * tbl->fd_max);
+    return new_tbl;
+}
diff --git a/linux-user/fd-trans-type.h b/linux-user/fd-trans-type.h
new file mode 100644
index 0000000000..06c4427642
--- /dev/null
+++ b/linux-user/fd-trans-type.h
@@ -0,0 +1,17 @@
+#ifndef FD_TRANS_TYPE_H
+#define FD_TRANS_TYPE_H
+
+/*
+ * Break out the TargetFdTrans typedefs into a separate file, to break
+ * the circular dependency between qemu.h and fd-trans.h.
+ */
+
+typedef abi_long (*TargetFdDataFunc)(void *, size_t);
+typedef abi_long (*TargetFdAddrFunc)(void *, abi_ulong, socklen_t);
+typedef struct TargetFdTrans {
+    TargetFdDataFunc host_to_target_data;
+    TargetFdDataFunc target_to_host_data;
+    TargetFdAddrFunc target_to_host_addr;
+} TargetFdTrans;
+
+#endif /* FD_TRANS_TYPE_H */
diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index c0687c52e6..c552034a5e 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -261,9 +261,6 @@ enum {
     QEMU___RTA_MAX
 };
 
-TargetFdTrans **target_fd_trans;
-unsigned int target_fd_max;
-
 static void tswap_nlmsghdr(struct nlmsghdr *nlh)
 {
     nlh->nlmsg_len = tswap32(nlh->nlmsg_len);
diff --git a/linux-user/fd-trans.h b/linux-user/fd-trans.h
index a3fcdaabc7..07ae04dad7 100644
--- a/linux-user/fd-trans.h
+++ b/linux-user/fd-trans.h
@@ -16,38 +16,45 @@
 #ifndef FD_TRANS_H
 #define FD_TRANS_H
 
-typedef abi_long (*TargetFdDataFunc)(void *, size_t);
-typedef abi_long (*TargetFdAddrFunc)(void *, abi_ulong, socklen_t);
-typedef struct TargetFdTrans {
-    TargetFdDataFunc host_to_target_data;
-    TargetFdDataFunc target_to_host_data;
-    TargetFdAddrFunc target_to_host_addr;
-} TargetFdTrans;
+#include "qemu.h"
+#include "fd-trans-type.h"
 
-extern TargetFdTrans **target_fd_trans;
-
-extern unsigned int target_fd_max;
+/*
+ * Return a duplicate of the given fd_trans_table. This function always
+ * succeeds. Ownership of the pointed-to table is yielded to the caller. The
+ * caller is responsible for freeing the table when it is no longer in-use.
+ */
+struct fd_trans_table *fd_trans_table_clone(struct fd_trans_table *tbl);
 
 static inline TargetFdDataFunc fd_trans_target_to_host_data(int fd)
 {
-    if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
-        return target_fd_trans[fd]->target_to_host_data;
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
+    if (fd >= 0 && fd < tbl->fd_max && tbl->entries[fd]) {
+        return tbl->entries[fd]->target_to_host_data;
     }
     return NULL;
 }
 
 static inline TargetFdDataFunc fd_trans_host_to_target_data(int fd)
 {
-    if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
-        return target_fd_trans[fd]->host_to_target_data;
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
+    if (fd >= 0 && fd < tbl->fd_max && tbl->entries[fd]) {
+        return tbl->entries[fd]->host_to_target_data;
     }
     return NULL;
 }
 
 static inline TargetFdAddrFunc fd_trans_target_to_host_addr(int fd)
 {
-    if (fd >= 0 && fd < target_fd_max && target_fd_trans[fd]) {
-        return target_fd_trans[fd]->target_to_host_addr;
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
+    if (fd >= 0 && fd < tbl->fd_max && tbl->entries[fd]) {
+        return tbl->entries[fd]->target_to_host_addr;
     }
     return NULL;
 }
@@ -56,29 +63,41 @@ static inline void fd_trans_register(int fd, TargetFdTrans *trans)
 {
     unsigned int oldmax;
 
-    if (fd >= target_fd_max) {
-        oldmax = target_fd_max;
-        target_fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
-        target_fd_trans = g_renew(TargetFdTrans *,
-                                  target_fd_trans, target_fd_max);
-        memset((void *)(target_fd_trans + oldmax), 0,
-               (target_fd_max - oldmax) * sizeof(TargetFdTrans *));
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
+    /*
+     * TODO: This is racy. Updates to tbl->entries should be guarded by
+     * a lock.
+     */
+    if (fd >= tbl->fd_max) {
+        oldmax = tbl->fd_max;
+        tbl->fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
+        tbl->entries = g_renew(TargetFdTrans *, tbl->entries, tbl->fd_max);
+        memset((void *)(tbl->entries + oldmax), 0,
+               (tbl->fd_max - oldmax) * sizeof(TargetFdTrans *));
     }
-    target_fd_trans[fd] = trans;
+    tbl->entries[fd] = trans;
 }
 
 static inline void fd_trans_unregister(int fd)
 {
-    if (fd >= 0 && fd < target_fd_max) {
-        target_fd_trans[fd] = NULL;
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
+    if (fd >= 0 && fd < tbl->fd_max) {
+        tbl->entries[fd] = NULL;
     }
 }
 
 static inline void fd_trans_dup(int oldfd, int newfd)
 {
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    struct fd_trans_table *tbl = ts->fd_trans_tbl;
+
     fd_trans_unregister(newfd);
-    if (oldfd < target_fd_max && target_fd_trans[oldfd]) {
-        fd_trans_register(newfd, target_fd_trans[oldfd]);
+    if (oldfd >= 0 && oldfd < tbl->fd_max && tbl->entries[oldfd]) {
+        fd_trans_register(newfd, tbl->entries[oldfd]);
     }
 }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index 3597e99bb1..d1ed0f6120 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -796,6 +796,7 @@ int main(int argc, char **argv, char **envp)
     ts->bprm = &bprm;
     cpu->opaque = ts;
     task_settid(ts);
+    ts->fd_trans_tbl = g_new0(struct fd_trans_table, 1);
 
     ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
         info, &bprm);
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index ce902f5132..989e01ad8d 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -5,6 +5,7 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "fd-trans-type.h"
 
 #undef DEBUG_REMAP
 #ifdef DEBUG_REMAP
@@ -96,6 +97,22 @@ struct emulated_sigtable {
     target_siginfo_t info;
 };
 
+/*
+ * The fd_trans_table is used the FD data translation subsystem to
+ * find FD data translators (i.e. functions). `entries` is an array of pointers
+ * with size `fd_max`, containing pointers to TargetFDTrans structs. A pointer
+ * to a struct of this type is stored TaskState, which allows the struct itself
+ * to be shared by all tasks (e.g., threads) that share a file descriptor
+ * namespace. Storing a pointer to this table in the TaskState struct is needed
+ * to support rare cases where tasks share an address space, but do not share
+ * a set of file descriptors (e.g., after clone(CLONE_VM) when CLONE_FILES is
+ * not set). See `fd-trans.h` for more info on the FD translation subsystem.
+ */
+struct fd_trans_table {
+    uint64_t fd_max;
+    TargetFdTrans **entries;
+};
+
 /* NOTE: we force a big alignment so that the stack stored after is
    aligned too */
 typedef struct TaskState {
@@ -153,6 +170,13 @@ typedef struct TaskState {
 
     /* This thread's sigaltstack, if it has one */
     struct target_sigaltstack sigaltstack_used;
+
+    /*
+     * A pointer to the FD trans table to be used by this task. Note that the
+     * task doesn't have exclusive control of the fd_trans_table so access to
+     * the table itself should be guarded.
+     */
+    struct fd_trans_table *fd_trans_tbl;
 } __attribute__((aligned(16))) TaskState;
 
 extern char *exec_path;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 7ce021cea2..ff1d07871f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6013,6 +6013,18 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
     ts->info = parent_ts->info;
     ts->signal_mask = parent_ts->signal_mask;
 
+    if (flags & CLONE_FILES) {
+        ts->fd_trans_tbl = parent_ts->fd_trans_tbl;
+    } else {
+        /*
+         * When CLONE_FILES is not set, the parent and child will have
+         * different file descriptor tables, so we need a new
+         * fd_trans_tbl. Clone from parent_ts, since child inherits all our
+         * file descriptors.
+         */
+        ts->fd_trans_tbl = fd_trans_table_clone(parent_ts->fd_trans_tbl);
+    }
+
     if (flags & CLONE_CHILD_CLEARTID) {
         ts->child_tidptr = child_tidptr;
     }
-- 
2.27.0.290.gba653c62da-goog



  parent reply	other threads:[~2020-06-12  1:49 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-12  1:46 [PATCH 0/5] linux-user: Support extended clone(CLONE_VM) Josh Kunz
2020-06-12  1:46 ` [PATCH 1/5] linux-user: Refactor do_fork to use new `qemu_clone` Josh Kunz
2020-06-16 15:51   ` Alex Bennée
2020-06-12  1:46 ` Josh Kunz [this message]
2020-06-12  1:46 ` [PATCH 3/5] linux-user: Make sigact_table part of the task state Josh Kunz
2020-06-12  1:46 ` [PATCH 4/5] linux-user: Support CLONE_VM and extended clone options Josh Kunz
2020-06-13  0:10   ` Josh Kunz
2020-06-16 16:08   ` Alex Bennée
2020-06-23  3:43     ` Josh Kunz
2020-06-23  8:21       ` Alex Bennée
     [not found]         ` <CADgy-2tB0Z133RB1i8OdnpKMD3xATL059dFoduHOjdim11G4-A@mail.gmail.com>
     [not found]           ` <87k0zw7opa.fsf@linaro.org>
2020-07-09  0:16             ` Josh Kunz
2020-07-16 10:41               ` Alex Bennée
2020-06-12  1:46 ` [PATCH 5/5] linux-user: Add PDEATHSIG test for clone process hierarchy Josh Kunz
2020-06-12  3:48 ` [PATCH 0/5] linux-user: Support extended clone(CLONE_VM) no-reply
2020-06-13 11:16 ` Alex Bennée
2020-06-16 16:02 ` Alex Bennée
2020-06-16 16:32   ` Peter Maydell
2020-06-16 23:38     ` Josh Kunz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200612014606.147691-3-jkz@google.com \
    --to=jkz@google.com \
    --cc=alex.bennee@linaro.org \
    --cc=laurent@vivier.eu \
    --cc=qemu-devel@nongnu.org \
    --cc=riku.voipio@iki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.