On 2/13/21 3:08 PM, Eric W. Biederman wrote: > Jens Axboe <axboe@xxxxxxxxx> writes: > >> Hi, >> >> Wanted to throw out what the current state of this is, as we keep >> getting closer to something palatable. >> >> This time I've included the io_uring change too. I've tested this through >> both openat2, and through io_uring as well. >> >> I'm pretty happy with this at this point. The core change is very simple, >> and the users end up being trivial too. >> >> Also available here: >> >> https://git.kernel.dk/cgit/linux-block/log/?h=nonblock-path-lookup >> >> Changes since v2: >> >> - Simplify the LOOKUP_NONBLOCK to _just_ cover LOOKUP_RCU and >> lookup_fast(), as per Linus's suggestion. I verified fast path >> operations are indeed just that, and it avoids having to mess with >> the inode lock and mnt_want_write() completely. >> >> - Make O_TMPFILE return -EAGAIN for LOOKUP_NONBLOCK. >> >> - Improve/add a few comments. >> >> - Folded what was left of the open part of LOOKUP_NONBLOCK into the >> main patch. > > I have to ask. Are you doing work to verify that performing > path traversals and opening of files yields the same results > when passed to io_uring as it does when performed by the caller? > > Looking at v5.11-rc7 it appears I can arbitrarily access the > io-wq thread in proc by traversing "/proc/thread-self/". Yes that looks like a bug, it needs similar treatment to /proc/self: diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c index a553273fbd41..e8ca19371a36 100644 --- a/fs/proc/thread_self.c +++ b/fs/proc/thread_self.c @@ -17,6 +17,13 @@ static const char *proc_thread_self_get_link(struct dentry *dentry, pid_t pid = task_pid_nr_ns(current, ns); char *name; + /* + * Not currently supported. Once we can inherit all of struct pid, + * we can allow this. + */ + if (current->flags & PF_KTHREAD) + return ERR_PTR(-EOPNOTSUPP); + if (!pid) return ERR_PTR(-ENOENT); name = kmalloc(10 + 6 + 10 + 1, dentry ? GFP_KERNEL : GFP_ATOMIC); as was done in this commit: commit 8d4c3e76e3be11a64df95ddee52e99092d42fc19 Author: Jens Axboe <axboe@xxxxxxxxx> Date: Fri Nov 13 16:47:52 2020 -0700 proc: don't allow async path resolution of /proc/self components > Similarly it looks like opening of "/dev/tty" fails to > return the tty of the caller but instead fails because > io-wq threads don't have a tty. > > > There are a non-trivial number of places where current is used in the > kernel both in path traversal and in opening files, and I may be blind > but I have not see any work to find all of those places and make certain > they are safe when called from io_uring. That worries me as that using > a kernel thread instead of a user thread could potential lead to > privilege escalation. I've got a patch queued up for 5.12 that clears ->fs and ->files for the thread if not explicitly inherited, and I'm working on similarly proactively catching these cases that could potentially be problematic. -- Jens Axboe
