[PATCH] xfs: don't set DAX flag for v2 inodes

[PATCH] xfs: don't set DAX flag for v2 inodes

[Darrick J. Wong]

The DAX flag can only be persisted for v3 inodes, so don't allow users
to set the flag on older filesystems.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9c0c7a9..fa17f89 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1043,6 +1043,10 @@ xfs_ioctl_setattr_xflags(
 	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
 		return -EINVAL;
 
+	/* Don't allow us to set DAX mode for a non-v3 inode. */
+	if ((fa->fsx_xflags & FS_XFLAG_DAX) && ip->i_d.di_version < 3)
+		return -EINVAL;
+
 	/*
 	 * Can't modify an immutable/append-only file unless
 	 * we have appropriate permission.

Re: [PATCH] xfs: don't set DAX flag for v2 inodes

[Christoph Hellwig]

How about returning an error from xfs_set_diflags for
anything that would go into di_flags2 for v2 inodes?

Re: [PATCH] xfs: don't set DAX flag for v2 inodes

[Darrick J. Wong]

On Wed, Aug 30, 2017 at 09:15:00AM -0700, Christoph Hellwig wrote:
> How about returning an error from xfs_set_diflags for
> anything that would go into di_flags2 for v2 inodes?

I had also thought about refactoring all those xfs_ioctl_setattr_xflags
validity checks into a single separate checking function, how about that?

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Darrick J. Wong]

Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
if the inode isn't a v3 inode, because di_flags2 only exists on v3.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 06ca244..82d14fe 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1016,32 +1016,37 @@ xfs_diflags_to_linux(
 #endif
 }
 
+#define XFS_V3_INODE_XFLAGS	(FS_XFLAG_DAX | \
+				 FS_XFLAG_COWEXTSIZE)
 static int
-xfs_ioctl_setattr_xflags(
-	struct xfs_trans	*tp,
+xfs_check_diflags(
 	struct xfs_inode	*ip,
-	struct fsxattr		*fa)
+	__u32			xflags)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 
+	/* Can't set flags2 fields on a v2 inode. */
+	if (ip->i_d.di_version < 3 && (xflags & XFS_V3_INODE_XFLAGS))
+		return -EINVAL;
+
 	/* Can't change realtime flag if any extents are allocated. */
 	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
-	    XFS_IS_REALTIME_INODE(ip) != (fa->fsx_xflags & FS_XFLAG_REALTIME))
+	    XFS_IS_REALTIME_INODE(ip) != (xflags & FS_XFLAG_REALTIME))
 		return -EINVAL;
 
 	/* If realtime flag is set then must have realtime device */
