xfsprogs: metadump/mdrestore warns about dirty journal

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c +0 -0

 		break;
 	case -1:
 		/* log detection error */

Signature · 6 lines

2.1.4

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

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

Brian

Quoted 10 lines

db/metadump.c +0 -0

 		break;
 	case -1:
 		/* log detection error */

Signature · 6 lines

2.1.4

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

Quoted 32 lines · Brian Foster

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

Quoted 3 lines

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool.

The other bummer is that we cannot detect whether an image has been obfuscated, so the restore tool can only say "if it's obfuscated...."

-Eric

Quoted 8 lines

Brian

Quoted 6 lines

db/metadump.c +0 -0

 		break;
 	case -1:
 		/* log detection error */

Signature · 1 line

2.1.4

Quoted 9 lines

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

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

Quoted 33 lines · Eric Sandeen

Quoted 32 lines · Brian Foster

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

Quoted 8 lines

Quoted 3 lines

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool.

Hm, yeah. Maybe the existing warning can be condensed a bit more to something like:

"Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause filesystem corruption. Please mount the source image to clean the log or disable obfuscation."

I suppose we could also just exit under such conditions unless the user passes a force flag or something. Maybe that's overkill too, though..

Quoted 46 lines · Brian Foster

Quoted 33 lines · Eric Sandeen

Quoted 32 lines · Brian Foster

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

Quoted 8 lines

Quoted 3 lines

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool.

Hm, yeah. Maybe the existing warning can be condensed a bit more to something like:

Quoted 3 lines

"Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause filesystem corruption. Please mount the source image to clean the log or disable obfuscation."

s/filesystem corruption/image corruption/ - we don't want anyone to think that it damaged the original fs!

Quoted 3 lines

I suppose we could also just exit under such conditions unless the user passes a force flag or something. Maybe that's overkill too, though..

yeah, let's not overengineer - dumping a dirty, obfuscated log is quite typical I think.

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

Quoted 51 lines · Eric Sandeen

Quoted 49 lines · Brian Foster

Quoted 33 lines · Eric Sandeen

Quoted 32 lines · Brian Foster

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

Quoted 8 lines

Quoted 3 lines

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool.

Hm, yeah. Maybe the existing warning can be condensed a bit more to something like:

"Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause filesystem corruption. Please mount the source image to clean the log or disable obfuscation."

Quoted 2 lines

s/filesystem corruption/image corruption/ - we don't want anyone to think that it damaged the original fs!

OTOH the fs might be damaged just badly enough that log recovery is impossible (which is why we're creating the metadump to send to support) so aborting metadump is the wrong thing to do.

It seems sort of silly even to lecture the user about log recovery when they might not be able to recover said log and might not ever even start the log recovery process.

--D

Quoted 4 lines

Quoted 3 lines

I suppose we could also just exit under such conditions unless the user passes a force flag or something. Maybe that's overkill too, though..

Quoted 8 lines

yeah, let's not overengineer - dumping a dirty, obfuscated log is quite typical I think.

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

Quoted 52 lines · Darrick J. Wong

Quoted 51 lines · Eric Sandeen

Quoted 49 lines · Brian Foster

Quoted 33 lines · Eric Sandeen

Quoted 32 lines · Brian Foster

Quoted 5 lines · Jan Tulak

Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata.

Signed-off-by Jan Tulak <[email protected]>

---

Thanks for posting this...

Quoted 8 lines

 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

db/metadump.c +0 -0

index 66952f6..74e24b2 100644

db/metadump.c copy_log(void) · line 2730

Quoted 6 lines

db/metadump.c +2 -1

@@ -2726,7 +2726,8 @@ copy_log(void)
               /* keep the dirty log */
               if (obfuscate)
                       print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Filesystem log is dirty; image will contain unobfuscated metadata in log "
+  "and a log replay can cause a corruption."));

I think a slightly more verbose message might be a good idea. For example, something like the following:

"Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation."

Quoted 8 lines

Quoted 3 lines

That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric?

I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool.

Hm, yeah. Maybe the existing warning can be condensed a bit more to something like:

"Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause filesystem corruption. Please mount the source image to clean the log or disable obfuscation."

Quoted 7 lines

Quoted 2 lines

s/filesystem corruption/image corruption/ - we don't want anyone to think that it damaged the original fs!

