* [PATCH 0/2] fs: avoid fdput() after failed fdget() @ 2020-05-07 23:57 Shuah Khan 2020-05-07 23:57 ` [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() Shuah Khan 2020-05-07 23:57 ` [PATCH 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd() Shuah Khan 0 siblings, 2 replies; 7+ messages in thread From: Shuah Khan @ 2020-05-07 23:57 UTC (permalink / raw) To: viro, axboe, zohar, mcgrof, keescook Cc: Shuah Khan, linux-fsdevel, linux-kernel While debugging an unrelated problem that got me down the path of reviewing at all the fdget() and fdput() paths in the kernel. While doing the review, I noticed these two places where fdput() is called after failed fdget(). Fixing them in these two patches. Shuah Khan (2): fs: avoid fdput() after failed fdget() in ksys_sync_file_range() fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd() fs/exec.c | 2 +- fs/sync.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() 2020-05-07 23:57 [PATCH 0/2] fs: avoid fdput() after failed fdget() Shuah Khan @ 2020-05-07 23:57 ` Shuah Khan 2020-05-08 0:05 ` Al Viro 2020-05-07 23:57 ` [PATCH 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd() Shuah Khan 1 sibling, 1 reply; 7+ messages in thread From: Shuah Khan @ 2020-05-07 23:57 UTC (permalink / raw) To: viro, axboe, zohar, mcgrof, keescook Cc: Shuah Khan, linux-fsdevel, linux-kernel Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput(). This was introdcued in a commit to add sync_file_range() helper. Fixes: 22f96b3808c1 ("fs: add sync_file_range() helper") Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- fs/sync.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/sync.c b/fs/sync.c index 4d1ff010bc5a..faaff835ef12 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -369,10 +369,11 @@ 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; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() 2020-05-07 23:57 ` [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() Shuah Khan @ 2020-05-08 0:05 ` Al Viro 2020-05-08 0:24 ` Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2020-05-08 0:05 UTC (permalink / raw) To: Shuah Khan; +Cc: axboe, zohar, mcgrof, keescook, linux-fsdevel, linux-kernel On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > in fd.flags. Fix it anyway since failed fdget() doesn't require > a fdput(). > > This was introdcued in a commit to add sync_file_range() helper. Er... What's the point microoptimizing the slow path here? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() 2020-05-08 0:05 ` Al Viro @ 2020-05-08 0:24 ` Al Viro 2020-05-08 2:21 ` Luis Chamberlain 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2020-05-08 0:24 UTC (permalink / raw) To: Shuah Khan; +Cc: axboe, zohar, mcgrof, keescook, linux-fsdevel, linux-kernel On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: > On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > > in fd.flags. Fix it anyway since failed fdget() doesn't require > > a fdput(). > > > > This was introdcued in a commit to add sync_file_range() helper. > > Er... What's the point microoptimizing the slow path here? PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the top. And looking at that thing, { struct fd f = fdget(fd); int ret; if (unlikely(!f.file)) return -EBADF; ret = sync_file_range(f.file, offset, nbytes, flags); fdput(f); return ret; } might be cleaner, but that's a matter of taste... ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() 2020-05-08 0:24 ` Al Viro @ 2020-05-08 2:21 ` Luis Chamberlain 2020-05-08 15:07 ` Shuah Khan 0 siblings, 1 reply; 7+ messages in thread From: Luis Chamberlain @ 2020-05-08 2:21 UTC (permalink / raw) To: Al Viro; +Cc: Shuah Khan, axboe, zohar, keescook, linux-fsdevel, linux-kernel On Fri, May 08, 2020 at 01:24:22AM +0100, Al Viro wrote: > On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: > > On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: > > > Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). > > > fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set > > > in fd.flags. Fix it anyway since failed fdget() doesn't require > > > a fdput(). > > > > > > This was introdcued in a commit to add sync_file_range() helper. > > > > Er... What's the point microoptimizing the slow path here? > > PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the > top. And looking at that thing, > { > struct fd f = fdget(fd); > int ret; > > if (unlikely(!f.file)) > return -EBADF; > > ret = sync_file_range(f.file, offset, nbytes, flags); > fdput(f); > return ret; > } > > might be cleaner, but that's a matter of taste... This makes it easier to read. Luis ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() 2020-05-08 2:21 ` Luis Chamberlain @ 2020-05-08 15:07 ` Shuah Khan 0 siblings, 0 replies; 7+ messages in thread From: Shuah Khan @ 2020-05-08 15:07 UTC (permalink / raw) To: Luis Chamberlain, Al Viro Cc: axboe, zohar, keescook, linux-fsdevel, linux-kernel, Shuah Khan On 5/7/20 8:21 PM, Luis Chamberlain wrote: > On Fri, May 08, 2020 at 01:24:22AM +0100, Al Viro wrote: >> On Fri, May 08, 2020 at 01:05:09AM +0100, Al Viro wrote: >>> On Thu, May 07, 2020 at 05:57:09PM -0600, Shuah Khan wrote: >>>> Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). >>>> fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set >>>> in fd.flags. Fix it anyway since failed fdget() doesn't require >>>> a fdput(). >>>> >>>> This was introdcued in a commit to add sync_file_range() helper. >>> >>> Er... What's the point microoptimizing the slow path here? >> >> PS: I'm not saying the patch is incorrect, but Fixes: is IMO over the >> top. And looking at that thing, >> { >> struct fd f = fdget(fd); >> int ret; >> >> if (unlikely(!f.file)) >> return -EBADF; >> >> ret = sync_file_range(f.file, offset, nbytes, flags); >> fdput(f); >> return ret; >> } >> >> might be cleaner, but that's a matter of taste... > > This makes it easier to read. > Yes it does. I will make the changes and send v2. thanks, -- Shuah ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd() 2020-05-07 23:57 [PATCH 0/2] fs: avoid fdput() after failed fdget() Shuah Khan 2020-05-07 23:57 ` [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() Shuah Khan @ 2020-05-07 23:57 ` Shuah Khan 1 sibling, 0 replies; 7+ messages in thread From: Shuah Khan @ 2020-05-07 23:57 UTC (permalink / raw) To: viro, axboe, zohar, mcgrof, keescook Cc: Shuah Khan, linux-fsdevel, linux-kernel Fix kernel_read_file_from_fd() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput(). This was introduced in a commit that added kernel_read_file_from_fd() as a wrapper for the VFS common kernel_read_file(). Fixes: b844f0ecbc56 ("vfs: define kernel_copy_file_from_fd()") Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..ea24bdce939d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, goto out; ret = kernel_read_file(f.file, buf, size, max_size, id); -out: fdput(f); +out: return ret; } EXPORT_SYMBOL_GPL(kernel_read_file_from_fd); -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-08 15:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-07 23:57 [PATCH 0/2] fs: avoid fdput() after failed fdget() Shuah Khan 2020-05-07 23:57 ` [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range() Shuah Khan 2020-05-08 0:05 ` Al Viro 2020-05-08 0:24 ` Al Viro 2020-05-08 2:21 ` Luis Chamberlain 2020-05-08 15:07 ` Shuah Khan 2020-05-07 23:57 ` [PATCH 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd() Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).