| Message ID | 20220105083006.2793559-2-sahil.kang@asilaycomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: reuse existing pointers from btrfs_ioctl | expand |
On 2022/1/5 16:30, Sahil Kang wrote: > btrfs_ioctl already contains pointers to the inode and btrfs_root > structs, so we can pass them into the subfunctions instead of the > toplevel struct file. > > Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Just some small nitpicks inlined below. [...] > -static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) > +static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, > + void __user *argp) I guess you're using the @subvol_root naming is to avoid the conflicts in the existing @root defined locally. It may be a better idea to rename @root to @tree_root, or doesn't define it at all. Since the existing @root is only used in 3 locations. [...] > @@ -4873,7 +4867,7 @@ long btrfs_ioctl(struct file *file, unsigned int > > switch (cmd) { > case FS_IOC_GETVERSION: > - return btrfs_ioctl_getversion(file, argp); > + return btrfs_ioctl_getversion(inode, argp); > case FS_IOC_GETFSLABEL: > return btrfs_ioctl_get_fslabel(fs_info, argp); > case FS_IOC_SETFSLABEL: > @@ -4921,7 +4915,7 @@ long btrfs_ioctl(struct file *file, unsigned int > case BTRFS_IOC_TREE_SEARCH_V2: > return btrfs_ioctl_tree_search_v2(file, argp); > case BTRFS_IOC_INO_LOOKUP: > - return btrfs_ioctl_ino_lookup(file, argp); > + return btrfs_ioctl_ino_lookup(root, argp); > case BTRFS_IOC_INO_PATHS: > return btrfs_ioctl_ino_to_path(root, argp); > case BTRFS_IOC_LOGICAL_INO: > @@ -5000,7 +4994,7 @@ long btrfs_ioctl(struct file *file, unsigned int > case BTRFS_IOC_GET_SUBVOL_INFO: > return btrfs_ioctl_get_subvol_info(file, argp); > case BTRFS_IOC_GET_SUBVOL_ROOTREF: > - return btrfs_ioctl_get_subvol_rootref(file, argp); > + return btrfs_ioctl_get_subvol_rootref(root, argp); > case BTRFS_IOC_INO_LOOKUP_USER: > return btrfs_ioctl_ino_lookup_user(file, argp); > case FS_IOC_ENABLE_VERITY: There are some remaining ioctl sub-routines which you may want to further cleanup, as they don't really rely on @file to do write check (aka, RO operations): - btrfs_ioctl_subvol_getflags() - btrfs_ioctl_tree_search() - btrfs_ioctl_tree_search_v2() - btrfs_ioctl_send() - btrfs_ioctl_get_subvol_info() - btrfs_ioctl_ino_lookup_user() One suspicious ioctls, they should require write permission, but don't: - btrfs_ioctl_start_sync() If there is no running trans, it would bail out without doing anything, but I'm wondering if we should call mnt_want_write_file(). As this means, if we mount a subvolume read-only, but the fs still has some part mounted RW, we can start a sync using the read-only mounted part. Thanks, Qu
On Wed, Jan 05, 2022 at 05:53:20PM +0800, Qu Wenruo wrote: > One suspicious ioctls, they should require write permission, but don't: > > - btrfs_ioctl_start_sync() > If there is no running trans, it would bail out without doing > anything, but I'm wondering if we should call mnt_want_write_file(). > > As this means, if we mount a subvolume read-only, but the fs still has > some part mounted RW, we can start a sync using the read-only mounted > part. We can possibly do the mnt_want_write but it should not propagate the errors, because calling sync on a fully read-only filesystem should not error out.
On Wed, Jan 05, 2022 at 12:30:06AM -0800, Sahil Kang wrote: > btrfs_ioctl already contains pointers to the inode and btrfs_root > structs, so we can pass them into the subfunctions instead of the > toplevel struct file. > > Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index edfecfe62b4b..26cade35eccd 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -414,10 +414,8 @@ void btrfs_exclop_finish(struct btrfs_fs_info *fs_info) sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation"); } -static int btrfs_ioctl_getversion(struct file *file, int __user *arg) +static int btrfs_ioctl_getversion(struct inode *inode, int __user *arg) { - struct inode *inode = file_inode(file); - return put_user(inode->i_generation, arg); } @@ -2559,25 +2557,22 @@ static int btrfs_search_path_in_tree_user(struct user_namespace *mnt_userns, return ret; } -static noinline int btrfs_ioctl_ino_lookup(struct file *file, +static noinline int btrfs_ioctl_ino_lookup(struct btrfs_root *root, void __user *argp) { struct btrfs_ioctl_ino_lookup_args *args; - struct inode *inode; int ret = 0; args = memdup_user(argp, sizeof(*args)); if (IS_ERR(args)) return PTR_ERR(args); - inode = file_inode(file); - /* * Unprivileged query to obtain the containing subvolume root id. The * path is reset so it's consistent with btrfs_search_path_in_tree. */ if (args->treeid == 0) - args->treeid = BTRFS_I(inode)->root->root_key.objectid; + args->treeid = root->root_key.objectid; if (args->objectid == BTRFS_FIRST_FREE_OBJECTID) { args->name[0] = 0; @@ -2589,7 +2584,7 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, goto out; } - ret = btrfs_search_path_in_tree(BTRFS_I(inode)->root->fs_info, + ret = btrfs_search_path_in_tree(root->fs_info, args->treeid, args->objectid, args->name); @@ -2765,7 +2760,8 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) * Return ROOT_REF information of the subvolume containing this inode * except the subvolume name. */ -static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) +static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, + void __user *argp) { struct btrfs_ioctl_get_subvol_rootref_args *rootrefs; struct btrfs_root_ref *rref; @@ -2773,7 +2769,6 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) struct btrfs_path *path; struct btrfs_key key; struct extent_buffer *leaf; - struct inode *inode; u64 objectid; int slot; int ret; @@ -2789,9 +2784,8 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) return PTR_ERR(rootrefs); } - inode = file_inode(file); - root = BTRFS_I(inode)->root->fs_info->tree_root; - objectid = BTRFS_I(inode)->root->root_key.objectid; + root = subvol_root->fs_info->tree_root; + objectid = subvol_root->root_key.objectid; key.objectid = objectid; key.type = BTRFS_ROOT_REF_KEY; @@ -4873,7 +4867,7 @@ long btrfs_ioctl(struct file *file, unsigned int switch (cmd) { case FS_IOC_GETVERSION: - return btrfs_ioctl_getversion(file, argp); + return btrfs_ioctl_getversion(inode, argp); case FS_IOC_GETFSLABEL: return btrfs_ioctl_get_fslabel(fs_info, argp); case FS_IOC_SETFSLABEL: @@ -4921,7 +4915,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_TREE_SEARCH_V2: return btrfs_ioctl_tree_search_v2(file, argp); case BTRFS_IOC_INO_LOOKUP: - return btrfs_ioctl_ino_lookup(file, argp); + return btrfs_ioctl_ino_lookup(root, argp); case BTRFS_IOC_INO_PATHS: return btrfs_ioctl_ino_to_path(root, argp); case BTRFS_IOC_LOGICAL_INO: @@ -5000,7 +4994,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_GET_SUBVOL_INFO: return btrfs_ioctl_get_subvol_info(file, argp); case BTRFS_IOC_GET_SUBVOL_ROOTREF: - return btrfs_ioctl_get_subvol_rootref(file, argp); + return btrfs_ioctl_get_subvol_rootref(root, argp); case BTRFS_IOC_INO_LOOKUP_USER: return btrfs_ioctl_ino_lookup_user(file, argp); case FS_IOC_ENABLE_VERITY:
btrfs_ioctl already contains pointers to the inode and btrfs_root structs, so we can pass them into the subfunctions instead of the toplevel struct file. Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> --- fs/btrfs/ioctl.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)