[PATCH] xfs: add selinux labels to whiteout inodes

[PATCH] xfs: add selinux labels to whiteout inodes

[Eric Sandeen]

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;
+	}
+
 	/*
 	 * 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__ */
-- 
1.8.3.1

Re: [PATCH] xfs: add selinux labels to whiteout inodes

[Eric Sandeen]

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;
}

Re: [PATCH] xfs: add selinux labels to whiteout inodes

[Dave Chinner]

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>

-- 
Dave Chinner
david@fromorbit.com

[PATCH V2] xfs: add selinux labels to whiteout inodes

[Eric Sandeen]

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>
---
 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

Re: [PATCH V2] xfs: add selinux labels to whiteout inodes

[Darrick J. Wong]

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
> 

Re: [PATCH V2] xfs: add selinux labels to whiteout inodes

[Eric Sandeen]

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