diff mbox series

vfs: move fdput() to right place in ksys_sync_file_range()

Message ID 20220511154503.28365-1-cgxu519@mykernel.net (mailing list archive)
State New
Headers show
Series vfs: move fdput() to right place in ksys_sync_file_range() | expand

Commit Message

Chengguang Xu May 11, 2022, 3:45 p.m. UTC
Move fdput() to right place in ksys_sync_file_range() to
avoid fdput() after failed fdget().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/sync.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox May 11, 2022, 3:51 p.m. UTC | #1
On Wed, May 11, 2022 at 11:45:03AM -0400, Chengguang Xu wrote:
> Move fdput() to right place in ksys_sync_file_range() to
> avoid fdput() after failed fdget().

Why?  fdput() is already conditional on FDPUT_FPUT so you're ...
optimising the failure case?
Eric Biggers May 11, 2022, 7:01 p.m. UTC | #2
On Wed, May 11, 2022 at 04:51:34PM +0100, Matthew Wilcox wrote:
> On Wed, May 11, 2022 at 11:45:03AM -0400, Chengguang Xu wrote:
> > Move fdput() to right place in ksys_sync_file_range() to
> > avoid fdput() after failed fdget().
> 
> Why?  fdput() is already conditional on FDPUT_FPUT so you're ...
> optimising the failure case?

"fdput() after failed fdget()" has confused people before, so IMO it's worth
cleaning this up.  But the commit message should make clear that it's a cleanup,
not a bug fix.  Also I recommend using an early return:

	f = fdget(fd);
	if (!f.file)
		return -EBADF;
	ret = sync_file_range(f.file, offset, nbytes, flags);
	fdput(f);
	return ret;
Al Viro May 11, 2022, 9:43 p.m. UTC | #3
[bpf folks Cc'd]

On Wed, May 11, 2022 at 07:01:34PM +0000, Eric Biggers wrote:
> On Wed, May 11, 2022 at 04:51:34PM +0100, Matthew Wilcox wrote:
> > On Wed, May 11, 2022 at 11:45:03AM -0400, Chengguang Xu wrote:
> > > Move fdput() to right place in ksys_sync_file_range() to
> > > avoid fdput() after failed fdget().
> > 
> > Why?  fdput() is already conditional on FDPUT_FPUT so you're ...
> > optimising the failure case?
> 
> "fdput() after failed fdget()" has confused people before, so IMO it's worth
> cleaning this up.  But the commit message should make clear that it's a cleanup,
> not a bug fix.  Also I recommend using an early return:
> 
> 	f = fdget(fd);
> 	if (!f.file)
> 		return -EBADF;
> 	ret = sync_file_range(f.file, offset, nbytes, flags);
> 	fdput(f);
> 	return ret;

FWIW, fdput() after failed fdget() is rare, but there's no fundamental reasons
why it would be wrong.  No objections against that patch, anyway.

Out of curiousity, I've just looked at the existing users.  In mainline we have
203 callers of fdput()/fdput_pos(); all but 7 never get reached with NULL ->file.

1) There's ksys_sync_file_range(), kernel_read_file_from_fd() and ksys_readahead() -
all with similar pattern.  I'm not sure that for readahead(2) "not opened for
read" should yield the same error as "bad descriptor", but since it's been a part
of userland ABI for a while...

2) two callers in perf_event_open(2) are playing silly buggers with explicit
        struct fd group = {NULL, 0};
and rely upon "fdput() is a no-op if we hadn't touched that" (note that if
we try to touch it and get NULL ->file from fdget(), we do not hit those fdput()
at all).

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.

4) bpf generic_map_update_batch() is really asking for trouble.  The comment in
there is wrong:
        f = fdget(ufd); /* bpf_map_do_batch() guarantees ufd is valid */
*NOTHING* we'd done earlier can guarantee that.  We might have a descriptor
table shared with another thread, and it might have very well done dup2() since
the last time we'd looked things up.  IOW, this fdget() is racy - the function
assumes it refers to the same thing that gave us map back in bpf_map_do_batch(),
but it's not guaranteed at all.

I hadn't put together a reproducer, but that code is very suspicious.  As a general
rule, you should treat descriptor table as shared object, modifiable by other
threads.  It can be explicitly locked and it can be explicitly unshared, but
short of that doing a lookup for the same descriptor twice in a row can yield
different results.

What's going on there?  Do you really want the same struct file you've got back in
bpf_map_do_batch() (i.e. the one you've got the map from)?  What should happen
if the descriptor changes its meaning during (or after) the operation?
Al Viro May 12, 2022, 12:28 a.m. UTC | #4
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...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 91de361ea9ab..b61ae18ef10a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -10760,14 +10760,14 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 			return -EINVAL;
 		fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
 		f.file = tctx->registered_rings[fd];
-		if (unlikely(!f.file))
-			return -EBADF;
+		f.flags = 0;
 	} else {
 		f = fdget(fd);
-		if (unlikely(!f.file))
-			return -EBADF;
 	}
 
