| Message ID | 1655775516-8936-1-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [V2] xfs: add selinux labels to whiteout inodes | expand |
On Mon, Jun 20, 2022 at 08:38:36PM -0500, Eric Sandeen wrote: > We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't > apply an SELinux label on xfs" as it does on other filesystems > (for example, ext4 and tmpfs.) While I'm not quite sure how labels > may interact w/ whiteout files, leaving them as unlabeled seems > inconsistent at best. Now that xfs_init_security is not static, > rename it to xfs_inode_init_security per dchinner's suggestion. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > Reviewed-by: Dave Chinner <dchinner@redhat.com> Looks fine to me. I wondered slightly if the label creation needs to be atomic with the file creation, but quickly realized that /never/ happens. Assuming this isn't high priority 5.19 stuff, I'll just roll this into 5.20 if that's ok? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_inode.c | 14 +++++++++++++- > fs/xfs/xfs_iops.c | 11 +++++------ > fs/xfs/xfs_iops.h | 3 +++ > 3 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 52d6f2c..58513a1 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3046,10 +3046,12 @@ struct xfs_iunlink { > static int > xfs_rename_alloc_whiteout( > struct user_namespace *mnt_userns, > + struct xfs_name *src_name, > struct xfs_inode *dp, > struct xfs_inode **wip) > { > struct xfs_inode *tmpfile; > + struct qstr name; > int error; > > error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE, > @@ -3057,6 +3059,15 @@ struct xfs_iunlink { > if (error) > return error; > > + name.name = src_name->name; > + name.len = src_name->len; > + error = xfs_inode_init_security(VFS_I(tmpfile), VFS_I(dp), &name); > + if (error) { > + xfs_finish_inode_setup(tmpfile); > + xfs_irele(tmpfile); > + return error; > + } > + > /* > * Prepare the tmpfile inode as if it were created through the VFS. > * Complete the inode setup and flag it as linkable. nlink is already > @@ -3107,7 +3118,8 @@ struct xfs_iunlink { > * appropriately. > */ > if (flags & RENAME_WHITEOUT) { > - error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip); > + error = xfs_rename_alloc_whiteout(mnt_userns, src_name, > + target_dp, &wip); > if (error) > return error; > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index 29f5b8b8..6720b60 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -75,9 +75,8 @@ > * these attrs can be journalled at inode creation time (along with the > * inode, of course, such that log replay can't cause these to be lost). > */ > - > -STATIC int > -xfs_init_security( > +int > +xfs_inode_init_security( > struct inode *inode, > struct inode *dir, > const struct qstr *qstr) > @@ -122,7 +121,7 @@ > > /* Oh, the horror. > * If we can't add the ACL or we fail in > - * xfs_init_security we must back out. > + * xfs_inode_init_security we must back out. > * ENOSPC can hit here, among other things. > */ > xfs_dentry_to_name(&teardown, dentry); > @@ -208,7 +207,7 @@ > > inode = VFS_I(ip); > > - error = xfs_init_security(inode, dir, &dentry->d_name); > + error = xfs_inode_init_security(inode, dir, &dentry->d_name); > if (unlikely(error)) > goto out_cleanup_inode; > > @@ -424,7 +423,7 @@ > > inode = VFS_I(cip); > > - error = xfs_init_security(inode, dir, &dentry->d_name); > + error = xfs_inode_init_security(inode, dir, &dentry->d_name); > if (unlikely(error)) > goto out_cleanup_inode; > > diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h > index 2789490..cb5fc68 100644 > --- a/fs/xfs/xfs_iops.h > +++ b/fs/xfs/xfs_iops.h > @@ -17,4 +17,7 @@ > int xfs_vn_setattr_size(struct user_namespace *mnt_userns, > struct dentry *dentry, struct iattr *vap); > > +int xfs_inode_init_security(struct inode *inode, struct inode *dir, > + const struct qstr *qstr); > + > #endif /* __XFS_IOPS_H__ */ > -- > 1.8.3.1 >
On 6/22/22 6:30 PM, Darrick J. Wong wrote: > On Mon, Jun 20, 2022 at 08:38:36PM -0500, Eric Sandeen wrote: >> We got a report that "renameat2() with flags=RENAME_WHITEOUT doesn't >> apply an SELinux label on xfs" as it does on other filesystems >> (for example, ext4 and tmpfs.) While I'm not quite sure how labels >> may interact w/ whiteout files, leaving them as unlabeled seems >> inconsistent at best. Now that xfs_init_security is not static, >> rename it to xfs_inode_init_security per dchinner's suggestion. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> Reviewed-by: Dave Chinner <dchinner@redhat.com> > > Looks fine to me. I wondered slightly if the label creation needs to be > atomic with the file creation, but quickly realized that /never/ > happens. Assuming this isn't high priority 5.19 stuff, I'll just roll > this into 5.20 if that's ok? > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks Darrick. I don't think it's high priority, I got a bug report about the behavior, but there was no indication that it was actively causing visible problems. -Eric
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 52d6f2c..58513a1 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3046,10 +3046,12 @@ struct xfs_iunlink { static int xfs_rename_alloc_whiteout( struct user_namespace *mnt_userns, + struct xfs_name *src_name, struct xfs_inode *dp, struct xfs_inode **wip) { struct xfs_inode *tmpfile; + struct qstr name; int error; error = xfs_create_tmpfile(mnt_userns, dp, S_IFCHR | WHITEOUT_MODE, @@ -3057,6 +3059,15 @@ struct xfs_iunlink { if (error) return error; + name.name = src_name->name; + name.len = src_name->len; + error = xfs_inode_init_security(VFS_I(tmpfile), VFS_I(dp), &name); + if (error) { + xfs_finish_inode_setup(tmpfile); + xfs_irele(tmpfile); + return error; + } + /* * Prepare the tmpfile inode as if it were created through the VFS. * Complete the inode setup and flag it as linkable. nlink is already @@ -3107,7 +3118,8 @@ struct xfs_iunlink { * appropriately. */ if (flags & RENAME_WHITEOUT) { - error = xfs_rename_alloc_whiteout(mnt_userns, target_dp, &wip); + error = xfs_rename_alloc_whiteout(mnt_userns, src_name, + target_dp, &wip); if (error) return error; diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 29f5b8b8..6720b60 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -75,9 +75,8 @@ * these attrs can be journalled at inode creation time (along with the * inode, of course, such that log replay can't cause these to be lost). */ - -STATIC int -xfs_init_security( +int +xfs_inode_init_security( struct inode *inode, struct inode *dir, const struct qstr *qstr) @@ -122,7 +121,7 @@ /* Oh, the horror. * If we can't add the ACL or we fail in - * xfs_init_security we must back out. + * xfs_inode_init_security we must back out. * ENOSPC can hit here, among other things. */ xfs_dentry_to_name(&teardown, dentry); @@ -208,7 +207,7 @@ inode = VFS_I(ip); - error = xfs_init_security(inode, dir, &dentry->d_name); + error = xfs_inode_init_security(inode, dir, &dentry->d_name); if (unlikely(error)) goto out_cleanup_inode; @@ -424,7 +423,7 @@ inode = VFS_I(cip); - error = xfs_init_security(inode, dir, &dentry->d_name); + error = xfs_inode_init_security(inode, dir, &dentry->d_name); if (unlikely(error)) goto out_cleanup_inode; diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h index 2789490..cb5fc68 100644 --- a/fs/xfs/xfs_iops.h +++ b/fs/xfs/xfs_iops.h @@ -17,4 +17,7 @@ int xfs_vn_setattr_size(struct user_namespace *mnt_userns, struct dentry *dentry, struct iattr *vap); +int xfs_inode_init_security(struct inode *inode, struct inode *dir, + const struct qstr *qstr); + #endif /* __XFS_IOPS_H__ */