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

[Dave Chinner <david@fromorbit.com>]

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