[PATCH 0/2] xfsprogs: metadump/mdrestore warns about dirty journal

[PATCH 0/2] xfsprogs: metadump/mdrestore warns about dirty journal

[Jan Tulak]

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

-- 
2.1.4

[PATCH 1/2] metadump: warn about corruption if log is dirty

[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 <jtulak@redhat.com>
---
 db/metadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/db/metadump.c b/db/metadump.c
index 66952f6..74e24b2 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -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."));
 		break;
 	case -1:
 		/* log detection error */
-- 
2.1.4

[PATCH 2/2] mdrestore: warn about corruption if log is dirty

[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 <jtulak@redhat.com>
---
 mdrestore/Makefile        |  4 +--
 mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/mdrestore/Makefile b/mdrestore/Makefile
index 5171306..6355df9 100644
--- a/mdrestore/Makefile
+++ b/mdrestore/Makefile
@@ -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
 
 default: depend $(LTCOMMAND)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 0d399f1..3797955 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -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"),
+			progname, target_name);
+		exit(1);
+	}
+
+	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;
+		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;
 }
-- 
2.1.4

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
> 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 <jtulak@redhat.com>
> ---

Thanks for posting this...

>  db/metadump.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 66952f6..74e24b2 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -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

>  		break;
>  	case -1:
>  		/* log detection error */
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> 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 <jtulak@redhat.com>
> ---
>  mdrestore/Makefile        |  4 +--
>  mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/mdrestore/Makefile b/mdrestore/Makefile
> index 5171306..6355df9 100644
> --- a/mdrestore/Makefile
> +++ b/mdrestore/Makefile
> @@ -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.

>  default: depend $(LTCOMMAND)
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 0d399f1..3797955 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -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"),
> +			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.

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

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

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Eric Sandeen]

On 4/11/17 1:30 PM, Brian Foster wrote:
> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
>> 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 <jtulak@redhat.com>
>> ---
> 
> Thanks for posting this...
> 
>>  db/metadump.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/db/metadump.c b/db/metadump.c
>> index 66952f6..74e24b2 100644
>> --- a/db/metadump.c
>> +++ b/db/metadump.c
>> @@ -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?

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

> Brian
> 
>>  		break;
>>  	case -1:
>>  		/* log detection error */
>> -- 
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Eric Sandeen]

On 4/11/17 1:33 PM, Brian Foster wrote:
> On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
>> 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 <jtulak@redhat.com>
>> ---
>>  mdrestore/Makefile        |  4 +--
>>  mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 86 insertions(+), 2 deletions(-)
>>
>> diff --git a/mdrestore/Makefile b/mdrestore/Makefile
>> index 5171306..6355df9 100644
>> --- a/mdrestore/Makefile
>> +++ b/mdrestore/Makefile
>> @@ -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.


need something like this in the top Makefile:

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

>>  default: depend $(LTCOMMAND)
>> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
>> index 0d399f1..3797955 100644
>> --- a/mdrestore/xfs_mdrestore.c
>> +++ b/mdrestore/xfs_mdrestore.c
>> @@ -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"),
>> +			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.

 
>> +
>> +	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...)?

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
 
> Brian
> 
>> +		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;
>>  }
>> -- 
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote:
> On 4/11/17 1:30 PM, Brian Foster wrote:
> > On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
> >> 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 <jtulak@redhat.com>
> >> ---
> > 
> > Thanks for posting this...
> > 
> >>  db/metadump.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/db/metadump.c b/db/metadump.c
> >> index 66952f6..74e24b2 100644
> >> --- a/db/metadump.c
> >> +++ b/db/metadump.c
> >> @@ -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?
> 
> 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..

Brian

> 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
> 
> > Brian
> > 
> >>  		break;
> >>  	case -1:
> >>  		/* log detection error */
> >> -- 
> >> 2.1.4
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 01:39:55PM -0500, Eric Sandeen wrote:
> On 4/11/17 1:33 PM, Brian Foster wrote:
> > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> >> 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 <jtulak@redhat.com>
> >> ---
> >>  mdrestore/Makefile        |  4 +--
> >>  mdrestore/xfs_mdrestore.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 86 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mdrestore/Makefile b/mdrestore/Makefile
> >> index 5171306..6355df9 100644
> >> --- a/mdrestore/Makefile
> >> +++ b/mdrestore/Makefile
> >> @@ -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.
> 
> 
> need something like this in the top Makefile:
> 
>  repair: libxlog libxcmd
>  copy: libxlog
>  mkfs: libxcmd
> +mdrestore: libxlog
>  
> 
> >>  default: depend $(LTCOMMAND)
> >> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> >> index 0d399f1..3797955 100644
> >> --- a/mdrestore/xfs_mdrestore.c
> >> +++ b/mdrestore/xfs_mdrestore.c
> >> @@ -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"),
> >> +			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.
> 
>  
> >> +
> >> +	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...)?
> 
> 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?)
> 

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

