Re: [PATCH] xfs: allow cross-linking special files without project quota

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 04, 2024 at 04:50:14PM +0100, Andrey Albershteyn wrote:
> There's an issue that if special files is created before quota
> project is enabled, then it's not possible to link this file. This
> works fine for normal files. This happens because xfs_quota skips
> special files (no ioctls to set necessary flags). The check for
> having the same project ID for source and destination then fails as
> source file doesn't have any ID.
> 
> mkfs.xfs -f /dev/sda
> mount -o prjquota /dev/sda /mnt/test
> 
> mkdir /mnt/test/foo
> mkfifo /mnt/test/foo/fifo1
> 
> xfs_quota -xc "project -sp /mnt/test/foo 9" /mnt/test
> > Setting up project 9 (path /mnt/test/foo)...
> > xfs_quota: skipping special file /mnt/test/foo/fifo1
> > Processed 1 (/etc/projects and cmdline) paths for project 9 with recursion depth infinite (-1).
> 
> ln /mnt/test/foo/fifo1 /mnt/test/foo/fifo1_link
> > ln: failed to create hard link '/mnt/test/testdir/fifo1_link' => '/mnt/test/testdir/fifo1': Invalid cross-device link
> 
> mkfifo /mnt/test/foo/fifo2
> ln /mnt/test/foo/fifo2 /mnt/test/foo/fifo2_link
> 
> Fix this by allowing linking of special files to the project quota
> if special files doesn't have any ID set (ID = 0).

Reasonable.

> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5ca561634164..641270f4d794 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1232,11 +1232,24 @@ xfs_link(
>  	 * the tree quota mechanism could be circumvented.
>  	 */
>  	if (unlikely((tdp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
> +		     !special_file(VFS_I(sip)->i_mode) &&
>  		     tdp->i_projid != sip->i_projid)) {
>  		error = -EXDEV;
>  		goto error_return;
>  	}
>  
> +	/*
> +	 * Don't allow cross-linking of special files. However, allow
> +	 * cross-linking if original file doesn't have any project.
> +	 */

The comment should explain why the code exists, not document what
the code does.

> +	if (unlikely((tdp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
> +				special_file(VFS_I(sip)->i_mode) &&
> +				sip->i_projid != 0 &&
> +				tdp->i_projid != sip->i_projid)) {
> +		error = -EXDEV;
> +		goto error_return;
> +	}

I think this would be better written as:

	/*
         * If we are using project inheritance, we only allow hard link
         * creation in our tree when the project IDs are the same; else
         * the tree quota mechanism could be circumvented.
         */
	if ((tdp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
	    tdp->i_projid != sip->i_projid) {
		/*
		 * Project quota setup skips special files which can
		 * leave inodes in a PROJINHERIT directory without a
		 * project ID set. We need to allow links to be made
		 * to these "project-less" inodes because userspace
		 * expects them to succeed after project ID setup,
		 * but everything else should be rejected.
		 */
		if (!special_file(VFS_I(sip)->i_mode) ||
		    sip->i_projid != 0) {
			error = -EXDEV;
			goto error_return;
		}
	}

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com