linux-kernel - Re: [GIT PULL] io_uring fixes for 5.10-rc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d7095e1d-0363-0aad-5c13-6d9bb189b2c8@kernel.dk>
Date:   Fri, 20 Nov 2020 20:00:32 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>
Cc:     io-uring <io-uring@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] io_uring fixes for 5.10-rc

On 11/20/20 7:41 PM, Jens Axboe wrote:
> On 11/20/20 5:23 PM, Linus Torvalds wrote:
>> On Fri, Nov 20, 2020 at 1:36 PM Jens Axboe <axboe@...nel.dk> wrote:
>>>
>>> I don't disagree with you on that. I've been a bit gun shy on touching
>>> the VFS side of things, but this one isn't too bad. I hacked up a patch
>>> that allows io_uring to do LOOKUP_RCU and a quick test seems to indicate
>>> it's fine. On top of that, we just propagate the error if we do fail and
>>> get rid of that odd retry loop.
>>
>> Ok, this looks better to me (but is obviously not 5.10 material).
>>
>> That said, I think I'd prefer to keep 'struct nameidata' internal to
>> just fs/namei.c, and maybe we can just expert that
>>
>>         struct nameidata nd;
>>
>>         set_nameidata(&nd, req->open.dfd, req->open.filename);
>>         file = path_openat(&nd, &op, op.lookup_flags | LOOKUP_RCU);
>>         restore_nameidata();
>>         return filp == ERR_PTR(-ECHILD) ? -EAGAIN : filp;
>>
>> as a helper from namei.c instead? Call it "do_filp_open_rcu()" or something?
> 
> Yes, that's probably a better idea. I'll move in that direction.

Actually, I think we can do even better. How about just having
do_filp_open() exit after LOOKUP_RCU fails, if LOOKUP_RCU was already
set in the lookup flags? Then we don't need to change much else, and
most of it falls out naturally.

Except it seems that should work, except LOOKUP_RCU does not guarantee
that we're not going to do IO:

[   20.463195]  schedule+0x5f/0xd0
[   20.463444]  io_schedule+0x45/0x70
[   20.463712]  bit_wait_io+0x11/0x50
[   20.463981]  __wait_on_bit+0x2c/0x90
[   20.464264]  out_of_line_wait_on_bit+0x86/0x90
[   20.464611]  ? var_wake_function+0x30/0x30
[   20.464932]  __ext4_find_entry+0x2b5/0x410
[   20.465254]  ? d_alloc_parallel+0x241/0x4e0
[   20.465581]  ext4_lookup+0x51/0x1b0
[   20.465855]  ? __d_lookup+0x77/0x120
[   20.466136]  path_openat+0x4e8/0xe40
[   20.466417]  do_filp_open+0x79/0x100
[   20.466720]  ? __kernel_text_address+0x30/0x70
[   20.467068]  ? __alloc_fd+0xb3/0x150
[   20.467349]  io_openat2+0x65/0x210
[   20.467618]  io_issue_sqe+0x3e/0xf70

Which I'm actually pretty sure that I discovered before and attempted to
do a LOOKUP_NONBLOCK, which was kind of half assed and that Al
(rightfully) hated because of that.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 43ba815e4107..9a0a21ac5227 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4069,36 +4069,25 @@ static int io_openat2(struct io_kiocb *req, bool force_nonblock)
 	struct file *file;
 	int ret;
 
-	if (force_nonblock && !req->open.ignore_nonblock)
-		return -EAGAIN;
-
 	ret = build_open_flags(&req->open.how, &op);
 	if (ret)
 		goto err;
+	if (force_nonblock)
+		op.lookup_flags |= LOOKUP_RCU;
 
 	ret = __get_unused_fd_flags(req->open.how.flags, req->open.nofile);
 	if (ret < 0)
 		goto err;
 
 	file = do_filp_open(req->open.dfd, req->open.filename, &op);
+	if (force_nonblock && file == ERR_PTR(-ECHILD)) {
+		put_unused_fd(ret);
+		return -EAGAIN;
+	}
+
 	if (IS_ERR(file)) {
 		put_unused_fd(ret);
 		ret = PTR_ERR(file);
-		/*
-		 * A work-around to ensure that /proc/self works that way
-		 * that it should - if we get -EOPNOTSUPP back, then assume
-		 * that proc_self_get_link() failed us because we're in async
-		 * context. We should be safe to retry this from the task
-		 * itself with force_nonblock == false set, as it should not
-		 * block on lookup. Would be nice to know this upfront and
-		 * avoid the async dance, but doesn't seem feasible.
-		 */
-		if (ret == -EOPNOTSUPP && io_wq_current_is_worker()) {
-			req->open.ignore_nonblock = true;
-			refcount_inc(&req->refs);
-			io_req_task_queue(req);
-			return 0;
-		}
 	} else {
 		fsnotify_open(file);
 		fd_install(ret, file);
diff --git a/fs/namei.c b/fs/namei.c
index 03d0e11e4f36..eb2c917986a5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3394,8 +3394,11 @@ struct file *do_filp_open(int dfd, struct filename *pathname,
 
 	set_nameidata(&nd, dfd, pathname);
 	filp = path_openat(&nd, op, flags | LOOKUP_RCU);
-	if (unlikely(filp == ERR_PTR(-ECHILD)))
+	if (unlikely(filp == ERR_PTR(-ECHILD))) {
+		if (flags & LOOKUP_RCU)
+			return filp;
 		filp = path_openat(&nd, op, flags);
+	}
 	if (unlikely(filp == ERR_PTR(-ESTALE)))
 		filp = path_openat(&nd, op, flags | LOOKUP_REVAL);
 	restore_nameidata();

-- 
Jens Axboe