> -Eric
>  
> > Brian
> > 
> >> +		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;
> >>  }
> >> -- 
> >> 2.1.4
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Eric Sandeen]

On 4/11/17 1:49 PM, Brian Foster wrote:
> On Tue, Apr 11, 2017 at 01:39:55PM -0500, Eric Sandeen wrote:
>> On 4/11/17 1:33 PM, Brian Foster wrote:

...

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

Right, like the code above.  :)

I guess the problem is that if a filesystem had no label, then we replaced
it with "no" L's, i.e. it stays empty.

So we may want to:

1) Start replacing it with 12 L's going forward, no matter what, and
2) Look for L's in mdrestore as a clue, which may sometimes fail ...
3) Any non-NULL label != "LLL..." probably isn't obfuscated, and can skip warning

-Eric

> Brian
> 
>> -Eric
>>  
>>> Brian
>>>
>>>> +		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;
>>>>  }
>>>> -- 
>>>> 2.1.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Eric Sandeen]

On 4/11/17 1:43 PM, Brian Foster wrote:
> On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote:
>> On 4/11/17 1:30 PM, Brian Foster wrote:
>>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
>>>> 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 <jtulak@redhat.com>
>>>> ---
>>>
>>> Thanks for posting this...
>>>
>>>>  db/metadump.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/db/metadump.c b/db/metadump.c
>>>> index 66952f6..74e24b2 100644
>>>> --- a/db/metadump.c
>>>> +++ b/db/metadump.c
>>>> @@ -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?
>>
>> 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."

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

> 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

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Dave Chinner]

On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> 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 <jtulak@redhat.com>

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Darrick J. Wong]

On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > 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 <jtulak@redhat.com>
> 
> 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...

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

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

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Darrick J. Wong]

On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote:
> On 4/11/17 1:43 PM, Brian Foster wrote:
> > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote:
> >> On 4/11/17 1:30 PM, Brian Foster wrote:
> >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
> >>>> 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 <jtulak@redhat.com>
> >>>> ---
> >>>
> >>> Thanks for posting this...
> >>>
> >>>>  db/metadump.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/db/metadump.c b/db/metadump.c
> >>>> index 66952f6..74e24b2 100644
> >>>> --- a/db/metadump.c
> >>>> +++ b/db/metadump.c
> >>>> @@ -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?
> >>
> >> 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."
> 
> 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

> 
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Eric Sandeen]

On 4/11/17 6:43 PM, Darrick J. Wong wrote:
> On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
>> On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
>>> 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 <jtulak@redhat.com>
>>
>> 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...
> 
> 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.

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

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

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

> 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

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 04:44:44PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote:
> > On 4/11/17 1:43 PM, Brian Foster wrote:
> > > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote:
> > >> On 4/11/17 1:30 PM, Brian Foster wrote:
> > >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
> > >>>> 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 <jtulak@redhat.com>
> > >>>> ---
> > >>>
> > >>> Thanks for posting this...
> > >>>
> > >>>>  db/metadump.c | 3 ++-
> > >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/db/metadump.c b/db/metadump.c
> > >>>> index 66952f6..74e24b2 100644
> > >>>> --- a/db/metadump.c
> > >>>> +++ b/db/metadump.c
> > >>>> @@ -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?
> > >>
> > >> 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."
> > 
> > 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..

> 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

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

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > 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 <jtulak@redhat.com>
> 
> 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.

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

Brian

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

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 04:43:26PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > > 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 <jtulak@redhat.com>
> > 
> > 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...
> 
> 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...

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

> 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
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Jan Tulak]

On Wed, Apr 12, 2017 at 1:03 PM, Brian Foster <bfoster@redhat.com> wrote:
> On Tue, Apr 11, 2017 at 04:44:44PM -0700, Darrick J. Wong wrote:
>> On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote:
>> > On 4/11/17 1:43 PM, Brian Foster wrote:
>> > > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote:
>> > >> On 4/11/17 1:30 PM, Brian Foster wrote:
>> > >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote:
>> > >>>> 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 <jtulak@redhat.com>
>> > >>>> ---
>> > >>>
>> > >>> Thanks for posting this...
>> > >>>
>> > >>>>  db/metadump.c | 3 ++-
>> > >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >>>>
>> > >>>> diff --git a/db/metadump.c b/db/metadump.c
>> > >>>> index 66952f6..74e24b2 100644
>> > >>>> --- a/db/metadump.c
>> > >>>> +++ b/db/metadump.c
>> > >>>> @@ -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?
>> > >>
>> > >> 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."
>> >
>> > 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..
>
>> 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
>

