Skip to contentSkip to conversation

xfs: don't set DAX flag for v2 inodes

PATCH Superseded Darrick J. Wong 3 participants 17 messages Aug 30, 2017 – Sep 1, 2017 XFS filesystem

lore
Share & tools
lore

Darrick J. Wong

author
#0 lore raw

PATCH 1/1 xfs: don't set DAX flag for v2 inodes

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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c |    4 ++++
 1 file changed, 4 insertions(+)
fs/xfs/xfs_ioctl.c +4 -0
index 9c0c7a9..fa17f89 100644
@@ -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.
#1 lore raw

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

author
#2 lore raw
Quoted 2 lines · Christoph Hellwig

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

Quoted 4 lines

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

author
#3 lore raw

PATCH v2 1/1 xfs: don't set v3 xflags for v2 inodes

In reply to 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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c |   37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)
fs/xfs/xfs_ioctl.c +29 -8
index 06ca244..82d14fe 100644
@@ -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);
#4 lore raw

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

Hmm. I thought of something like that patch below:

---
From: Christoph Hellwig <[email protected]>
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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
fs/xfs/xfs_ioctl.c +8 -3
index 9c0c7a920304..9fea768e9f70 100644
@@ -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);
Signature · 1 line
2.11.0
#5 lore raw

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

Quoted 29 lines · Christoph Hellwig

Hmm. I thought of something like that patch below:

---
From: Christoph Hellwig <[email protected]>
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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
fs/xfs/xfs_ioctl.c +1 -1
index 9c0c7a920304..9fea768e9f70 100644
@@ -931,7 +931,7 @@ xfs_ioc_fsgetxattr(
 	return 0;
 }
 
-STATIC void
+STATIC int
 xfs_set_diflags(
 	struct xfs_inode	*ip,
 	unsigned int		xflags)

fs/xfs/xfs_ioctl.c xfs_set_diflags( · line 977

Quoted 5 lines
fs/xfs/xfs_ioctl.c +1 -1
@@ -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

Quoted 38 lines
fs/xfs/xfs_ioctl.c +6 -1
 	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);
Signature · 6 lines
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#6 lore raw

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

Quoted 4 lines · Brian Foster

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.

#7 lore raw

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

In reply to Brian Foster

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

Quoted 1 line

From d4da594cd1c42b88673b7eae5187827a3099ec42 Mon Sep 17 00:00:00 2001

From: Christoph Hellwig <[email protected]>
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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
fs/xfs/xfs_ioctl.c +12 -7
index 9c0c7a920304..84a80210b2e1 100644
@@ -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);
Signature · 1 line
2.11.0
#8 lore raw

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

Quoted 4 lines · Christoph Hellwig

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

That looks reasonable/correct to me, fwiw.

Brian

Quoted 79 lines

From d4da594cd1c42b88673b7eae5187827a3099ec42 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
fs/xfs/xfs_ioctl.c +12 -7
index 9c0c7a920304..84a80210b2e1 100644
@@ -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);
Signature · 6 lines
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#9 lore raw

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

In reply to Christoph Hellwig
Quoted 43 lines · 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 <[email protected]>
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 <[email protected]>
---
 fs/xfs/xfs_ioctl.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
fs/xfs/xfs_ioctl.c +1 -6
index 9c0c7a920304..84a80210b2e1 100644
@@ -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)

fs/xfs/xfs_ioctl.c xfs_set_diflags( · line 982

Quoted 6 lines
fs/xfs/xfs_ioctl.c +3 -0
@@ -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

Quoted 34 lines
fs/xfs/xfs_ioctl.c +8 -1
+
+	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);
Signature · 6 lines
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#10 lore raw

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

Quoted 4 lines · Darrick J. Wong

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:

fs/xfs/xfs_ioctl.c +25 -13
index 9c0c7a920304..511cd7c830ab 100644
@@ -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);
author
#11 lore raw

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

Quoted 27 lines · Christoph Hellwig
Quoted 4 lines · Darrick J. Wong

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:

fs/xfs/xfs_ioctl.c +2 -2
index 9c0c7a920304..511cd7c830ab 100644
@@ -931,16 +931,15 @@ xfs_ioc_fsgetxattr(
 	return 0;
 }
 
-STATIC void
-xfs_set_diflags(
+STATIC unsigned int
+xfs_flags2diflags(

fs/xfs/xfs_ioctl.c xfs_ioc_fsgetxattr( · line 941

Quoted 10 lines
fs/xfs/xfs_ioctl.c +2 -4
 	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

Quoted 63 lines
fs/xfs/xfs_ioctl.c +21 -8
+
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw

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

Quoted 10 lines · Darrick J. Wong

fs/xfs/xfs_ioctl.c xfs_ioc_fsgetxattr( · line 941

Quoted 8 lines

{

fs/xfs/xfs_ioctl.c +2 -4
-	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.

Quoted 1 line

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

author
#13 lore raw

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

Quoted 15 lines · Christoph Hellwig
Quoted 10 lines · Darrick J. Wong

fs/xfs/xfs_ioctl.c xfs_ioc_fsgetxattr( · line 941

Quoted 8 lines

{

fs/xfs/xfs_ioctl.c +2 -4
-	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.

Quoted 6 lines
Quoted 1 line

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

Quoted 4 lines

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

author
#14 lore raw

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

Quoted 16 lines · Darrick J. Wong
Quoted 15 lines · Christoph Hellwig
Quoted 10 lines · Darrick J. Wong

fs/xfs/xfs_ioctl.c xfs_ioc_fsgetxattr( · line 941

Quoted 8 lines

{

fs/xfs/xfs_ioctl.c +2 -4
-	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.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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

Quoted 11 lines

--D

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#15 lore raw

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

Review

Quoted 6 lines · Darrick J. Wong
Quoted 1 line

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

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

(quoted diff) +1 -1
-       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..

Reviewed-by on v2 1/1
#16 lore raw

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

In reply to Darrick J. Wong
Quoted 5 lines · 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 <[email protected]>

---

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

Reviewed-by Brian Foster <[email protected]>
Quoted 84 lines
 fs/xfs/xfs_ioctl.c |   37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)
fs/xfs/xfs_ioctl.c +29 -9
index 06ca244..82d14fe 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html