-	if (fa->fsx_xflags & FS_XFLAG_REALTIME) {
+	if (xflags & FS_XFLAG_REALTIME) {
 		if (mp->m_sb.sb_rblocks == 0 || mp->m_sb.sb_rextsize == 0 ||
 		    (ip->i_d.di_extsize % mp->m_sb.sb_rextsize))
 			return -EINVAL;
 	}
 
 	/* Clear reflink if we are actually able to set the rt flag. */
-	if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
+	if ((xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
 		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
 
 	/* Don't allow us to set DAX mode for a reflinked file for now. */
-	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
+	if ((xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
 		return -EINVAL;
 
 	/*
@@ -1049,10 +1054,26 @@ xfs_ioctl_setattr_xflags(
 	 * we have appropriate permission.
 	 */
 	if (((ip->i_d.di_flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND)) ||
-	     (fa->fsx_xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
+	     (xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
 	    !capable(CAP_LINUX_IMMUTABLE))
 		return -EPERM;
 
+	return 0;
+}
+
+static int
+xfs_ioctl_setattr_xflags(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	struct fsxattr		*fa)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	int			ret;
+
+	ret = xfs_check_diflags(ip, fa->fsx_xflags);
+	if (ret)
+		return ret;
+
 	xfs_set_diflags(ip, fa->fsx_xflags);
 	xfs_diflags_to_linux(ip);
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

Hmm.  I thought of something like that patch below:

---
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 31 Aug 2017 15:14:36 +0200
Subject: xfs: don't set v3 xflags for v2 inodes

Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
if the inode isn't a v3 inode, because di_flags2 only exists on v3.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9c0c7a920304..9fea768e9f70 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
 	return 0;
 }
 
-STATIC void
+STATIC int
 xfs_set_diflags(
 	struct xfs_inode	*ip,
 	unsigned int		xflags)
@@ -974,7 +974,7 @@ xfs_set_diflags(
 
 	/* diflags2 only valid for v3 inodes. */
 	if (ip->i_d.di_version < 3)
-		return;
+		return -EINVAL;
 
 	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
 	if (xflags & FS_XFLAG_DAX)
@@ -983,6 +983,7 @@ xfs_set_diflags(
 		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
 
 	ip->i_d.di_flags2 = di_flags2;
+	return 0;
 }
 
 STATIC void
@@ -1022,6 +1023,7 @@ xfs_ioctl_setattr_xflags(
 	struct fsxattr		*fa)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	int			error;
 
 	/* Can't change realtime flag if any extents are allocated. */
 	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
@@ -1052,7 +1054,10 @@ xfs_ioctl_setattr_xflags(
 	    !capable(CAP_LINUX_IMMUTABLE))
 		return -EPERM;
 
-	xfs_set_diflags(ip, fa->fsx_xflags);
+	error = xfs_set_diflags(ip, fa->fsx_xflags);
+	if (error)
+		return error;
+
 	xfs_diflags_to_linux(ip);
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-- 
2.11.0

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Brian Foster]

On Thu, Aug 31, 2017 at 06:17:23AM -0700, Christoph Hellwig wrote:
> Hmm.  I thought of something like that patch below:
> 
> ---
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 31 Aug 2017 15:14:36 +0200
> Subject: xfs: don't set v3 xflags for v2 inodes
> 
> Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
> if the inode isn't a v3 inode, because di_flags2 only exists on v3.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 9c0c7a920304..9fea768e9f70 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
>  	return 0;
>  }
>  
> -STATIC void
> +STATIC int
>  xfs_set_diflags(
>  	struct xfs_inode	*ip,
>  	unsigned int		xflags)
> @@ -974,7 +974,7 @@ xfs_set_diflags(
>  
>  	/* diflags2 only valid for v3 inodes. */
>  	if (ip->i_d.di_version < 3)
> -		return;
> +		return -EINVAL;

Don't we need to check that v3 flags are actually attempted on the v2
inode before returning failure? Also, what about the just previous
di_flags update if an error occurs and the transaction ends up
cancelled?

Brian

>  
>  	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
>  	if (xflags & FS_XFLAG_DAX)
> @@ -983,6 +983,7 @@ xfs_set_diflags(
>  		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  
>  	ip->i_d.di_flags2 = di_flags2;
> +	return 0;
>  }
>  
>  STATIC void
> @@ -1022,6 +1023,7 @@ xfs_ioctl_setattr_xflags(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
>  
>  	/* Can't change realtime flag if any extents are allocated. */
>  	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> @@ -1052,7 +1054,10 @@ xfs_ioctl_setattr_xflags(
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> -	xfs_set_diflags(ip, fa->fsx_xflags);
> +	error = xfs_set_diflags(ip, fa->fsx_xflags);
> +	if (error)
> +		return error;
> +
>  	xfs_diflags_to_linux(ip);
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

On Thu, Aug 31, 2017 at 09:34:20AM -0400, Brian Foster wrote:
> Don't we need to check that v3 flags are actually attempted on the v2
> inode before returning failure? Also, what about the just previous
> di_flags update if an error occurs and the transaction ends up
> cancelled?

True.  Let me think about this a bit more.

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

So maybe something like this (totally untested, will attempt for
a QA run over the night):


>From d4da594cd1c42b88673b7eae5187827a3099ec42 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 31 Aug 2017 15:14:36 +0200
Subject: xfs: don't set v3 xflags for v2 inodes

Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
if the inode isn't a v3 inode, because di_flags2 only exists on v3.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9c0c7a920304..84a80210b2e1 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
 	return 0;
 }
 
-STATIC void
+STATIC int
 xfs_set_diflags(
 	struct xfs_inode	*ip,
 	unsigned int		xflags)
@@ -970,11 +970,6 @@ xfs_set_diflags(
 		if (xflags & FS_XFLAG_EXTSIZE)
 			di_flags |= XFS_DIFLAG_EXTSIZE;
 	}
-	ip->i_d.di_flags = di_flags;
-
-	/* diflags2 only valid for v3 inodes. */
-	if (ip->i_d.di_version < 3)
-		return;
 
 	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
 	if (xflags & FS_XFLAG_DAX)
@@ -982,7 +977,13 @@ xfs_set_diflags(
 	if (xflags & FS_XFLAG_COWEXTSIZE)
 		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
 
+	/* diflags2 only valid for v3 inodes. */
+	if (di_flags2 && ip->i_d.di_version < 3)
+		return -EINVAL;
+
+	ip->i_d.di_flags = di_flags;
 	ip->i_d.di_flags2 = di_flags2;
+	return 0;
 }
 
 STATIC void
@@ -1022,6 +1023,7 @@ xfs_ioctl_setattr_xflags(
 	struct fsxattr		*fa)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	int			error;
 
 	/* Can't change realtime flag if any extents are allocated. */
 	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
@@ -1052,7 +1054,10 @@ xfs_ioctl_setattr_xflags(
 	    !capable(CAP_LINUX_IMMUTABLE))
 		return -EPERM;
 
-	xfs_set_diflags(ip, fa->fsx_xflags);
+	error = xfs_set_diflags(ip, fa->fsx_xflags);
+	if (error)
+		return error;
+
 	xfs_diflags_to_linux(ip);
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-- 
2.11.0

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Brian Foster]

On Thu, Aug 31, 2017 at 07:09:32AM -0700, Christoph Hellwig wrote:
> So maybe something like this (totally untested, will attempt for
> a QA run over the night):
> 
> 

That looks reasonable/correct to me, fwiw.

Brian

> From d4da594cd1c42b88673b7eae5187827a3099ec42 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 31 Aug 2017 15:14:36 +0200
> Subject: xfs: don't set v3 xflags for v2 inodes
> 
> Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
> if the inode isn't a v3 inode, because di_flags2 only exists on v3.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 9c0c7a920304..84a80210b2e1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
>  	return 0;
>  }
>  
> -STATIC void
> +STATIC int
>  xfs_set_diflags(
>  	struct xfs_inode	*ip,
>  	unsigned int		xflags)
> @@ -970,11 +970,6 @@ xfs_set_diflags(
>  		if (xflags & FS_XFLAG_EXTSIZE)
>  			di_flags |= XFS_DIFLAG_EXTSIZE;
>  	}
> -	ip->i_d.di_flags = di_flags;
> -
> -	/* diflags2 only valid for v3 inodes. */
> -	if (ip->i_d.di_version < 3)
> -		return;
>  
>  	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
>  	if (xflags & FS_XFLAG_DAX)
> @@ -982,7 +977,13 @@ xfs_set_diflags(
>  	if (xflags & FS_XFLAG_COWEXTSIZE)
>  		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  
> +	/* diflags2 only valid for v3 inodes. */
> +	if (di_flags2 && ip->i_d.di_version < 3)
> +		return -EINVAL;
> +
> +	ip->i_d.di_flags = di_flags;
>  	ip->i_d.di_flags2 = di_flags2;
> +	return 0;
>  }
>  
>  STATIC void
> @@ -1022,6 +1023,7 @@ xfs_ioctl_setattr_xflags(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
>  
>  	/* Can't change realtime flag if any extents are allocated. */
>  	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> @@ -1052,7 +1054,10 @@ xfs_ioctl_setattr_xflags(
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> -	xfs_set_diflags(ip, fa->fsx_xflags);
> +	error = xfs_set_diflags(ip, fa->fsx_xflags);
> +	if (error)
> +		return error;
> +
>  	xfs_diflags_to_linux(ip);
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Darrick J. Wong]

On Thu, Aug 31, 2017 at 07:09:32AM -0700, Christoph Hellwig wrote:
> So maybe something like this (totally untested, will attempt for
> a QA run over the night):
> 
> 
> From d4da594cd1c42b88673b7eae5187827a3099ec42 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 31 Aug 2017 15:14:36 +0200
> Subject: xfs: don't set v3 xflags for v2 inodes
> 
> Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
> if the inode isn't a v3 inode, because di_flags2 only exists on v3.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 9c0c7a920304..84a80210b2e1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
>  	return 0;
>  }
>  
> -STATIC void
> +STATIC int
>  xfs_set_diflags(
>  	struct xfs_inode	*ip,
>  	unsigned int		xflags)
> @@ -970,11 +970,6 @@ xfs_set_diflags(
>  		if (xflags & FS_XFLAG_EXTSIZE)
>  			di_flags |= XFS_DIFLAG_EXTSIZE;
>  	}
> -	ip->i_d.di_flags = di_flags;
> -
> -	/* diflags2 only valid for v3 inodes. */
> -	if (ip->i_d.di_version < 3)
> -		return;
>  
>  	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
>  	if (xflags & FS_XFLAG_DAX)
> @@ -982,7 +977,13 @@ xfs_set_diflags(
>  	if (xflags & FS_XFLAG_COWEXTSIZE)
>  		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  
> +	/* diflags2 only valid for v3 inodes. */
> +	if (di_flags2 && ip->i_d.di_version < 3)
> +		return -EINVAL;

TBH I like this less because now the responsibility for checking valid
inputs is split between xfs_ioctl_setattr_xflags and xfs_set_diflags.
I'd rather increase the function count by one than morph the setting
function into check-and-set.

Though for debugging purposes it might be useful to ASSERT that we're
not trying to set nonzero di_flags2 on a v2 inode.  How about that?

--D

> +
> +	ip->i_d.di_flags = di_flags;
>  	ip->i_d.di_flags2 = di_flags2;
> +	return 0;
>  }
>  
>  STATIC void
> @@ -1022,6 +1023,7 @@ xfs_ioctl_setattr_xflags(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
>  
>  	/* Can't change realtime flag if any extents are allocated. */
>  	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> @@ -1052,7 +1054,10 @@ xfs_ioctl_setattr_xflags(
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> -	xfs_set_diflags(ip, fa->fsx_xflags);
> +	error = xfs_set_diflags(ip, fa->fsx_xflags);
> +	if (error)
> +		return error;
> +
>  	xfs_diflags_to_linux(ip);
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

On Thu, Aug 31, 2017 at 12:57:29PM -0700, Darrick J. Wong wrote:
> TBH I like this less because now the responsibility for checking valid
> inputs is split between xfs_ioctl_setattr_xflags and xfs_set_diflags.
> I'd rather increase the function count by one than morph the setting
> function into check-and-set.

I'm not worried about the function count - I'm worried about duplicating
the information of which flags are stored in di_flags2.  With this patch
we have one point where we can naturally check this.  In your patch
we need another define that needs to be kept uptodate.

If you are worried about the check and set we could move the set into
the caller, but to me that doesn't seem any cleaner.  Example attached
below:

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9c0c7a920304..511cd7c830ab 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -931,16 +931,15 @@ xfs_ioc_fsgetxattr(
 	return 0;
 }
 
-STATIC void
-xfs_set_diflags(
+STATIC unsigned int
+xfs_flags2diflags(
 	struct xfs_inode	*ip,
 	unsigned int		xflags)
 {
-	unsigned int		di_flags;
-	uint64_t		di_flags2;
-
 	/* can't set PREALLOC this way, just preserve it */
-	di_flags = (ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
+	unsigned int		di_flags =
+		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
+
 	if (xflags & FS_XFLAG_IMMUTABLE)
 		di_flags |= XFS_DIFLAG_IMMUTABLE;
 	if (xflags & FS_XFLAG_APPEND)
@@ -970,19 +969,24 @@ xfs_set_diflags(
 		if (xflags & FS_XFLAG_EXTSIZE)
 			di_flags |= XFS_DIFLAG_EXTSIZE;
 	}
-	ip->i_d.di_flags = di_flags;
 
-	/* diflags2 only valid for v3 inodes. */
-	if (ip->i_d.di_version < 3)
-		return;
+	return di_flags;
+}
+
+STATIC uint64_t
+xfs_flags2diflags2(
+	struct xfs_inode	*ip,
+	unsigned int		xflags)
+{
+	uint64_t		di_flags2 =
+		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
 
-	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
 	if (xflags & FS_XFLAG_DAX)
 		di_flags2 |= XFS_DIFLAG2_DAX;
 	if (xflags & FS_XFLAG_COWEXTSIZE)
 		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
 
-	ip->i_d.di_flags2 = di_flags2;
+	return di_flags2;
 }
 
 STATIC void
@@ -1022,6 +1026,7 @@ xfs_ioctl_setattr_xflags(
 	struct fsxattr		*fa)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	uint64_t		di_flags2;
 
 	/* Can't change realtime flag if any extents are allocated. */
 	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
@@ -1052,7 +1057,14 @@ xfs_ioctl_setattr_xflags(
 	    !capable(CAP_LINUX_IMMUTABLE))
 		return -EPERM;
 
-	xfs_set_diflags(ip, fa->fsx_xflags);
+	/* diflags2 only valid for v3 inodes. */
+	di_flags2 = xfs_flags2diflags(ip, fa->fsx_xflags);
+	if (di_flags2 && ip->i_d.di_version < 3)
+		return -EINVAL;
+
+	ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
+	ip->i_d.di_flags2 = di_flags2;
+
 	xfs_diflags_to_linux(ip);
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Darrick J. Wong]

On Fri, Sep 01, 2017 at 12:21:49AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 31, 2017 at 12:57:29PM -0700, Darrick J. Wong wrote:
> > TBH I like this less because now the responsibility for checking valid
> > inputs is split between xfs_ioctl_setattr_xflags and xfs_set_diflags.
> > I'd rather increase the function count by one than morph the setting
> > function into check-and-set.
> 
> I'm not worried about the function count - I'm worried about duplicating
> the information of which flags are stored in di_flags2.  With this patch
> we have one point where we can naturally check this.  In your patch
> we need another define that needs to be kept uptodate.
> 
> If you are worried about the check and set we could move the set into
> the caller, but to me that doesn't seem any cleaner.  Example attached
> below:
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 9c0c7a920304..511cd7c830ab 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -931,16 +931,15 @@ xfs_ioc_fsgetxattr(
>  	return 0;
>  }
>  
> -STATIC void
> -xfs_set_diflags(
> +STATIC unsigned int
> +xfs_flags2diflags(
>  	struct xfs_inode	*ip,
>  	unsigned int		xflags)
>  {
> -	unsigned int		di_flags;
> -	uint64_t		di_flags2;
> -
>  	/* can't set PREALLOC this way, just preserve it */
> -	di_flags = (ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> +	unsigned int		di_flags =
> +		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);

ip->i_d.di_flags is uint16_t, so di_flags ought to match, right?

Otherwise, I guess this looks ok, want to send it as a real patch?

--D

> +
>  	if (xflags & FS_XFLAG_IMMUTABLE)
>  		di_flags |= XFS_DIFLAG_IMMUTABLE;
>  	if (xflags & FS_XFLAG_APPEND)
> @@ -970,19 +969,24 @@ xfs_set_diflags(
>  		if (xflags & FS_XFLAG_EXTSIZE)
>  			di_flags |= XFS_DIFLAG_EXTSIZE;
>  	}
> -	ip->i_d.di_flags = di_flags;
>  
> -	/* diflags2 only valid for v3 inodes. */
> -	if (ip->i_d.di_version < 3)
> -		return;
> +	return di_flags;
> +}
> +
> +STATIC uint64_t
> +xfs_flags2diflags2(
> +	struct xfs_inode	*ip,
> +	unsigned int		xflags)
> +{
> +	uint64_t		di_flags2 =
> +		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
>  
> -	di_flags2 = (ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
>  	if (xflags & FS_XFLAG_DAX)
>  		di_flags2 |= XFS_DIFLAG2_DAX;
>  	if (xflags & FS_XFLAG_COWEXTSIZE)
>  		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  
> -	ip->i_d.di_flags2 = di_flags2;
> +	return di_flags2;
>  }
>  
>  STATIC void
> @@ -1022,6 +1026,7 @@ xfs_ioctl_setattr_xflags(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> +	uint64_t		di_flags2;
>  
>  	/* Can't change realtime flag if any extents are allocated. */
>  	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> @@ -1052,7 +1057,14 @@ xfs_ioctl_setattr_xflags(
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> -	xfs_set_diflags(ip, fa->fsx_xflags);
> +	/* diflags2 only valid for v3 inodes. */
> +	di_flags2 = xfs_flags2diflags(ip, fa->fsx_xflags);
> +	if (di_flags2 && ip->i_d.di_version < 3)
> +		return -EINVAL;
> +
> +	ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> +	ip->i_d.di_flags2 = di_flags2;
> +
>  	xfs_diflags_to_linux(ip);
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

On Fri, Sep 01, 2017 at 10:52:23AM -0700, Darrick J. Wong wrote:
> >  {
> > -	unsigned int		di_flags;
> > -	uint64_t		di_flags2;
> > -
> >  	/* can't set PREALLOC this way, just preserve it */
> > -	di_flags = (ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > +	unsigned int		di_flags =
> > +		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> 
> ip->i_d.di_flags is uint16_t, so di_flags ought to match, right?

The existing code uses unsigned int as seen above.  But yes, it
could be fixed to be a uint16_t.

> Otherwise, I guess this looks ok, want to send it as a real patch?

Sure.  Doing some quick QA runs and it will be out.

Note that I'll assume it'll be for a tree without the previous
patch, unlike current for-next..

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Darrick J. Wong]

On Fri, Sep 01, 2017 at 12:29:58PM -0700, Christoph Hellwig wrote:
> On Fri, Sep 01, 2017 at 10:52:23AM -0700, Darrick J. Wong wrote:
> > >  {
> > > -	unsigned int		di_flags;
> > > -	uint64_t		di_flags2;
> > > -
> > >  	/* can't set PREALLOC this way, just preserve it */
> > > -	di_flags = (ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > > +	unsigned int		di_flags =
> > > +		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > 
> > ip->i_d.di_flags is uint16_t, so di_flags ought to match, right?
> 
> The existing code uses unsigned int as seen above.  But yes, it
> could be fixed to be a uint16_t.
> 
> > Otherwise, I guess this looks ok, want to send it as a real patch?
> 
> Sure.  Doing some quick QA runs and it will be out.
> 
> Note that I'll assume it'll be for a tree without the previous
> patch, unlike current for-next..

Yeah, I'll rebase that whole mess...

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Darrick J. Wong]

On Fri, Sep 01, 2017 at 12:40:24PM -0700, Darrick J. Wong wrote:
> On Fri, Sep 01, 2017 at 12:29:58PM -0700, Christoph Hellwig wrote:
> > On Fri, Sep 01, 2017 at 10:52:23AM -0700, Darrick J. Wong wrote:
> > > >  {
> > > > -	unsigned int		di_flags;
> > > > -	uint64_t		di_flags2;
> > > > -
> > > >  	/* can't set PREALLOC this way, just preserve it */
> > > > -	di_flags = (ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > > > +	unsigned int		di_flags =
> > > > +		(ip->i_d.di_flags & XFS_DIFLAG_PREALLOC);
> > > 
> > > ip->i_d.di_flags is uint16_t, so di_flags ought to match, right?
> > 
> > The existing code uses unsigned int as seen above.  But yes, it
> > could be fixed to be a uint16_t.
> > 
> > > Otherwise, I guess this looks ok, want to send it as a real patch?
> > 
> > Sure.  Doing some quick QA runs and it will be out.
> > 
> > Note that I'll assume it'll be for a tree without the previous
> > patch, unlike current for-next..
> 
> Yeah, I'll rebase that whole mess...

...also, I assume you already fixed up:

-       di_flags2 = xfs_flags2diflags(ip, fa->fsx_xflags);
+       di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);

?

--D

> 
> --D
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Christoph Hellwig]

On Fri, Sep 01, 2017 at 01:06:32PM -0700, Darrick J. Wong wrote:
> > Yeah, I'll rebase that whole mess...
> 
> ...also, I assume you already fixed up:
> 
> -       di_flags2 = xfs_flags2diflags(ip, fa->fsx_xflags);
> +       di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);

I finally did half an hour ago after starting at crashes that didn't make
any sense for far too long..

Re: [PATCH v2] xfs: don't set v3 xflags for v2 inodes

[Brian Foster]

On Wed, Aug 30, 2017 at 09:38:25AM -0700, Darrick J. Wong wrote:
> Reject attempts to set XFLAGS that correspond to di_flags2 inode flags
> if the inode isn't a v3 inode, because di_flags2 only exists on v3.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

That lazy reflink flag clear still seems hacky. That aside, this looks
fine to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_ioctl.c |   37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 06ca244..82d14fe 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1016,32 +1016,37 @@ xfs_diflags_to_linux(
>  #endif
>  }
>  
> +#define XFS_V3_INODE_XFLAGS	(FS_XFLAG_DAX | \
> +				 FS_XFLAG_COWEXTSIZE)
>  static int
> -xfs_ioctl_setattr_xflags(
> -	struct xfs_trans	*tp,
> +xfs_check_diflags(
>  	struct xfs_inode	*ip,
> -	struct fsxattr		*fa)
> +	__u32			xflags)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  
> +	/* Can't set flags2 fields on a v2 inode. */
> +	if (ip->i_d.di_version < 3 && (xflags & XFS_V3_INODE_XFLAGS))
> +		return -EINVAL;
> +
>  	/* Can't change realtime flag if any extents are allocated. */
>  	if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> -	    XFS_IS_REALTIME_INODE(ip) != (fa->fsx_xflags & FS_XFLAG_REALTIME))
> +	    XFS_IS_REALTIME_INODE(ip) != (xflags & FS_XFLAG_REALTIME))
>  		return -EINVAL;
>  
>  	/* If realtime flag is set then must have realtime device */
> -	if (fa->fsx_xflags & FS_XFLAG_REALTIME) {
> +	if (xflags & FS_XFLAG_REALTIME) {
>  		if (mp->m_sb.sb_rblocks == 0 || mp->m_sb.sb_rextsize == 0 ||
>  		    (ip->i_d.di_extsize % mp->m_sb.sb_rextsize))
>  			return -EINVAL;
>  	}
>  
>  	/* Clear reflink if we are actually able to set the rt flag. */
> -	if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
> +	if ((xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
>  		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
>  
>  	/* Don't allow us to set DAX mode for a reflinked file for now. */
> -	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
> +	if ((xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
>  		return -EINVAL;
>  
>  	/*
> @@ -1049,10 +1054,26 @@ xfs_ioctl_setattr_xflags(
>  	 * we have appropriate permission.
>  	 */
>  	if (((ip->i_d.di_flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND)) ||
> -	     (fa->fsx_xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
> +	     (xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> +	return 0;
> +}
> +
> +static int
> +xfs_ioctl_setattr_xflags(
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	struct fsxattr		*fa)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			ret;
> +
> +	ret = xfs_check_diflags(ip, fa->fsx_xflags);
> +	if (ret)
> +		return ret;
> +
>  	xfs_set_diflags(ip, fa->fsx_xflags);
>  	xfs_diflags_to_linux(ip);
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html