Re: [PATCH] vfs: move fdput() to right place in ksys_sync_file_range()

From: Jens Axboe <axboe@kernel.dk>
To: Al Viro <viro@zeniv.linux.org.uk>, Eric Biggers <ebiggers@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>,
	Chengguang Xu <cgxu519@mykernel.net>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] vfs: move fdput() to right place in ksys_sync_file_range()
Date: Wed, 11 May 2022 18:42:18 -0600	[thread overview]
Message-ID: <cdcd5596-9eaa-a7cb-eeaf-6269394627c9@kernel.dk> (raw)
In-Reply-To: <YnxUwQve8D39zxBz@zeniv-ca.linux.org.uk>

On 5/11/22 6:28 PM, Al Viro wrote:
> On Wed, May 11, 2022 at 09:43:46PM +0000, Al Viro wrote:
> 
>> 3) ovl_aio_put() is hard to follow (and some of the callers are poking
>> where they shouldn't), no idea if it's correct.  struct fd is manually
>> constructed there, anyway.
> 
> Speaking of poking in the internals:
> 
> SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>                 u32, min_complete, u32, flags, const void __user *, argp,
>                 size_t, argsz)
> {
> ...
>         struct fd f;
> ...
>         if (flags & IORING_ENTER_REGISTERED_RING) {
>                 struct io_uring_task *tctx = current->io_uring;
> 
>                 if (!tctx || fd >= IO_RINGFD_REG_MAX)
>                         return -EINVAL;
>                 fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
>                 f.file = tctx->registered_rings[fd];
>                 if (unlikely(!f.file))
>                         return -EBADF;
>         } else {
>                 f = fdget(fd);
>                 if (unlikely(!f.file))
>                         return -EBADF;
>         }
> ...
> a bunch of accesses to f.file
> ...
>         if (!(flags & IORING_ENTER_REGISTERED_RING))
>                 fdput(f);
> 
> Note that f.flags is left uninitialized in the first case; it doesn't
> break since we have fdput(f) (which does look at f.flags) done only
> in the case where we don't have IORING_ENTER_REGISTERED_RING in flags
> and since flags remains unchanged since the first if.  But it would
> be just as easy to set f.flags to 0 and use fdput() in both cases...
> 
> Jens, do you have any objections against the following?  Easier to
> follow that way...

No, I think that looks fine. If you need it for other changes:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

(or let me know if you want me to take it).

-- 
Jens Axboe