[patch] detect and correct bad features2 superblock field

[patch] detect and correct bad features2 superblock field

[David Chinner]

There is a bug in mkfs.xfs that can result in writing the features2
field in the superblock to the wrong location. This only occurs
on some architectures, typically those with 32 bit userspace and
64 bit kernels.

This patch detects the defect at mount time, logs a warning
such as:

XFS: correcting sb_features alignment problem

in dmesg and corrects the problem so that everything is OK.
it also blacklists the bad field in the superblock so it does
not get used for something else later on.

Signed-off-by: Dave Chinner <dgc@sgi.com>
---
 fs/xfs/xfs_mount.c |   34 ++++++++++++++++++++++++++++------
 fs/xfs/xfs_sb.h    |   37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 62 insertions(+), 9 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_mount.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_mount.c	2007-11-22 10:25:25.590278381 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_mount.c	2007-11-22 10:31:27.891999961 +1100
@@ -44,7 +44,7 @@
 #include "xfs_quota.h"
 #include "xfs_fsops.h"
 
-STATIC void	xfs_mount_log_sbunit(xfs_mount_t *, __int64_t);
+STATIC void	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
 STATIC int	xfs_uuid_mount(xfs_mount_t *);
 STATIC void	xfs_uuid_unmount(xfs_mount_t *mp);
 STATIC void	xfs_unmountfs_wait(xfs_mount_t *);
@@ -119,6 +119,7 @@ static const struct {
     { offsetof(xfs_sb_t, sb_logsectsize),0 },
     { offsetof(xfs_sb_t, sb_logsunit),	 0 },
     { offsetof(xfs_sb_t, sb_features2),	 0 },
+    { offsetof(xfs_sb_t, sb_bad_features2), 0 },
     { sizeof(xfs_sb_t),			 0 }
 };
 
@@ -455,6 +456,7 @@ xfs_sb_from_disk(
 	to->sb_logsectsize = be16_to_cpu(from->sb_logsectsize);
 	to->sb_logsunit = be32_to_cpu(from->sb_logsunit);
 	to->sb_features2 = be32_to_cpu(from->sb_features2);
+	to->sb_bad_features2 = be32_to_cpu(from->sb_bad_features2);
 }
 
 /*
@@ -976,6 +978,26 @@ xfs_mountfs(
 	xfs_mount_common(mp, sbp);
 
 	/*
+	 * Check for a bad features2 field alignment. This happened on
+	 * some platforms due to xfs_sb_t not being 64bit size aligned
+	 * when sb_features was added and hence the compiler put it in
+	 * the wrong place.
+	 *
+	 * If we detect a bad field, we or the set bits into the existing
+	 * features2 field in case it has already been modified and we
+	 * don't want to lose any features. Zero the bad one and mark
+	 * the two fields as needing updates once the transaction subsystem
+	 * is online.
+	 */
+	if (xfs_sb_has_bad_features2(sbp)) {
+		cmn_err(CE_WARN,
+			"XFS: correcting sb_features alignment problem");
+		sbp->sb_features2 |= sbp->sb_bad_features2;
+		sbp->sb_bad_features2 = 0;
+		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
+	}
+
+	/*
 	 * Check if sb_agblocks is aligned at stripe boundary
 	 * If sb_agblocks is NOT aligned turn off m_dalign since
 	 * allocator alignment is within an ag, therefore ag has
@@ -1165,11 +1187,10 @@ xfs_mountfs(
 	}
 
 	/*
-	 * If fs is not mounted readonly, then update the superblock
-	 * unit and width changes.
+	 * If fs is not mounted readonly, then update the superblock changes.
 	 */
 	if (update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY))
-		xfs_mount_log_sbunit(mp, update_flags);
+		xfs_mount_log_sb(mp, update_flags);
 
 	/*
 	 * Initialise the XFS quota management subsystem for this mount
@@ -1884,13 +1905,14 @@ xfs_uuid_unmount(
  * be altered by the mount options. Only the first superblock is updated.
  */
 STATIC void