OTOH the fs might be damaged just badly enough that log recovery is impossible (which is why we're creating the metadump to send to support) so aborting metadump is the wrong thing to do.

Indeed..

Quoted 4 lines

It seems sort of silly even to lecture the user about log recovery when they might not be able to recover said log and might not ever even start the log recovery process.

There's a bit of a balance here between handling dirty log + obfuscation cases where a log recovery issue is the purpose of the metadump, it is not the purpose and the original fs can run log recovery without disrupting problem diagnosis, or something in between and the resulting image is explicitly verified to not lead to such metadump-specific corruption.

I think adjusting the preexisting warning wrt to the fact that log data is not obfuscated to point out this issue is a relatively minor change given the potential result.. (put another way, it seems kind of silly that we'd warn about leaking unobfuscated data but not this..).

Brian

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

---
 mdrestore/Makefile        |  4 +--
 mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

mdrestore/Makefile +0 -0

index 5171306..6355df9 100644

mdrestore/Makefile +0 -0

 default: depend $(LTCOMMAND)

mdrestore/xfs_mdrestore.c +51 -0

index 0d399f1..3797955 100644
@@ -17,6 +17,7 @@
  */
 
 #include "libxfs.h"
+#include "libxlog.h"
 #include "xfs_metadump.h"
 
 char 		*progname;

@@ -190,6 +191,87 @@ perform_restore(
 	free(metablock);
 }
 
+/* workaround craziness in the xlog routines */
+int xlog_recover_do_trans(struct xlog *log, xlog_recover_t *t, int p)
+{
+	return 0;
+}
+
+/*
+ * Warn if we just wrote a dump with a dirty log.
+ */
+void
+test_dirty_log(
+	bool	is_target_file,
+	char*	target_name)
+{
+	struct xfs_sb	*sbp;
+	struct xfs_buf	*bp;
+	struct xfs_mount	xmount;
+	struct xfs_mount	*mp;
+	struct xlog		xlog;
+	libxfs_init_t		x;
+
+	x.isreadonly = LIBXFS_ISREADONLY;
+	if (is_target_file) {
+		x.dname = target_name;
+		x.disfile = true;
+	} else {
+		x.disfile = false;
+		x.volname = target_name;
+	}
+
+	if (!libxfs_init(&x)) {
+		fatal(_("\nfatal error -- couldn't initialize XFS library\n"),
+		      strerror(errno));
+	}
+
+	memset(&xmount, 0, sizeof(struct xfs_mount));
+	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
+	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error) {
+		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
+			"bytes)\n"), progname, target_name);
+		exit(1);
+	}
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);

mdrestore/xfs_mdrestore.c +10 -0

+		case 0:
+			 /* Everything is ok. */
+			break;
+	}
+
+}
+
+
 static void
 usage(void)
 {

@@ -271,5 +353,7 @@ main(
 	if (src_f != stdin)
 		fclose(src_f);
 
+	test_dirty_log(is_target_file, argv[optind]);
+
 	return 0;
 }

Signature · 6 lines

2.1.4

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

Quoted 18 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

---
 mdrestore/Makefile        |  4 +--
 mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

mdrestore/Makefile +0 -0

index 5171306..6355df9 100644

mdrestore/Makefile include $(TOPDIR)/include/builddefs · line 14

Quoted 9 lines

mdrestore/Makefile +2 -2

@@ -8,8 +8,8 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = xfs_mdrestore
 CFILES = xfs_mdrestore.c
 
-LLDLIBS = $(LIBXFS) $(LIBRT) $(LIBPTHREAD) $(LIBUUID)
-LTDEPENDENCIES = $(LIBXFS)
+LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBRT) $(LIBPTHREAD) $(LIBUUID)
+LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG)
 LLDFLAGS = -static
 

FYI, I get the following on a 'make -j 4':

...
Building mdrestore
gmake[3]: *** No rule to make target '../libxlog/libxlog.la', needed by 'xfs_mdrestore'. Stop. gmake[3]: *** Waiting for unfinished jobs....

It succeeds if I restart the build so I suspect something might be up with the dependency tracking here.

Quoted 74 lines

mdrestore/Makefile +0 -0

 default: depend $(LTCOMMAND)

mdrestore/xfs_mdrestore.c +58 -0

index 0d399f1..3797955 100644
@@ -17,6 +17,7 @@
  */
 
 #include "libxfs.h"
+#include "libxlog.h"
 #include "xfs_metadump.h"
 
 char 		*progname;

@@ -190,6 +191,87 @@ perform_restore(
 	free(metablock);
 }
 