I think that this is something we really should notify about. It
serves as a notification for a rare issue, "hey, are you aware of
this," rather than a list of instructions or a manual. So, I would
keep the message short. This (slightly modified) variant sounds good
to me:

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

Jan

-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Tue, Apr 11, 2017 at 08:48:32PM -0500, Eric Sandeen wrote:
> On 4/11/17 6:43 PM, Darrick J. Wong wrote:
> > On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> >> On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> >>> 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 <jtulak@redhat.com>
> >>
> >> 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...
> > 
> > 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.
> 

Yeah, it would be nice if this could be done a bit more cleanly without
another duplication of the libxfs init stuff. It's a bit unfortunate to
include all of that just to emit a warning.

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

It's not obvious at all, IMO. I don't think it would be a bad idea to
update the xfs_metadump manpage or whatnot too, but I don't think that
eliminates the need for _at least_ a warning.

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

... or:

"ERROR: The filesystem has valuable metadata changes in a log which needs to
be replayed.  Mount the filesystem to replay the log, and unmount it before
re-running xfs_repair.  If you are unable to mount the filesystem, then use
the -L option to destroy the log and attempt a repair.
Note that destroying the log may cause corruption -- please attempt a mount
of the filesystem before doing this."

I don't see the need for a warning here as much different from the
above, tbh.

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

Yep.

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

Agreed.

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

Another way to look at this is obfuscation is out of the control of the
kernel. E.g., in theory (I don't see how in practice..), perhaps
xfs_metadump could someday learn how to obfuscate such images
correctly/selectively so as to avoid corruption. How is the kernel
supposed to interpret such images and know when to allow or not allow
log recovery?

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

I think we do need it on the metadump side. It nudges the user to submit
a valid image if at all possible, to verify the result or at least ask
somebody about the warning if they run into it.

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

That sounds interesting/reasonable to me on first thought. Without
digging into the code, I take it that embedding this into a metadump
header means this won't affect the resulting filesystem image in any
way..? If that doesn't conflict with any other use of that field, it
sounds like that would eliminate the need for all of the libxfs bits on
the mdrestore side as well.

Brian

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

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Darrick J. Wong]

On Wed, Apr 12, 2017 at 07:06:33AM -0400, Brian Foster wrote:
> On Tue, Apr 11, 2017 at 04:43:26PM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> > > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > > > 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 <jtulak@redhat.com>
> > > 
> > > 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...
> > 
> > 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...
> 
> > 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 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.

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

--D

> 
> Brian
> 
> > 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
> > 
> > > Cheers,
> > > 
> > > Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Dave Chinner]

On Wed, Apr 12, 2017 at 07:04:04AM -0400, Brian Foster wrote:
> On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > > 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 <jtulak@redhat.com>
> > 
> > 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.
> 
> > 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.

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Jan Tulak]

On Wed, Apr 12, 2017 at 7:45 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Wed, Apr 12, 2017 at 07:06:33AM -0400, Brian Foster wrote:
>> On Tue, Apr 11, 2017 at 04:43:26PM -0700, Darrick J. Wong wrote:
>> > On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
>> > > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
>> > > > 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 <jtulak@redhat.com>
>> > >
>> > > 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...
>> >
>> > 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...
>>
>> > 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 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.
>
> <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.
>

Ok, I will send again only the dump patch with modified message (+ man
page update), without this mdrestore patch. That way it should pass
and meanwhile, we can continue here about what to do (if anything)
with mdrestore.

Jan

-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Thu, Apr 13, 2017 at 12:51:05PM +1000, Dave Chinner wrote:
> On Wed, Apr 12, 2017 at 07:04:04AM -0400, Brian Foster wrote:
> > On Wed, Apr 12, 2017 at 08:34:05AM +1000, Dave Chinner wrote:
> > > On Tue, Apr 11, 2017 at 04:12:37PM +0200, Jan Tulak wrote:
> > > > 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 <jtulak@redhat.com>
> > > 
> > > 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.
> > 
> > > 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.
> 
> 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.
> 

What might be obvious to a design architect from a code standpoint isn't
necessarily obvious to others nor obvious in practice. With such an
image in hand, it's not obvious (enough, IMO) that the corruption is
even a result of log recovery.

Rather, that is something that has to be worked out, distinguished from
any other potentially similar corruption of the source image and
isolated to the metadump image itself. This is a waste of time for
everybody involved.

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

