| Message ID | Yn16M/fayt6tK/Gp@zeniv-ca.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] unify the file-closing stuff in fs/file.c | expand |
On 5/12/22 3:20 PM, Al Viro wrote: > Right now we have two places that do such removals - pick_file() > and {__,}close_fd_get_file(). > > They are almost identical - the only difference is in calling > conventions (well, and the fact that __... is called with descriptor > table locked). > > Calling conventions are... interesting. > > 1) pick_file() - returns file or ERR_PTR(-EBADF) or ERR_PTR(-EINVAL). > The latter is for "descriptor is greater than size of descriptor table". > One of the callers treats all ERR_PTR(...) as "return -EBADF"; another > uses ERR_PTR(-EINVAL) as "end the loop now" indicator. > > 2) {__,}close_fd_get_file() returns 0 or -ENOENT (huh?), with file (or NULL) > passed to caller by way of struct file ** argument. One of the callers > (binder) ignores the return value completely and checks if the file is NULL. > Another (io_uring) checks for return value being negative, then maps > -ENOENT to -EBADF, not that any other value would be possible. > > ERR_PTR(-EINVAL) magic in case of pick_file() is borderline defensible; > {__,}close_fd_get_file() conventions are insane. The older caller > (in binder) had never even looked at return value; the newer one > patches the bogus -ENOENT to what it wants to report, with strange > "defensive" BS logics just in case __close_fd_get_file() would somehow > find a different error to report. > > At the very least, {__,}close_fd_get_file() callers would've been happier > if it just returned file or NULL. What's more, I'm seriously tempted > to make pick_file() do the same thing. close_fd() won't care (checking > for NULL is just as easy as for IS_ERR) and __range_close() could just > as well cap the max_fd argument with last_fd(files_fdtable(current->files)). > > Does anybody see problems with the following? Looks good to me, and much better than passing in the pointer to the file pointer imho.
On Thu, May 12, 2022 at 4:48 PM Jens Axboe <axboe@kernel.dk> wrote: > > On 5/12/22 3:20 PM, Al Viro wrote: > > Right now we have two places that do such removals - pick_file() > > and {__,}close_fd_get_file(). > > > > They are almost identical - the only difference is in calling > > conventions (well, and the fact that __... is called with descriptor > > table locked). > > > > Calling conventions are... interesting. > > > > 1) pick_file() - returns file or ERR_PTR(-EBADF) or ERR_PTR(-EINVAL). > > The latter is for "descriptor is greater than size of descriptor table". > > One of the callers treats all ERR_PTR(...) as "return -EBADF"; another > > uses ERR_PTR(-EINVAL) as "end the loop now" indicator. > > > > 2) {__,}close_fd_get_file() returns 0 or -ENOENT (huh?), with file (or NULL) > > passed to caller by way of struct file ** argument. One of the callers > > (binder) ignores the return value completely and checks if the file is NULL. > > Another (io_uring) checks for return value being negative, then maps > > -ENOENT to -EBADF, not that any other value would be possible. > > > > ERR_PTR(-EINVAL) magic in case of pick_file() is borderline defensible; > > {__,}close_fd_get_file() conventions are insane. The older caller > > (in binder) had never even looked at return value; the newer one > > patches the bogus -ENOENT to what it wants to report, with strange > > "defensive" BS logics just in case __close_fd_get_file() would somehow > > find a different error to report. > > > > At the very least, {__,}close_fd_get_file() callers would've been happier > > if it just returned file or NULL. What's more, I'm seriously tempted > > to make pick_file() do the same thing. close_fd() won't care (checking > > for NULL is just as easy as for IS_ERR) and __range_close() could just > > as well cap the max_fd argument with last_fd(files_fdtable(current->files)). > > > > Does anybody see problems with the following? > > Looks good to me, and much better than passing in the pointer to the > file pointer imho. I agree. This looks good. > > -- > Jens Axboe >
On Thu, May 12, 2022 at 09:20:51PM +0000, Al Viro wrote: > Right now we have two places that do such removals - pick_file() > and {__,}close_fd_get_file(). > > They are almost identical - the only difference is in calling > conventions (well, and the fact that __... is called with descriptor > table locked). > > Calling conventions are... interesting. > > 1) pick_file() - returns file or ERR_PTR(-EBADF) or ERR_PTR(-EINVAL). > The latter is for "descriptor is greater than size of descriptor table". > One of the callers treats all ERR_PTR(...) as "return -EBADF"; another > uses ERR_PTR(-EINVAL) as "end the loop now" indicator. > > 2) {__,}close_fd_get_file() returns 0 or -ENOENT (huh?), with file (or NULL) > passed to caller by way of struct file ** argument. One of the callers > (binder) ignores the return value completely and checks if the file is NULL. > Another (io_uring) checks for return value being negative, then maps > -ENOENT to -EBADF, not that any other value would be possible. > > ERR_PTR(-EINVAL) magic in case of pick_file() is borderline defensible; > {__,}close_fd_get_file() conventions are insane. The older caller > (in binder) had never even looked at return value; the newer one > patches the bogus -ENOENT to what it wants to report, with strange > "defensive" BS logics just in case __close_fd_get_file() would somehow > find a different error to report. > > At the very least, {__,}close_fd_get_file() callers would've been happier > if it just returned file or NULL. What's more, I'm seriously tempted > to make pick_file() do the same thing. close_fd() won't care (checking > for NULL is just as easy as for IS_ERR) and __range_close() could just > as well cap the max_fd argument with last_fd(files_fdtable(current->files)). Originally, __close_range() did that last_fd() thing for both the cloexec and the non-cloexec case. But that proved buggy for the cloexec part (dumb oversight). So the cloexec part retrieves the last_fd() and marks cloexec under the spinlock to make sure it's stable. We could've done the same that this patch does now and retrieved last_fd() in __range_close() too. But without having looked at {__,}close_fd_get_file() it was more obvious imho to let pick_file() tell the caller when to terminate the loop as it avoids fiddling last_fd() out from under the rcu lock given that we below we had to take the spinlock anyway. With the motivation of reducing __close_fd_get_file() to pick_file() the other way is more sensible. > > Does anybody see problems with the following? > > commit 8819510a641800a63ab10d6b5ab283cada1cbd50 > Author: Al Viro <viro@zeniv.linux.org.uk> > Date: Thu May 12 17:08:03 2022 -0400 > > Unify the primitives for file descriptor closing > > Currently we have 3 primitives for removing an opened file from descriptor > table - pick_file(), __close_fd_get_file() and close_fd_get_file(). Their > calling conventions are rather odd and there's a code duplication for no > good reason. They can be unified - > > 1) have __range_close() cap max_fd in the very beginning; that way > we don't need separate way for pick_file() to report being past the end > of descriptor table. > > 2) make {__,}close_fd_get_file() return file (or NULL) directly, rather > than returning it via struct file ** argument. Don't bother with > (bogus) return value - nobody wants that -ENOENT. > > 3) make pick_file() return NULL on unopened descriptor - the only caller > that used to care about the distinction between descriptor past the end > of descriptor table and finding NULL in descriptor table doesn't give > a damn after (1). > > 4) lift ->files_lock out of pick_file() > > That actually simplifies the callers, as well as the primitives themselves. > Code duplication is also gone... > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> The change in io_close() looked a bit subtle because ret was overriden in there by __close_fd_get_file() prior to changing it to return struct file but ret is set to -EBADF at the top of io_close() so it looks fine. Since you change pick_file() to require the caller to hold the lock it'd be good to add a: Context: Caller must hold files_lock. to the kernel-doc I added; similar to what I did for last_fd(). Also, there's a bunch of regression tests I added in: tools/testing/selftests/core/close_range_test.c including various tests for issues reported by syzbot. Might be worth running to verify we didn't regress anything. Thanks! Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
On Fri, May 13, 2022 at 12:52:18PM +0200, Christian Brauner wrote: > Context: Caller must hold files_lock. Done and force-pushed to #work.fd > Also, there's a bunch of regression tests I added in: > > tools/testing/selftests/core/close_range_test.c > > including various tests for issues reported by syzbot. Might be worth > running to verify we didn't regress anything. # PASSED: 7 / 7 tests passed. # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
On Sat, May 14, 2022 at 11:33:01PM +0000, Al Viro wrote: > On Fri, May 13, 2022 at 12:52:18PM +0200, Christian Brauner wrote: > > > Context: Caller must hold files_lock. > > Done and force-pushed to #work.fd > > > Also, there's a bunch of regression tests I added in: > > > > tools/testing/selftests/core/close_range_test.c > > > > including various tests for issues reported by syzbot. Might be worth > > running to verify we didn't regress anything. > > # PASSED: 7 / 7 tests passed. > # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0 Thank you, appreciate it! Christian
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 8351c5638880..27c9b004823a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -1855,7 +1855,7 @@ static void binder_deferred_fd_close(int fd) if (!twcb) return; init_task_work(&twcb->twork, binder_do_fd_close); - close_fd_get_file(fd, &twcb->file); + twcb->file = close_fd_get_file(fd); if (twcb->file) { filp_close(twcb->file, current->files); task_work_add(current, &twcb->twork, TWA_RESUME); diff --git a/fs/file.c b/fs/file.c index ee9317346702..9780888fa2da 100644 --- a/fs/file.c +++ b/fs/file.c @@ -630,32 +630,21 @@ EXPORT_SYMBOL(fd_install); * @files: file struct to retrieve file from * @fd: file descriptor to retrieve file for * - * If this functions returns an EINVAL error pointer the fd was beyond the - * current maximum number of file descriptors for that fdtable. - * - * Returns: The file associated with @fd, on error returns an error pointer. + * Returns: The file associated with @fd (NULL if @fd is not open) */ static struct file *pick_file(struct files_struct *files, unsigned fd) { + struct fdtable *fdt = files_fdtable(files); struct file *file; - struct fdtable *fdt; - spin_lock(&files->file_lock); - fdt = files_fdtable(files); - if (fd >= fdt->max_fds) { - file = ERR_PTR(-EINVAL); - goto out_unlock; - } + if (fd >= fdt->max_fds) + return NULL; + file = fdt->fd[fd]; - if (!file) { - file = ERR_PTR(-EBADF); - goto out_unlock; + if (file) { + rcu_assign_pointer(fdt->fd[fd], NULL); + __put_unused_fd(files, fd); } - rcu_assign_pointer(fdt->fd[fd], NULL); - __put_unused_fd(files, fd); - -out_unlock: - spin_unlock(&files->file_lock); return file; } @@ -664,8 +653,10 @@ int close_fd(unsigned fd) struct files_struct *files = current->files; struct file *file; + spin_lock(&files->file_lock); file = pick_file(files, fd); - if (IS_ERR(file)) + spin_unlock(&files->file_lock); + if (!file) return -EBADF; return filp_close(file, files); @@ -702,20 +693,25 @@ static inline void __range_cloexec(struct files_struct *cur_fds, static inline void __range_close(struct files_struct *cur_fds, unsigned int fd, unsigned int max_fd) { + unsigned n; + + rcu_read_lock(); + n = last_fd(files_fdtable(cur_fds)); + rcu_read_unlock(); + max_fd = min(max_fd, n); + while (fd <= max_fd) { struct file *file; + spin_lock(&cur_fds->file_lock); file = pick_file(cur_fds, fd++); - if (!IS_ERR(file)) { + spin_unlock(&cur_fds->file_lock); + + if (file) { /* found a valid file to close */ filp_close(file, cur_fds); cond_resched(); - continue; } - - /* beyond the last fd in that table */ - if (PTR_ERR(file) == -EINVAL) - return; } } @@ -795,26 +791,9 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags) * See close_fd_get_file() below, this variant assumes current->files->file_lock * is held. */ -int __close_fd_get_file(unsigned int fd, struct file **res) +struct file *__close_fd_get_file(unsigned int fd) { - struct files_struct *files = current->files; - struct file *file; - struct fdtable *fdt; - - fdt = files_fdtable(files); - if (fd >= fdt->max_fds) - goto out_err; - file = fdt->fd[fd]; - if (!file) - goto out_err; - rcu_assign_pointer(fdt->fd[fd], NULL); - __put_unused_fd(files, fd); - get_file(file); - *res = file; - return 0; -out_err: - *res = NULL; - return -ENOENT; + return pick_file(current->files, fd); } /* @@ -822,16 +801,16 @@ int __close_fd_get_file(unsigned int fd, struct file **res) * 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 file *close_fd_get_file(unsigned int fd) { struct files_struct *files = current->files; - int ret; + struct file *file; spin_lock(&files->file_lock); - ret = __close_fd_get_file(fd, res); + file = pick_file(files, fd); spin_unlock(&files->file_lock); - return ret; + return file; } void do_close_on_exec(struct files_struct *files) diff --git a/fs/internal.h b/fs/internal.h index 08503dc68d2b..4065e2679103 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -125,7 +125,7 @@ extern struct file *do_file_open_root(const struct path *, const char *, const struct open_flags *); extern struct open_how build_open_how(int flags, umode_t mode); extern int build_open_flags(const struct open_how *how, struct open_flags *op); -extern int __close_fd_get_file(unsigned int fd, struct file **res); +extern struct file *__close_fd_get_file(unsigned int fd); long do_sys_ftruncate(unsigned int fd, loff_t length, int small); int chmod_common(const struct path *path, umode_t mode); diff --git a/fs/io_uring.c b/fs/io_uring.c index dc580a30723d..7257b0870353 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -5137,13 +5137,10 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags) return -EAGAIN; } - ret = __close_fd_get_file(close->fd, &file); + file = __close_fd_get_file(close->fd); spin_unlock(&files->file_lock); - if (ret < 0) { - if (ret == -ENOENT) - ret = -EBADF; + if (!file) goto err; - } /* No ->flush() or already async, safely close from here */ ret = filp_close(file, current->files); diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h index d0e78174874a..e066816f3519 100644 --- a/include/linux/fdtable.h +++ b/include/linux/fdtable.h @@ -125,7 +125,7 @@ int iterate_fd(struct files_struct *, unsigned, extern int close_fd(unsigned int fd); extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags); -extern int close_fd_get_file(unsigned int fd, struct file **res); +extern struct file *close_fd_get_file(unsigned int fd); extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds, struct files_struct **new_fdp);
Right now we have two places that do such removals - pick_file() and {__,}close_fd_get_file(). They are almost identical - the only difference is in calling conventions (well, and the fact that __... is called with descriptor table locked). Calling conventions are... interesting. 1) pick_file() - returns file or ERR_PTR(-EBADF) or ERR_PTR(-EINVAL). The latter is for "descriptor is greater than size of descriptor table". One of the callers treats all ERR_PTR(...) as "return -EBADF"; another uses ERR_PTR(-EINVAL) as "end the loop now" indicator. 2) {__,}close_fd_get_file() returns 0 or -ENOENT (huh?), with file (or NULL) passed to caller by way of struct file ** argument. One of the callers (binder) ignores the return value completely and checks if the file is NULL. Another (io_uring) checks for return value being negative, then maps -ENOENT to -EBADF, not that any other value would be possible. ERR_PTR(-EINVAL) magic in case of pick_file() is borderline defensible; {__,}close_fd_get_file() conventions are insane. The older caller (in binder) had never even looked at return value; the newer one patches the bogus -ENOENT to what it wants to report, with strange "defensive" BS logics just in case __close_fd_get_file() would somehow find a different error to report. At the very least, {__,}close_fd_get_file() callers would've been happier if it just returned file or NULL. What's more, I'm seriously tempted to make pick_file() do the same thing. close_fd() won't care (checking for NULL is just as easy as for IS_ERR) and __range_close() could just as well cap the max_fd argument with last_fd(files_fdtable(current->files)). Does anybody see problems with the following? commit 8819510a641800a63ab10d6b5ab283cada1cbd50 Author: Al Viro <viro@zeniv.linux.org.uk> Date: Thu May 12 17:08:03 2022 -0400 Unify the primitives for file descriptor closing Currently we have 3 primitives for removing an opened file from descriptor table - pick_file(), __close_fd_get_file() and close_fd_get_file(). Their calling conventions are rather odd and there's a code duplication for no good reason. They can be unified - 1) have __range_close() cap max_fd in the very beginning; that way we don't need separate way for pick_file() to report being past the end of descriptor table. 2) make {__,}close_fd_get_file() return file (or NULL) directly, rather than returning it via struct file ** argument. Don't bother with (bogus) return value - nobody wants that -ENOENT. 3) make pick_file() return NULL on unopened descriptor - the only caller that used to care about the distinction between descriptor past the end of descriptor table and finding NULL in descriptor table doesn't give a damn after (1). 4) lift ->files_lock out of pick_file() That actually simplifies the callers, as well as the primitives themselves. Code duplication is also gone... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>