+/* workaround craziness in the xlog routines */
+int xlog_recover_do_trans(struct xlog *log, xlog_recover_t *t, int p)
+{
+	return 0;
+}
+
+/*
+ * Warn if we just wrote a dump with a dirty log.
+ */
+void
+test_dirty_log(
+	bool	is_target_file,
+	char*	target_name)
+{
+	struct xfs_sb	*sbp;
+	struct xfs_buf	*bp;
+	struct xfs_mount	xmount;
+	struct xfs_mount	*mp;
+	struct xlog		xlog;
+	libxfs_init_t		x;
+
+	x.isreadonly = LIBXFS_ISREADONLY;
+	if (is_target_file) {
+		x.dname = target_name;
+		x.disfile = true;
+	} else {
+		x.disfile = false;
+		x.volname = target_name;
+	}
+
+	if (!libxfs_init(&x)) {
+		fatal(_("\nfatal error -- couldn't initialize XFS library\n"),
+		      strerror(errno));
+	}
+
+	memset(&xmount, 0, sizeof(struct xfs_mount));
+	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
+	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error) {
+		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
+			"bytes)\n"), progname, target_name);
+		exit(1);
+	}
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	sbp = &xmount.m_sb;
+	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
+			  LIBXFS_MOUNT_DEBUGGER);
+	if (!mp) {
+		fprintf(stderr,
+			_("%s: restored device %s unusable (not an XFS filesystem?)\n"),

mdrestore/xfs_mdrestore.c perform_restore( · line 253

Quoted 3 lines

mdrestore/xfs_mdrestore.c +3 -0

+			progname, target_name);
+		exit(1);
+	}

It might be cleaner to bury all of this init stuff in a separate init function to be called before the log check. We also may not want to exit the program if parsing the log or something happens to fail, given that this is a debug tool.

mdrestore/xfs_mdrestore.c perform_restore( · line 266

Quoted 13 lines

mdrestore/xfs_mdrestore.c +13 -0

+
+	switch (xlog_is_dirty(mp, &xlog, &x,0)) {
+		case -1:
+			/* An error occured and we can't read the log. */
+			fprintf(stderr,
+			_("Warning: can't discern a log.\n"));
+			break;
+		case 1:
+			/* The log is dirty, warn. */
+			fprintf(stderr,
+			_("Warning: The log is dirty. If the image was obfuscated, "
+			  "an attempt to replay the log may lead to corruption.\n"));
+			break;

Then the above can remain in the test_log_dirty() helper or just be open-coded at the end of main(), provided the mount was initialized successfully (or we could still warn if we can't make sense of the log).

BTW, this is going to warn on every xfs_mdrestore of an image with a dirty log, right? That is slightly unfortunate, if so. Do we have any method to track or determine whether an image is obfuscated (I'm guessing not easily...)?

Brian

Quoted 26 lines

mdrestore/xfs_mdrestore.c +10 -0

+		case 0:
+			 /* Everything is ok. */
+			break;
+	}
+
+}
+
+
 static void
 usage(void)
 {

@@ -271,5 +353,7 @@ main(
 	if (src_f != stdin)
 		fclose(src_f);
 
+	test_dirty_log(is_target_file, argv[optind]);
+
 	return 0;
 }

Signature · 6 lines

2.1.4

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

Quoted 31 lines · Brian Foster

Quoted 18 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

---
 mdrestore/Makefile        |  4 +--
 mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

mdrestore/Makefile +0 -0

index 5171306..6355df9 100644

mdrestore/Makefile include $(TOPDIR)/include/builddefs · line 14

Quoted 9 lines

mdrestore/Makefile +2 -2

@@ -8,8 +8,8 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = xfs_mdrestore
 CFILES = xfs_mdrestore.c
 
-LLDLIBS = $(LIBXFS) $(LIBRT) $(LIBPTHREAD) $(LIBUUID)
-LTDEPENDENCIES = $(LIBXFS)
+LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBRT) $(LIBPTHREAD) $(LIBUUID)
+LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG)
 LLDFLAGS = -static
 

FYI, I get the following on a 'make -j 4':

Quoted 7 lines

...
Building mdrestore
gmake[3]: *** No rule to make target '../libxlog/libxlog.la', needed by 'xfs_mdrestore'. Stop. gmake[3]: *** Waiting for unfinished jobs....

It succeeds if I restart the build so I suspect something might be up with the dependency tracking here.

need something like this in the top Makefile:

repair: libxlog libxcmd
copy: libxlog
mkfs: libxcmd
+mdrestore: libxlog

Quoted 74 lines

Quoted 74 lines

mdrestore/Makefile +0 -0

 default: depend $(LTCOMMAND)

mdrestore/xfs_mdrestore.c +58 -0

index 0d399f1..3797955 100644
@@ -17,6 +17,7 @@
  */
 
 #include "libxfs.h"
+#include "libxlog.h"
 #include "xfs_metadump.h"
 
 char 		*progname;

@@ -190,6 +191,87 @@ perform_restore(
 	free(metablock);
 }
 
+/* workaround craziness in the xlog routines */
+int xlog_recover_do_trans(struct xlog *log, xlog_recover_t *t, int p)
+{
+	return 0;
+}
+
+/*
+ * Warn if we just wrote a dump with a dirty log.
+ */
+void
+test_dirty_log(
+	bool	is_target_file,
+	char*	target_name)
+{
+	struct xfs_sb	*sbp;
+	struct xfs_buf	*bp;
+	struct xfs_mount	xmount;
+	struct xfs_mount	*mp;
+	struct xlog		xlog;
+	libxfs_init_t		x;
+
+	x.isreadonly = LIBXFS_ISREADONLY;
+	if (is_target_file) {
+		x.dname = target_name;
+		x.disfile = true;
+	} else {
+		x.disfile = false;
+		x.volname = target_name;
+	}
+
+	if (!libxfs_init(&x)) {
+		fatal(_("\nfatal error -- couldn't initialize XFS library\n"),
+		      strerror(errno));
+	}
+
+	memset(&xmount, 0, sizeof(struct xfs_mount));
+	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
+	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error) {
+		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
+			"bytes)\n"), progname, target_name);
+		exit(1);
+	}
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	sbp = &xmount.m_sb;
+	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
+			  LIBXFS_MOUNT_DEBUGGER);
+	if (!mp) {
+		fprintf(stderr,
+			_("%s: restored device %s unusable (not an XFS filesystem?)\n"),

Quoted 8 lines

mdrestore/xfs_mdrestore.c perform_restore( · line 253

Quoted 3 lines

mdrestore/xfs_mdrestore.c +3 -0

+			progname, target_name);
+		exit(1);
+	}

It might be cleaner to bury all of this init stuff in a separate init function to be called before the log check. We also may not want to exit the program if parsing the log or something happens to fail, given that this is a debug tool.

This is at the very end of restore, so the image is what it is at this point. But sure, no need to exit with failure I think, all it means is that we can't issue a meaningful warning.

Quoted 18 lines

mdrestore/xfs_mdrestore.c perform_restore( · line 266

Quoted 13 lines

mdrestore/xfs_mdrestore.c +13 -0

+
+	switch (xlog_is_dirty(mp, &xlog, &x,0)) {
+		case -1:
+			/* An error occured and we can't read the log. */
+			fprintf(stderr,
+			_("Warning: can't discern a log.\n"));
+			break;
+		case 1:
+			/* The log is dirty, warn. */
+			fprintf(stderr,
+			_("Warning: The log is dirty. If the image was obfuscated, "
+			  "an attempt to replay the log may lead to corruption.\n"));
+			break;

Then the above can remain in the test_log_dirty() helper or just be open-coded at the end of main(), provided the mount was initialized successfully (or we could still warn if we can't make sense of the log).

Quoted 4 lines

BTW, this is going to warn on every xfs_mdrestore of an image with a dirty log, right? That is slightly unfortunate, if so. Do we have any method to track or determine whether an image is obfuscated (I'm guessing not easily...)?

Nope! There is an unused slot in the header, maybe we could add flags?

Hm, or we could do a trick like setting the fs label to "OBFUSCATED" instead of "label" like we currently do. That might be reasonable...

                /* Replace any filesystem label with "L's" */
                if (obfuscate) {
                        struct xfs_sb *sb = iocur_top->data;
                        memset(sb->sb_fname, 'L',
                               min(strlen(sb->sb_fname), sizeof(sb->sb_fname)));
                        iocur_top->need_crc = 1;
                }

(today we keep the same label length, but that's probably not necessary?)

-Eric

Quoted 24 lines

Brian

Quoted 22 lines

mdrestore/xfs_mdrestore.c +10 -0

+		case 0:
+			 /* Everything is ok. */
+			break;
+	}
+
+}
+
+
 static void
 usage(void)
 {

@@ -271,5 +353,7 @@ main(
 	if (src_f != stdin)
 		fclose(src_f);
 
+	test_dirty_log(is_target_file, argv[optind]);
+
 	return 0;
 }

Signature · 1 line

2.1.4

Quoted 9 lines

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

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

Quoted 1 line · Eric Sandeen

On 4/11/17 1:33 PM, Brian Foster wrote:

Quoted 18 lines

Quoted 4 lines

BTW, this is going to warn on every xfs_mdrestore of an image with a dirty log, right? That is slightly unfortunate, if so. Do we have any method to track or determine whether an image is obfuscated (I'm guessing not easily...)?

Nope! There is an unused slot in the header, maybe we could add flags?

Hm, or we could do a trick like setting the fs label to "OBFUSCATED" instead of "label" like we currently do. That might be reasonable...

                /* Replace any filesystem label with "L's" */
                if (obfuscate) {
                        struct xfs_sb *sb = iocur_top->data;
                        memset(sb->sb_fname, 'L',
                               min(strlen(sb->sb_fname), sizeof(sb->sb_fname)));
                        iocur_top->need_crc = 1;
                }

Quoted 2 lines

(today we keep the same label length, but that's probably not necessary?)

That's an interesting idea. It looks like we currently set the fname to L's when obfuscated. Could we just key off that in mdrestore?

Brian

Quoted 35 lines

-Eric

Quoted 24 lines

Brian

Quoted 22 lines

mdrestore/xfs_mdrestore.c +10 -0

+		case 0:
+			 /* Everything is ok. */
+			break;
+	}
+
+}
+
+
 static void
 usage(void)
 {

@@ -271,5 +353,7 @@ main(
 	if (src_f != stdin)
 		fclose(src_f);
 
+	test_dirty_log(is_target_file, argv[optind]);
+
 	return 0;
 }

Signature · 1 line

2.1.4

Quoted 9 lines

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

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

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

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

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

Quoted 4 lines

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Looking at the patch, that's a lot of code to add to mdrestore that has nothing to do with metadump restoration.

The key change we're trying to make is to prevent people incorrectly replaying an XFS with a dirty log when the fs image has been restored from an obfuscated metadump.

So in my mind this brings up two questions: First, how do we prevent log replay in such situations? Second, how do we teach people not to attempt log replay?

As you point out, it's better that we educate
people as what problems each tool tries to solve and where the sharp edges might be on the debugging tools, but the answer to the first question ensures that us fallible developers can't do something stupid even though we theoretically know better.

Frankly, if the goal is to nudge n00b members of support teams away from a behavior that won't help them towards starting their failure analysis, then then I think we ought to patch the log recovery code to detect an obfuscated fs image, complain to dmesg about someone making an illogical move, and then refuse to mount the log.

I'd rather push back on the incorrect behavior at the time it is done, instead of training people to ignore a priori warning messages.

Quoted 21 lines · Darrick J. Wong

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

Quoted 7 lines

Quoted 4 lines

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Looking at the patch, that's a lot of code to add to mdrestore that has nothing to do with metadump restoration.

(tl;dr: slightly different approach suggested at the end)

So, yeah. I hadn't really thought about the bulk of code when I made the suggestion. The whole "given a device or file, get to a libxfs_mount" thing is repeated enough that I wonder if it shouldn't find its way into some factored code, which would make it nicer to read, at least. That doesn't address the issue of linking with new libs, or the overall elegance of the tool, though.

Quoted 3 lines

The key change we're trying to make is to prevent people incorrectly replaying an XFS with a dirty log when the fs image has been restored from an obfuscated metadump.

Yeah, the way this started was a metadump from the field which ended up "showing" corruption which was only a side-effect of this situation.

I had never considered this outcome; Brian diagnosed it and I don't know if it was obvious to him either. It clearly wasn't obvious to support personnel. I'm not sure I've ever seen it before, and I have replayed obfuscated metadumps. And yes, we could talk about "better training" but in reality "you should have read the docs we just updated!" only goes so far.

We have a lot of warnings to help people not use tools in bad ways or that duplicate information from a manpage, if it's highly relevant to that point of operation:

"Filesystem log is dirty; image will contain unobfuscated metadata in log."

"ERROR: The log head and/or tail cannot be discovered. Attempt to mount the filesystem to replay the log or use the -L option to destroy the log and attempt a repair."

"Only one AG detected - cannot validate filesystem geometry. Use the -o force_geometry option to proceed."

Handed a metadump, AFAIK there is obvious way to even /know/ whether it's safe to replay the log. It would involve a mount -ro,norecovery and intuiting that the filenames are obfuscated, or digging in with xfs_db, coupled with more xfs_db work or logprint to see if the log is even dirty, etc.

Anyway, there seem to be two intertwined objections:

1) xfs_metadump has no business cracking open the image log, and
2) xfs_metadump has no business alerting the user to this situation

I'm pretty sympathetic to the first argument, a little less so to the second.

Quoted 3 lines

So in my mind this brings up two questions: First, how do we prevent log replay in such situations? Second, how do we teach people not to attempt log replay?

(honestly, in many cases log replay is fine, right?)

Quoted 11 lines

As you point out, it's better that we educate
people as what problems each tool tries to solve and where the sharp edges might be on the debugging tools, but the answer to the first question ensures that us fallible developers can't do something stupid even though we theoretically know better.

Frankly, if the goal is to nudge n00b members of support teams away from a behavior that won't help them towards starting their failure analysis, then then I think we ought to patch the log recovery code to detect an obfuscated fs image, complain to dmesg about someone making an illogical move, and then refuse to mount the log.

(I think I like that less than the current approach; if given a choice beetween cruft in userspace or kernelspace I'll take userspace each time ;) but I see what you mean about denying at the point of potential failure...)

Quoted 2 lines

I'd rather push back on the incorrect behavior at the time it is done, instead of training people to ignore a priori warning messages.

I don't think that a:

WARNING: Replaying the log on this image may corrupt the filesystem!

would be ignored, tbh.

Anyway -

My hope here was to alert the person handling the metadump that it may require special handling. I don't think we need that warning on the xfs_metadump side; it's not that user's concern (the unobfuscated data /is/ their privacy concern.)

What if we used the mb_reserved field in the first metablock header as a short flags field to indicate OBFUSCATED | DIRTY_LOG | XXX??? and fill it in at metadump time, so that the state of the metadump can be trivially indicated and/or observed without all the somewhat inappropriate extra libxfs/libxlog code baggage?

I see value in that by itself, because the recipient of a metadump doesn't /really/ know what they've been handed; what (if any) messages we present to the mdrestore user based on that information could be debated later.

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

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

Quoted 4 lines

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Looking at the patch, that's a lot of code to add to mdrestore that has nothing to do with metadump restoration. For that matter, who's to say that the metadump'd image is even an XFS filesystem, and not just some garbage with the just the right superblock values to pass the perform_restore() checks? (Ok, ok, that was a little over the top.)

The key change we're trying to make is to prevent people incorrectly replaying an XFS with a dirty log when the fs image has been restored from an obfuscated metadump.

So in my mind this brings up two questions: First, how do we prevent log replay in such situations? Second, how do we teach people not to attempt log replay? As you point out, it's better that we educate people as what problems each tool tries to solve and where the sharp edges might be on the debugging tools, but the answer to the first question ensures that us fallible developers can't do something stupid even though we theoretically know better.

Frankly, if the goal is to nudge n00b members of support teams away from a behavior that won't help them towards starting their failure analysis, then then I think we ought to patch the log recovery code to detect an obfuscated fs image, complain to dmesg about someone making an illogical move, and then refuse to mount the log.

I'd rather push back on the incorrect behavior at the time it is done, instead of training people to ignore a priori warning messages.

--D

Quoted 10 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

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

Quoted 26 lines · Darrick J. Wong

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

Quoted 4 lines

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Quoted 6 lines

Looking at the patch, that's a lot of code to add to mdrestore that has nothing to do with metadump restoration. For that matter, who's to say that the metadump'd image is even an XFS filesystem, and not just some garbage with the just the right superblock values to pass the perform_restore() checks? (Ok, ok, that was a little over the top.)

Agreed wrt to the mdrestore bits...

Quoted 12 lines

The key change we're trying to make is to prevent people incorrectly replaying an XFS with a dirty log when the fs image has been restored from an obfuscated metadump.

So in my mind this brings up two questions: First, how do we prevent log replay in such situations? Second, how do we teach people not to attempt log replay? As you point out, it's better that we educate people as what problems each tool tries to solve and where the sharp edges might be on the debugging tools, but the answer to the first question ensures that us fallible developers can't do something stupid even though we theoretically know better.

Quoted 6 lines

Frankly, if the goal is to nudge n00b members of support teams away from a behavior that won't help them towards starting their failure analysis, then then I think we ought to patch the log recovery code to detect an obfuscated fs image, complain to dmesg about someone making an illogical move, and then refuse to mount the log.

I don't think this is really appropriate. Some users may very well have no other option but to create a dirty log + obfuscated metadump for whatever security/privacy reasons they have. The purpose of warning in that case is to notify the user to either verify the resulting image shows whatever problems are exhibited by the original fs and no others, or to notify the developer that other corruption might exist and to ignore it as a side effect of the metadump process itself (provided it doesn't interfere with rca of the original problem). Refusing to run log recovery in such cases just gets in the way.

I'm not tied to having an mdrestore warning at all, but I'd much prefer to see it there rather than include obfuscation logic in the kernel just to facilitate a userspace tool to continue on silently corrupting filesystem images.

Brian

Quoted 15 lines

I'd rather push back on the incorrect behavior at the time it is done, instead of training people to ignore a priori warning messages.

--D

Quoted 10 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

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

Quoted 65 lines · Brian Foster

Quoted 26 lines · Darrick J. Wong

Quoted 20 lines · Dave Chinner

Quoted 8 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

Quoted 4 lines

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Quoted 6 lines

Looking at the patch, that's a lot of code to add to mdrestore that has nothing to do with metadump restoration. For that matter, who's to say that the metadump'd image is even an XFS filesystem, and not just some garbage with the just the right superblock values to pass the perform_restore() checks? (Ok, ok, that was a little over the top.)

Agreed wrt to the mdrestore bits...

Quoted 12 lines

The key change we're trying to make is to prevent people incorrectly replaying an XFS with a dirty log when the fs image has been restored from an obfuscated metadump.

So in my mind this brings up two questions: First, how do we prevent log replay in such situations? Second, how do we teach people not to attempt log replay? As you point out, it's better that we educate people as what problems each tool tries to solve and where the sharp edges might be on the debugging tools, but the answer to the first question ensures that us fallible developers can't do something stupid even though we theoretically know better.

Quoted 6 lines

Frankly, if the goal is to nudge n00b members of support teams away from a behavior that won't help them towards starting their failure analysis, then then I think we ought to patch the log recovery code to detect an obfuscated fs image, complain to dmesg about someone making an illogical move, and then refuse to mount the log.

I don't think this is really appropriate. Some users may very well have no other option but to create a dirty log + obfuscated metadump for whatever security/privacy reasons they have. The purpose of warning in that case is to notify the user to either verify the resulting image shows whatever problems are exhibited by the original fs and no others, or to notify the developer that other corruption might exist and to ignore it as a side effect of the metadump process itself (provided it doesn't interfere with rca of the original problem). Refusing to run log recovery in such cases just gets in the way.

Quoted 4 lines

I'm not tied to having an mdrestore warning at all, but I'd much prefer to see it there rather than include obfuscation logic in the kernel just to facilitate a userspace tool to continue on silently corrupting filesystem images.

<nod> I've changed my mind overnight. Now I agree that we could put a message in at metadump time, because it's not too late to ask the user to try to send us a metadump w/ clean log. Eric also convinced me that it's not so trivial to detect an obfuscated image, so that simply won't work without a bunch of hackery.

Quoted 7 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Quoted 1 line

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

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

Quoted 8 lines · Dave Chinner

Quoted 7 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Quoted 7 lines

Quoted 1 line

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

I think that's a reasonable argument for the mdrestore side. I'm less interested in seeing a warning on the restore side in general, personally. I was initially thinking it would have required less code and the whole obfuscation detection thing is getting into hackish territory, to be fair.

Quoted 10 lines

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Put me in the clueless users bucket, then. This started with a customer with a corrupted filesystem that provided a metadump that exhibited filesystem corruption. A support person began the process of diagnosing the problem and it eventually got to me, who had to spend a nontrivial amount of time trying to identify what the problem was, see if I could reproduce it on my own to verify it was actually specific to the metadump, etc.

This is not an obvious "your metadump is broken" log recovery failure. It's a latent directory corruption that doesn't obviously have anything to do with log recovery in the first place. I'm sure I'll be able to spot it going forward for some time while it's fresh in my mind, but I expect to lose track of that eventually given the rarity (of debugging log recovery issues). It's not reasonable at all to expect regular users or support people to understand this enough to filter out bad images or know when to use or not use a certain combination of metadump options, because it otherwise requires a detailed understanding of XFS logging and directory internals.

Quoted 42 lines · Brian Foster

Quoted 8 lines · Dave Chinner

Quoted 7 lines · Jan Tulak

A dirty log in an obfuscated dump means that a corruption can happen when replaying the log (which contains unobfuscated data). Warn the user about this possibility.

The xlog workaround is copy&paste solution from repair/phase2.c and other tools, because the function is not implemented in libxlog.

Quoted 7 lines

Quoted 1 line

Signed-off-by Jan Tulak <[email protected]>

I think this is overkill. mdrestore is not the place
to be interpreting the state of the dumped image - it is a basic "restore the image" program, not a "check the validity of the image" program.

I think that's a reasonable argument for the mdrestore side. I'm less interested in seeing a warning on the restore side in general, personally. I was initially thinking it would have required less code and the whole obfuscation detection thing is getting into hackish territory, to be fair.

Quoted 10 lines

Secondly, if people are having problems with running log recovery on a restored obfuscated image and getting corruption and not knowing why or what to do, then that is a /documentation and training/ problem, not a code problem.

i.e. the problem is that people who aren't developers are trying to use tools that were written for developers to do forensic analysis of failures. Don't dumb down the tool for clueless users - point the users at the documentation that the tool requires to use correctly...

Put me in the clueless users bucket, then. This started with a customer with a corrupted filesystem that provided a metadump that exhibited filesystem corruption. A support person began the process of diagnosing the problem and it eventually got to me, who had to spend a nontrivial amount of time trying to identify what the problem was, see if I could reproduce it on my own to verify it was actually specific to the metadump, etc.

Quoted 10 lines

This is not an obvious "your metadump is broken" log recovery failure. It's a latent directory corruption that doesn't obviously have anything to do with log recovery in the first place. I'm sure I'll be able to spot it going forward for some time while it's fresh in my mind, but I expect to lose track of that eventually given the rarity (of debugging log recovery issues). It's not reasonable at all to expect regular users or support people to understand this enough to filter out bad images or know when to use or not use a certain combination of metadump options, because it otherwise requires a detailed understanding of XFS logging and directory internals.

Log recovery on an obfuscated directory is, to me, a known obvious vector for directory corruption because we replay unobfuscated dirents over obfuscated on-disk data. Buffer logging is done in aligned 128 byte chunks, so it /should/ be obvious that the recovery of directory data buffers will partially overwrite dirents on disk even when they were not directly modified by the user. And because this causes an obfuscated/clear text mismatch in the dirent name, the hash will not calculate to teh same as what the directory stored for that dirent. Hence the corruption reports that repair will now spew...

This was always considered a known problem for obfuscated metadump restorations - the unobfuscated log will result in recovery issues and name/data corruptions for dirs and xattrs. In hindsight, this should have been documented long ago so you didn't have to waste the time to "rediscover" it like you did. It wasn't documented because both developers and users were far more concerned about the data exposure issues than they were about whether the log unobfuscated log replayed correctly or not.

IMO - and as I said to Eric on IRC - we should not be trying to work around institutional problems (i.e. inability to train or impart the necessary knowledge on support engineers) with code changes. Training support engineers properly requires documentation and knowledge distribution processes; the code implementing the tools they are being taught about is not the right instrument to perform this knowledge transfer....

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

IOW, this documentation problem exists because the tool is broken. The tool will remain broken despite the fact that the problem is documented. Therefore, we are not just working around a documentation issue by attempting to improve the tool.

Quoted 4 lines · Brian Foster

IOW, this documentation problem exists because the tool is broken. The tool will remain broken despite the fact that the problem is documented. Therefore, we are not just working around a documentation issue by attempting to improve the tool.

I'm not sure that you understood my point. That is, if a developer tool is considered broken, then adding warnings to tell the /user/ the developer tool is broken is not solving the "tool is broken" problem any better than documenting in the man page. The underlying problem is that the log is unobfuscated and so the tool will, by your definition, remain "broken" until that problem is fixed.

And, IMO, "broken" is an incorrect classification of the issue. We *chose* not to obfuscate the log because the effort required to implement it falls far, far to the wrong side of the cost-benefit analysis line. Months of work for something that may be relevant only to a developer once or twice a year? Further, bfuscating the log may actually be an unsolvable problem due to the way we do relogging and reuse freed blocks - the obfuscation of log entries has to exactly match the obfuscation that is done on disk, and we may have multiple overwrites of the same directory blocks to obfuscate and all need to be correct. It's a damn hard problem that I'll still strongly suggest we should never attempt to solve.

... they should have rough edges, ...

If we are exporting obfuscated metadata with a dirty journal, the journal is not obfuscated. So when we restore it and then replay a log, the obfuscated metadata can clash with what is in journal and corrupt the image. So, tell the user about this risk, both when exporting and importing the dump.

Git tree on github: https://github.com/jtulak/xfsprogs-dev/tree/metadump
Based on revision fc1d645

Jan Tulak (2):
metadump: warn about corruption if log is dirty
mdrestore: warn about corruption if log is dirty

 db/metadump.c             |  3 +-
 mdrestore/Makefile        |  4 +--
 mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 3 deletions(-)