On Thu, May 07, 2020 at 05:03:17PM -0600, Jens Axboe wrote:
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.
Actually, what _is_ the reason for that check? Note, BTW, that if the file in question happens to be an AF_UNIX socket, closing it will close all references held in SCM_RIGHTS datagrams sitting in its queue, which might very well include io_uring files.
IOW, if tries to avoid something really unpleasant, it's not enough. And if it doesn't, then what is it for?