Documentation is good. Jan's latest series updates the man page on the
issue.

That aside, this does not reduce to solely a documentation problem.
xfs_metadump is simply broken for particular source filesystems in that
it may corrupt the resulting fs image (by default, no less). The
fundamentally correct approach is to fix metadump obfuscation to not
corrupt the output image (then we wouldn't need to document the fact
that it does ;).

Fixing xfs_metadump to obfuscate the log correctly is not a trivial
matter, however, so that isn't a realistic solution atm. The next
available option is to disallow obfuscation of such filesystems, but
that limits the use of a valuable support tool to avoid a
rare/particular case. The next available option after that is the
approach implemented by these patches: to warn about the situation to
hopefully avoid the effects of the problem in the field.

So while I'm actually fine with dropping the mdrestore side bits here
for practical reasons (it's more code than I anticipated for the purpose
of emitting a warning), and I agree that we should update documentation,
the hightest priority here should be to provide a usable tool that
functions correctly.

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.

Brian

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

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Dave Chinner]

On Thu, Apr 13, 2017 at 09:10:19AM -0400, Brian Foster wrote:
> 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.

Part of playing the maintainer game is knowing how many resources
you have available, the relative complexity of the problems that
need to be solved and guiding the use of your limited resources
appropriately.  Debug tools that are rarely used are at the low end
of the priority list - they should have rough edges, because that
indicates we spend more time caring about making the production code
reliable than we do about polishing tools that are only used when
the production code has failed....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] mdrestore: warn about corruption if log is dirty

[Brian Foster]

On Fri, Apr 14, 2017 at 10:29:30AM +1000, Dave Chinner wrote:
> On Thu, Apr 13, 2017 at 09:10:19AM -0400, Brian Foster wrote:
> > 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.
> 

If xfs_metadump spits out something that doesn't resemble the source fs
from a filesystem coherency perspective, then yes, I consider that a bug
(or "broken"). And yes, I agree that the tool warning about that fact
does not fix the bug. That doesn't mean the usability of the tool cannot
be improved.

What I'm saying is that if xfs_metadump issued a warning about using
obfuscation with a dirty log, then it's much more likely this user would
have disabled obfuscation (as the original problem report was a log
recovery issue), sent a valid metadump and we wouldn't have lost a
valuable metadata image (that I believe had since been repaired) in the
process. Further, if that didn't work, but xfs_mdrestore issued a
similar warning, the support person probably wouldn't have filed a bug
(or even with a bug filed, I wouldn't have wasted time root causing a
spurious filesystem corruption).

In contrast... I can't speak for others with certainty, but I highly
doubt anybody involved in this exchange had a need to refer to the
xfs_metadump manpage. Both xfs_metadump and xfs_mdrestore worked fine
with default parameters from the customer and support personnel
perspective. The user reported a post-log recovery corruption and that's
what the support person observed. I certainly didn't have any impetus to
'man xfs_metadump,' as I generally don't refer to the manpage in
response to encountering runtime filesystem corruption errors.

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

I'm pretty sure I stated in my last email that obfuscating the log was
not a realistic solution, so I'm not sure what point you're arguing with
here...

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

Then why the fuss about a warning that is otherwise technically
accurate?

Brian

Re: [PATCH 0/2] xfsprogs: metadump/mdrestore warns about dirty journal

[Eric Sandeen]

On 4/11/17 9:12 AM, Jan Tulak wrote:
> 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.


I'm going to go ahead & commit these doc changes with some of the
suggestions from the list; once it's in, if people want further tweaks,
send patches.  Trying to wind up the bikeshedding, though.

Thanks,
-Eric

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

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Brian Foster]

On Wed, Jun 14, 2017 at 07:06:52PM -0500, Eric Sandeen wrote:
> On 4/13/17 6:54 AM, Brian Foster wrote:
> > On Thu, Apr 13, 2017 at 10:13:53AM +0200, Jan Tulak wrote:
> >> 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 <jtulak@redhat.com>
> >> ---
> >> Change: More elaborate warning message.
> >> ---
> >>  db/metadump.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/db/metadump.c b/db/metadump.c
> >> index 66952f6..2dd8593 100644
> >> --- a/db/metadump.c
> >> +++ b/db/metadump.c
> >> @@ -2726,7 +2726,9 @@ copy_log(void)
> >>  		/* keep the dirty log */
> >>  		if (obfuscate)
> >>  			print_warning(
> >> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
> >> +_("Warning: log recovery of an obfuscated metadata image can leak "
> >> +"unobfuscated metadata and/or cause image corruption. Please mount "
> >> +"the source filesystem to clean the log or disable obfuscation, if possible."));
> >>  		break;
> > 
> > Thanks Jan, just one very minor nit having read this again... could we
> > put the "if possible" closer to the part about mounting the source
> > image? Otherwise it reads to me that it might not be technically
> > possible to disable obfuscation, which is not the case (though the user
> > may not want to do that as a matter of policy). For example:
> > 
> > "... Please mount the source filesystem to clean the log, if possible,
> > or disable obfuscation."
> 
> Hoovering up old patches ... 
> 
> To me, Jan's original is ok.  "If possible" applying to both replay and
> disabling obfuscation seems reasonable, because "policy" may make it
> impossible ;)  So I hate to direct the user to disable obfuscation.
> 
> "If possible, please mount the filesystem to clean the log, or disable
> obfuscation."
> 
> Is that OK?  Gives the user options, and wiggle room..
> 

