Skip to contentSkip to conversation

fs: avoid fdput() after failed fdget()

PATCH Superseded Shuah Khan 3 participants 7 messages May 7, 2020 – May 8, 2020

Subsystems (2)
VFS exec and binfmt

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Shuah Khan

author
#0 lore raw

cover fs: avoid fdput() after failed fdget()

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(-)
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH 1/2 fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

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 <[email protected]>
---
 fs/sync.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/sync.c +3 -2
index 4d1ff010bc5a..faaff835ef12 100644
@@ -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;
 }
Signature · 1 line
2.20.1
maintainer
#2 lore raw

Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Quoted 6 lines · Shuah Khan

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?

maintainer
#3 lore raw

Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Quoted 9 lines · Al Viro
Quoted 6 lines · Shuah Khan

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...

#4 lore raw

Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Quoted 25 lines · Al Viro
Quoted 9 lines · Al Viro
Quoted 6 lines · Shuah Khan

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;
}
Quoted 1 line

might be cleaner, but that's a matter of taste...

This makes it easier to read.

Luis

author
#5 lore raw

Re: [PATCH 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Quoted 21 lines · Luis Chamberlain
Quoted 20 lines · Al Viro
Quoted 9 lines · Al Viro
Quoted 6 lines · Shuah Khan

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;
Quoted 9 lines
Quoted 6 lines
	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

author
#6 lore raw

PATCH 2/2 fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

In reply to Shuah Khan

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 <[email protected]>
---
 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/exec.c +1 -1
index 06b4c550af5d..ea24bdce939d 100644
@@ -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);
Signature · 1 line
2.20.1