Hi all,
the first two patches should fix the issue where ext4 doesn't
properly check the max file size for bitmap files in fiemap.
The rest cleans up the fiemap support in ext4 and in general.
Changes since v1:
- rename fiemap_validate to fiemap_prep
- lift FIEMAP_FLAG_SYNC handling to common code
- add a new linux/fiemap.h header
- remove __generic_block_fiemap
- remove access_ok calls from fiemap and ext4
Add an extra validation of the len parameter, as for ext4 some files
might have smaller file size limits than others. This also means the
redundant size check in ext4_ioctl_get_es_cache can go away, as all
size checking is done in the shared fiemap handler.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ext4/extents.c | 31 +++++++++++++++++++++++++++++++
fs/ext4/ioctl.c | 33 ++-------------------------------
2 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index f2b577b315a09..2b4b94542e34d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4832,6 +4832,28 @@ static const struct iomap_ops ext4_iomap_xattr_ops = {
.iomap_begin = ext4_iomap_xattr_begin,
};
+static int ext4_fiemap_check_ranges(struct inode *inode, u64 start, u64 *len)
+{
+ u64 maxbytes;
+
+ if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
+ maxbytes = inode->i_sb->s_maxbytes;
+ else
+ maxbytes = EXT4_SB(inode->i_sb)->s_bitmap_maxbytes;
+
+ if (*len == 0)
+ return -EINVAL;
+ if (start > maxbytes)
+ return -EFBIG;
+
+ /*
+ * Shrink request scope to what the fs can actually handle.
+ */
+ if (*len > maxbytes || (maxbytes - *len) < start)
+ *len = maxbytes - start;
+ return 0;
+}
+
static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
__u64 start, __u64 len, bool from_es_cache)
{
@@ -4852,6 +4874,15 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
if (fiemap_check_flags(fieinfo, ext4_fiemap_flags))
return -EBADR;
+ /*
+ * For bitmap files the maximum size limit could be smaller than
+ * s_maxbytes, so check len here manually instead of just relying on the
+ * generic check.
+ */
+ error = ext4_fiemap_check_ranges(inode, start, &len);
+ if (error)
+ return error;
+
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
error = iomap_fiemap(inode, fieinfo, start, len,
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index bfc1281fc4cbc..0746532ba463d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -733,29 +733,6 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
}
-/* copied from fs/ioctl.c */
-static int fiemap_check_ranges(struct super_block *sb,
- u64 start, u64 len, u64 *new_len)
-{
- u64 maxbytes = (u64) sb->s_maxbytes;
-
- *new_len = len;
-
- if (len == 0)
- return -EINVAL;
-
- if (start > maxbytes)
- return -EFBIG;
-
- /*
- * Shrink request scope to what the fs can actually handle.
- */
- if (len > maxbytes || (maxbytes - len) < start)
- *new_len = maxbytes - start;
-
- return 0;
-}
-
/* So that the fiemap access checks can't overflow on 32 bit machines. */
#define FIEMAP_MAX_EXTENTS (UINT_MAX / sizeof(struct fiemap_extent))
@@ -765,8 +742,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
struct fiemap __user *ufiemap = (struct fiemap __user *) arg;
struct fiemap_extent_info fieinfo = { 0, };
struct inode *inode = file_inode(filp);
- struct super_block *sb = inode->i_sb;
- u64 len;
int error;
if (copy_from_user(&fiemap, ufiemap, sizeof(fiemap)))
@@ -775,11 +750,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
return -EINVAL;
- error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
- &len);
- if (error)
- return error;
-
fieinfo.fi_flags = fiemap.fm_flags;
fieinfo.fi_extents_max = fiemap.fm_extent_count;
fieinfo.fi_extents_start = ufiemap->fm_extents;
@@ -792,7 +762,8 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
filemap_write_and_wait(inode->i_mapping);
- error = ext4_get_es_cache(inode, &fieinfo, fiemap.fm_start, len);
+ error = ext4_get_es_cache(inode, &fieinfo, fiemap.fm_start,
+ fiemap.fm_length);
fiemap.fm_flags = fieinfo.fi_flags;
fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
--
2.26.1
iomap_fiemap already calls fiemap_check_flags first thing, so this
additional check is redundant.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ext4/extents.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d2a2a3ba5c44a..a41ae7c510170 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4866,9 +4866,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
}
- if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR))
- return -EBADR;
-
/*
* For bitmap files the maximum size limit could be smaller than
* s_maxbytes, so check len here manually instead of just relying on the
--
2.26.1
There is no caller left outside of ioctl.c.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ioctl.c | 4 +---
include/linux/fs.h | 4 ----
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 282d45be6f453..f55f53c7824bb 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -299,8 +299,7 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
* If you use this function directly, you need to do your own locking. Use
* generic_block_fiemap if you want the locking done for you.
*/
-
-int __generic_block_fiemap(struct inode *inode,
+static int __generic_block_fiemap(struct inode *inode,
struct fiemap_extent_info *fieinfo, loff_t start,
loff_t len, get_block_t *get_block)
{
@@ -445,7 +444,6 @@ int __generic_block_fiemap(struct inode *inode,
return ret;
}
-EXPORT_SYMBOL(__generic_block_fiemap);
/**
* generic_block_fiemap - FIEMAP for block based inodes
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f6f59b4f22a8..3104c6f7527b5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3299,10 +3299,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
extern int vfs_readlink(struct dentry *, char __user *, int);
-extern int __generic_block_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo,
- loff_t start, loff_t len,
- get_block_t *get_block);
extern int generic_block_fiemap(struct inode *inode,
struct fiemap_extent_info *fieinfo, u64 start,
u64 len, get_block_t *get_block);
--
2.26.1
No need to pull the fiemap definitions into almost every file in the
kernel build.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/bad_inode.c | 1 +
fs/btrfs/extent_io.h | 1 +
fs/cifs/inode.c | 1 +
fs/cifs/smb2ops.c | 1 +
fs/ext2/inode.c | 1 +
fs/ext4/ext4.h | 1 +
fs/f2fs/data.c | 1 +
fs/f2fs/inline.c | 1 +
fs/gfs2/inode.c | 1 +
fs/hpfs/file.c | 1 +
fs/ioctl.c | 1 +
fs/iomap/fiemap.c | 1 +
fs/nilfs2/inode.c | 1 +
fs/overlayfs/inode.c | 1 +
fs/xfs/xfs_iops.c | 1 +
include/linux/fiemap.h | 24 ++++++++++++++++++++++++
include/linux/fs.h | 19 +------------------
include/uapi/linux/fiemap.h | 6 +++---
18 files changed, 43 insertions(+), 21 deletions(-)
create mode 100644 include/linux/fiemap.h
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index 8035d2a445617..54f0ce4442720 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -15,6 +15,7 @@
#include <linux/time.h>
#include <linux/namei.h>
#include <linux/poll.h>
+#include <linux/fiemap.h>
static int bad_file_open(struct inode *inode, struct file *filp)
{
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 2ed65bd0760ea..817698bc06693 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -5,6 +5,7 @@
#include <linux/rbtree.h>
#include <linux/refcount.h>
+#include <linux/fiemap.h>
#include "ulist.h"
/*
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 390d2b15ef6ef..3f276eb8ca68d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -25,6 +25,7 @@
#include <linux/freezer.h>
#include <linux/sched/signal.h>
#include <linux/wait_bit.h>
+#include <linux/fiemap.h>
#include <asm/div64.h>
#include "cifsfs.h"
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index f829f4165d38c..09047f1ddfb66 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -12,6 +12,7 @@
#include <linux/uuid.h>
#include <linux/sort.h>
#include <crypto/aead.h>
+#include <linux/fiemap.h>
#include "cifsfs.h"
#include "cifsglob.h"
#include "smb2pdu.h"
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index c885cf7d724b4..0f12a0e8a8d97 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -36,6 +36,7 @@
#include <linux/iomap.h>
#include <linux/namei.h>
#include <linux/uio.h>
+#include <linux/fiemap.h>
#include "ext2.h"
#include "acl.h"
#include "xattr.h"
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ad2dbf6e49245..06f97a3a943f6 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -36,6 +36,7 @@
#include <crypto/hash.h>
#include <linux/falloc.h>
#include <linux/percpu-rwsem.h>
+#include <linux/fiemap.h>
#ifdef __KERNEL__
#include <linux/compat.h>
#endif
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index cdf2f626bea7a..25abbbb65ba09 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -19,6 +19,7 @@
#include <linux/uio.h>
#include <linux/cleancache.h>
#include <linux/sched/signal.h>
+#include <linux/fiemap.h>
#include "f2fs.h"
#include "node.h"
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 4167e54081518..9686ffea177e7 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -8,6 +8,7 @@
#include <linux/fs.h>
#include <linux/f2fs_fs.h>
+#include <linux/fiemap.h>
#include "f2fs.h"
#include "node.h"
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 70b2d3a1e8668..4842f313a8084 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -17,6 +17,7 @@
#include <linux/crc32.h>
#include <linux/iomap.h>
#include <linux/security.h>
+#include <linux/fiemap.h>
#include <linux/uaccess.h>
#include "gfs2.h"
diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
index b36abf9cb345a..62959a8e43ad8 100644
--- a/fs/hpfs/file.c
+++ b/fs/hpfs/file.c
@@ -9,6 +9,7 @@
#include "hpfs_fn.h"
#include <linux/mpage.h>
+#include <linux/fiemap.h>
#define BLOCKS(size) (((size) + 511) >> 9)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index f55f53c7824bb..cbc84e23d00bd 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -18,6 +18,7 @@
#include <linux/buffer_head.h>
#include <linux/falloc.h>
#include <linux/sched/signal.h>
+#include <linux/fiemap.h>
#include "internal.h"
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index bccf305ea9ce2..fca3dfb9d964a 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -6,6 +6,7 @@
#include <linux/compiler.h>
#include <linux/fs.h>
#include <linux/iomap.h>
+#include <linux/fiemap.h>
struct fiemap_ctx {
struct fiemap_extent_info *fi;
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 671085512e0fd..6e1aca38931f3 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -14,6 +14,7 @@
#include <linux/pagemap.h>
#include <linux/writeback.h>
#include <linux/uio.h>
+#include <linux/fiemap.h>
#include "nilfs.h"
#include "btnode.h"
#include "segment.h"
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b0d42ece4d7cc..b5fec34105569 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -10,6 +10,7 @@
#include <linux/xattr.h>
#include <linux/posix_acl.h>
#include <linux/ratelimit.h>
+#include <linux/fiemap.h>
#include "overlayfs.h"
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f7a99b3bbcf7a..44c353998ac5c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -25,6 +25,7 @@
#include <linux/posix_acl.h>
#include <linux/security.h>
#include <linux/iversion.h>
+#include <linux/fiemap.h>
/*
* Directories have different lock order w.r.t. mmap_sem compared to regular
diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
new file mode 100644
index 0000000000000..240d4f7d9116a
--- /dev/null
+++ b/include/linux/fiemap.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FIEMAP_H
+#define _LINUX_FIEMAP_H 1
+
+#include <uapi/linux/fiemap.h>
+#include <linux/fs.h>
+
+struct fiemap_extent_info {
+ unsigned int fi_flags; /* Flags as passed from user */
+ unsigned int fi_extents_mapped; /* Number of mapped extents */
+ unsigned int fi_extents_max; /* Size of fiemap_extent array */
+ struct fiemap_extent __user *fi_extents_start; /* Start of
+ fiemap_extent array */
+};
+
+int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
+ u64 phys, u64 len, u32 flags);
+int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
+
+int generic_block_fiemap(struct inode *inode,
+ struct fiemap_extent_info *fieinfo, u64 start, u64 len,
+ get_block_t *get_block);
+
+#endif /* _LINUX_FIEMAP_H 1 */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3104c6f7527b5..09bcd329c0628 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -24,7 +24,6 @@
#include <linux/capability.h>
#include <linux/semaphore.h>
#include <linux/fcntl.h>
-#include <linux/fiemap.h>
#include <linux/rculist_bl.h>
#include <linux/atomic.h>
#include <linux/shrinker.h>
@@ -48,6 +47,7 @@ struct backing_dev_info;
struct bdi_writeback;
struct bio;
struct export_operations;
+struct fiemap_extent_info;
struct hd_geometry;
struct iovec;
struct kiocb;
@@ -1745,19 +1745,6 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
extern void inode_init_owner(struct inode *inode, const struct inode *dir,
umode_t mode);
extern bool may_open_dev(const struct path *path);
-/*
- * VFS FS_IOC_FIEMAP helper definitions.
- */
-struct fiemap_extent_info {
- unsigned int fi_flags; /* Flags as passed from user */
- unsigned int fi_extents_mapped; /* Number of mapped extents */
- unsigned int fi_extents_max; /* Size of fiemap_extent array */
- struct fiemap_extent __user *fi_extents_start; /* Start of
- fiemap_extent array */
-};
-int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
- u64 phys, u64 len, u32 flags);
-int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
/*
* This is the "filldir" function type, used by readdir() to let
@@ -3299,10 +3286,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
extern int vfs_readlink(struct dentry *, char __user *, int);
-extern int generic_block_fiemap(struct inode *inode,
- struct fiemap_extent_info *fieinfo, u64 start,
- u64 len, get_block_t *get_block);
-
extern struct file_system_type *get_filesystem(struct file_system_type *fs);
extern void put_filesystem(struct file_system_type *fs);
extern struct file_system_type *get_fs_type(const char *name);
diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index 7a900b2377b60..24ca0c00cae36 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h
@@ -9,8 +9,8 @@
* Andreas Dilger <[email protected]>
*/
-#ifndef _LINUX_FIEMAP_H
-#define _LINUX_FIEMAP_H
+#ifndef _UAPI_LINUX_FIEMAP_H
+#define _UAPI_LINUX_FIEMAP_H
#include <linux/types.h>
@@ -67,4 +67,4 @@ struct fiemap {
#define FIEMAP_EXTENT_SHARED 0x00002000 /* Space shared with other
* files. */
-#endif /* _LINUX_FIEMAP_H */
+#endif /* _UAPI_LINUX_FIEMAP_H */
--
2.26.1
iomap_fiemap should take u64 start and len arguments, just like the
->fiemap prototype.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/iomap/fiemap.c | 2 +-
include/linux/iomap.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index fca3dfb9d964a..dd04e4added15 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -66,7 +66,7 @@ iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
}
int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
- loff_t start, loff_t len, const struct iomap_ops *ops)
+ u64 start, u64 len, const struct iomap_ops *ops)
{
struct fiemap_ctx ctx;
loff_t ret;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 8b09463dae0db..63db02528b702 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -178,7 +178,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
const struct iomap_ops *ops);
int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- loff_t start, loff_t len, const struct iomap_ops *ops);
+ u64 start, u64 len, const struct iomap_ops *ops);
loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
const struct iomap_ops *ops);
loff_t iomap_seek_data(struct inode *inode, loff_t offset,
--
2.26.1
The fiemap and EXT4_IOC_GET_ES_CACHE cases share almost no code, so split
them into entirely separate functions.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ext4/extents.c | 72 +++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 37 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 2b4b94542e34d..d2a2a3ba5c44a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4854,11 +4854,9 @@ static int ext4_fiemap_check_ranges(struct inode *inode, u64 start, u64 *len)
return 0;
}
-static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len, bool from_es_cache)
+int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
+ u64 start, u64 len)
{
- ext4_lblk_t start_blk;
- u32 ext4_fiemap_flags = FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR;
int error = 0;
if (fieinfo->fi_flags & FIEMAP_FLAG_CACHE) {
@@ -4868,10 +4866,7 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
}
- if (from_es_cache)
- ext4_fiemap_flags &= FIEMAP_FLAG_XATTR;
-
- if (fiemap_check_flags(fieinfo, ext4_fiemap_flags))
+ if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR))
return -EBADR;
/*
@@ -4885,40 +4880,20 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
- error = iomap_fiemap(inode, fieinfo, start, len,
- &ext4_iomap_xattr_ops);
- } else if (!from_es_cache) {
- error = iomap_fiemap(inode, fieinfo, start, len,
- &ext4_iomap_report_ops);
- } else {
- ext4_lblk_t len_blks;
- __u64 last_blk;
-
- start_blk = start >> inode->i_sb->s_blocksize_bits;
- last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
- if (last_blk >= EXT_MAX_BLOCKS)
- last_blk = EXT_MAX_BLOCKS-1;
- len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
-
- /*
- * Walk the extent tree gathering extent information
- * and pushing extents back to the user.
- */
- error = ext4_fill_es_cache_info(inode, start_blk, len_blks,
- fieinfo);
+ return iomap_fiemap(inode, fieinfo, start, len,
+ &ext4_iomap_xattr_ops);
}
- return error;
-}
-int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
- __u64 start, __u64 len)
-{
- return _ext4_fiemap(inode, fieinfo, start, len, false);
+ return iomap_fiemap(inode, fieinfo, start, len, &ext4_iomap_report_ops);
}
int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
__u64 start, __u64 len)
{
+ ext4_lblk_t start_blk, len_blks;
+ __u64 last_blk;
+ int error = 0;
+
if (ext4_has_inline_data(inode)) {
int has_inline;
@@ -4929,9 +4904,32 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
return 0;
}
- return _ext4_fiemap(inode, fieinfo, start, len, true);
-}
+ if (fieinfo->fi_flags & FIEMAP_FLAG_CACHE) {
+ error = ext4_ext_precache(inode);
+ if (error)
+ return error;
+ fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
+ }
+
+ if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC))
+ return -EBADR;
+ error = ext4_fiemap_check_ranges(inode, start, &len);
+ if (error)
+ return error;
+
+ start_blk = start >> inode->i_sb->s_blocksize_bits;
+ last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
+ if (last_blk >= EXT_MAX_BLOCKS)
+ last_blk = EXT_MAX_BLOCKS-1;
+ len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
+
+ /*
+ * Walk the extent tree gathering extent information
+ * and pushing extents back to the user.
+ */
+ return ext4_fill_es_cache_info(inode, start_blk, len_blks, fieinfo);
+}
/*
* ext4_access_path:
--
2.26.1
access_ok just checks we are fed a proper user pointer. We also do that
in copy_to_user itself, so no need to do this early.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ioctl.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index ae0d228d18a16..d24afce649037 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -209,13 +209,9 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
fieinfo.fi_extents_max = fiemap.fm_extent_count;
fieinfo.fi_extents_start = ufiemap->fm_extents;
- if (fiemap.fm_extent_count != 0 &&
- !access_ok(fieinfo.fi_extents_start,
- fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
- return -EFAULT;
-
error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
fiemap.fm_length);
+
fiemap.fm_flags = fieinfo.fi_flags;
fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
--
2.26.1
access_ok just checks we are fed a proper user pointer. We also do that
in copy_to_user itself, so no need to do this early.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ext4/ioctl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 0746532ba463d..7fded54d2dba9 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -754,11 +754,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
fieinfo.fi_extents_max = fiemap.fm_extent_count;
fieinfo.fi_extents_start = ufiemap->fm_extents;
- if (fiemap.fm_extent_count != 0 &&
- !access_ok(fieinfo.fi_extents_start,
- fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
- return -EFAULT;
-
if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
filemap_write_and_wait(inode->i_mapping);
--
2.26.1
By moving FIEMAP_FLAG_SYNC handling to fiemap_prep we ensure it is
handled once instead of duplicated, but can still be done under fs locks,
like xfs/iomap intended with its duplicate handling. Also make sure the
error value of filemap_write_and_wait is propagated to user space.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/btrfs/inode.c | 4 +---
fs/cifs/smb2ops.c | 3 +--
fs/ext4/extents.c | 2 +-
fs/f2fs/data.c | 3 +--
fs/ioctl.c | 10 ++++++----
fs/iomap/fiemap.c | 8 +-------
fs/nilfs2/inode.c | 2 +-
fs/ocfs2/extent_map.c | 5 +----
fs/overlayfs/inode.c | 4 ----
9 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1f1ec361089b3..529ffa5e7b452 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8243,14 +8243,12 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
return ret;
}
-#define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
-
static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
__u64 start, __u64 len)
{
int ret;
- ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
+ ret = fiemap_prep(inode, fieinfo, start, &len, 0);
if (ret)
return ret;
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 8a2e94931dc96..32880fca6d8d8 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3408,8 +3408,7 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
int i, num, rc, flags, last_blob;
u64 next;
- rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
- FIEMAP_FLAG_SYNC);
+ rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len, 0);
if (rc)
rc;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 41f73dea92cac..93574e88f6543 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4908,7 +4908,7 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
}
- error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
+ error = fiemap_prep(inode, fieinfo, start, &len, 0);
if (error)
return error;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 03faafc591b17..9de7dc476ed16 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1825,8 +1825,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
return ret;
}
- ret = fiemap_prep(inode, fieinfo, start, &len,
- FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
+ ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_XATTR);
if (ret)
return ret;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 4d94c20c9596b..ae0d228d18a16 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -162,6 +162,7 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
{
u64 maxbytes = inode->i_sb->s_maxbytes;
u32 incompat_flags;
+ int ret = 0;
if (*len == 0)
return -EINVAL;
@@ -174,13 +175,17 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
if (*len > maxbytes || (maxbytes - *len) < start)
*len = maxbytes - start;
+ supported_flags |= FIEMAP_FLAG_SYNC;
supported_flags &= FIEMAP_FLAGS_COMPAT;
incompat_flags = fieinfo->fi_flags & ~supported_flags;
if (incompat_flags) {
fieinfo->fi_flags = incompat_flags;
return -EBADR;
}
- return 0;
+
+ if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
+ ret = filemap_write_and_wait(inode->i_mapping);
+ return ret;
}
EXPORT_SYMBOL(fiemap_prep);
@@ -209,9 +214,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
return -EFAULT;
- if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
- filemap_write_and_wait(inode->i_mapping);
-
error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
fiemap.fm_length);
fiemap.fm_flags = fieinfo.fi_flags;
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index 5e4e3520424da..fffd9eedfd880 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -75,16 +75,10 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
ctx.fi = fi;
ctx.prev.type = IOMAP_HOLE;
- ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
+ ret = fiemap_prep(inode, fi, start, &len, 0);
if (ret)
return ret;
- if (fi->fi_flags & FIEMAP_FLAG_SYNC) {
- ret = filemap_write_and_wait(inode->i_mapping);
- if (ret)
- return ret;
- }
-
while (len > 0) {
ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
iomap_fiemap_actor);
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 052c2da11e4d7..25b0d368ecdb2 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
unsigned int blkbits = inode->i_blkbits;
int ret, n;
- ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
+ ret = fiemap_prep(inode, fieinfo, start, &len, 0);
if (ret)
return ret;
diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index 3744179b73fa1..a94852af5510d 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -733,8 +733,6 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
return 0;
}
-#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
-
int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
u64 map_start, u64 map_len)
{
@@ -746,8 +744,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
struct buffer_head *di_bh = NULL;
struct ocfs2_extent_rec rec;
- ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
- OCFS2_FIEMAP_FLAGS);
+ ret = fiemap_prep(inode, fieinfo, map_start, &map_len, 0);
if (ret)
return ret;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b5fec34105569..c7cb883c47b86 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -462,10 +462,6 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
return -EOPNOTSUPP;
old_cred = ovl_override_creds(inode->i_sb);
-
- if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
- filemap_write_and_wait(realinode->i_mapping);
-
err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
revert_creds(old_cred);
--
2.26.1
From: Ritesh Harjani <[email protected]>
ext4 supports max number of logical blocks in a file to be 0xffffffff.
(This is since ext4_extent's ee_block is __le32).
This means that EXT4_MAX_LOGICAL_BLOCK should be 0xfffffffe (starting
from 0 logical offset). This patch fixes this.
The issue was seen when ext4 moved to iomap_fiemap API and when
overlayfs was mounted on top of ext4. Since overlayfs was missing
filemap_check_ranges(), so it could pass a arbitrary huge length which
lead to overflow of map.m_len logic.
This patch fixes that.
Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
Reported-by: [email protected]
Signed-off-by: Ritesh Harjani <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/ext4/ext4.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 91eb4381cae5b..ad2dbf6e49245 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -722,7 +722,7 @@ enum {
#define EXT4_MAX_BLOCK_FILE_PHYS 0xFFFFFFFF
/* Max logical block we can support */
-#define EXT4_MAX_LOGICAL_BLOCK 0xFFFFFFFF
+#define EXT4_MAX_LOGICAL_BLOCK 0xFFFFFFFE
/*
* Structure of an inode on the disk
--
2.26.1
Replace fiemap_check_flags with a fiemap_validate helpers that also takes
the inode and mapped range, and performs the sanity check and truncation
previously done in fiemap_check_range. This way the validation is inside
the file system itself and thus properly works for the stacked overlayfs
case as well.
Signed-off-by: Christoph Hellwig <[email protected]>
---
Documentation/filesystems/fiemap.txt | 8 ++---
fs/btrfs/inode.c | 2 +-
fs/cifs/smb2ops.c | 6 ++--
fs/ext4/extents.c | 5 +--
fs/f2fs/data.c | 3 +-
fs/ioctl.c | 53 +++++++++++-----------------
fs/iomap/fiemap.c | 2 +-
fs/nilfs2/inode.c | 2 +-
fs/ocfs2/extent_map.c | 3 +-
include/linux/fiemap.h | 3 +-
10 files changed, 40 insertions(+), 47 deletions(-)
diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
index ac87e6fda842b..05926b7f92809 100644
--- a/Documentation/filesystems/fiemap.txt
+++ b/Documentation/filesystems/fiemap.txt
@@ -203,15 +203,13 @@ EINTR once fatal signal received.
Flag checking should be done at the beginning of the ->fiemap callback via the
-fiemap_check_flags() helper:
-
-int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
+fiemap_prep() helper.
The struct fieinfo should be passed in as received from ioctl_fiemap(). The
set of fiemap flags which the fs understands should be passed via fs_flags. If
-fiemap_check_flags finds invalid user flags, it will place the bad values in
+fiemap_prep finds invalid user flags, it will place the bad values in
fieinfo->fi_flags and return -EBADR. If the file system gets -EBADR, from
-fiemap_check_flags(), it should immediately exit, returning that error back to
+fiemap_prep(), it should immediately exit, returning that error back to
ioctl_fiemap().
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 320d1062068d3..1f1ec361089b3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8250,7 +8250,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
{
int ret;
- ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS);
+ ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
if (ret)
return ret;
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 09047f1ddfb66..8a2e94931dc96 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3408,8 +3408,10 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
int i, num, rc, flags, last_blob;
u64 next;
- if (fiemap_check_flags(fei, FIEMAP_FLAG_SYNC))
- return -EBADR;
+ rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
+ FIEMAP_FLAG_SYNC);
+ if (rc)
+ rc;
xid = get_xid();
again:
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a41ae7c510170..41f73dea92cac 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4908,8 +4908,9 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
}
- if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC))
- return -EBADR;
+ error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
+ if (error)
+ return error;
error = ext4_fiemap_check_ranges(inode, start, &len);
if (error)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 25abbbb65ba09..03faafc591b17 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1825,7 +1825,8 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
return ret;
}
- ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
+ ret = fiemap_prep(inode, fieinfo, start, &len,
+ FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
if (ret)
return ret;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index cbc84e23d00bd..4d94c20c9596b 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -141,9 +141,12 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
EXPORT_SYMBOL(fiemap_fill_next_extent);
/**
- * fiemap_check_flags - check validity of requested flags for fiemap
+ * fiemap_prep - check validity of requested flags for fiemap
+ * @inode: Inode to operate on
* @fieinfo: Fiemap context passed into ->fiemap
- * @fs_flags: Set of fiemap flags that the file system understands
+ * @start: Start of the mapped range
+ * @len: Length of the mapped range, can be truncated by this function.
+ * @supported_flags: Set of fiemap flags that the file system understands
*
* Called from file system ->fiemap callback. This will compute the
* intersection of valid fiemap flags and those that the fs supports. That
@@ -154,48 +157,38 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
*
* Returns 0 on success, -EBADR on bad flags.
*/
-int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
+int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
+ u64 start, u64 *len, u32 supported_flags)
{
+ u64 maxbytes = inode->i_sb->s_maxbytes;
u32 incompat_flags;
- incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
- if (incompat_flags) {
- fieinfo->fi_flags = incompat_flags;
- return -EBADR;
- }
- return 0;
-}
-EXPORT_SYMBOL(fiemap_check_flags);
-
-static int fiemap_check_ranges(struct super_block *sb,
- u64 start, u64 len, u64 *new_len)
-{
- u64 maxbytes = (u64) sb->s_maxbytes;
-
- *new_len = len;
-
- if (len == 0)
+ if (*len == 0)
return -EINVAL;
-
if (start > maxbytes)
return -EFBIG;
/*
* Shrink request scope to what the fs can actually handle.
*/
- if (len > maxbytes || (maxbytes - len) < start)
- *new_len = maxbytes - start;
+ if (*len > maxbytes || (maxbytes - *len) < start)
+ *len = maxbytes - start;
+ supported_flags &= FIEMAP_FLAGS_COMPAT;
+ incompat_flags = fieinfo->fi_flags & ~supported_flags;
+ if (incompat_flags) {
+ fieinfo->fi_flags = incompat_flags;
+ return -EBADR;
+ }
return 0;
}
+EXPORT_SYMBOL(fiemap_prep);
static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
{
struct fiemap fiemap;
struct fiemap_extent_info fieinfo = { 0, };
struct inode *inode = file_inode(filp);
- struct super_block *sb = inode->i_sb;
- u64 len;
int error;
if (!inode->i_op->fiemap)
@@ -207,11 +200,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
return -EINVAL;
- error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
- &len);
- if (error)
- return error;
-
fieinfo.fi_flags = fiemap.fm_flags;
fieinfo.fi_extents_max = fiemap.fm_extent_count;
fieinfo.fi_extents_start = ufiemap->fm_extents;
@@ -224,7 +212,8 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
filemap_write_and_wait(inode->i_mapping);
- error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
+ error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
+ fiemap.fm_length);
fiemap.fm_flags = fieinfo.fi_flags;
fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
@@ -312,7 +301,7 @@ static int __generic_block_fiemap(struct inode *inode,
bool past_eof = false, whole_file = false;
int ret = 0;
- ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+ ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
if (ret)
return ret;
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index dd04e4added15..5e4e3520424da 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -75,7 +75,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
ctx.fi = fi;
ctx.prev.type = IOMAP_HOLE;
- ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
+ ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
if (ret)
return ret;
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 6e1aca38931f3..052c2da11e4d7 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
unsigned int blkbits = inode->i_blkbits;
int ret, n;
- ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+ ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
if (ret)
return ret;
diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index e3e2d1b2af51a..3744179b73fa1 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -746,7 +746,8 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
struct buffer_head *di_bh = NULL;
struct ocfs2_extent_rec rec;
- ret = fiemap_check_flags(fieinfo, OCFS2_FIEMAP_FLAGS);
+ ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
+ OCFS2_FIEMAP_FLAGS);
if (ret)
return ret;
diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
index 240d4f7d9116a..4e624c4665837 100644
--- a/include/linux/fiemap.h
+++ b/include/linux/fiemap.h
@@ -13,9 +13,10 @@ struct fiemap_extent_info {
fiemap_extent array */
};
+int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
+ u64 start, u64 *len, u32 supported_flags);
int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
u64 phys, u64 len, u32 flags);
-int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
int generic_block_fiemap(struct inode *inode,
struct fiemap_extent_info *fieinfo, u64 start, u64 len,
--
2.26.1
On Mon, Apr 27, 2020 at 9:20 PM Christoph Hellwig <[email protected]> wrote:
>
> By moving FIEMAP_FLAG_SYNC handling to fiemap_prep we ensure it is
> handled once instead of duplicated, but can still be done under fs locks,
> like xfs/iomap intended with its duplicate handling. Also make sure the
> error value of filemap_write_and_wait is propagated to user space.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Nice!
Reviewed-by: Amir Goldstein <[email protected]>
Thanks for fixing this,
Amir.
On Mon, Apr 27, 2020 at 9:20 PM Christoph Hellwig <[email protected]> wrote:
>
> Replace fiemap_check_flags with a fiemap_validate helpers that also takes
Leftover fiemap_validate
> the inode and mapped range, and performs the sanity check and truncation
> previously done in fiemap_check_range. This way the validation is inside
> the file system itself and thus properly works for the stacked overlayfs
> case as well.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Thanks for fixing this,
Amir.
On Mon, Apr 27, 2020 at 08:19:52PM +0200, Christoph Hellwig wrote:
> No need to pull the fiemap definitions into almost every file in the
> kernel build.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Hooray, I hate the overloaded mess that fs.h has become...
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/bad_inode.c | 1 +
> fs/btrfs/extent_io.h | 1 +
> fs/cifs/inode.c | 1 +
> fs/cifs/smb2ops.c | 1 +
> fs/ext2/inode.c | 1 +
> fs/ext4/ext4.h | 1 +
> fs/f2fs/data.c | 1 +
> fs/f2fs/inline.c | 1 +
> fs/gfs2/inode.c | 1 +
> fs/hpfs/file.c | 1 +
> fs/ioctl.c | 1 +
> fs/iomap/fiemap.c | 1 +
> fs/nilfs2/inode.c | 1 +
> fs/overlayfs/inode.c | 1 +
> fs/xfs/xfs_iops.c | 1 +
> include/linux/fiemap.h | 24 ++++++++++++++++++++++++
> include/linux/fs.h | 19 +------------------
> include/uapi/linux/fiemap.h | 6 +++---
> 18 files changed, 43 insertions(+), 21 deletions(-)
> create mode 100644 include/linux/fiemap.h
>
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index 8035d2a445617..54f0ce4442720 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -15,6 +15,7 @@
> #include <linux/time.h>
> #include <linux/namei.h>
> #include <linux/poll.h>
> +#include <linux/fiemap.h>
>
> static int bad_file_open(struct inode *inode, struct file *filp)
> {
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 2ed65bd0760ea..817698bc06693 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -5,6 +5,7 @@
>
> #include <linux/rbtree.h>
> #include <linux/refcount.h>
> +#include <linux/fiemap.h>
> #include "ulist.h"
>
> /*
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 390d2b15ef6ef..3f276eb8ca68d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -25,6 +25,7 @@
> #include <linux/freezer.h>
> #include <linux/sched/signal.h>
> #include <linux/wait_bit.h>
> +#include <linux/fiemap.h>
>
> #include <asm/div64.h>
> #include "cifsfs.h"
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index f829f4165d38c..09047f1ddfb66 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -12,6 +12,7 @@
> #include <linux/uuid.h>
> #include <linux/sort.h>
> #include <crypto/aead.h>
> +#include <linux/fiemap.h>
> #include "cifsfs.h"
> #include "cifsglob.h"
> #include "smb2pdu.h"
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index c885cf7d724b4..0f12a0e8a8d97 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -36,6 +36,7 @@
> #include <linux/iomap.h>
> #include <linux/namei.h>
> #include <linux/uio.h>
> +#include <linux/fiemap.h>
> #include "ext2.h"
> #include "acl.h"
> #include "xattr.h"
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index ad2dbf6e49245..06f97a3a943f6 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -36,6 +36,7 @@
> #include <crypto/hash.h>
> #include <linux/falloc.h>
> #include <linux/percpu-rwsem.h>
> +#include <linux/fiemap.h>
> #ifdef __KERNEL__
> #include <linux/compat.h>
> #endif
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cdf2f626bea7a..25abbbb65ba09 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -19,6 +19,7 @@
> #include <linux/uio.h>
> #include <linux/cleancache.h>
> #include <linux/sched/signal.h>
> +#include <linux/fiemap.h>
>
> #include "f2fs.h"
> #include "node.h"
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 4167e54081518..9686ffea177e7 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -8,6 +8,7 @@
>
> #include <linux/fs.h>
> #include <linux/f2fs_fs.h>
> +#include <linux/fiemap.h>
>
> #include "f2fs.h"
> #include "node.h"
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 70b2d3a1e8668..4842f313a8084 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -17,6 +17,7 @@
> #include <linux/crc32.h>
> #include <linux/iomap.h>
> #include <linux/security.h>
> +#include <linux/fiemap.h>
> #include <linux/uaccess.h>
>
> #include "gfs2.h"
> diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
> index b36abf9cb345a..62959a8e43ad8 100644
> --- a/fs/hpfs/file.c
> +++ b/fs/hpfs/file.c
> @@ -9,6 +9,7 @@
>
> #include "hpfs_fn.h"
> #include <linux/mpage.h>
> +#include <linux/fiemap.h>
>
> #define BLOCKS(size) (((size) + 511) >> 9)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f55f53c7824bb..cbc84e23d00bd 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -18,6 +18,7 @@
> #include <linux/buffer_head.h>
> #include <linux/falloc.h>
> #include <linux/sched/signal.h>
> +#include <linux/fiemap.h>
>
> #include "internal.h"
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce2..fca3dfb9d964a 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -6,6 +6,7 @@
> #include <linux/compiler.h>
> #include <linux/fs.h>
> #include <linux/iomap.h>
> +#include <linux/fiemap.h>
>
> struct fiemap_ctx {
> struct fiemap_extent_info *fi;
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 671085512e0fd..6e1aca38931f3 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -14,6 +14,7 @@
> #include <linux/pagemap.h>
> #include <linux/writeback.h>
> #include <linux/uio.h>
> +#include <linux/fiemap.h>
> #include "nilfs.h"
> #include "btnode.h"
> #include "segment.h"
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b0d42ece4d7cc..b5fec34105569 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -10,6 +10,7 @@
> #include <linux/xattr.h>
> #include <linux/posix_acl.h>
> #include <linux/ratelimit.h>
> +#include <linux/fiemap.h>
> #include "overlayfs.h"
>
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f7a99b3bbcf7a..44c353998ac5c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -25,6 +25,7 @@
> #include <linux/posix_acl.h>
> #include <linux/security.h>
> #include <linux/iversion.h>
> +#include <linux/fiemap.h>
>
> /*
> * Directories have different lock order w.r.t. mmap_sem compared to regular
> diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
> new file mode 100644
> index 0000000000000..240d4f7d9116a
> --- /dev/null
> +++ b/include/linux/fiemap.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_FIEMAP_H
> +#define _LINUX_FIEMAP_H 1
> +
> +#include <uapi/linux/fiemap.h>
> +#include <linux/fs.h>
> +
> +struct fiemap_extent_info {
> + unsigned int fi_flags; /* Flags as passed from user */
> + unsigned int fi_extents_mapped; /* Number of mapped extents */
> + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> + struct fiemap_extent __user *fi_extents_start; /* Start of
> + fiemap_extent array */
> +};
> +
> +int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> + u64 phys, u64 len, u32 flags);
> +int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
> +
> +int generic_block_fiemap(struct inode *inode,
> + struct fiemap_extent_info *fieinfo, u64 start, u64 len,
> + get_block_t *get_block);
> +
> +#endif /* _LINUX_FIEMAP_H 1 */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3104c6f7527b5..09bcd329c0628 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -24,7 +24,6 @@
> #include <linux/capability.h>
> #include <linux/semaphore.h>
> #include <linux/fcntl.h>
> -#include <linux/fiemap.h>
> #include <linux/rculist_bl.h>
> #include <linux/atomic.h>
> #include <linux/shrinker.h>
> @@ -48,6 +47,7 @@ struct backing_dev_info;
> struct bdi_writeback;
> struct bio;
> struct export_operations;
> +struct fiemap_extent_info;
> struct hd_geometry;
> struct iovec;
> struct kiocb;
> @@ -1745,19 +1745,6 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
> extern void inode_init_owner(struct inode *inode, const struct inode *dir,
> umode_t mode);
> extern bool may_open_dev(const struct path *path);
> -/*
> - * VFS FS_IOC_FIEMAP helper definitions.
> - */
> -struct fiemap_extent_info {
> - unsigned int fi_flags; /* Flags as passed from user */
> - unsigned int fi_extents_mapped; /* Number of mapped extents */
> - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> - struct fiemap_extent __user *fi_extents_start; /* Start of
> - fiemap_extent array */
> -};
> -int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> - u64 phys, u64 len, u32 flags);
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>
> /*
> * This is the "filldir" function type, used by readdir() to let
> @@ -3299,10 +3286,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
> extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
> extern int vfs_readlink(struct dentry *, char __user *, int);
>
> -extern int generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, u64 start,
> - u64 len, get_block_t *get_block);
> -
> extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> extern void put_filesystem(struct file_system_type *fs);
> extern struct file_system_type *get_fs_type(const char *name);
> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
> index 7a900b2377b60..24ca0c00cae36 100644
> --- a/include/uapi/linux/fiemap.h
> +++ b/include/uapi/linux/fiemap.h
> @@ -9,8 +9,8 @@
> * Andreas Dilger <[email protected]>
> */
>
> -#ifndef _LINUX_FIEMAP_H
> -#define _LINUX_FIEMAP_H
> +#ifndef _UAPI_LINUX_FIEMAP_H
> +#define _UAPI_LINUX_FIEMAP_H
>
> #include <linux/types.h>
>
> @@ -67,4 +67,4 @@ struct fiemap {
> #define FIEMAP_EXTENT_SHARED 0x00002000 /* Space shared with other
> * files. */
>
> -#endif /* _LINUX_FIEMAP_H */
> +#endif /* _UAPI_LINUX_FIEMAP_H */
> --
> 2.26.1
>
On Mon, Apr 27, 2020 at 08:19:51PM +0200, Christoph Hellwig wrote:
> There is no caller left outside of ioctl.c.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks good to me,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/ioctl.c | 4 +---
> include/linux/fs.h | 4 ----
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 282d45be6f453..f55f53c7824bb 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -299,8 +299,7 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> * If you use this function directly, you need to do your own locking. Use
> * generic_block_fiemap if you want the locking done for you.
> */
> -
> -int __generic_block_fiemap(struct inode *inode,
> +static int __generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, loff_t start,
> loff_t len, get_block_t *get_block)
> {
> @@ -445,7 +444,6 @@ int __generic_block_fiemap(struct inode *inode,
>
> return ret;
> }
> -EXPORT_SYMBOL(__generic_block_fiemap);
>
> /**
> * generic_block_fiemap - FIEMAP for block based inodes
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4f6f59b4f22a8..3104c6f7527b5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3299,10 +3299,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
> extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
> extern int vfs_readlink(struct dentry *, char __user *, int);
>
> -extern int __generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len,
> - get_block_t *get_block);
> extern int generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, u64 start,
> u64 len, get_block_t *get_block);
> --
> 2.26.1
>
On Mon, Apr 27, 2020 at 08:19:53PM +0200, Christoph Hellwig wrote:
> iomap_fiemap should take u64 start and len arguments, just like the
> ->fiemap prototype.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks ok,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/iomap/fiemap.c | 2 +-
> include/linux/iomap.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index fca3dfb9d964a..dd04e4added15 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -66,7 +66,7 @@ iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
> }
>
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> - loff_t start, loff_t len, const struct iomap_ops *ops)
> + u64 start, u64 len, const struct iomap_ops *ops)
> {
> struct fiemap_ctx ctx;
> loff_t ret;
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 8b09463dae0db..63db02528b702 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -178,7 +178,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
> const struct iomap_ops *ops);
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len, const struct iomap_ops *ops);
> + u64 start, u64 len, const struct iomap_ops *ops);
> loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
> const struct iomap_ops *ops);
> loff_t iomap_seek_data(struct inode *inode, loff_t offset,
> --
> 2.26.1
>
On Mon, Apr 27, 2020 at 08:19:54PM +0200, Christoph Hellwig wrote:
> Replace fiemap_check_flags with a fiemap_validate helpers that also takes
"...with a fiemap_prep helper..." ?
(new to this patchset, confused)
--D
> the inode and mapped range, and performs the sanity check and truncation
> previously done in fiemap_check_range. This way the validation is inside
> the file system itself and thus properly works for the stacked overlayfs
> case as well.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> Documentation/filesystems/fiemap.txt | 8 ++---
> fs/btrfs/inode.c | 2 +-
> fs/cifs/smb2ops.c | 6 ++--
> fs/ext4/extents.c | 5 +--
> fs/f2fs/data.c | 3 +-
> fs/ioctl.c | 53 +++++++++++-----------------
> fs/iomap/fiemap.c | 2 +-
> fs/nilfs2/inode.c | 2 +-
> fs/ocfs2/extent_map.c | 3 +-
> include/linux/fiemap.h | 3 +-
> 10 files changed, 40 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
> index ac87e6fda842b..05926b7f92809 100644
> --- a/Documentation/filesystems/fiemap.txt
> +++ b/Documentation/filesystems/fiemap.txt
> @@ -203,15 +203,13 @@ EINTR once fatal signal received.
>
>
> Flag checking should be done at the beginning of the ->fiemap callback via the
> -fiemap_check_flags() helper:
> -
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
> +fiemap_prep() helper.
>
> The struct fieinfo should be passed in as received from ioctl_fiemap(). The
> set of fiemap flags which the fs understands should be passed via fs_flags. If
> -fiemap_check_flags finds invalid user flags, it will place the bad values in
> +fiemap_prep finds invalid user flags, it will place the bad values in
> fieinfo->fi_flags and return -EBADR. If the file system gets -EBADR, from
> -fiemap_check_flags(), it should immediately exit, returning that error back to
> +fiemap_prep(), it should immediately exit, returning that error back to
> ioctl_fiemap().
>
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 320d1062068d3..1f1ec361089b3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8250,7 +8250,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> {
> int ret;
>
> - ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
> if (ret)
> return ret;
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 09047f1ddfb66..8a2e94931dc96 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3408,8 +3408,10 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> int i, num, rc, flags, last_blob;
> u64 next;
>
> - if (fiemap_check_flags(fei, FIEMAP_FLAG_SYNC))
> - return -EBADR;
> + rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
> + FIEMAP_FLAG_SYNC);
> + if (rc)
> + rc;
>
> xid = get_xid();
> again:
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a41ae7c510170..41f73dea92cac 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4908,8 +4908,9 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC))
> - return -EBADR;
> + error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + if (error)
> + return error;
>
> error = ext4_fiemap_check_ranges(inode, start, &len);
> if (error)
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 25abbbb65ba09..03faafc591b17 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1825,7 +1825,8 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return ret;
> }
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> + ret = fiemap_prep(inode, fieinfo, start, &len,
> + FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> if (ret)
> return ret;
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index cbc84e23d00bd..4d94c20c9596b 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -141,9 +141,12 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> EXPORT_SYMBOL(fiemap_fill_next_extent);
>
> /**
> - * fiemap_check_flags - check validity of requested flags for fiemap
> + * fiemap_prep - check validity of requested flags for fiemap
> + * @inode: Inode to operate on
> * @fieinfo: Fiemap context passed into ->fiemap
> - * @fs_flags: Set of fiemap flags that the file system understands
> + * @start: Start of the mapped range
> + * @len: Length of the mapped range, can be truncated by this function.
> + * @supported_flags: Set of fiemap flags that the file system understands
> *
> * Called from file system ->fiemap callback. This will compute the
> * intersection of valid fiemap flags and those that the fs supports. That
> @@ -154,48 +157,38 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
> *
> * Returns 0 on success, -EBADR on bad flags.
> */
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
> +int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> + u64 start, u64 *len, u32 supported_flags)
> {
> + u64 maxbytes = inode->i_sb->s_maxbytes;
> u32 incompat_flags;
>
> - incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
> - if (incompat_flags) {
> - fieinfo->fi_flags = incompat_flags;
> - return -EBADR;
> - }
> - return 0;
> -}
> -EXPORT_SYMBOL(fiemap_check_flags);
> -
> -static int fiemap_check_ranges(struct super_block *sb,
> - u64 start, u64 len, u64 *new_len)
> -{
> - u64 maxbytes = (u64) sb->s_maxbytes;
> -
> - *new_len = len;
> -
> - if (len == 0)
> + if (*len == 0)
> return -EINVAL;
> -
> if (start > maxbytes)
> return -EFBIG;
>
> /*
> * Shrink request scope to what the fs can actually handle.
> */
> - if (len > maxbytes || (maxbytes - len) < start)
> - *new_len = maxbytes - start;
> + if (*len > maxbytes || (maxbytes - *len) < start)
> + *len = maxbytes - start;
>
> + supported_flags &= FIEMAP_FLAGS_COMPAT;
> + incompat_flags = fieinfo->fi_flags & ~supported_flags;
> + if (incompat_flags) {
> + fieinfo->fi_flags = incompat_flags;
> + return -EBADR;
> + }
> return 0;
> }
> +EXPORT_SYMBOL(fiemap_prep);
>
> static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> {
> struct fiemap fiemap;
> struct fiemap_extent_info fieinfo = { 0, };
> struct inode *inode = file_inode(filp);
> - struct super_block *sb = inode->i_sb;
> - u64 len;
> int error;
>
> if (!inode->i_op->fiemap)
> @@ -207,11 +200,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> return -EINVAL;
>
> - error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> - &len);
> - if (error)
> - return error;
> -
> fieinfo.fi_flags = fiemap.fm_flags;
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
> @@ -224,7 +212,8 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(inode->i_mapping);
>
> - error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
> + error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> + fiemap.fm_length);
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> @@ -312,7 +301,7 @@ static int __generic_block_fiemap(struct inode *inode,
> bool past_eof = false, whole_file = false;
> int ret = 0;
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index dd04e4added15..5e4e3520424da 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -75,7 +75,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> ctx.fi = fi;
> ctx.prev.type = IOMAP_HOLE;
>
> - ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 6e1aca38931f3..052c2da11e4d7 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> unsigned int blkbits = inode->i_blkbits;
> int ret, n;
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index e3e2d1b2af51a..3744179b73fa1 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -746,7 +746,8 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
>
> - ret = fiemap_check_flags(fieinfo, OCFS2_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
> + OCFS2_FIEMAP_FLAGS);
> if (ret)
> return ret;
>
> diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
> index 240d4f7d9116a..4e624c4665837 100644
> --- a/include/linux/fiemap.h
> +++ b/include/linux/fiemap.h
> @@ -13,9 +13,10 @@ struct fiemap_extent_info {
> fiemap_extent array */
> };
>
> +int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> + u64 start, u64 *len, u32 supported_flags);
> int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> u64 phys, u64 len, u32 flags);
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>
> int generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, u64 start, u64 len,
> --
> 2.26.1
>
On Mon, Apr 27, 2020 at 08:19:55PM +0200, Christoph Hellwig wrote:
> By moving FIEMAP_FLAG_SYNC handling to fiemap_prep we ensure it is
> handled once instead of duplicated, but can still be done under fs locks,
> like xfs/iomap intended with its duplicate handling. Also make sure the
> error value of filemap_write_and_wait is propagated to user space.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Seems reasonable,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/btrfs/inode.c | 4 +---
> fs/cifs/smb2ops.c | 3 +--
> fs/ext4/extents.c | 2 +-
> fs/f2fs/data.c | 3 +--
> fs/ioctl.c | 10 ++++++----
> fs/iomap/fiemap.c | 8 +-------
> fs/nilfs2/inode.c | 2 +-
> fs/ocfs2/extent_map.c | 5 +----
> fs/overlayfs/inode.c | 4 ----
> 9 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 1f1ec361089b3..529ffa5e7b452 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8243,14 +8243,12 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> return ret;
> }
>
> -#define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> -
> static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> __u64 start, __u64 len)
> {
> int ret;
>
> - ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 8a2e94931dc96..32880fca6d8d8 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3408,8 +3408,7 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> int i, num, rc, flags, last_blob;
> u64 next;
>
> - rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
> - FIEMAP_FLAG_SYNC);
> + rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len, 0);
> if (rc)
> rc;
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 41f73dea92cac..93574e88f6543 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4908,7 +4908,7 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + error = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (error)
> return error;
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 03faafc591b17..9de7dc476ed16 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1825,8 +1825,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return ret;
> }
>
> - ret = fiemap_prep(inode, fieinfo, start, &len,
> - FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_XATTR);
> if (ret)
> return ret;
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 4d94c20c9596b..ae0d228d18a16 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -162,6 +162,7 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> {
> u64 maxbytes = inode->i_sb->s_maxbytes;
> u32 incompat_flags;
> + int ret = 0;
>
> if (*len == 0)
> return -EINVAL;
> @@ -174,13 +175,17 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> if (*len > maxbytes || (maxbytes - *len) < start)
> *len = maxbytes - start;
>
> + supported_flags |= FIEMAP_FLAG_SYNC;
> supported_flags &= FIEMAP_FLAGS_COMPAT;
> incompat_flags = fieinfo->fi_flags & ~supported_flags;
> if (incompat_flags) {
> fieinfo->fi_flags = incompat_flags;
> return -EBADR;
> }
> - return 0;
> +
> + if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> + ret = filemap_write_and_wait(inode->i_mapping);
> + return ret;
> }
> EXPORT_SYMBOL(fiemap_prep);
>
> @@ -209,9 +214,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> return -EFAULT;
>
> - if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> - filemap_write_and_wait(inode->i_mapping);
> -
> error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> fiemap.fm_length);
> fiemap.fm_flags = fieinfo.fi_flags;
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index 5e4e3520424da..fffd9eedfd880 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -75,16 +75,10 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> ctx.fi = fi;
> ctx.prev.type = IOMAP_HOLE;
>
> - ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fi, start, &len, 0);
> if (ret)
> return ret;
>
> - if (fi->fi_flags & FIEMAP_FLAG_SYNC) {
> - ret = filemap_write_and_wait(inode->i_mapping);
> - if (ret)
> - return ret;
> - }
> -
> while (len > 0) {
> ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
> iomap_fiemap_actor);
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 052c2da11e4d7..25b0d368ecdb2 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> unsigned int blkbits = inode->i_blkbits;
> int ret, n;
>
> - ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index 3744179b73fa1..a94852af5510d 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -733,8 +733,6 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
> return 0;
> }
>
> -#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> -
> int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> u64 map_start, u64 map_len)
> {
> @@ -746,8 +744,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
>
> - ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
> - OCFS2_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, map_start, &map_len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b5fec34105569..c7cb883c47b86 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -462,10 +462,6 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return -EOPNOTSUPP;
>
> old_cred = ovl_override_creds(inode->i_sb);
> -
> - if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> - filemap_write_and_wait(realinode->i_mapping);
> -
> err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
> revert_creds(old_cred);
>
> --
> 2.26.1
>
On Mon, Apr 27, 2020 at 08:19:56PM +0200, Christoph Hellwig wrote:
> access_ok just checks we are fed a proper user pointer. We also do that
> in copy_to_user itself, so no need to do this early.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Hmm. It's a minor behavioral change that we no longer require the
entire extent array to be accessible at the start even if parts of it
would never have gotten accessed, but I don't think that matters, so:
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/ioctl.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index ae0d228d18a16..d24afce649037 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -209,13 +209,9 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
>
> - if (fiemap.fm_extent_count != 0 &&
> - !access_ok(fieinfo.fi_extents_start,
> - fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> - return -EFAULT;
> -
> error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> fiemap.fm_length);
> +
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> --
> 2.26.1
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> Replace fiemap_check_flags with a fiemap_validate helpers that also takes
> the inode and mapped range, and performs the sanity check and truncation
> previously done in fiemap_check_range. This way the validation is inside
> the file system itself and thus properly works for the stacked overlayfs
> case as well.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> Documentation/filesystems/fiemap.txt | 8 ++---
> fs/btrfs/inode.c | 2 +-
> fs/cifs/smb2ops.c | 6 ++--
> fs/ext4/extents.c | 5 +--
> fs/f2fs/data.c | 3 +-
> fs/ioctl.c | 53 +++++++++++-----------------
> fs/iomap/fiemap.c | 2 +-
> fs/nilfs2/inode.c | 2 +-
> fs/ocfs2/extent_map.c | 3 +-
> include/linux/fiemap.h | 3 +-
> 10 files changed, 40 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
> index ac87e6fda842b..05926b7f92809 100644
> --- a/Documentation/filesystems/fiemap.txt
> +++ b/Documentation/filesystems/fiemap.txt
> @@ -203,15 +203,13 @@ EINTR once fatal signal received.
>
>
> Flag checking should be done at the beginning of the ->fiemap callback via the
> -fiemap_check_flags() helper:
> -
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
> +fiemap_prep() helper.
Let's add the fiemap_prep() function prototype here.
So that below description would make more sense.
>
> The struct fieinfo should be passed in as received from ioctl_fiemap(). The
> set of fiemap flags which the fs understands should be passed via fs_flags. If
> -fiemap_check_flags finds invalid user flags, it will place the bad values in
> +fiemap_prep finds invalid user flags, it will place the bad values in
> fieinfo->fi_flags and return -EBADR. If the file system gets -EBADR, from
> -fiemap_check_flags(), it should immediately exit, returning that error back to
> +fiemap_prep(), it should immediately exit, returning that error back to
> ioctl_fiemap().
Also maybe we should also add more info about fiemap_prep() helper.
Since it also checks for invalid len and invalid start range and hence
could return -EINVAL or -EFBIG.
>
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 320d1062068d3..1f1ec361089b3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8250,7 +8250,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> {
> int ret;
>
> - ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
> if (ret)
> return ret;
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 09047f1ddfb66..8a2e94931dc96 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3408,8 +3408,10 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> int i, num, rc, flags, last_blob;
> u64 next;
>
> - if (fiemap_check_flags(fei, FIEMAP_FLAG_SYNC))
> - return -EBADR;
> + rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
> + FIEMAP_FLAG_SYNC);
How about d_inode(cfile->dentry) ?
> + if (rc)
> + rc;
missed "return rc" here?
>
> xid = get_xid();
> again:
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a41ae7c510170..41f73dea92cac 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4908,8 +4908,9 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC))
> - return -EBADR;
> + error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + if (error)
> + return error;
>
> error = ext4_fiemap_check_ranges(inode, start, &len);
> if (error)
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 25abbbb65ba09..03faafc591b17 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1825,7 +1825,8 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return ret;
> }
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> + ret = fiemap_prep(inode, fieinfo, start, &len,
> + FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> if (ret)
> return ret;
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index cbc84e23d00bd..4d94c20c9596b 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -141,9 +141,12 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
> EXPORT_SYMBOL(fiemap_fill_next_extent);
>
> /**
> - * fiemap_check_flags - check validity of requested flags for fiemap
> + * fiemap_prep - check validity of requested flags for fiemap
> + * @inode: Inode to operate on
> * @fieinfo: Fiemap context passed into ->fiemap
> - * @fs_flags: Set of fiemap flags that the file system understands
> + * @start: Start of the mapped range
> + * @len: Length of the mapped range, can be truncated by this function.
> + * @supported_flags: Set of fiemap flags that the file system understands
> *
> * Called from file system ->fiemap callback. This will compute the
> * intersection of valid fiemap flags and those that the fs supports. That
Let's also add more documentation about this new fiemap_prep() helper.
Earlier comments above this function description only talks about
incompat flags. We should add more info about check_ranges part as well
here.
> @@ -154,48 +157,38 @@ EXPORT_SYMBOL(fiemap_fill_next_extent);
> *
> * Returns 0 on success, -EBADR on bad flags.
> */
Also let's add ", -EINVAL or -EFBIG on invalid len or start range
respectively"
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
> +int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> + u64 start, u64 *len, u32 supported_flags)
> {
> + u64 maxbytes = inode->i_sb->s_maxbytes;
> u32 incompat_flags;
>
> - incompat_flags = fieinfo->fi_flags & ~(FIEMAP_FLAGS_COMPAT & fs_flags);
> - if (incompat_flags) {
> - fieinfo->fi_flags = incompat_flags;
> - return -EBADR;
> - }
> - return 0;
> -}
> -EXPORT_SYMBOL(fiemap_check_flags);
> -
> -static int fiemap_check_ranges(struct super_block *sb,
> - u64 start, u64 len, u64 *new_len)
> -{
> - u64 maxbytes = (u64) sb->s_maxbytes;
> -
> - *new_len = len;
> -
> - if (len == 0)
> + if (*len == 0)
> return -EINVAL;
> -
> if (start > maxbytes)
> return -EFBIG;
>
> /*
> * Shrink request scope to what the fs can actually handle.
> */
> - if (len > maxbytes || (maxbytes - len) < start)
> - *new_len = maxbytes - start;
> + if (*len > maxbytes || (maxbytes - *len) < start)
> + *len = maxbytes - start;
>
> + supported_flags &= FIEMAP_FLAGS_COMPAT;
> + incompat_flags = fieinfo->fi_flags & ~supported_flags;
> + if (incompat_flags) {
> + fieinfo->fi_flags = incompat_flags;
> + return -EBADR;
> + }
> return 0;
> }
> +EXPORT_SYMBOL(fiemap_prep);
>
> static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> {
> struct fiemap fiemap;
> struct fiemap_extent_info fieinfo = { 0, };
> struct inode *inode = file_inode(filp);
> - struct super_block *sb = inode->i_sb;
> - u64 len;
> int error;
>
> if (!inode->i_op->fiemap)
> @@ -207,11 +200,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> return -EINVAL;
>
> - error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> - &len);
> - if (error)
> - return error;
> -
> fieinfo.fi_flags = fiemap.fm_flags;
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
> @@ -224,7 +212,8 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(inode->i_mapping);
>
> - error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
> + error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> + fiemap.fm_length);
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
> @@ -312,7 +301,7 @@ static int __generic_block_fiemap(struct inode *inode,
> bool past_eof = false, whole_file = false;
> int ret = 0;
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index dd04e4added15..5e4e3520424da 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -75,7 +75,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> ctx.fi = fi;
> ctx.prev.type = IOMAP_HOLE;
>
> - ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 6e1aca38931f3..052c2da11e4d7 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> unsigned int blkbits = inode->i_blkbits;
> int ret, n;
>
> - ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> if (ret)
> return ret;
>
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index e3e2d1b2af51a..3744179b73fa1 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -746,7 +746,8 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
>
> - ret = fiemap_check_flags(fieinfo, OCFS2_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
> + OCFS2_FIEMAP_FLAGS);
> if (ret)
> return ret;
>
> diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
> index 240d4f7d9116a..4e624c4665837 100644
> --- a/include/linux/fiemap.h
> +++ b/include/linux/fiemap.h
> @@ -13,9 +13,10 @@ struct fiemap_extent_info {
> fiemap_extent array */
> };
>
> +int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> + u64 start, u64 *len, u32 supported_flags);
> int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> u64 phys, u64 len, u32 flags);
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>
While we are at it, could remove this above additional line as well.
> int generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, u64 start, u64 len,
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> By moving FIEMAP_FLAG_SYNC handling to fiemap_prep we ensure it is
> handled once instead of duplicated, but can still be done under fs locks,
> like xfs/iomap intended with its duplicate handling. Also make sure the
> error value of filemap_write_and_wait is propagated to user space.
Forgot to remove filemap_write_and_wait() from
ext4_ioctl_get_es_cache() ?
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/btrfs/inode.c | 4 +---
> fs/cifs/smb2ops.c | 3 +--
> fs/ext4/extents.c | 2 +-
> fs/f2fs/data.c | 3 +--
> fs/ioctl.c | 10 ++++++----
> fs/iomap/fiemap.c | 8 +-------
> fs/nilfs2/inode.c | 2 +-
> fs/ocfs2/extent_map.c | 5 +----
> fs/overlayfs/inode.c | 4 ----
> 9 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 1f1ec361089b3..529ffa5e7b452 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8243,14 +8243,12 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> return ret;
> }
>
> -#define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> -
> static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> __u64 start, __u64 len)
> {
> int ret;
>
> - ret = fiemap_prep(inode, fieinfo, start, &len, BTRFS_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 8a2e94931dc96..32880fca6d8d8 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3408,8 +3408,7 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> int i, num, rc, flags, last_blob;
> u64 next;
>
> - rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
> - FIEMAP_FLAG_SYNC);
> + rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len, 0);
> if (rc)
> rc;
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 41f73dea92cac..93574e88f6543 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4908,7 +4908,7 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - error = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + error = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (error)
> return error;
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 03faafc591b17..9de7dc476ed16 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1825,8 +1825,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return ret;
> }
>
> - ret = fiemap_prep(inode, fieinfo, start, &len,
> - FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
> + ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_XATTR);
> if (ret)
> return ret;
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 4d94c20c9596b..ae0d228d18a16 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -162,6 +162,7 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> {
> u64 maxbytes = inode->i_sb->s_maxbytes;
> u32 incompat_flags;
> + int ret = 0;
>
> if (*len == 0)
> return -EINVAL;
> @@ -174,13 +175,17 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
> if (*len > maxbytes || (maxbytes - *len) < start)
> *len = maxbytes - start;
>
> + supported_flags |= FIEMAP_FLAG_SYNC;
> supported_flags &= FIEMAP_FLAGS_COMPAT;
> incompat_flags = fieinfo->fi_flags & ~supported_flags;
> if (incompat_flags) {
> fieinfo->fi_flags = incompat_flags;
> return -EBADR;
> }
> - return 0;
> +
> + if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> + ret = filemap_write_and_wait(inode->i_mapping);
> + return ret;
Since we could return an error from here.
I think we should again update the function description that
it could return a negative error value if filemap_write_and_wait()
fails. And also should document this in Documentation/*
> }
> EXPORT_SYMBOL(fiemap_prep);
>
> @@ -209,9 +214,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> return -EFAULT;
>
> - if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> - filemap_write_and_wait(inode->i_mapping);
> -
> error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> fiemap.fm_length);
> fiemap.fm_flags = fieinfo.fi_flags;
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index 5e4e3520424da..fffd9eedfd880 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -75,16 +75,10 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> ctx.fi = fi;
> ctx.prev.type = IOMAP_HOLE;
>
> - ret = fiemap_prep(inode, fi, start, &len, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fi, start, &len, 0);
> if (ret)
> return ret;
>
> - if (fi->fi_flags & FIEMAP_FLAG_SYNC) {
> - ret = filemap_write_and_wait(inode->i_mapping);
> - if (ret)
> - return ret;
> - }
> -
> while (len > 0) {
> ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
> iomap_fiemap_actor);
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 052c2da11e4d7..25b0d368ecdb2 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -1006,7 +1006,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> unsigned int blkbits = inode->i_blkbits;
> int ret, n;
>
> - ret = fiemap_prep(inode, fieinfo, start, &len, FIEMAP_FLAG_SYNC);
> + ret = fiemap_prep(inode, fieinfo, start, &len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
> index 3744179b73fa1..a94852af5510d 100644
> --- a/fs/ocfs2/extent_map.c
> +++ b/fs/ocfs2/extent_map.c
> @@ -733,8 +733,6 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
> return 0;
> }
>
> -#define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC)
> -
> int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> u64 map_start, u64 map_len)
> {
> @@ -746,8 +744,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> struct buffer_head *di_bh = NULL;
> struct ocfs2_extent_rec rec;
>
> - ret = fiemap_prep(inode, fieinfo, map_start, &map_len,
> - OCFS2_FIEMAP_FLAGS);
> + ret = fiemap_prep(inode, fieinfo, map_start, &map_len, 0);
> if (ret)
> return ret;
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b5fec34105569..c7cb883c47b86 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -462,10 +462,6 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return -EOPNOTSUPP;
>
> old_cred = ovl_override_creds(inode->i_sb);
> -
> - if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
> - filemap_write_and_wait(realinode->i_mapping);
> -
> err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
> revert_creds(old_cred);
>
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> access_ok just checks we are fed a proper user pointer. We also do that
> in copy_to_user itself, so no need to do this early.
Ok.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ioctl.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index ae0d228d18a16..d24afce649037 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -209,13 +209,9 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
>
> - if (fiemap.fm_extent_count != 0 &&
> - !access_ok(fieinfo.fi_extents_start,
> - fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> - return -EFAULT;
> -
> error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start,
> fiemap.fm_length);
> +
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> access_ok just checks we are fed a proper user pointer. We also do that
> in copy_to_user itself, so no need to do this early.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ext4/ioctl.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0746532ba463d..7fded54d2dba9 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -754,11 +754,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
>
> - if (fiemap.fm_extent_count != 0 &&
> - !access_ok(fieinfo.fi_extents_start,
> - fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
> - return -EFAULT;
> -
> if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(inode->i_mapping);
>
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> Add an extra validation of the len parameter, as for ext4 some files
> might have smaller file size limits than others. This also means the
> redundant size check in ext4_ioctl_get_es_cache can go away, as all
> size checking is done in the shared fiemap handler.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ext4/extents.c | 31 +++++++++++++++++++++++++++++++
> fs/ext4/ioctl.c | 33 ++-------------------------------
> 2 files changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index f2b577b315a09..2b4b94542e34d 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4832,6 +4832,28 @@ static const struct iomap_ops ext4_iomap_xattr_ops = {
> .iomap_begin = ext4_iomap_xattr_begin,
> };
>
> +static int ext4_fiemap_check_ranges(struct inode *inode, u64 start, u64 *len)
> +{
> + u64 maxbytes;
> +
> + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> + maxbytes = inode->i_sb->s_maxbytes;
> + else
> + maxbytes = EXT4_SB(inode->i_sb)->s_bitmap_maxbytes;
> +
> + if (*len == 0)
> + return -EINVAL;
> + if (start > maxbytes)
> + return -EFBIG;
> +
> + /*
> + * Shrink request scope to what the fs can actually handle.
> + */
> + if (*len > maxbytes || (maxbytes - *len) < start)
> + *len = maxbytes - start;
> + return 0;
> +}
> +
> static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> __u64 start, __u64 len, bool from_es_cache)
> {
> @@ -4852,6 +4874,15 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> if (fiemap_check_flags(fieinfo, ext4_fiemap_flags))
> return -EBADR;
>
> + /*
> + * For bitmap files the maximum size limit could be smaller than
> + * s_maxbytes, so check len here manually instead of just relying on the
> + * generic check.
> + */
> + error = ext4_fiemap_check_ranges(inode, start, &len);
> + if (error)
> + return error;
> +
> if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
> error = iomap_fiemap(inode, fieinfo, start, len,
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index bfc1281fc4cbc..0746532ba463d 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -733,29 +733,6 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
> fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
> }
>
> -/* copied from fs/ioctl.c */
> -static int fiemap_check_ranges(struct super_block *sb,
> - u64 start, u64 len, u64 *new_len)
> -{
> - u64 maxbytes = (u64) sb->s_maxbytes;
> -
> - *new_len = len;
> -
> - if (len == 0)
> - return -EINVAL;
> -
> - if (start > maxbytes)
> - return -EFBIG;
> -
> - /*
> - * Shrink request scope to what the fs can actually handle.
> - */
> - if (len > maxbytes || (maxbytes - len) < start)
> - *new_len = maxbytes - start;
> -
> - return 0;
> -}
> -
> /* So that the fiemap access checks can't overflow on 32 bit machines. */
> #define FIEMAP_MAX_EXTENTS (UINT_MAX / sizeof(struct fiemap_extent))
>
> @@ -765,8 +742,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
> struct fiemap __user *ufiemap = (struct fiemap __user *) arg;
> struct fiemap_extent_info fieinfo = { 0, };
> struct inode *inode = file_inode(filp);
> - struct super_block *sb = inode->i_sb;
> - u64 len;
> int error;
>
> if (copy_from_user(&fiemap, ufiemap, sizeof(fiemap)))
> @@ -775,11 +750,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
> if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
> return -EINVAL;
>
> - error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> - &len);
> - if (error)
> - return error;
> -
> fieinfo.fi_flags = fiemap.fm_flags;
> fieinfo.fi_extents_max = fiemap.fm_extent_count;
> fieinfo.fi_extents_start = ufiemap->fm_extents;
> @@ -792,7 +762,8 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
> if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
> filemap_write_and_wait(inode->i_mapping);
>
> - error = ext4_get_es_cache(inode, &fieinfo, fiemap.fm_start, len);
> + error = ext4_get_es_cache(inode, &fieinfo, fiemap.fm_start,
> + fiemap.fm_length);
> fiemap.fm_flags = fieinfo.fi_flags;
> fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
> if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> The fiemap and EXT4_IOC_GET_ES_CACHE cases share almost no code, so split
> them into entirely separate functions.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ext4/extents.c | 72 +++++++++++++++++++++++------------------------
> 1 file changed, 35 insertions(+), 37 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 2b4b94542e34d..d2a2a3ba5c44a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4854,11 +4854,9 @@ static int ext4_fiemap_check_ranges(struct inode *inode, u64 start, u64 *len)
> return 0;
> }
>
> -static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len, bool from_es_cache)
> +int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> + u64 start, u64 len)
> {
> - ext4_lblk_t start_blk;
> - u32 ext4_fiemap_flags = FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR;
> int error = 0;
>
> if (fieinfo->fi_flags & FIEMAP_FLAG_CACHE) {
> @@ -4868,10 +4866,7 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - if (from_es_cache)
> - ext4_fiemap_flags &= FIEMAP_FLAG_XATTR;
> -
> - if (fiemap_check_flags(fieinfo, ext4_fiemap_flags))
> + if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR))
> return -EBADR;
>
> /*
> @@ -4885,40 +4880,20 @@ static int _ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
> if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
> - error = iomap_fiemap(inode, fieinfo, start, len,
> - &ext4_iomap_xattr_ops);
> - } else if (!from_es_cache) {
> - error = iomap_fiemap(inode, fieinfo, start, len,
> - &ext4_iomap_report_ops);
> - } else {
> - ext4_lblk_t len_blks;
> - __u64 last_blk;
> -
> - start_blk = start >> inode->i_sb->s_blocksize_bits;
> - last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
> - if (last_blk >= EXT_MAX_BLOCKS)
> - last_blk = EXT_MAX_BLOCKS-1;
> - len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
> -
> - /*
> - * Walk the extent tree gathering extent information
> - * and pushing extents back to the user.
> - */
> - error = ext4_fill_es_cache_info(inode, start_blk, len_blks,
> - fieinfo);
> + return iomap_fiemap(inode, fieinfo, start, len,
> + &ext4_iomap_xattr_ops);
> }
> - return error;
> -}
>
> -int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - __u64 start, __u64 len)
> -{
> - return _ext4_fiemap(inode, fieinfo, start, len, false);
> + return iomap_fiemap(inode, fieinfo, start, len, &ext4_iomap_report_ops);
> }
>
> int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> __u64 start, __u64 len)
> {
> + ext4_lblk_t start_blk, len_blks;
> + __u64 last_blk;
> + int error = 0;
> +
> if (ext4_has_inline_data(inode)) {
> int has_inline;
>
> @@ -4929,9 +4904,32 @@ int ext4_get_es_cache(struct inode *inode, struct fiemap_extent_info *fieinfo,
> return 0;
> }
>
> - return _ext4_fiemap(inode, fieinfo, start, len, true);
> -}
> + if (fieinfo->fi_flags & FIEMAP_FLAG_CACHE) {
> + error = ext4_ext_precache(inode);
> + if (error)
> + return error;
> + fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> + }
> +
> + if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC))
> + return -EBADR;
>
> + error = ext4_fiemap_check_ranges(inode, start, &len);
> + if (error)
> + return error;
> +
> + start_blk = start >> inode->i_sb->s_blocksize_bits;
> + last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
> + if (last_blk >= EXT_MAX_BLOCKS)
> + last_blk = EXT_MAX_BLOCKS-1;
> + len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
> +
> + /*
> + * Walk the extent tree gathering extent information
> + * and pushing extents back to the user.
> + */
> + return ext4_fill_es_cache_info(inode, start_blk, len_blks, fieinfo);
> +}
>
> /*
> * ext4_access_path:
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> iomap_fiemap already calls fiemap_check_flags first thing, so this
> additional check is redundant.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ext4/extents.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d2a2a3ba5c44a..a41ae7c510170 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4866,9 +4866,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
> }
>
> - if (fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR))
> - return -EBADR;
> -
> /*
> * For bitmap files the maximum size limit could be smaller than
> * s_maxbytes, so check len here manually instead of just relying on the
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> There is no caller left outside of ioctl.c.
Looks fine. Feel free to add
Reviewed-by: Ritesh Harjani <[email protected]>
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/ioctl.c | 4 +---
> include/linux/fs.h | 4 ----
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 282d45be6f453..f55f53c7824bb 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -299,8 +299,7 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> * If you use this function directly, you need to do your own locking. Use
> * generic_block_fiemap if you want the locking done for you.
> */
> -
> -int __generic_block_fiemap(struct inode *inode,
> +static int __generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, loff_t start,
> loff_t len, get_block_t *get_block)
> {
> @@ -445,7 +444,6 @@ int __generic_block_fiemap(struct inode *inode,
>
> return ret;
> }
> -EXPORT_SYMBOL(__generic_block_fiemap);
>
> /**
> * generic_block_fiemap - FIEMAP for block based inodes
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4f6f59b4f22a8..3104c6f7527b5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3299,10 +3299,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
> extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
> extern int vfs_readlink(struct dentry *, char __user *, int);
>
> -extern int __generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len,
> - get_block_t *get_block);
> extern int generic_block_fiemap(struct inode *inode,
> struct fiemap_extent_info *fieinfo, u64 start,
> u64 len, get_block_t *get_block);
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> No need to pull the fiemap definitions into almost every file in the
> kernel build.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Nice,
Feel free to add:
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/bad_inode.c | 1 +
> fs/btrfs/extent_io.h | 1 +
> fs/cifs/inode.c | 1 +
> fs/cifs/smb2ops.c | 1 +
> fs/ext2/inode.c | 1 +
> fs/ext4/ext4.h | 1 +
> fs/f2fs/data.c | 1 +
> fs/f2fs/inline.c | 1 +
> fs/gfs2/inode.c | 1 +
> fs/hpfs/file.c | 1 +
> fs/ioctl.c | 1 +
> fs/iomap/fiemap.c | 1 +
> fs/nilfs2/inode.c | 1 +
> fs/overlayfs/inode.c | 1 +
> fs/xfs/xfs_iops.c | 1 +
> include/linux/fiemap.h | 24 ++++++++++++++++++++++++
> include/linux/fs.h | 19 +------------------
> include/uapi/linux/fiemap.h | 6 +++---
> 18 files changed, 43 insertions(+), 21 deletions(-)
> create mode 100644 include/linux/fiemap.h
>
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index 8035d2a445617..54f0ce4442720 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -15,6 +15,7 @@
> #include <linux/time.h>
> #include <linux/namei.h>
> #include <linux/poll.h>
> +#include <linux/fiemap.h>
>
> static int bad_file_open(struct inode *inode, struct file *filp)
> {
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 2ed65bd0760ea..817698bc06693 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -5,6 +5,7 @@
>
> #include <linux/rbtree.h>
> #include <linux/refcount.h>
> +#include <linux/fiemap.h>
> #include "ulist.h"
>
> /*
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 390d2b15ef6ef..3f276eb8ca68d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -25,6 +25,7 @@
> #include <linux/freezer.h>
> #include <linux/sched/signal.h>
> #include <linux/wait_bit.h>
> +#include <linux/fiemap.h>
>
> #include <asm/div64.h>
> #include "cifsfs.h"
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index f829f4165d38c..09047f1ddfb66 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -12,6 +12,7 @@
> #include <linux/uuid.h>
> #include <linux/sort.h>
> #include <crypto/aead.h>
> +#include <linux/fiemap.h>
> #include "cifsfs.h"
> #include "cifsglob.h"
> #include "smb2pdu.h"
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index c885cf7d724b4..0f12a0e8a8d97 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -36,6 +36,7 @@
> #include <linux/iomap.h>
> #include <linux/namei.h>
> #include <linux/uio.h>
> +#include <linux/fiemap.h>
> #include "ext2.h"
> #include "acl.h"
> #include "xattr.h"
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index ad2dbf6e49245..06f97a3a943f6 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -36,6 +36,7 @@
> #include <crypto/hash.h>
> #include <linux/falloc.h>
> #include <linux/percpu-rwsem.h>
> +#include <linux/fiemap.h>
> #ifdef __KERNEL__
> #include <linux/compat.h>
> #endif
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cdf2f626bea7a..25abbbb65ba09 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -19,6 +19,7 @@
> #include <linux/uio.h>
> #include <linux/cleancache.h>
> #include <linux/sched/signal.h>
> +#include <linux/fiemap.h>
>
> #include "f2fs.h"
> #include "node.h"
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 4167e54081518..9686ffea177e7 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -8,6 +8,7 @@
>
> #include <linux/fs.h>
> #include <linux/f2fs_fs.h>
> +#include <linux/fiemap.h>
>
> #include "f2fs.h"
> #include "node.h"
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 70b2d3a1e8668..4842f313a8084 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -17,6 +17,7 @@
> #include <linux/crc32.h>
> #include <linux/iomap.h>
> #include <linux/security.h>
> +#include <linux/fiemap.h>
> #include <linux/uaccess.h>
>
> #include "gfs2.h"
> diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
> index b36abf9cb345a..62959a8e43ad8 100644
> --- a/fs/hpfs/file.c
> +++ b/fs/hpfs/file.c
> @@ -9,6 +9,7 @@
>
> #include "hpfs_fn.h"
> #include <linux/mpage.h>
> +#include <linux/fiemap.h>
>
> #define BLOCKS(size) (((size) + 511) >> 9)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f55f53c7824bb..cbc84e23d00bd 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -18,6 +18,7 @@
> #include <linux/buffer_head.h>
> #include <linux/falloc.h>
> #include <linux/sched/signal.h>
> +#include <linux/fiemap.h>
>
> #include "internal.h"
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce2..fca3dfb9d964a 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -6,6 +6,7 @@
> #include <linux/compiler.h>
> #include <linux/fs.h>
> #include <linux/iomap.h>
> +#include <linux/fiemap.h>
>
> struct fiemap_ctx {
> struct fiemap_extent_info *fi;
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 671085512e0fd..6e1aca38931f3 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -14,6 +14,7 @@
> #include <linux/pagemap.h>
> #include <linux/writeback.h>
> #include <linux/uio.h>
> +#include <linux/fiemap.h>
> #include "nilfs.h"
> #include "btnode.h"
> #include "segment.h"
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b0d42ece4d7cc..b5fec34105569 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -10,6 +10,7 @@
> #include <linux/xattr.h>
> #include <linux/posix_acl.h>
> #include <linux/ratelimit.h>
> +#include <linux/fiemap.h>
> #include "overlayfs.h"
>
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f7a99b3bbcf7a..44c353998ac5c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -25,6 +25,7 @@
> #include <linux/posix_acl.h>
> #include <linux/security.h>
> #include <linux/iversion.h>
> +#include <linux/fiemap.h>
>
> /*
> * Directories have different lock order w.r.t. mmap_sem compared to regular
> diff --git a/include/linux/fiemap.h b/include/linux/fiemap.h
> new file mode 100644
> index 0000000000000..240d4f7d9116a
> --- /dev/null
> +++ b/include/linux/fiemap.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_FIEMAP_H
> +#define _LINUX_FIEMAP_H 1
> +
> +#include <uapi/linux/fiemap.h>
> +#include <linux/fs.h>
> +
> +struct fiemap_extent_info {
> + unsigned int fi_flags; /* Flags as passed from user */
> + unsigned int fi_extents_mapped; /* Number of mapped extents */
> + unsigned int fi_extents_max; /* Size of fiemap_extent array */
> + struct fiemap_extent __user *fi_extents_start; /* Start of
> + fiemap_extent array */
> +};
> +
> +int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> + u64 phys, u64 len, u32 flags);
> +int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
> +
> +int generic_block_fiemap(struct inode *inode,
> + struct fiemap_extent_info *fieinfo, u64 start, u64 len,
> + get_block_t *get_block);
> +
> +#endif /* _LINUX_FIEMAP_H 1 */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3104c6f7527b5..09bcd329c0628 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -24,7 +24,6 @@
> #include <linux/capability.h>
> #include <linux/semaphore.h>
> #include <linux/fcntl.h>
> -#include <linux/fiemap.h>
> #include <linux/rculist_bl.h>
> #include <linux/atomic.h>
> #include <linux/shrinker.h>
> @@ -48,6 +47,7 @@ struct backing_dev_info;
> struct bdi_writeback;
> struct bio;
> struct export_operations;
> +struct fiemap_extent_info;
> struct hd_geometry;
> struct iovec;
> struct kiocb;
> @@ -1745,19 +1745,6 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
> extern void inode_init_owner(struct inode *inode, const struct inode *dir,
> umode_t mode);
> extern bool may_open_dev(const struct path *path);
> -/*
> - * VFS FS_IOC_FIEMAP helper definitions.
> - */
> -struct fiemap_extent_info {
> - unsigned int fi_flags; /* Flags as passed from user */
> - unsigned int fi_extents_mapped; /* Number of mapped extents */
> - unsigned int fi_extents_max; /* Size of fiemap_extent array */
> - struct fiemap_extent __user *fi_extents_start; /* Start of
> - fiemap_extent array */
> -};
> -int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> - u64 phys, u64 len, u32 flags);
> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>
> /*
> * This is the "filldir" function type, used by readdir() to let
> @@ -3299,10 +3286,6 @@ static inline int vfs_fstat(int fd, struct kstat *stat)
> extern const char *vfs_get_link(struct dentry *, struct delayed_call *);
> extern int vfs_readlink(struct dentry *, char __user *, int);
>
> -extern int generic_block_fiemap(struct inode *inode,
> - struct fiemap_extent_info *fieinfo, u64 start,
> - u64 len, get_block_t *get_block);
> -
> extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> extern void put_filesystem(struct file_system_type *fs);
> extern struct file_system_type *get_fs_type(const char *name);
> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
> index 7a900b2377b60..24ca0c00cae36 100644
> --- a/include/uapi/linux/fiemap.h
> +++ b/include/uapi/linux/fiemap.h
> @@ -9,8 +9,8 @@
> * Andreas Dilger <[email protected]>
> */
>
> -#ifndef _LINUX_FIEMAP_H
> -#define _LINUX_FIEMAP_H
> +#ifndef _UAPI_LINUX_FIEMAP_H
> +#define _UAPI_LINUX_FIEMAP_H
>
> #include <linux/types.h>
>
> @@ -67,4 +67,4 @@ struct fiemap {
> #define FIEMAP_EXTENT_SHARED 0x00002000 /* Space shared with other
> * files. */
>
> -#endif /* _LINUX_FIEMAP_H */
> +#endif /* _UAPI_LINUX_FIEMAP_H */
>
On 4/27/20 11:49 PM, Christoph Hellwig wrote:
> iomap_fiemap should take u64 start and len arguments, just like the
> ->fiemap prototype.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
hmm.. I guess,
it's only ->fiemap ops in inode_operations which has
start and len arguments as u64.
While such other ops in struct file_operations have the
arguments of type loff_t. (e.g. ->fallocate, -->llseek etc).
But sure to match the ->fiemap prototype, this patch looks ok to me.
Feel free to add:
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/iomap/fiemap.c | 2 +-
> include/linux/iomap.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index fca3dfb9d964a..dd04e4added15 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -66,7 +66,7 @@ iomap_fiemap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
> }
>
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> - loff_t start, loff_t len, const struct iomap_ops *ops)
> + u64 start, u64 len, const struct iomap_ops *ops)
> {
> struct fiemap_ctx ctx;
> loff_t ret;
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 8b09463dae0db..63db02528b702 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -178,7 +178,7 @@ int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf,
> const struct iomap_ops *ops);
> int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> - loff_t start, loff_t len, const struct iomap_ops *ops);
> + u64 start, u64 len, const struct iomap_ops *ops);
> loff_t iomap_seek_hole(struct inode *inode, loff_t offset,
> const struct iomap_ops *ops);
> loff_t iomap_seek_data(struct inode *inode, loff_t offset,
>
On Sat, May 02, 2020 at 05:04:01AM +0530, Ritesh Harjani wrote:
>
>
> On 4/27/20 11:49 PM, Christoph Hellwig wrote:
>> iomap_fiemap should take u64 start and len arguments, just like the
>> ->fiemap prototype.
>>
>> Signed-off-by: Christoph Hellwig <[email protected]>
>
> hmm.. I guess,
> it's only ->fiemap ops in inode_operations which has
> start and len arguments as u64.
>
> While such other ops in struct file_operations have the
> arguments of type loff_t. (e.g. ->fallocate, -->llseek etc).
>
> But sure to match the ->fiemap prototype, this patch looks ok to me.
Yes, fiemap is rather weird here, but it matches the ioctl prototype,
so I'd rather pass it on to the method where fiemap_prep will catch
anything that overflows s_maxbytes due to the signeness of loff_t.
On Sat, May 02, 2020 at 04:19:05AM +0530, Ritesh Harjani wrote:
>> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>> +fiemap_prep() helper.
>
> Let's add the fiemap_prep() function prototype here.
> So that below description would make more sense.
The annoying tendency to copy prototypes into docs just means they
get out of date. Anyone who can't do a quick grep should not be
writing code. I've added it back for consistency, but we really
should remove all of them in this file.
>> ioctl_fiemap(). The
>> set of fiemap flags which the fs understands should be passed via fs_flags. If
>> -fiemap_check_flags finds invalid user flags, it will place the bad values in
>> +fiemap_prep finds invalid user flags, it will place the bad values in
>> fieinfo->fi_flags and return -EBADR. If the file system gets -EBADR, from
>> -fiemap_check_flags(), it should immediately exit, returning that error back to
>> +fiemap_prep(), it should immediately exit, returning that error back to
>> ioctl_fiemap().
>
> Also maybe we should also add more info about fiemap_prep() helper.
> Since it also checks for invalid len and invalid start range and hence
> could return -EINVAL or -EFBIG.
I've added it. But this kind of documentation that just badly spells
out what is done is not very useful. A quick look at the function
conveys the information much better.
>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -3408,8 +3408,10 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
>> int i, num, rc, flags, last_blob;
>> u64 next;
>> - if (fiemap_check_flags(fei, FIEMAP_FLAG_SYNC))
>> - return -EBADR;
>> + rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
>> + FIEMAP_FLAG_SYNC);
>
> How about d_inode(cfile->dentry) ?
Ok.
>> + if (rc)
>> + rc;
>
> missed "return rc" here?
Indeed.
>> + * @start: Start of the mapped range
>> + * @len: Length of the mapped range, can be truncated by this function.
>> + * @supported_flags: Set of fiemap flags that the file system understands
>> *
>> * Called from file system ->fiemap callback. This will compute the
>> * intersection of valid fiemap flags and those that the fs supports. That
>
> Let's also add more documentation about this new fiemap_prep() helper.
> Earlier comments above this function description only talks about
> incompat flags. We should add more info about check_ranges part as well
> here.
Seriously, the details are in the f^$^$ing code. No need to have an
easily out of sync version of the code in the comment. I'll just
radically shorten this.
On Sat, May 02, 2020 at 04:22:31AM +0530, Ritesh Harjani wrote:
>
>
> On 4/27/20 11:49 PM, Christoph Hellwig wrote:
>> By moving FIEMAP_FLAG_SYNC handling to fiemap_prep we ensure it is
>> handled once instead of duplicated, but can still be done under fs locks,
>> like xfs/iomap intended with its duplicate handling. Also make sure the
>> error value of filemap_write_and_wait is propagated to user space.
>
>
> Forgot to remove filemap_write_and_wait() from
> ext4_ioctl_get_es_cache() ?
Fixed.
On Tue, Apr 28, 2020 at 08:21:24AM -0700, Darrick J. Wong wrote:
> On Mon, Apr 27, 2020 at 08:19:56PM +0200, Christoph Hellwig wrote:
> > access_ok just checks we are fed a proper user pointer. We also do that
> > in copy_to_user itself, so no need to do this early.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
>
> Hmm. It's a minor behavioral change that we no longer require the
> entire extent array to be accessible at the start even if parts of it
> would never have gotten accessed, but I don't think that matters, so:
Note that access_ok only checks if the memory actually is in userspace,
so they only thing seeing a behavior difference would be an exploit of
some kind.