Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] fs/io_uring: fix O_PATH fds in openat, openat2, statx
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Max Kellermann <mk@cm4all.com>, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20200507185725.15840-1-mk@cm4all.com>
 <20200507190131.GF23230@ZenIV.linux.org.uk>
 <4cac0e53-656c-50f0-3766-ae3cc6c0310a@kernel.dk>
 <20200507192903.GG23230@ZenIV.linux.org.uk>
 <8e3c88cc-027b-4f90-b4f8-a20d11d35c4b@kernel.dk>
 <20200507220637.GH23230@ZenIV.linux.org.uk>
 <283c8edb-fea2-5192-f1d6-3cc57815b1e2@kernel.dk>
 <20200507224447.GI23230@ZenIV.linux.org.uk>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <e16125f2-c3ec-f029-c607-19bede54fa17@kernel.dk>
Date:   Thu, 7 May 2020 17:03:17 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200507224447.GI23230@ZenIV.linux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 5/7/20 4:44 PM, Al Viro wrote:
> On Thu, May 07, 2020 at 04:25:24PM -0600, Jens Axboe wrote:
> 
>>  static int io_close(struct io_kiocb *req, bool force_nonblock)
>>  {
>> +	struct files_struct *files = current->files;
>>  	int ret;
>>  
>>  	req->close.put_file = NULL;
>> -	ret = __close_fd_get_file(req->close.fd, &req->close.put_file);
>> +	spin_lock(&files->file_lock);
>> +	if (req->file->f_op == &io_uring_fops ||
>> +	    req->close.fd == req->ctx->ring_fd) {
>> +		spin_unlock(&files->file_lock);
>> +		return -EBADF;
>> +	}
>> +
>> +	ret = __close_fd_get_file_locked(files, req->close.fd,
>> +						&req->close.put_file);
> 
> Pointless.  By that point req->file might have nothing in common with
> anything in any descriptor table.

How about the below then? Stop using req->file, defer the lookup until
we're in the handler instead. Not sure the 'fd' check makes sense
at this point, but at least we should be consistent in terms of
once we lookup the file and check the f_op.

> Al, carefully _not_ saying anything about the taste and style of the
> entire thing...

It's just a quickie, didn't put much concern into the style and naming
of the locked helper. What do you prefer there? Normally I'd do __,
but it's already that, so... There's only one other user of it, so
we could just make the regular one be close_fd_get_file() and use
the __ prefix for the new locked variant.

But I figured it was more important to get the details right first,
the style is easier to polish.


diff --git a/fs/file.c b/fs/file.c
index c8a4e4c86e55..50ee73b76d17 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -646,18 +646,13 @@ int __close_fd(struct files_struct *files, unsigned fd)
 }
 EXPORT_SYMBOL(__close_fd); /* for ksys_close() */
 
-/*
- * variant of __close_fd that gets a ref on the file for later fput.
- * The caller must ensure that filp_close() called on the file, and then
- * an fput().
- */
-int __close_fd_get_file(unsigned int fd, struct file **res)
+int __close_fd_get_file_locked(struct files_struct *files, unsigned int fd,
+			       struct file **res)
+	__releases(&files->file_lock)
 {
-	struct files_struct *files = current->files;
 	struct file *file;
 	struct fdtable *fdt;
 
-	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
 	if (fd >= fdt->max_fds)
 		goto out_unlock;
@@ -677,6 +672,19 @@ int __close_fd_get_file(unsigned int fd, struct file **res)
 	return -ENOENT;
 }
 
+/*
+ * variant of __close_fd that gets a ref on the file for later fput.
+ * The caller must ensure that filp_close() called on the file, and then
+ * an fput().
+ */
+int __close_fd_get_file(unsigned int fd, struct file **res)
+{
+	struct files_struct *files = current->files;
+
+	spin_lock(&files->file_lock);
+	return __close_fd_get_file_locked(files, fd, res);
+}
+
 void do_close_on_exec(struct files_struct *files)
 {
 	unsigned i;
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 979d9f977409..54ef10240bf3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -786,7 +786,6 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_fs		= 1,
 	},
 	[IORING_OP_CLOSE] = {
-		.needs_file		= 1,
 		.file_table		= 1,
 	},
 	[IORING_OP_FILES_UPDATE] = {
@@ -3399,10 +3398,6 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
-	if (req->file->f_op == &io_uring_fops ||
-	    req->close.fd == req->ctx->ring_fd)
-		return -EBADF;
-
 	return 0;
 }
 
@@ -3430,10 +3425,21 @@ static void io_close_finish(struct io_wq_work **workptr)
 
 static int io_close(struct io_kiocb *req, bool force_nonblock)
 {
+	struct files_struct *files = current->files;
+	struct file *file;
 	int ret;
 
 	req->close.put_file = NULL;
-	ret = __close_fd_get_file(req->close.fd, &req->close.put_file);
+	spin_lock(&files->file_lock);
+	if (req->close.fd == req->ctx->ring_fd)
+		goto badf;
+
+	file = fcheck_files(files, req->close.fd);
+	if (!file || file->f_op == &io_uring_fops)
+		goto badf;
+
+	ret = __close_fd_get_file_locked(files, req->close.fd,
+						&req->close.put_file);
 	if (ret < 0)
 		return ret;
 
@@ -3458,6 +3464,9 @@ static int io_close(struct io_kiocb *req, bool force_nonblock)
 	 */
 	__io_close_finish(req);
 	return 0;
+badf:
+	spin_unlock(&files->file_lock);
+	return -EBADF;
 }
 
 static int io_prep_sfr(struct io_kiocb *req, const struct io_uring_sqe *sqe)
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index f07c55ea0c22..11d19303af46 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -122,6 +122,8 @@ extern void __fd_install(struct files_struct *files,
 extern int __close_fd(struct files_struct *files,
 		      unsigned int fd);
 extern int __close_fd_get_file(unsigned int fd, struct file **res);
+extern int __close_fd_get_file_locked(struct files_struct *files,
+				      unsigned int fd, struct file **res);
 
 extern struct kmem_cache *files_cachep;
 

-- 
Jens Axboe