vfs: fix FIGETBSZ ioctl on an overlayfs file

PATCH Superseded Amir Goldstein 2 participants 4 messages Oct 11, 2018

Subsystems (2)

VFS Landlock security module

lore

Share & tools

lore

Amir Goldstein

author

2018-10-11 11:24 UTC #0 lore raw

PATCH 1/1 vfs: fix FIGETBSZ ioctl on an overlayfs file

Implement the FIGETBSZ query with the statfs(2) filesystem interface. Reading inode->i_sb->s_blocksize directly is incorrect on stacked filesystems (i.e. overlayfs).

This fixes a Floating point exception in e2fsprogs utility filefrag, which divides the size of the file with the value returned by FIGETBSZ.

Fixes d1d04ef8572b ("ovl: stack file ops")

Signed-off-by Amir Goldstein <[email protected]>

---

Miklos,

Consider this for-rc8?
There is another trivial kfree(ERR_PTR()) fix I posted yesterday.

xfstests actually has coverage for FIGETBSZ with fiemap-tester program. However, the tests that use fiemap-tester (generic/094 generic/225) did not fail on v4.19-rc1, but rather thier runtime dropped from > 10s to 0s, so I'll need to go fix those tests.

Thanks,
Amir.

 fs/ioctl.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

fs/ioctl.c +14 -1

index 2005529af560..a0e55a8e94fd 100644
@@ -17,6 +17,7 @@
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
 #include <linux/sched/signal.h>
+#include <linux/statfs.h>
 
 #include "internal.h"

@@ -219,6 +220,18 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
 	return error;
 }
 
+static int ioctl_getbsize(struct file *file, int __user *argp)
+{
+	struct kstatfs buf;
+	int err;
+
+	err = vfs_statfs(&file->f_path, &buf);
+	if (err)
+		return err;
+
+	return put_user(buf.f_bsize, argp);
+}
+
 static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 			     u64 off, u64 olen, u64 destoff)
 {

@@ -669,7 +682,7 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		return ioctl_fiemap(filp, arg);
 
 	case FIGETBSZ:
-		return put_user(inode->i_sb->s_blocksize, argp);
+		return ioctl_getbsize(filp, argp);
 
 	case FICLONE:
 		return ioctl_file_clone(filp, arg, 0, 0, 0);

Signature · 1 line

2.17.1

Miklos Szeredi

2018-10-11 12:25 UTC #1 lore raw

Quoted 43 lines · Amir Goldstein

Implement the FIGETBSZ query with the statfs(2) filesystem interface. Reading inode->i_sb->s_blocksize directly is incorrect on stacked filesystems (i.e. overlayfs).
This fixes a Floating point exception in e2fsprogs utility filefrag, which divides the size of the file with the value returned by FIGETBSZ.
Fixes d1d04ef8572b ("ovl: stack file ops")
Signed-off-by Amir Goldstein <[email protected]>
---
Miklos,
Consider this for-rc8?
There is another trivial kfree(ERR_PTR()) fix I posted yesterday.
xfstests actually has coverage for FIGETBSZ with fiemap-tester program. However, the tests that use fiemap-tester (generic/094 generic/225) did not fail on v4.19-rc1, but rather thier runtime dropped from > 10s to 0s, so I'll need to go fix those tests.
Thanks,
Amir.
 fs/ioctl.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
fs/ioctl.c +1 -0
index 2005529af560..a0e55a8e94fd 100644
@@ -17,6 +17,7 @@
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
 #include <linux/sched/signal.h>
+#include <linux/statfs.h>
 
 #include "internal.h"
 
@@ -219,6 +220,18 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
        return error;
 }
 

fs/ioctl.c static int ioctl_fiemap(struct file *filp, unsigned long arg) · line 232

Quoted 10 lines

fs/ioctl.c +10 -0

+static int ioctl_getbsize(struct file *file, int __user *argp)
+{
+       struct kstatfs buf;
+       int err;
+
+       err = vfs_statfs(&file->f_path, &buf);
+       if (err)
+               return err;
+
+       return put_user(buf.f_bsize, argp);

We must be careful not to change the interface for non-overlayfs while trying to fix overlayfs, and I don't see any guarantees, that for
every other fs f_bsize would be the same as s_blocksize. There are
other, minor side effects, like possibly blocking and/or generating network traffic, unlike the previous version.

But first let's understand what this interface is for. Haven't found any other documentation than this:

#define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */

So it's for the deprecated bmap interface, and we no longer support that. Hooray! Then just let's just put some dummy value into overlayfs's s_blocksize and s_blocksize_bits (e.g. PAGE_SIZE/PAGE_SHIFT) and be done with that.

Thanks,
Miklos

Amir Goldstein

author

2018-10-11 12:47 UTC #2 lore raw

Quoted 65 lines · Miklos Szeredi

Quoted 43 lines · Amir Goldstein
Implement the FIGETBSZ query with the statfs(2) filesystem interface. Reading inode->i_sb->s_blocksize directly is incorrect on stacked filesystems (i.e. overlayfs).
This fixes a Floating point exception in e2fsprogs utility filefrag, which divides the size of the file with the value returned by FIGETBSZ.
Fixes d1d04ef8572b ("ovl: stack file ops")
Signed-off-by Amir Goldstein <[email protected]>
---
Miklos,
Consider this for-rc8?
There is another trivial kfree(ERR_PTR()) fix I posted yesterday.
xfstests actually has coverage for FIGETBSZ with fiemap-tester program. However, the tests that use fiemap-tester (generic/094 generic/225) did not fail on v4.19-rc1, but rather thier runtime dropped from > 10s to 0s, so I'll need to go fix those tests.
Thanks,
Amir.
 fs/ioctl.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
fs/ioctl.c +1 -0
index 2005529af560..a0e55a8e94fd 100644
@@ -17,6 +17,7 @@
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
 #include <linux/sched/signal.h>
+#include <linux/statfs.h>
 
 #include "internal.h"
 
@@ -219,6 +220,18 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
        return error;
 }
 