-xfs_mount_log_sbunit(
+xfs_mount_log_sb(
 	xfs_mount_t	*mp,
 	__int64_t	fields)
 {
 	xfs_trans_t	*tp;
 
-	ASSERT(fields & (XFS_SB_UNIT|XFS_SB_WIDTH|XFS_SB_UUID));
+	ASSERT(fields & (XFS_SB_UNIT | XFS_SB_WIDTH | XFS_SB_UUID |
+			 XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2));
 
 	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
 	if (xfs_trans_reserve(tp, 0, mp->m_sb.sb_sectsize + 128, 0, 0,
Index: 2.6.x-xfs-new/fs/xfs/xfs_sb.h
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_sb.h	2007-11-22 10:25:25.598277360 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_sb.h	2007-11-22 10:31:27.891999961 +1100
@@ -89,6 +89,7 @@ struct xfs_mount;
 
 /*
  * Superblock - in core version.  Must match the ondisk version below.
+ * Must be padded to 64 bit alignment.
  */
 typedef struct xfs_sb {
 	__uint32_t	sb_magicnum;	/* magic number == XFS_SB_MAGIC */
@@ -145,10 +146,21 @@ typedef struct xfs_sb {
 	__uint16_t	sb_logsectsize;	/* sector size for the log, bytes */
 	__uint32_t	sb_logsunit;	/* stripe unit size for the log */
 	__uint32_t	sb_features2;	/* additional feature bits */
+
+	/*
+	 * bad features2 field as a result of failing to pad the sb
+	 * structure to 64 bits. Some machines will be using this field
+	 * for features2 bits. Easiest just to mark it bad and not use
+	 * it for anything else.
+	 */
+	__uint32_t	sb_bad_features2;
+
+	/* must be padded to 64 bit alignment */
 } xfs_sb_t;
 
 /*
- * Superblock - on disk version.  Must match the in core version below.
+ * Superblock - on disk version.  Must match the in core version above.
+ * Must be padded to 64 bit alignment.
  */
 typedef struct xfs_dsb {
 	__be32		sb_magicnum;	/* magic number == XFS_SB_MAGIC */
@@ -205,6 +217,15 @@ typedef struct xfs_dsb {
 	__be16		sb_logsectsize;	/* sector size for the log, bytes */
 	__be32		sb_logsunit;	/* stripe unit size for the log */
 	__be32		sb_features2;	/* additional feature bits */
+	/*
+	 * bad features2 field as a result of failing to pad the sb
+	 * structure to 64 bits. Some machines will be using this field
+	 * for features2 bits. Easiest just to mark it bad and not use
+	 * it for anything else.
+	 */
+	__be32	sb_bad_features2;
+
+	/* must be padded to 64 bit alignment */
 } xfs_dsb_t;
 
 /*
@@ -223,7 +244,7 @@ typedef enum {
 	XFS_SBS_GQUOTINO, XFS_SBS_QFLAGS, XFS_SBS_FLAGS, XFS_SBS_SHARED_VN,
 	XFS_SBS_INOALIGNMT, XFS_SBS_UNIT, XFS_SBS_WIDTH, XFS_SBS_DIRBLKLOG,
 	XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
-	XFS_SBS_FEATURES2,
+	XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2,
 	XFS_SBS_FIELDCOUNT
 } xfs_sb_field_t;
 
@@ -248,13 +269,15 @@ typedef enum {
 #define XFS_SB_IFREE		XFS_SB_MVAL(IFREE)
 #define XFS_SB_FDBLOCKS		XFS_SB_MVAL(FDBLOCKS)
 #define XFS_SB_FEATURES2	XFS_SB_MVAL(FEATURES2)
+#define XFS_SB_BAD_FEATURES2	XFS_SB_MVAL(BAD_FEATURES2)
 #define	XFS_SB_NUM_BITS		((int)XFS_SBS_FIELDCOUNT)
 #define	XFS_SB_ALL_BITS		((1LL << XFS_SB_NUM_BITS) - 1)
 #define	XFS_SB_MOD_BITS		\
 	(XFS_SB_UUID | XFS_SB_ROOTINO | XFS_SB_RBMINO | XFS_SB_RSUMINO | \
 	 XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO | XFS_SB_GQUOTINO | \
 	 XFS_SB_QFLAGS | XFS_SB_SHARED_VN | XFS_SB_UNIT | XFS_SB_WIDTH | \
-	 XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2)
+	 XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2 | \
+	 XFS_SB_BAD_FEATURES2)
 
 
 /*
@@ -297,6 +320,14 @@ static inline int xfs_sb_good_version(xf
 }
 #endif /* __KERNEL__ */
 
+/*
+ * Detect a bad features2 field
+ */
+static inline int xfs_sb_has_bad_features2(xfs_sb_t *sbp)
+{
+	return (sbp->sb_bad_features2 != 0);
+}
+
 #define	XFS_SB_VERSION_TONEW(v)	xfs_sb_version_tonew(v)
 static inline unsigned xfs_sb_version_tonew(unsigned v)
 {

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

David Chinner wrote:
> There is a bug in mkfs.xfs that can result in writing the features2
> field in the superblock to the wrong location. This only occurs
> on some architectures, typically those with 32 bit userspace and
> 64 bit kernels.
> 
> This patch detects the defect at mount time, logs a warning
> such as:
> 
> XFS: correcting sb_features alignment problem
> 
> in dmesg and corrects the problem so that everything is OK.
> it also blacklists the bad field in the superblock so it does
> not get used for something else later on.

Looks good to me, thanks for fixing it up properly.

You can add Reviewed-by: or Acked-by: or whatever is appropriate...
Eric Sandeen <sandeen@sandeen.net>

Thanks,
-Eric

> Signed-off-by: Dave Chinner <dgc@sgi.com>
> ---
>  fs/xfs/xfs_mount.c |   34 ++++++++++++++++++++++++++++------
>  fs/xfs/xfs_sb.h    |   37 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 62 insertions(+), 9 deletions(-)
> 
> Index: 2.6.x-xfs-new/fs/xfs/xfs_mount.c
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_mount.c	2007-11-22 10:25:25.590278381 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_mount.c	2007-11-22 10:31:27.891999961 +1100
> @@ -44,7 +44,7 @@
>  #include "xfs_quota.h"
>  #include "xfs_fsops.h"
>  
> -STATIC void	xfs_mount_log_sbunit(xfs_mount_t *, __int64_t);
> +STATIC void	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
>  STATIC int	xfs_uuid_mount(xfs_mount_t *);
>  STATIC void	xfs_uuid_unmount(xfs_mount_t *mp);
>  STATIC void	xfs_unmountfs_wait(xfs_mount_t *);
> @@ -119,6 +119,7 @@ static const struct {
>      { offsetof(xfs_sb_t, sb_logsectsize),0 },
>      { offsetof(xfs_sb_t, sb_logsunit),	 0 },
>      { offsetof(xfs_sb_t, sb_features2),	 0 },
> +    { offsetof(xfs_sb_t, sb_bad_features2), 0 },
>      { sizeof(xfs_sb_t),			 0 }
>  };
>  
> @@ -455,6 +456,7 @@ xfs_sb_from_disk(
>  	to->sb_logsectsize = be16_to_cpu(from->sb_logsectsize);
>  	to->sb_logsunit = be32_to_cpu(from->sb_logsunit);
>  	to->sb_features2 = be32_to_cpu(from->sb_features2);
> +	to->sb_bad_features2 = be32_to_cpu(from->sb_bad_features2);
>  }
>  
>  /*
> @@ -976,6 +978,26 @@ xfs_mountfs(
>  	xfs_mount_common(mp, sbp);
>  
>  	/*
> +	 * Check for a bad features2 field alignment. This happened on
> +	 * some platforms due to xfs_sb_t not being 64bit size aligned
> +	 * when sb_features was added and hence the compiler put it in
> +	 * the wrong place.
> +	 *
> +	 * If we detect a bad field, we or the set bits into the existing
> +	 * features2 field in case it has already been modified and we
> +	 * don't want to lose any features. Zero the bad one and mark
> +	 * the two fields as needing updates once the transaction subsystem
> +	 * is online.
> +	 */
> +	if (xfs_sb_has_bad_features2(sbp)) {
> +		cmn_err(CE_WARN,
> +			"XFS: correcting sb_features alignment problem");
> +		sbp->sb_features2 |= sbp->sb_bad_features2;
> +		sbp->sb_bad_features2 = 0;
> +		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
> +	}
> +
> +	/*
>  	 * Check if sb_agblocks is aligned at stripe boundary
>  	 * If sb_agblocks is NOT aligned turn off m_dalign since
>  	 * allocator alignment is within an ag, therefore ag has
> @@ -1165,11 +1187,10 @@ xfs_mountfs(
>  	}
>  
>  	/*
> -	 * If fs is not mounted readonly, then update the superblock
> -	 * unit and width changes.
> +	 * If fs is not mounted readonly, then update the superblock changes.
>  	 */
>  	if (update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY))
> -		xfs_mount_log_sbunit(mp, update_flags);
> +		xfs_mount_log_sb(mp, update_flags);
>  
>  	/*
>  	 * Initialise the XFS quota management subsystem for this mount
> @@ -1884,13 +1905,14 @@ xfs_uuid_unmount(
>   * be altered by the mount options. Only the first superblock is updated.
>   */
>  STATIC void
> -xfs_mount_log_sbunit(
> +xfs_mount_log_sb(
>  	xfs_mount_t	*mp,
>  	__int64_t	fields)
>  {
>  	xfs_trans_t	*tp;
>  
> -	ASSERT(fields & (XFS_SB_UNIT|XFS_SB_WIDTH|XFS_SB_UUID));
> +	ASSERT(fields & (XFS_SB_UNIT | XFS_SB_WIDTH | XFS_SB_UUID |
> +			 XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2));
>  
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
>  	if (xfs_trans_reserve(tp, 0, mp->m_sb.sb_sectsize + 128, 0, 0,
> Index: 2.6.x-xfs-new/fs/xfs/xfs_sb.h
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_sb.h	2007-11-22 10:25:25.598277360 +1100
> +++ 2.6.x-xfs-new/fs/xfs/xfs_sb.h	2007-11-22 10:31:27.891999961 +1100
> @@ -89,6 +89,7 @@ struct xfs_mount;
>  
>  /*
>   * Superblock - in core version.  Must match the ondisk version below.
> + * Must be padded to 64 bit alignment.
>   */
>  typedef struct xfs_sb {
>  	__uint32_t	sb_magicnum;	/* magic number == XFS_SB_MAGIC */
> @@ -145,10 +146,21 @@ typedef struct xfs_sb {
>  	__uint16_t	sb_logsectsize;	/* sector size for the log, bytes */
>  	__uint32_t	sb_logsunit;	/* stripe unit size for the log */
>  	__uint32_t	sb_features2;	/* additional feature bits */
> +
> +	/*
> +	 * bad features2 field as a result of failing to pad the sb
> +	 * structure to 64 bits. Some machines will be using this field
> +	 * for features2 bits. Easiest just to mark it bad and not use
> +	 * it for anything else.
> +	 */
> +	__uint32_t	sb_bad_features2;
> +
> +	/* must be padded to 64 bit alignment */
>  } xfs_sb_t;
>  
>  /*
> - * Superblock - on disk version.  Must match the in core version below.
> + * Superblock - on disk version.  Must match the in core version above.
> + * Must be padded to 64 bit alignment.
>   */
>  typedef struct xfs_dsb {
>  	__be32		sb_magicnum;	/* magic number == XFS_SB_MAGIC */
> @@ -205,6 +217,15 @@ typedef struct xfs_dsb {
>  	__be16		sb_logsectsize;	/* sector size for the log, bytes */
>  	__be32		sb_logsunit;	/* stripe unit size for the log */
>  	__be32		sb_features2;	/* additional feature bits */
> +	/*
> +	 * bad features2 field as a result of failing to pad the sb
> +	 * structure to 64 bits. Some machines will be using this field
> +	 * for features2 bits. Easiest just to mark it bad and not use
> +	 * it for anything else.
> +	 */
> +	__be32	sb_bad_features2;
> +
> +	/* must be padded to 64 bit alignment */
>  } xfs_dsb_t;
>  
>  /*
> @@ -223,7 +244,7 @@ typedef enum {
>  	XFS_SBS_GQUOTINO, XFS_SBS_QFLAGS, XFS_SBS_FLAGS, XFS_SBS_SHARED_VN,
>  	XFS_SBS_INOALIGNMT, XFS_SBS_UNIT, XFS_SBS_WIDTH, XFS_SBS_DIRBLKLOG,
>  	XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
> -	XFS_SBS_FEATURES2,
> +	XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2,
>  	XFS_SBS_FIELDCOUNT
>  } xfs_sb_field_t;
>  
> @@ -248,13 +269,15 @@ typedef enum {
>  #define XFS_SB_IFREE		XFS_SB_MVAL(IFREE)
>  #define XFS_SB_FDBLOCKS		XFS_SB_MVAL(FDBLOCKS)
>  #define XFS_SB_FEATURES2	XFS_SB_MVAL(FEATURES2)
> +#define XFS_SB_BAD_FEATURES2	XFS_SB_MVAL(BAD_FEATURES2)
>  #define	XFS_SB_NUM_BITS		((int)XFS_SBS_FIELDCOUNT)
>  #define	XFS_SB_ALL_BITS		((1LL << XFS_SB_NUM_BITS) - 1)
>  #define	XFS_SB_MOD_BITS		\
>  	(XFS_SB_UUID | XFS_SB_ROOTINO | XFS_SB_RBMINO | XFS_SB_RSUMINO | \
>  	 XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO | XFS_SB_GQUOTINO | \
>  	 XFS_SB_QFLAGS | XFS_SB_SHARED_VN | XFS_SB_UNIT | XFS_SB_WIDTH | \
> -	 XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2)
> +	 XFS_SB_ICOUNT | XFS_SB_IFREE | XFS_SB_FDBLOCKS | XFS_SB_FEATURES2 | \
> +	 XFS_SB_BAD_FEATURES2)
>  
>  
>  /*
> @@ -297,6 +320,14 @@ static inline int xfs_sb_good_version(xf
>  }
>  #endif /* __KERNEL__ */
>  
> +/*
> + * Detect a bad features2 field
> + */
> +static inline int xfs_sb_has_bad_features2(xfs_sb_t *sbp)
> +{
> +	return (sbp->sb_bad_features2 != 0);
> +}
> +
>  #define	XFS_SB_VERSION_TONEW(v)	xfs_sb_version_tonew(v)
>  static inline unsigned xfs_sb_version_tonew(unsigned v)
>  {
> 
> 

Re: [patch] detect and correct bad features2 superblock field

[Christoph Hellwig]

On Wed, Feb 20, 2008 at 04:40:41PM +1100, David Chinner wrote:
> There is a bug in mkfs.xfs that can result in writing the features2
> field in the superblock to the wrong location. This only occurs
> on some architectures, typically those with 32 bit userspace and
> 64 bit kernels.

Well, we don't use different ABIs for kernel vs userspace so some
kernels will get it wrong aswell, you just won't notice until moving
to a different box because userspace is the same.

> +
> +	/* must be padded to 64 bit alignment */
>  } xfs_dsb_t;

I'm pretty sure there is some gcc __packed__ magic to enfore that,
might it be worth to poke some gcc experts to add it?


But the actual patch looks fine, ACK from me.

Re: [patch] detect and correct bad features2 superblock field

[David Chinner]

On Wed, Feb 20, 2008 at 02:13:28PM -0500, Christoph Hellwig wrote:
> On Wed, Feb 20, 2008 at 04:40:41PM +1100, David Chinner wrote:
> > There is a bug in mkfs.xfs that can result in writing the features2
> > field in the superblock to the wrong location. This only occurs
> > on some architectures, typically those with 32 bit userspace and
> > 64 bit kernels.
> 
> Well, we don't use different ABIs for kernel vs userspace so some
> kernels will get it wrong aswell, you just won't notice until moving
> to a different box because userspace is the same.

True.

> > +
> > +	/* must be padded to 64 bit alignment */
> >  } xfs_dsb_t;
> 
> I'm pretty sure there is some gcc __packed__ magic to enfore that,
> might it be worth to poke some gcc experts to add it?

Yes, it's probably the right thing to do. However, if we are going
to "pack" disk structures, I'd like to do that all in one series
of patches rather than mixed up in other fixes....

> But the actual patch looks fine, ACK from me.

Thanks.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

David Chinner wrote:
> There is a bug in mkfs.xfs that can result in writing the features2
> field in the superblock to the wrong location. This only occurs
> on some architectures, typically those with 32 bit userspace and
> 64 bit kernels.
>
> This patch detects the defect at mount time, logs a warning
> such as:
> ...

>  	/*
> +	 * Check for a bad features2 field alignment. This happened on
> +	 * some platforms due to xfs_sb_t not being 64bit size aligned
> +	 * when sb_features was added and hence the compiler put it in
> +	 * the wrong place.
> +	 *
> +	 * If we detect a bad field, we or the set bits into the existing
> +	 * features2 field in case it has already been modified and we
> +	 * don't want to lose any features. Zero the bad one and mark
> +	 * the two fields as needing updates once the transaction subsystem
> +	 * is online.
> +	 */
> +	if (xfs_sb_has_bad_features2(sbp)) {
> +		cmn_err(CE_WARN,
> +			"XFS: correcting sb_features alignment problem");
> +		sbp->sb_features2 |= sbp->sb_bad_features2;
> +		sbp->sb_bad_features2 = 0;
> +		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
> +	}
>   
I think there's a minor problem here that while this will update the
superblock with the proper features2 values, features2 has already been
checked, so mp->m_flags won't have, for example, the attr2 flags...

So attr2 will show up next time, but not on this mount.

This probably wouldn't normally matter, except in a weird corner case
I think I've found:

x86_64 set attr2 in bad_features2
bad_features2 was found, kernel & userspace pad the same
filesystem created attr2 attributes
hch's sb endianness annotation actually made bad_features2 *not* found
mount after that thinks there is no attr2
another corner case bug corrupts the fs, would be avoided if attr2 were not lost.

-Eric

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

Eric Sandeen wrote:
> David Chinner wrote:
>> There is a bug in mkfs.xfs that can result in writing the features2
>> field in the superblock to the wrong location. This only occurs
>> on some architectures, typically those with 32 bit userspace and
>> 64 bit kernels.
>>
>> This patch detects the defect at mount time, logs a warning
>> such as:
>> ...
> 
>>  	/*
>> +	 * Check for a bad features2 field alignment. This happened on
>> +	 * some platforms due to xfs_sb_t not being 64bit size aligned
>> +	 * when sb_features was added and hence the compiler put it in
>> +	 * the wrong place.
>> +	 *
>> +	 * If we detect a bad field, we or the set bits into the existing
>> +	 * features2 field in case it has already been modified and we
>> +	 * don't want to lose any features. Zero the bad one and mark
>> +	 * the two fields as needing updates once the transaction subsystem
>> +	 * is online.
>> +	 */
>> +	if (xfs_sb_has_bad_features2(sbp)) {
>> +		cmn_err(CE_WARN,
>> +			"XFS: correcting sb_features alignment problem");
>> +		sbp->sb_features2 |= sbp->sb_bad_features2;
>> +		sbp->sb_bad_features2 = 0;
>> +		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
>> +	}
>>   
> I think there's a minor problem here that while this will update the
> superblock with the proper features2 values, features2 has already been
> checked, so mp->m_flags won't have, for example, the attr2 flags...
> 
> So attr2 will show up next time, but not on this mount.

How's this look for a fixup:

========================

[XFS]: set ATTR2 in m_flags if flag found in bad_features2

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---


Index: linux-2.6.24.x86_64/fs/xfs/xfs_mount.c
===================================================================
--- linux-2.6.24.x86_64.orig/fs/xfs/xfs_mount.c
+++ linux-2.6.24.x86_64/fs/xfs/xfs_mount.c
@@ -1925,6 +1925,12 @@ xfs_mount_log_sb(
 	}
 	xfs_mod_sb(tp, fields);
 	xfs_trans_commit(tp, 0);
+
+	/* if we updated features2, recheck attr2 & set flag */
+	if ((fields & (XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2)) &&
+				XFS_SB_VERSION_HASATTR2(&mp->m_sb)) {
+		mp->m_flags |= XFS_MOUNT_ATTR2;
+	}
 }

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

David Chinner wrote:
> There is a bug in mkfs.xfs that can result in writing the features2
> field in the superblock to the wrong location. This only occurs
> on some architectures, typically those with 32 bit userspace and
> 64 bit kernels.
> 
> This patch detects the defect at mount time, logs a warning
> such as:
> 
> XFS: correcting sb_features alignment problem
> 
> in dmesg and corrects the problem so that everything is OK.
> it also blacklists the bad field in the superblock so it does
> not get used for something else later on.

...

>  	/*
> +	 * Check for a bad features2 field alignment. This happened on
> +	 * some platforms due to xfs_sb_t not being 64bit size aligned
> +	 * when sb_features was added and hence the compiler put it in
> +	 * the wrong place.
> +	 *
> +	 * If we detect a bad field, we or the set bits into the existing
> +	 * features2 field in case it has already been modified and we
> +	 * don't want to lose any features. Zero the bad one and mark
> +	 * the two fields as needing updates once the transaction subsystem
> +	 * is online.
> +	 */
> +	if (xfs_sb_has_bad_features2(sbp)) {
> +		cmn_err(CE_WARN,
> +			"XFS: correcting sb_features alignment problem");
> +		sbp->sb_features2 |= sbp->sb_bad_features2;
> +		sbp->sb_bad_features2 = 0;
> +		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
> +	}

Hm, the other problem here may be that if we zero bad_features2, then
any older kernel will mount up as attr2... and run into the corruption
problem I found on F8...

Should we make features2 and bad_features2 match rather than zeroing
bad_features2?

-Eric

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

Eric Sandeen wrote:

> Hm, the other problem here may be that if we zero bad_features2, then
> any older kernel will mount up as attr2... and run into the corruption
> problem I found on F8...
>   
Er, as attr1

> Should we make features2 and bad_features2 match rather than zeroing
> bad_features2?
>   
Maybe like this...?  Though I suppose there's still a minor issue
with older tools modifying flags...

diff -u linux-2.6.24.x86_64/fs/xfs/xfs_sb.h linux-2.6.24.x86_64/fs/xfs/xfs_sb.h
--- linux-2.6.24.x86_64/fs/xfs/xfs_sb.h
+++ linux-2.6.24.x86_64/fs/xfs/xfs_sb.h
@@ -325,7 +325,7 @@
  */
 static inline int xfs_sb_has_bad_features2(xfs_sb_t *sbp)
 {
-	return (sbp->sb_bad_features2 != 0);
+	return (sbp->sb_features2 != sbp->sb_bad_features2);
 }
 
 #define	XFS_SB_VERSION_TONEW(v)	xfs_sb_version_tonew(v)
diff -u linux-2.6.24.x86_64/fs/xfs/xfs_mount.c linux-2.6.24.x86_64/fs/xfs/xfs_mount.c
--- linux-2.6.24.x86_64/fs/xfs/xfs_mount.c
+++ linux-2.6.24.x86_64/fs/xfs/xfs_mount.c
@@ -994,7 +994,7 @@
 		cmn_err(CE_WARN,
 			"XFS: correcting sb_features alignment problem");
 		sbp->sb_features2 |= sbp->sb_bad_features2;
-		sbp->sb_bad_features2 = 0;
+		sbp->sb_bad_features2 = sbp->sb_features2;
 		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
 	}
 

Re: [patch] detect and correct bad features2 superblock field

[Josef 'Jeff' Sipek]

On Sat, Mar 29, 2008 at 08:30:00PM -0500, Eric Sandeen wrote:
> David Chinner wrote:
> > There is a bug in mkfs.xfs that can result in writing the features2
> > field in the superblock to the wrong location. This only occurs
> > on some architectures, typically those with 32 bit userspace and
> > 64 bit kernels.
> > 
> > This patch detects the defect at mount time, logs a warning
> > such as:
> > 
> > XFS: correcting sb_features alignment problem
> > 
> > in dmesg and corrects the problem so that everything is OK.
> > it also blacklists the bad field in the superblock so it does
> > not get used for something else later on.
> 
> ...
> 
> >  	/*
> > +	 * Check for a bad features2 field alignment. This happened on
> > +	 * some platforms due to xfs_sb_t not being 64bit size aligned
> > +	 * when sb_features was added and hence the compiler put it in
> > +	 * the wrong place.
> > +	 *
> > +	 * If we detect a bad field, we or the set bits into the existing
> > +	 * features2 field in case it has already been modified and we
> > +	 * don't want to lose any features. Zero the bad one and mark
> > +	 * the two fields as needing updates once the transaction subsystem
> > +	 * is online.
> > +	 */
> > +	if (xfs_sb_has_bad_features2(sbp)) {
> > +		cmn_err(CE_WARN,
> > +			"XFS: correcting sb_features alignment problem");
> > +		sbp->sb_features2 |= sbp->sb_bad_features2;
> > +		sbp->sb_bad_features2 = 0;
> > +		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
> > +	}
> 
> Hm, the other problem here may be that if we zero bad_features2, then
> any older kernel will mount up as attr2... and run into the corruption
> problem I found on F8...
> 
> Should we make features2 and bad_features2 match rather than zeroing
> bad_features2?

I thought that was discussed here (or was it on IRC?), and the conclusion
was the best way is to always have features2 == bad_features2.  It is the
safest way to handle things - the filesystem is guaranteed to work
everywhere properly (old & new kernels).  Both the userspace (xfs_repair)
and kernel have to of course do the same thing (or bad_features2 with
features2, and save the result in both locations).

At least that's what I seem to remember.

Josef 'Jeff' Sipek.

-- 
Once you have their hardware. Never give it back.
(The First Rule of Hardware Acquisition)

Re: [patch] detect and correct bad features2 superblock field

[Eric Sandeen]

Josef 'Jeff' Sipek wrote:
> On Sat, Mar 29, 2008 at 08:30:00PM -0500, Eric Sandeen wrote:

>> Hm, the other problem here may be that if we zero bad_features2, then
>> any older kernel will mount up as attr2... and run into the corruption
>> problem I found on F8...
>>
>> Should we make features2 and bad_features2 match rather than zeroing
>> bad_features2?
> 
> I thought that was discussed here (or was it on IRC?), and the conclusion
> was the best way is to always have features2 == bad_features2.  It is the
> safest way to handle things - the filesystem is guaranteed to work
> everywhere properly (old & new kernels).  Both the userspace (xfs_repair)
> and kernel have to of course do the same thing (or bad_features2 with
> features2, and save the result in both locations).
> 
> At least that's what I seem to remember.
> 
> Josef 'Jeff' Sipek.
> 

It might have been, but it's not what was checked in... *shrug*

http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/xfs_mount.c#rev1.419


-Eric

Re: [patch] detect and correct bad features2 superblock field

[Josef 'Jeff' Sipek]

On Sat, Mar 29, 2008 at 11:53:40PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
> > On Sat, Mar 29, 2008 at 08:30:00PM -0500, Eric Sandeen wrote:
> 
> >> Hm, the other problem here may be that if we zero bad_features2, then
> >> any older kernel will mount up as attr2... and run into the corruption
> >> problem I found on F8...
> >>
> >> Should we make features2 and bad_features2 match rather than zeroing
> >> bad_features2?
> > 
> > I thought that was discussed here (or was it on IRC?), and the conclusion
> > was the best way is to always have features2 == bad_features2.  It is the
> > safest way to handle things - the filesystem is guaranteed to work
> > everywhere properly (old & new kernels).  Both the userspace (xfs_repair)
> > and kernel have to of course do the same thing (or bad_features2 with
> > features2, and save the result in both locations).
> > 
> > At least that's what I seem to remember.
> > 
> > Josef 'Jeff' Sipek.
> > 
> 
> It might have been, but it's not what was checked in... *shrug*
> 
> http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/xfs_mount.c#rev1.419

I remember the discussion taking place _after_ that commit..so it must have
been userspace-related.

Josef 'Jeff' Sipek.

-- 
I'm somewhere between geek and normal.
		- Linus Torvalds