Skip to contentSkip to conversation

xfs: don't fail when converting shortform attr to long form during ATTR_REPLACE

PATCH In review Darrick J. Wong 3 participants 5 messages Apr 18, 2018 XFS filesystem

lore
Share & tools
lore

Darrick J. Wong

author
#0 lore raw

PATCH 1/1 xfs: don't fail when converting shortform attr to long form during ATTR_REPLACE

From: Darrick J. Wong <[email protected]>

Kanda Motohiro reported that expanding a tiny xattr into a large xattr fails on XFS because we remove the tiny xattr from a shortform fork and then try to re-add it after converting the fork to extents format having not removed the ATTR_REPLACE flag. This fails because the attr is no longer present, causing a fs shutdown.

This is derived from the patch in his bug report, but we really shouldn't ignore a nonzero retval from the remove call. Something's broken and we should bail out and shut down.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199119

Reported-by [email protected]
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/libxfs/xfs_attr.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_attr.c +8 -1
index ce4a34a..2588c73 100644
@@ -511,7 +511,14 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
 		if (args->flags & ATTR_CREATE)
 			return retval;
 		retval = xfs_attr_shortform_remove(args);
-		ASSERT(retval == 0);
+		if (retval)
+			return retval;
+		/*
+		 * Since we have removed the old attr here,
+		 * further lookup will fail with ENOATTR.
+		 * Ignore this was a replace and go on creating new attr.
+		 */
+		args->flags &= ~ATTR_REPLACE;
 	}
 
 	if (args->namelen >= XFS_ATTR_SF_ENTSIZE_MAX ||
#1 lore raw
Quoted 24 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Kanda Motohiro reported that expanding a tiny xattr into a large xattr fails on XFS because we remove the tiny xattr from a shortform fork and then try to re-add it after converting the fork to extents format having not removed the ATTR_REPLACE flag. This fails because the attr is no longer present, causing a fs shutdown.

This is derived from the patch in his bug report, but we really shouldn't ignore a nonzero retval from the remove call. Something's broken and we should bail out and shut down.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199119

Reported-by [email protected]
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/libxfs/xfs_attr.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_attr.c +0 -0
index ce4a34a..2588c73 100644

fs/xfs/libxfs/xfs_attr.c xfs_attr_shortform_addname(xfs_da_args_t *args) · line 519

Quoted 10 lines
fs/xfs/libxfs/xfs_attr.c +6 -1
@@ -511,7 +511,14 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
 		if (args->flags & ATTR_CREATE)
 			return retval;
 		retval = xfs_attr_shortform_remove(args);
-		ASSERT(retval == 0);
+		if (retval)
+			return retval;
+		/*
+		 * Since we have removed the old attr here,
+		 * further lookup will fail with ENOATTR.
+		 * Ignore this was a replace and go on creating new attr.

The last line of that comment doesn't make sense to me. not sure what it's meant to say...

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#2 lore raw
Quoted 30 lines · Dave Chinner
Quoted 29 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Kanda Motohiro reported that expanding a tiny xattr into a large xattr fails on XFS because we remove the tiny xattr from a shortform fork and then try to re-add it after converting the fork to extents format having not removed the ATTR_REPLACE flag. This fails because the attr is no longer present, causing a fs shutdown.

This is derived from the patch in his bug report, but we really shouldn't ignore a nonzero retval from the remove call. Something's broken and we should bail out and shut down.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199119

Reported-by [email protected]
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/libxfs/xfs_attr.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_attr.c +1 -1
index ce4a34a..2588c73 100644
@@ -511,7 +511,14 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
 		if (args->flags & ATTR_CREATE)
 			return retval;
 		retval = xfs_attr_shortform_remove(args);
-		ASSERT(retval == 0);
+		if (retval)
Quoted 8 lines

fs/xfs/libxfs/xfs_attr.c xfs_attr_shortform_addname(xfs_da_args_t *args) · line 519

Quoted 5 lines
fs/xfs/libxfs/xfs_attr.c +5 -0
+			return retval;
+		/*
+		 * Since we have removed the old attr here,
+		 * further lookup will fail with ENOATTR.
+		 * Ignore this was a replace and go on creating new attr.

The last line of that comment doesn't make sense to me. not sure what it's meant to say...

That was what was in the patch attached to the bugzilla. :)

How about:

/*
* Since we have removed the old attr, clear ATTR_REPLACE so that the
* leaf format add routine won't trip over the attr not being around. */

instead?

--D

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines
Dave Chinner
[email protected]
--
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
Reviewed-by
#3 lore raw
Quoted 42 lines · Darrick J. Wong
Quoted 30 lines · Dave Chinner
Quoted 29 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Kanda Motohiro reported that expanding a tiny xattr into a large xattr fails on XFS because we remove the tiny xattr from a shortform fork and then try to re-add it after converting the fork to extents format having not removed the ATTR_REPLACE flag. This fails because the attr is no longer present, causing a fs shutdown.

This is derived from the patch in his bug report, but we really shouldn't ignore a nonzero retval from the remove call. Something's broken and we should bail out and shut down.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199119

Reported-by [email protected]
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/libxfs/xfs_attr.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_attr.c +1 -1
index ce4a34a..2588c73 100644
@@ -511,7 +511,14 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
 		if (args->flags & ATTR_CREATE)
 			return retval;
 		retval = xfs_attr_shortform_remove(args);
-		ASSERT(retval == 0);
+		if (retval)
Quoted 8 lines

fs/xfs/libxfs/xfs_attr.c xfs_attr_shortform_addname(xfs_da_args_t *args) · line 519

Quoted 5 lines
fs/xfs/libxfs/xfs_attr.c +5 -0
+			return retval;
+		/*
+		 * Since we have removed the old attr here,
+		 * further lookup will fail with ENOATTR.
+		 * Ignore this was a replace and go on creating new attr.

The last line of that comment doesn't make sense to me. not sure what it's meant to say...

That was what was in the patch attached to the bugzilla. :)

Quoted 8 lines

How about:

/*
* Since we have removed the old attr, clear ATTR_REPLACE so that the
* leaf format add routine won't trip over the attr not being around. */

instead?

Yup, that makes sense now.

Reviewed-by Dave Chinner <[email protected]>

-Dave.

Signature · 2 lines
Dave Chinner
[email protected]