+	if (unlikely(!f.file))
+		return -EBADF;
+
 	ret = -EOPNOTSUPP;
 	if (unlikely(f.file->f_op != &io_uring_fops))
 		goto out_fput;
@@ -10840,8 +10840,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 out:
 	percpu_ref_put(&ctx->refs);
 out_fput:
-	if (!(flags & IORING_ENTER_REGISTERED_RING))
-		fdput(f);
+	fdput(f);
 	return submitted ? submitted : ret;
 }
Jens Axboe May 12, 2022, 12:42 a.m. UTC | #5
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).
Alexei Starovoitov May 12, 2022, 2:03 a.m. UTC | #6
On Wed, May 11, 2022 at 2:43 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> [bpf folks Cc'd]
>
> On Wed, May 11, 2022 at 07:01:34PM +0000, Eric Biggers wrote:
> > On Wed, May 11, 2022 at 04:51:34PM +0100, Matthew Wilcox wrote:
> > > On Wed, May 11, 2022 at 11:45:03AM -0400, Chengguang Xu wrote:
> > > > Move fdput() to right place in ksys_sync_file_range() to
> > > > avoid fdput() after failed fdget().
> > >
> > > Why?  fdput() is already conditional on FDPUT_FPUT so you're ...
> > > optimising the failure case?
> >
> > "fdput() after failed fdget()" has confused people before, so IMO it's worth
> > cleaning this up.  But the commit message should make clear that it's a cleanup,
> > not a bug fix.  Also I recommend using an early return:
> >
> >       f = fdget(fd);
> >       if (!f.file)
> >               return -EBADF;
> >       ret = sync_file_range(f.file, offset, nbytes, flags);
> >       fdput(f);
> >       return ret;
>
> FWIW, fdput() after failed fdget() is rare, but there's no fundamental reasons
> why it would be wrong.  No objections against that patch, anyway.
>
> Out of curiousity, I've just looked at the existing users.  In mainline we have
> 203 callers of fdput()/fdput_pos(); all but 7 never get reached with NULL ->file.
>
> 1) There's ksys_sync_file_range(), kernel_read_file_from_fd() and ksys_readahead() -
> all with similar pattern.  I'm not sure that for readahead(2) "not opened for
> read" should yield the same error as "bad descriptor", but since it's been a part
> of userland ABI for a while...
>
> 2) two callers in perf_event_open(2) are playing silly buggers with explicit
>         struct fd group = {NULL, 0};
> and rely upon "fdput() is a no-op if we hadn't touched that" (note that if
> we try to touch it and get NULL ->file from fdget(), we do not hit those fdput()
> at all).
>
> 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.
>
> 4) bpf generic_map_update_batch() is really asking for trouble.  The comment in
> there is wrong:
>         f = fdget(ufd); /* bpf_map_do_batch() guarantees ufd is valid */
> *NOTHING* we'd done earlier can guarantee that.  We might have a descriptor
> table shared with another thread, and it might have very well done dup2() since
> the last time we'd looked things up.  IOW, this fdget() is racy - the function
> assumes it refers to the same thing that gave us map back in bpf_map_do_batch(),
> but it's not guaranteed at all.
>
> I hadn't put together a reproducer, but that code is very suspicious.  As a general
> rule, you should treat descriptor table as shared object, modifiable by other
> threads.  It can be explicitly locked and it can be explicitly unshared, but
> short of that doing a lookup for the same descriptor twice in a row can yield
> different results.
>
> What's going on there?  Do you really want the same struct file you've got back in
> bpf_map_do_batch() (i.e. the one you've got the map from)?  What should happen
> if the descriptor changes its meaning during (or after) the operation?

Interesting.
If I got this right... in the following:

f = fdget(ufd);
map = __bpf_map_get(f);
if (IS_ERR(map))
   return PTR_ERR(map);
...
f = fdget(ufd);
here there are no guarantees that 'f' is valid and points
to the same map.
Argh. In hindsight that makes sense.

generic_map_update_batch calls bpf_map_update_value.
That could use 'f' for prog_array, fd_array and hash_of_maps
types of maps.
The first two types don't' define .map_update_batch callback.
So BPF_DO_BATCH(map->ops->map_update_batch); will error out
with -ENOTSUPP since that callback is NULL for that map type.

The hash_of_maps does seem to support it, but
that's an odd one to use with batch access.

Anyhow we certainly need to clean this up.

Brian,
do you mind fixing it up, since you've added that
secondary fdget() in the first place?

Thanks!
Brian Vazquez May 12, 2022, 12:48 p.m. UTC | #7
Sure, let me take a look.

On Wed, May 11, 2022 at 7:04 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 11, 2022 at 2:43 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > [bpf folks Cc'd]
> >
> > On Wed, May 11, 2022 at 07:01:34PM +0000, Eric Biggers wrote:
> > > On Wed, May 11, 2022 at 04:51:34PM +0100, Matthew Wilcox wrote:
> > > > On Wed, May 11, 2022 at 11:45:03AM -0400, Chengguang Xu wrote:
> > > > > Move fdput() to right place in ksys_sync_file_range() to
> > > > > avoid fdput() after failed fdget().
> > > >
> > > > Why?  fdput() is already conditional on FDPUT_FPUT so you're ...
> > > > optimising the failure case?
> > >
> > > "fdput() after failed fdget()" has confused people before, so IMO it's worth
> > > cleaning this up.  But the commit message should make clear that it's a cleanup,
> > > not a bug fix.  Also I recommend using an early return:
> > >
> > >       f = fdget(fd);
> > >       if (!f.file)
> > >               return -EBADF;
> > >       ret = sync_file_range(f.file, offset, nbytes, flags);
> > >       fdput(f);
> > >       return ret;
> >
> > FWIW, fdput() after failed fdget() is rare, but there's no fundamental reasons
> > why it would be wrong.  No objections against that patch, anyway.
> >
> > Out of curiousity, I've just looked at the existing users.  In mainline we have
> > 203 callers of fdput()/fdput_pos(); all but 7 never get reached with NULL ->file.
> >
> > 1) There's ksys_sync_file_range(), kernel_read_file_from_fd() and ksys_readahead() -
> > all with similar pattern.  I'm not sure that for readahead(2) "not opened for
> > read" should yield the same error as "bad descriptor", but since it's been a part
> > of userland ABI for a while...
> >
> > 2) two callers in perf_event_open(2) are playing silly buggers with explicit
> >         struct fd group = {NULL, 0};
> > and rely upon "fdput() is a no-op if we hadn't touched that" (note that if
> > we try to touch it and get NULL ->file from fdget(), we do not hit those fdput()
> > at all).
> >
> > 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.
> >
> > 4) bpf generic_map_update_batch() is really asking for trouble.  The comment in
> > there is wrong:
> >         f = fdget(ufd); /* bpf_map_do_batch() guarantees ufd is valid */
> > *NOTHING* we'd done earlier can guarantee that.  We might have a descriptor
> > table shared with another thread, and it might have very well done dup2() since
> > the last time we'd looked things up.  IOW, this fdget() is racy - the function
> > assumes it refers to the same thing that gave us map back in bpf_map_do_batch(),
> > but it's not guaranteed at all.
> >
> > I hadn't put together a reproducer, but that code is very suspicious.  As a general
> > rule, you should treat descriptor table as shared object, modifiable by other
> > threads.  It can be explicitly locked and it can be explicitly unshared, but
> > short of that doing a lookup for the same descriptor twice in a row can yield
> > different results.
> >
> > What's going on there?  Do you really want the same struct file you've got back in
> > bpf_map_do_batch() (i.e. the one you've got the map from)?  What should happen
> > if the descriptor changes its meaning during (or after) the operation?
>
> Interesting.
> If I got this right... in the following:
>
> f = fdget(ufd);
> map = __bpf_map_get(f);
> if (IS_ERR(map))
>    return PTR_ERR(map);
> ...
> f = fdget(ufd);
> here there are no guarantees that 'f' is valid and points
> to the same map.
> Argh. In hindsight that makes sense.
>
> generic_map_update_batch calls bpf_map_update_value.
> That could use 'f' for prog_array, fd_array and hash_of_maps
> types of maps.
> The first two types don't' define .map_update_batch callback.
> So BPF_DO_BATCH(map->ops->map_update_batch); will error out
> with -ENOTSUPP since that callback is NULL for that map type.
>
> The hash_of_maps does seem to support it, but
> that's an odd one to use with batch access.
>
> Anyhow we certainly need to clean this up.
>
> Brian,
> do you mind fixing it up, since you've added that
> secondary fdget() in the first place?
>
> Thanks!
diff mbox series

Patch

diff --git a/fs/sync.c b/fs/sync.c
index c7690016453e..b217d908bee8 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -360,10 +360,10 @@  int ksys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
 
 	ret = -EBADF;
 	f = fdget(fd);
-	if (f.file)
+	if (f.file) {
 		ret = sync_file_range(f.file, offset, nbytes, flags);
-
-	fdput(f);
+		fdput(f);
+	}
 	return ret;
 }