fs/ioctl.c static int ioctl_fiemap(struct file *filp, unsigned long arg) · line 232
Quoted 10 lines
fs/ioctl.c +10 -0
+static int ioctl_getbsize(struct file *file, int __user *argp)
+{
+       struct kstatfs buf;
+       int err;
+
+       err = vfs_statfs(&file->f_path, &buf);
+       if (err)
+               return err;
+
+       return put_user(buf.f_bsize, argp);
We must be careful not to change the interface for non-overlayfs while trying to fix overlayfs, and I don't see any guarantees, that for
every other fs f_bsize would be the same as s_blocksize. There are
other, minor side effects, like possibly blocking and/or generating network traffic, unlike the previous version.
But first let's understand what this interface is for. Haven't found any other documentation than this:

Quoted 7 lines

#define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */
So it's for the deprecated bmap interface, and we no longer support that. Hooray! Then just let's just put some dummy value into overlayfs's s_blocksize and s_blocksize_bits (e.g. PAGE_SIZE/PAGE_SHIFT) and be done with that.

OK. That's fine by me, but notice that filefrag will use the value it gets from FIGETBSZ even in the FIEMAP implementation, which is the default implementation.

I think we can set s_blocksize to upper s_blocksize and cover some common cases. If s_blocksize doesn't match lower s_blocksize we can warn and set s_blocksize back to 0.

Then, we can return -EINVAL from FIGETBSZ in case s_blocksize is not set, which is the equivalent of what we do with FIBMAP.

It that too much?

Thanks,
Amir.

Miklos Szeredi

2018-10-11 13:16 UTC #3 lore raw

Quoted 78 lines · Amir Goldstein

Quoted 65 lines · Miklos Szeredi
Quoted 43 lines · Amir Goldstein
Implement the FIGETBSZ query with the statfs(2) filesystem interface. Reading inode->i_sb->s_blocksize directly is incorrect on stacked filesystems (i.e. overlayfs).
This fixes a Floating point exception in e2fsprogs utility filefrag, which divides the size of the file with the value returned by FIGETBSZ.
Fixes d1d04ef8572b ("ovl: stack file ops")
Signed-off-by Amir Goldstein <[email protected]>
---
Miklos,
Consider this for-rc8?
There is another trivial kfree(ERR_PTR()) fix I posted yesterday.
xfstests actually has coverage for FIGETBSZ with fiemap-tester program. However, the tests that use fiemap-tester (generic/094 generic/225) did not fail on v4.19-rc1, but rather thier runtime dropped from > 10s to 0s, so I'll need to go fix those tests.
Thanks,
Amir.
 fs/ioctl.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
fs/ioctl.c +1 -0
index 2005529af560..a0e55a8e94fd 100644
@@ -17,6 +17,7 @@
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
 #include <linux/sched/signal.h>
+#include <linux/statfs.h>
 
 #include "internal.h"
 
@@ -219,6 +220,18 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
        return error;
 }
 
fs/ioctl.c static int ioctl_fiemap(struct file *filp, unsigned long arg) · line 232
Quoted 10 lines
fs/ioctl.c +10 -0
+static int ioctl_getbsize(struct file *file, int __user *argp)
+{
+       struct kstatfs buf;
+       int err;
+
+       err = vfs_statfs(&file->f_path, &buf);
+       if (err)
+               return err;
+
+       return put_user(buf.f_bsize, argp);
We must be careful not to change the interface for non-overlayfs while trying to fix overlayfs, and I don't see any guarantees, that for
every other fs f_bsize would be the same as s_blocksize. There are
other, minor side effects, like possibly blocking and/or generating network traffic, unlike the previous version.
But first let's understand what this interface is for. Haven't found any other documentation than this:
Quoted 7 lines
#define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */
So it's for the deprecated bmap interface, and we no longer support that. Hooray! Then just let's just put some dummy value into overlayfs's s_blocksize and s_blocksize_bits (e.g. PAGE_SIZE/PAGE_SHIFT) and be done with that.
OK. That's fine by me, but notice that filefrag will use the value it gets from FIGETBSZ even in the FIEMAP implementation, which is the default implementation.

Quoted 8 lines

I think we can set s_blocksize to upper s_blocksize and cover some common cases. If s_blocksize doesn't match lower s_blocksize we can warn and set s_blocksize back to 0.
Then, we can return -EINVAL from FIGETBSZ in case s_blocksize is not set, which is the equivalent of what we do with FIBMAP.
It that too much?

Does any other filesystem leave s_blocksize unset? Apparently ceph
does, for example, so that's not without precedent.

Just returning -EINVAL from FIGETBSZ if s_blocksize is zero would fix filefrag, since it would gracefully fall back to f_bsize.

Thanks,
Miklos