Sounds good to me, thanks!

Brian

> -Eric
> 
> > With that tweak:
> > 
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > 
> >>  	case -1:
> >>  		/* log detection error */
> >> -- 
> >> 2.1.4
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Eric Sandeen]

On 4/13/17 6:54 AM, Brian Foster wrote:
> On Thu, Apr 13, 2017 at 10:13:53AM +0200, Jan Tulak wrote:
>> 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 <jtulak@redhat.com>
>> ---
>> Change: More elaborate warning message.
>> ---
>>  db/metadump.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/db/metadump.c b/db/metadump.c
>> index 66952f6..2dd8593 100644
>> --- a/db/metadump.c
>> +++ b/db/metadump.c
>> @@ -2726,7 +2726,9 @@ copy_log(void)
>>  		/* keep the dirty log */
>>  		if (obfuscate)
>>  			print_warning(
>> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
>> +_("Warning: log recovery of an obfuscated metadata image can leak "
>> +"unobfuscated metadata and/or cause image corruption. Please mount "
>> +"the source filesystem to clean the log or disable obfuscation, if possible."));
>>  		break;
> 
> Thanks Jan, just one very minor nit having read this again... could we
> put the "if possible" closer to the part about mounting the source
> image? Otherwise it reads to me that it might not be technically
> possible to disable obfuscation, which is not the case (though the user
> may not want to do that as a matter of policy). For example:
> 
> "... Please mount the source filesystem to clean the log, if possible,
> or disable obfuscation."

Hoovering up old patches ... 

To me, Jan's original is ok.  "If possible" applying to both replay and
disabling obfuscation seems reasonable, because "policy" may make it
impossible ;)  So I hate to direct the user to disable obfuscation.

"If possible, please mount the filesystem to clean the log, or disable
obfuscation."

Is that OK?  Gives the user options, and wiggle room..

-Eric

> With that tweak:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
>>  	case -1:
>>  		/* log detection error */
>> -- 
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] metadump: warn about corruption if log is dirty

[Brian Foster]

On Thu, Apr 13, 2017 at 10:13:53AM +0200, Jan Tulak wrote:
> 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 <jtulak@redhat.com>
> ---
> Change: More elaborate warning message.
> ---
>  db/metadump.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 66952f6..2dd8593 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2726,7 +2726,9 @@ copy_log(void)
>  		/* keep the dirty log */
>  		if (obfuscate)
>  			print_warning(
> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
> +_("Warning: log recovery of an obfuscated metadata image can leak "
> +"unobfuscated metadata and/or cause image corruption. Please mount "
> +"the source filesystem to clean the log or disable obfuscation, if possible."));
>  		break;

Thanks Jan, just one very minor nit having read this again... could we
put the "if possible" closer to the part about mounting the source
image? Otherwise it reads to me that it might not be technically
possible to disable obfuscation, which is not the case (though the user
may not want to do that as a matter of policy). For example:

"... Please mount the source filesystem to clean the log, if possible,
or disable obfuscation."

With that tweak:

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

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

[PATCH 1/2] metadump: warn about corruption if log is dirty

[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 <jtulak@redhat.com>
---
Change: More elaborate warning message.
---
 db/metadump.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/db/metadump.c b/db/metadump.c
index 66952f6..2dd8593 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2726,7 +2726,9 @@ copy_log(void)
 		/* keep the dirty log */
 		if (obfuscate)
 			print_warning(
-_("Filesystem log is dirty; image will contain unobfuscated metadata in log."));
+_("Warning: log recovery of an obfuscated metadata image can leak "
+"unobfuscated metadata and/or cause image corruption. Please mount "
+"the source filesystem to clean the log or disable obfuscation, if possible."));
 		break;
 	case -1:
 		/* log detection error */
-- 
2.1.4