| Message ID | 1655765731-21078-1-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: add selinux labels to whiteout inodes | expand |
I had mentioned this problem in passing to dchinner, and came away with
a vague sense that I might be Doing It Wrong. So I should have labeled
the patch RFC, I suppose.
An easy way to demonstrate the selinux label result is with this test
program, as provided by the bug reporter. ls -lZ will then show you
the resulting labels on each file.
#define _GNU_SOURCE
#include <sys/mount.h>
#include <sys/stat.h>
#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <stdio.h>
#include <unistd.h>
int main(int argc, char **argv) {
int rc, fd, dirfd;
rc = mkdir("upper", 0700);
if ((rc != 0) && (errno != EEXIST)) {
perror("mkdir");
return rc;
}
rc = unlink("upper/0");
if ((rc != 0) && (errno != ENOENT)) {
perror("unlink");
return rc;
}
rc = unlink("upper/empty");
if ((rc != 0) && (errno != ENOENT)) {
perror("unlink");
return rc;
}
dirfd = open("upper", O_PATH);
if (dirfd == -1) {
perror("open");
return dirfd;
}
fd = creat("upper/empty", 0600);
if (fd == -1) {
perror("creat");
return fd;
}
close(fd);
rc = renameat2(dirfd, "empty", dirfd, "0", RENAME_WHITEOUT);
if (rc == -1) {
perror("renameat2");
return rc;
}
close(dirfd);
return 0;
}
On Mon, Jun 20, 2022 at 05:55:31PM -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. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/xfs/xfs_inode.c | 14 +++++++++++++- > fs/xfs/xfs_iops.c | 2 +- > fs/xfs/xfs_iops.h | 3 +++ > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 52d6f2c..9a43060 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_init_security(VFS_I(tmpfile), VFS_I(dp), &name); > + if (error) { > + xfs_finish_inode_setup(tmpfile); > + xfs_irele(tmpfile); > + return error; > + } > + I was worried that this would be inside an existing transaction, but the tmpfile create is outside the rename transaction so this will be fine. > /* > * 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..c7775b7 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -76,7 +76,7 @@ > * inode, of course, such that log replay can't cause these to be lost). > */ > > -STATIC int > +int > xfs_init_security( This function needs renaming, though. As a static function it can get away with not having a namespace, but as a globally visible function it needs to have an "xfs_inode_" prefix.... Otherwise OK. Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 52d6f2c..9a43060 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_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..c7775b7 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -76,7 +76,7 @@ * inode, of course, such that log replay can't cause these to be lost). */ -STATIC int +int xfs_init_security( struct inode *inode, struct inode *dir, diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h index 2789490..fd92c82 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_init_security(struct inode *inode, struct inode *dir, + const struct qstr *qstr); + #endif /* __XFS_IOPS_H__ */
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. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/xfs/xfs_inode.c | 14 +++++++++++++- fs/xfs/xfs_iops.c | 2 +- fs/xfs/xfs_iops.h | 3 +++ 3 files changed, 17 insertions(+), 2 deletions(-)