| Message ID | 20231129-idmap-fscap-refactor-v1-11-da5a26058a5b@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > > Add handlers which read fs caps from the lower or upper filesystem and > write/remove fs caps to the upper filesystem, performing copy-up as > necessary. > > While it doesn't make sense to use fscaps on directories, nothing in the > kernel actually prevents setting or getting fscaps xattrs for directory > inodes. If we omit fscaps handlers in ovl_dir_inode_operations then the > generic handlers will be used. These handlers will use the xattr inode > operations, bypassing any idmapping on lower mounts, so fscaps handlers > are also installed for ovl_dir_inode_operations. > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > --- > fs/overlayfs/dir.c | 3 ++ > fs/overlayfs/inode.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/overlayfs/overlayfs.h | 6 ++++ > 3 files changed, 93 insertions(+) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index aab3f5d93556..d9ab3c9ce10a 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -1303,6 +1303,9 @@ const struct inode_operations ovl_dir_inode_operations = { > .get_inode_acl = ovl_get_inode_acl, > .get_acl = ovl_get_acl, > .set_acl = ovl_set_acl, > + .get_fscaps = ovl_get_fscaps, > + .set_fscaps = ovl_set_fscaps, > + .remove_fscaps = ovl_remove_fscaps, > .update_time = ovl_update_time, > .fileattr_get = ovl_fileattr_get, > .fileattr_set = ovl_fileattr_set, > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index c63b31a460be..82fc6e479d45 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -568,6 +568,87 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > } > #endif > > +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > + struct vfs_caps *caps) > +{ > + int err; > + const struct cred *old_cred; > + struct path realpath; > + > + ovl_path_real(dentry, &realpath); > + old_cred = ovl_override_creds(dentry->d_sb); > + err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realpath.dentry, caps); > + revert_creds(old_cred); > + return err; > +} > + > +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > + const struct vfs_caps *caps, int flags) > +{ > + int err; > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); > + struct dentry *upperdentry = ovl_dentry_upper(dentry); > + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); > + const struct cred *old_cred; > + > + err = ovl_want_write(dentry); > + if (err) > + goto out; > + > + if (!upperdentry) { > + err = ovl_copy_up(dentry); > + if (err) > + goto out_drop_write; > + > + realdentry = ovl_dentry_upper(dentry); > + } > + > + old_cred = ovl_override_creds(dentry->d_sb); > + err = vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), realdentry, caps, flags); > + revert_creds(old_cred); > + > + /* copy c/mtime */ > + ovl_copyattr(d_inode(dentry)); > + > +out_drop_write: > + ovl_drop_write(dentry); > +out: > + return err; > +} > + > +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > +{ > + int err; > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); > + struct dentry *upperdentry = ovl_dentry_upper(dentry); > + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); > + const struct cred *old_cred; > + > + err = ovl_want_write(dentry); > + if (err) > + goto out; > + > + if (!upperdentry) { > + err = ovl_copy_up(dentry); > + if (err) > + goto out_drop_write; > + > + realdentry = ovl_dentry_upper(dentry); > + } > + This construct is peculiar. Most of the operations just do this unconditionally: err = ovl_copy_up(dentry); if (err) goto out_drop_write; and then use ovl_dentry_upper(dentry) directly, because a modification will always be done on the upper dentry, regardless of the state before the operation started. I was wondering where you copied this from and I found it right above in ovl_set_or_remove_acl(). In that case, there was also no justification for this construct. There is also no justification for open coding: struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); when later on, ovl_path_lower(dentry, &realpath) is called anyway. The only reason to do anything special in ovl_set_or_remove_acl() is: /* * If ACL is to be removed from a lower file, check if it exists in * the first place before copying it up. */ Do you not want to do the same for ovl_remove_fscaps()? Also, the comparison to remove_acl API bares the question, why did you need to add a separate method for remove_fscaps? Why not use set_fscaps(NULL), just like setxattr() and set_acl() APIs? Thanks, Amir.
On Thu, Nov 30, 2023 at 07:56:48AM +0200, Amir Goldstein wrote: > On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean) > <sforshee@kernel.org> wrote: > > > > Add handlers which read fs caps from the lower or upper filesystem and > > write/remove fs caps to the upper filesystem, performing copy-up as > > necessary. > > > > While it doesn't make sense to use fscaps on directories, nothing in the > > kernel actually prevents setting or getting fscaps xattrs for directory > > inodes. If we omit fscaps handlers in ovl_dir_inode_operations then the > > generic handlers will be used. These handlers will use the xattr inode > > operations, bypassing any idmapping on lower mounts, so fscaps handlers > > are also installed for ovl_dir_inode_operations. > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > --- > > fs/overlayfs/dir.c | 3 ++ > > fs/overlayfs/inode.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++ > > fs/overlayfs/overlayfs.h | 6 ++++ > > 3 files changed, 93 insertions(+) > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > index aab3f5d93556..d9ab3c9ce10a 100644 > > --- a/fs/overlayfs/dir.c > > +++ b/fs/overlayfs/dir.c > > @@ -1303,6 +1303,9 @@ const struct inode_operations ovl_dir_inode_operations = { > > .get_inode_acl = ovl_get_inode_acl, > > .get_acl = ovl_get_acl, > > .set_acl = ovl_set_acl, > > + .get_fscaps = ovl_get_fscaps, > > + .set_fscaps = ovl_set_fscaps, > > + .remove_fscaps = ovl_remove_fscaps, > > .update_time = ovl_update_time, > > .fileattr_get = ovl_fileattr_get, > > .fileattr_set = ovl_fileattr_set, > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > > index c63b31a460be..82fc6e479d45 100644 > > --- a/fs/overlayfs/inode.c > > +++ b/fs/overlayfs/inode.c > > @@ -568,6 +568,87 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > > } > > #endif > > > > +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > > + struct vfs_caps *caps) > > +{ > > + int err; > > + const struct cred *old_cred; > > + struct path realpath; > > + > > + ovl_path_real(dentry, &realpath); > > + old_cred = ovl_override_creds(dentry->d_sb); > > + err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realpath.dentry, caps); > > + revert_creds(old_cred); > > + return err; > > +} > > + > > +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > > + const struct vfs_caps *caps, int flags) > > +{ > > + int err; > > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); > > + struct dentry *upperdentry = ovl_dentry_upper(dentry); > > + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); > > + const struct cred *old_cred; > > + > > + err = ovl_want_write(dentry); > > + if (err) > > + goto out; > > + > > + if (!upperdentry) { > > + err = ovl_copy_up(dentry); > > + if (err) > > + goto out_drop_write; > > + > > + realdentry = ovl_dentry_upper(dentry); > > + } > > + > > + old_cred = ovl_override_creds(dentry->d_sb); > > + err = vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), realdentry, caps, flags); > > + revert_creds(old_cred); > > + > > + /* copy c/mtime */ > > + ovl_copyattr(d_inode(dentry)); > > + > > +out_drop_write: > > + ovl_drop_write(dentry); > > +out: > > + return err; > > +} > > + > > +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > > +{ > > + int err; > > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); > > + struct dentry *upperdentry = ovl_dentry_upper(dentry); > > + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); > > + const struct cred *old_cred; > > + > > + err = ovl_want_write(dentry); > > + if (err) > > + goto out; > > + > > + if (!upperdentry) { > > + err = ovl_copy_up(dentry); > > + if (err) > > + goto out_drop_write; > > + > > + realdentry = ovl_dentry_upper(dentry); > > + } > > + > > This construct is peculiar. > Most of the operations just do this unconditionally: > > err = ovl_copy_up(dentry); > if (err) > goto out_drop_write; > > and then use ovl_dentry_upper(dentry) directly, because a modification > will always be done on the upper dentry, regardless of the state before > the operation started. > > I was wondering where you copied this from and I found it right above > in ovl_set_or_remove_acl(). > In that case, there was also no justification for this construct. Yes, I did use ovl_set_or_remove_acl() as a reference here. But looking around at some of the other code, I see what you mean, so I'll rework this code. > There is also no justification for open coding: > struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); > when later on, ovl_path_lower(dentry, &realpath) is called anyway. > > The only reason to do anything special in ovl_set_or_remove_acl() is: > > /* > * If ACL is to be removed from a lower file, check if it exists in > * the first place before copying it up. > */ > > Do you not want to do the same for ovl_remove_fscaps()? Yes, I will add this. > Also, the comparison to remove_acl API bares the question, > why did you need to add a separate method for remove_fscaps? > Why not use set_fscaps(NULL), just like setxattr() and set_acl() APIs? I had started on these patches a while back and then picked them up again recently, and honestly I don't remember why I did that originally. I don't see a need for it now though, so I can change that.
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index aab3f5d93556..d9ab3c9ce10a 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -1303,6 +1303,9 @@ const struct inode_operations ovl_dir_inode_operations = { .get_inode_acl = ovl_get_inode_acl, .get_acl = ovl_get_acl, .set_acl = ovl_set_acl, + .get_fscaps = ovl_get_fscaps, + .set_fscaps = ovl_set_fscaps, + .remove_fscaps = ovl_remove_fscaps, .update_time = ovl_update_time, .fileattr_get = ovl_fileattr_get, .fileattr_set = ovl_fileattr_set, diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index c63b31a460be..82fc6e479d45 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -568,6 +568,87 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, } #endif +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, + struct vfs_caps *caps) +{ + int err; + const struct cred *old_cred; + struct path realpath; + + ovl_path_real(dentry, &realpath); + old_cred = ovl_override_creds(dentry->d_sb); + err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realpath.dentry, caps); + revert_creds(old_cred); + return err; +} + +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, + const struct vfs_caps *caps, int flags) +{ + int err; + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); + struct dentry *upperdentry = ovl_dentry_upper(dentry); + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); + const struct cred *old_cred; + + err = ovl_want_write(dentry); + if (err) + goto out; + + if (!upperdentry) { + err = ovl_copy_up(dentry); + if (err) + goto out_drop_write; + + realdentry = ovl_dentry_upper(dentry); + } + + old_cred = ovl_override_creds(dentry->d_sb); + err = vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), realdentry, caps, flags); + revert_creds(old_cred); + + /* copy c/mtime */ + ovl_copyattr(d_inode(dentry)); + +out_drop_write: + ovl_drop_write(dentry); +out: + return err; +} + +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) +{ + int err; + struct ovl_fs *ofs = OVL_FS(dentry->d_sb); + struct dentry *upperdentry = ovl_dentry_upper(dentry); + struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry); + const struct cred *old_cred; + + err = ovl_want_write(dentry); + if (err) + goto out; + + if (!upperdentry) { + err = ovl_copy_up(dentry); + if (err) + goto out_drop_write; + + realdentry = ovl_dentry_upper(dentry); + } + + old_cred = ovl_override_creds(dentry->d_sb); + err = vfs_remove_fscaps(ovl_upper_mnt_idmap(ofs), realdentry); + revert_creds(old_cred); + + /* copy c/mtime */ + ovl_copyattr(d_inode(dentry)); + +out_drop_write: + ovl_drop_write(dentry); +out: + return err; +} + int ovl_update_time(struct inode *inode, int flags) { if (flags & S_ATIME) { @@ -747,6 +828,9 @@ static const struct inode_operations ovl_file_inode_operations = { .get_inode_acl = ovl_get_inode_acl, .get_acl = ovl_get_acl, .set_acl = ovl_set_acl, + .get_fscaps = ovl_get_fscaps, + .set_fscaps = ovl_set_fscaps, + .remove_fscaps = ovl_remove_fscaps, .update_time = ovl_update_time, .fiemap = ovl_fiemap, .fileattr_get = ovl_fileattr_get, diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 05c3dd597fa8..e72ee2374f96 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -764,6 +764,12 @@ static inline struct posix_acl *ovl_get_acl_path(const struct path *path, } #endif +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, + struct vfs_caps *caps); +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, + const struct vfs_caps *caps, int flags); +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry); + int ovl_update_time(struct inode *inode, int flags); bool ovl_is_private_xattr(struct super_block *sb, const char *name);
Add handlers which read fs caps from the lower or upper filesystem and write/remove fs caps to the upper filesystem, performing copy-up as necessary. While it doesn't make sense to use fscaps on directories, nothing in the kernel actually prevents setting or getting fscaps xattrs for directory inodes. If we omit fscaps handlers in ovl_dir_inode_operations then the generic handlers will be used. These handlers will use the xattr inode operations, bypassing any idmapping on lower mounts, so fscaps handlers are also installed for ovl_dir_inode_operations. Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> --- fs/overlayfs/dir.c | 3 ++ fs/overlayfs/inode.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++ fs/overlayfs/overlayfs.h | 6 ++++ 3 files changed, 93 insertions(+)