[PATCH] xfs_db: add -R option

[PATCH] xfs_db: add -R option

[hal]

From: Hal Pomeranz <hal@deer-run.com>

-R is similar to -r, but allows for blockget on a mounted file system
or "dirty" file system image with pending log entries.  This makes
xfs_db more useful for forensics where we are often dealing with these
types of images.  Flushing log entries to disk by mounting/unmounting
the file system would allow us to use blockget, but would make changes
to the file system state which are not desirable in forensics
contexts.

Signed-off-by: Hal Pomeranz <hal@deer-run.com>
---
 db/init.c         | 7 +++++--
 db/init.h         | 1 +
 db/sb.c           | 2 +-
 man/man8/xfs_db.8 | 8 ++++++++
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/db/init.c b/db/init.c
index 29fc344..96a0e78 100644
--- a/db/init.c
+++ b/db/init.c
@@ -36,6 +36,7 @@ int			blkbb;
 int			exitcode;
 int			expert_mode;
 int			force;
+int                     ignore_dirty;
 struct xfs_mount	xmount;
 struct xfs_mount	*mp;
 struct xlog		xlog;
@@ -46,7 +47,7 @@ static void
 usage(void)
 {
 	fprintf(stderr, _(
-		"Usage: %s [-ifFrxV] [-p prog] [-l logdev] [-c cmd]... device\n"
+		"Usage: %s [-ifFrRxV] [-p prog] [-l logdev] [-c cmd]... device\n"
 		), progname);
 	exit(1);
 }
@@ -66,7 +67,7 @@ init(
 	textdomain(PACKAGE);
 
 	progname = basename(argv[0]);
-	while ((c = getopt(argc, argv, "c:fFip:rxVl:")) != EOF) {
+	while ((c = getopt(argc, argv, "c:fFip:rRxVl:")) != EOF) {
 		switch (c) {
 		case 'c':
 			cmdline = xrealloc(cmdline, (ncmdline+1)*sizeof(char*));
@@ -84,6 +85,8 @@ init(
 		case 'p':
 			progname = optarg;
 			break;
+		case 'R':
+		        ignore_dirty = 1;
 		case 'r':
 			x.isreadonly = LIBXFS_ISREADONLY;
 			break;
diff --git a/db/init.h b/db/init.h
index b09389e..f6bfda9 100644
--- a/db/init.h
+++ b/db/init.h
@@ -20,6 +20,7 @@ extern char		*fsdevice;
 extern int		blkbb;
 extern int		exitcode;
 extern int		expert_mode;
+extern int              ignore_dirty;
 extern xfs_mount_t	*mp;
 extern libxfs_init_t	x;
 extern xfs_agnumber_t	cur_agno;
diff --git a/db/sb.c b/db/sb.c
index c7fbfd6..4c04d79 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -251,7 +251,7 @@ sb_logcheck(void)
 	if (dirty == -1) {
 		dbprintf(_("ERROR: cannot find log head/tail, run xfs_repair\n"));
 		return 0;
-	} else if (dirty == 1) {
+	} else if (dirty == 1 && ignore_dirty == 0) {
 		dbprintf(_(
 "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
 "be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 524b1ef..89be1aa 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -90,6 +90,14 @@ It is only necessary to omit this flag if a command that changes data
 .RB ( write ", " blocktrash ", " crc )
 is to be used.
 .TP
+.B -R
+Like
+.B -r
+but allows the 
+.B blockget
+command even if the file system is "dirty" (unreconciled transactions
+in the log).
+.TP
 .B \-x
 Specifies expert mode.
 This enables the
-- 
1.8.3.1

Re: [PATCH] xfs_db: add -R option

[Darrick J. Wong]

On Fri, May 04, 2018 at 09:02:44AM -0500, hal@deer-run.com wrote:
> From: Hal Pomeranz <hal@deer-run.com>
> 
> -R is similar to -r, but allows for blockget on a mounted file system
> or "dirty" file system image with pending log entries.  This makes
> xfs_db more useful for forensics where we are often dealing with these
> types of images.  Flushing log entries to disk by mounting/unmounting
> the file system would allow us to use blockget, but would make changes
> to the file system state which are not desirable in forensics
> contexts.
> 
> Signed-off-by: Hal Pomeranz <hal@deer-run.com>
> ---
>  db/init.c         | 7 +++++--
>  db/init.h         | 1 +
>  db/sb.c           | 2 +-
>  man/man8/xfs_db.8 | 8 ++++++++
>  4 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index 29fc344..96a0e78 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -36,6 +36,7 @@ int			blkbb;
>  int			exitcode;
>  int			expert_mode;
>  int			force;
> +int                     ignore_dirty;

tabs vs. spaces...

>  struct xfs_mount	xmount;
>  struct xfs_mount	*mp;
>  struct xlog		xlog;
> @@ -46,7 +47,7 @@ static void
>  usage(void)
>  {
>  	fprintf(stderr, _(
> -		"Usage: %s [-ifFrxV] [-p prog] [-l logdev] [-c cmd]... device\n"
> +		"Usage: %s [-ifFrRxV] [-p prog] [-l logdev] [-c cmd]... device\n"
>  		), progname);
>  	exit(1);
>  }
> @@ -66,7 +67,7 @@ init(
>  	textdomain(PACKAGE);
>  
>  	progname = basename(argv[0]);
> -	while ((c = getopt(argc, argv, "c:fFip:rxVl:")) != EOF) {
> +	while ((c = getopt(argc, argv, "c:fFip:rRxVl:")) != EOF) {
>  		switch (c) {
>  		case 'c':
>  			cmdline = xrealloc(cmdline, (ncmdline+1)*sizeof(char*));
> @@ -84,6 +85,8 @@ init(
>  		case 'p':
>  			progname = optarg;
>  			break;
> +		case 'R':
> +		        ignore_dirty = 1;

In general, fall through cases should have a comment to document that
explicitly.

/* fall through */

>  		case 'r':
>  			x.isreadonly = LIBXFS_ISREADONLY;
>  			break;
> diff --git a/db/init.h b/db/init.h
> index b09389e..f6bfda9 100644
> --- a/db/init.h
> +++ b/db/init.h
> @@ -20,6 +20,7 @@ extern char		*fsdevice;
>  extern int		blkbb;
>  extern int		exitcode;
>  extern int		expert_mode;
> +extern int              ignore_dirty;
>  extern xfs_mount_t	*mp;
>  extern libxfs_init_t	x;
>  extern xfs_agnumber_t	cur_agno;
> diff --git a/db/sb.c b/db/sb.c
> index c7fbfd6..4c04d79 100644
> --- a/db/sb.c
> +++ b/db/sb.c
> @@ -251,7 +251,7 @@ sb_logcheck(void)
>  	if (dirty == -1) {
>  		dbprintf(_("ERROR: cannot find log head/tail, run xfs_repair\n"));
>  		return 0;
> -	} else if (dirty == 1) {
> +	} else if (dirty == 1 && ignore_dirty == 0) {
>  		dbprintf(_(
>  "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
>  "be replayed.  Mount the filesystem to replay the log, and unmount it before\n"

Why skip the warning?  I think xfs_db should always warn on a dirty log,
but perhaps we could relax the 'return 0' at the end of this hunk if the
fs was opened readonly?

i.e.

dbprintf(_("ERROR: The filesystem has..."));
return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0;

which would also make adding the -R option unnecessary.

--D

> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 524b1ef..89be1aa 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -90,6 +90,14 @@ It is only necessary to omit this flag if a command that changes data
>  .RB ( write ", " blocktrash ", " crc )
>  is to be used.
>  .TP
> +.B -R
> +Like
> +.B -r
> +but allows the 
> +.B blockget
> +command even if the file system is "dirty" (unreconciled transactions
> +in the log).
> +.TP
>  .B \-x
>  Specifies expert mode.
>  This enables the
> -- 
> 1.8.3.1
> 
> --
> 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] xfs_db: add -R option

[hal]

> Why skip the warning?  I think xfs_db should always warn on a dirty log,
> but perhaps we could relax the 'return 0' at the end of this hunk if the
> fs was opened readonly?
> 
> i.e.
> 
> dbprintf(_("ERROR: The filesystem has..."));
> return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0;
> 
> which would also make adding the -R option unnecessary.

I like this solution very much. I'll send in this patch in a
separate email.

Hal Pomeranz

Re: [PATCH] xfs_db: add -R option

[hal]

Darrick's suggested patch to sb_logcheck() is the way to accomplish
what I want with the minimum amount of code changes.  But as I look at
the code, I'm not sure doing it the way Darrick suggests actually
expresses what we're trying to accomplish.

Let's state the problem as "Recognize that the log is dirty but allow
blockget to proceed if '-r' is used". If you accept that problem
statement, then sb_logcheck() should just tell the caller whether the
log is dirty or not. It's up to the caller to decide what to do with
that information-- in this case the init() function in db/check.c.

However, right now sb_logcheck() returns 0 for the case where 
there's a fatal error (like not being able to find the log) and for
the log being dirty. init() would need to distinguish between those
two cases and act appropriately. That means changing the return
semantics of sb_logcheck() to something like -1 on error, 0 on dirty,
and 1 on clean. 

Note that sb_logcheck() is also called by sb_logzero() and
that code would have to be tweaked to handle the new return values.

So the end result is more code changes to do it this way, but I
feel like it's a more "correct" fix. I have patches created for
both scenarios-- does anybody have a strong opinion about which way
to proceed?

--Hal


On Mon May 07 06:35, hal@deer-run.com wrote:
> > Why skip the warning?  I think xfs_db should always warn on a dirty log,
> > but perhaps we could relax the 'return 0' at the end of this hunk if the
> > fs was opened readonly?
> > 
> > i.e.
> > 
> > dbprintf(_("ERROR: The filesystem has..."));
> > return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0;
> > 
> > which would also make adding the -R option unnecessary.
> 
> I like this solution very much. I'll send in this patch in a
> separate email.
> 
> Hal Pomeranz

Re: [PATCH] xfs_db: add -R option

[Darrick J. Wong]

On Mon, May 07, 2018 at 07:56:21AM -0500, hal@deer-run.com wrote:
> Darrick's suggested patch to sb_logcheck() is the way to accomplish
> what I want with the minimum amount of code changes.  But as I look at
> the code, I'm not sure doing it the way Darrick suggests actually
> expresses what we're trying to accomplish.
> 
> Let's state the problem as "Recognize that the log is dirty but allow
> blockget to proceed if '-r' is used". If you accept that problem
> statement, then sb_logcheck() should just tell the caller whether the
> log is dirty or not. It's up to the caller to decide what to do with
> that information-- in this case the init() function in db/check.c.
> 
> However, right now sb_logcheck() returns 0 for the case where 
> there's a fatal error (like not being able to find the log) and for
> the log being dirty. init() would need to distinguish between those
> two cases and act appropriately. That means changing the return
> semantics of sb_logcheck() to something like -1 on error, 0 on dirty,
> and 1 on clean. 
> 
> Note that sb_logcheck() is also called by sb_logzero() and
> that code would have to be tweaked to handle the new return values.

Ultimately I defer to Eric on this one, but I don't see how sb_logzero
would run to completion in readonly mode, since the only caller of it is
uuid_f, which bails out if we're in readonly mode.

(Not to mention the buffer writes should fail in ro mode).

--D

> 
> So the end result is more code changes to do it this way, but I
> feel like it's a more "correct" fix. I have patches created for
> both scenarios-- does anybody have a strong opinion about which way
> to proceed?
> 
> --Hal
> 
> 
> On Mon May 07 06:35, hal@deer-run.com wrote:
> > > Why skip the warning?  I think xfs_db should always warn on a dirty log,
> > > but perhaps we could relax the 'return 0' at the end of this hunk if the
> > > fs was opened readonly?
> > > 
> > > i.e.
> > > 
> > > dbprintf(_("ERROR: The filesystem has..."));
> > > return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0;
> > > 
> > > which would also make adding the -R option unnecessary.
> > 
> > I like this solution very much. I'll send in this patch in a
> > separate email.
> > 
> > Hal Pomeranz
> 
> --
> 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] xfs_db: add -R option

[Eric Sandeen]

On 5/4/18 9:02 AM, hal@deer-run.com wrote:
> From: Hal Pomeranz <hal@deer-run.com>
> 
> -R is similar to -r, but allows for blockget on a mounted file system
> or "dirty" file system image with pending log entries.

Hm, so this probably makes it possible that blockget will wander off into
inconsistencies and fail; there's a /reason/ for the check.  So if anything
this is probably best-effort.  I doubt you can rely on blockget even completing
without error, let alone being correct.  In that case, is it still useful
to you?

Of course if you simply desire to not perturb the image you're analyzing
you'd be operating on a copy in any case, and so that's presumably not the
issue here; I assume you wish to analyze the state of the filesystem before
any log changes are applied.

What sort of information do you hope to gather after running blockget without
a replay?

I guess what I'm getting at here is yes, we could short-circuit the check
in one way or another, but I want to be sure that it really would advance
your goals.

In any case:

Using -r seems a bit odd; blockget is already a readonly operation, so
using "-r" / readonly as a modifier seems out of place.  (The same warning
would come up for -c check).

Given that xfs_db is a command for experts, and forensics analysis on an
unreplayed filesystem even more so, what about modifying the logformat
command to allow zeroing of a dirty log with some type of force option,
which would then allow blkget to proceed?  (assuming you were working on
a copy, of course.)  That would make things pretty explicit.

Is that too convoluted?

Otherwise I might suggest adding a switch specific to the blockget_f
function, which would just skip the sb_logcheck() altogether.  That would
be more targeted, and wouldn't be some global switch which affects
every current and future caller of sb_logcheck.  A warning about how
the log is being ignored and results may be inconsistent could then
be added specifically to blockget_f.

-Eric

>  This makes
> xfs_db more useful for forensics where we are often dealing with these
> types of images.  Flushing log entries to disk by mounting/unmounting
> the file system would allow us to use blockget, but would make changes
> to the file system state which are not desirable in forensics
> contexts.
> 
> Signed-off-by: Hal Pomeranz <hal@deer-run.com>
> ---
>  db/init.c         | 7 +++++--
>  db/init.h         | 1 +
>  db/sb.c           | 2 +-
>  man/man8/xfs_db.8 | 8 ++++++++
>  4 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index 29fc344..96a0e78 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -36,6 +36,7 @@ int			blkbb;
>  int			exitcode;
>  int			expert_mode;
>  int			force;
> +int                     ignore_dirty;
>  struct xfs_mount	xmount;
>  struct xfs_mount	*mp;
>  struct xlog		xlog;
> @@ -46,7 +47,7 @@ static void
>  usage(void)
>  {
>  	fprintf(stderr, _(
> -		"Usage: %s [-ifFrxV] [-p prog] [-l logdev] [-c cmd]... device\n"
> +		"Usage: %s [-ifFrRxV] [-p prog] [-l logdev] [-c cmd]... device\n"
>  		), progname);
>  	exit(1);
>  }
> @@ -66,7 +67,7 @@ init(
>  	textdomain(PACKAGE);
>  
>  	progname = basename(argv[0]);
> -	while ((c = getopt(argc, argv, "c:fFip:rxVl:")) != EOF) {
> +	while ((c = getopt(argc, argv, "c:fFip:rRxVl:")) != EOF) {
>  		switch (c) {
>  		case 'c':
>  			cmdline = xrealloc(cmdline, (ncmdline+1)*sizeof(char*));
> @@ -84,6 +85,8 @@ init(
>  		case 'p':
>  			progname = optarg;
>  			break;
> +		case 'R':
> +		        ignore_dirty = 1;
>  		case 'r':
>  			x.isreadonly = LIBXFS_ISREADONLY;
>  			break;
> diff --git a/db/init.h b/db/init.h
> index b09389e..f6bfda9 100644
> --- a/db/init.h
> +++ b/db/init.h
> @@ -20,6 +20,7 @@ extern char		*fsdevice;
>  extern int		blkbb;
>  extern int		exitcode;
>  extern int		expert_mode;
> +extern int              ignore_dirty;
>  extern xfs_mount_t	*mp;
>  extern libxfs_init_t	x;
>  extern xfs_agnumber_t	cur_agno;
> diff --git a/db/sb.c b/db/sb.c
> index c7fbfd6..4c04d79 100644
> --- a/db/sb.c
> +++ b/db/sb.c
> @@ -251,7 +251,7 @@ sb_logcheck(void)
>  	if (dirty == -1) {
>  		dbprintf(_("ERROR: cannot find log head/tail, run xfs_repair\n"));
>  		return 0;
> -	} else if (dirty == 1) {
> +	} else if (dirty == 1 && ignore_dirty == 0) {
>  		dbprintf(_(
>  "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
>  "be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 524b1ef..89be1aa 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -90,6 +90,14 @@ It is only necessary to omit this flag if a command that changes data
>  .RB ( write ", " blocktrash ", " crc )
>  is to be used.
>  .TP
> +.B -R
> +Like
> +.B -r
> +but allows the 
> +.B blockget
> +command even if the file system is "dirty" (unreconciled transactions
> +in the log).
> +.TP
>  .B \-x
>  Specifies expert mode.
>  This enables the
> 

Re: [PATCH] xfs_db: add -R option

[hal]

> Hm, so this probably makes it possible that blockget will wander off
> into inconsistencies and fail; there's a /reason/ for the check.  So
> if anything this is probably best-effort.  I doubt you can rely on
> blockget even completing without error, let alone being correct.  In
> that case, is it still useful to you?

I've been testing a modified version of xfs_db on live file systems
and dirty image files, and I have yet to have blockget fail. I agree
that it's certainly possible for blockget to blow up under these
circumstances, but it's working often enough to be useful.

> What sort of information do you hope to gather after running
> blockget without a replay?

Suppose I can match a string or magic number for a file of interest
at a specific byte offset in the file system image. A little arithmetic
and I have a daddr value. xfs_db let's me convert that to an fsblock
and then call blockuse -n to get the inode and file path. I've tried
it, and it works with my modifed xfs_db.

Until we get XFS support into libsleuthkit (which I'm working towards,
but it's going to be a while), having this functionality lets me
use xfs_db as a forensic tool. And it also lets me use xfs_db to
validate other forensic tools.

> Given that xfs_db is a command for experts, and forensics analysis on an
> unreplayed filesystem even more so, what about modifying the logformat
> command to allow zeroing of a dirty log with some type of force option,
> which would then allow blkget to proceed?  (assuming you were working on
> a copy, of course.)  That would make things pretty explicit.
> 
> Is that too convoluted?

This is not something you'd want to do on a live file system, and 
unfortunately I sometimes have to forensicate live file systems.
And I'd rather not have to modify my working copy of a forensic
image by zeroing the log if I could avoid it.

> Otherwise I might suggest adding a switch specific to the blockget_f
> function, which would just skip the sb_logcheck() altogether.  That would
> be more targeted, and wouldn't be some global switch which affects
> every current and future caller of sb_logcheck.  A warning about how
> the log is being ignored and results may be inconsistent could then
> be added specifically to blockget_f.

Which sort of brings us back to the conversation that Darrick and I
have been having. Adding a command-line switch seems wrong-- I'm fully
convinced of that. But I'd still like to be able to blockget to try
to work if the file system is dirty but "-r" is used.

Darrick came up with one fix, which is the first patch option below.
After staring at Darrick's suggestion a while, I came up with a
different fix (second patch) that requires more code changes but I
think more accurately addresses the problem statement. See previous
messages in this thread for more detail.

Let me know which you prefer and I'll submit the patch in the proper
format to the list.

Cheers!

--Hal


[PATCH OPTION #1]
---
 db/sb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/db/sb.c b/db/sb.c
index c7fbfd6..fca048c 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -259,7 +259,10 @@ sb_logcheck(void)
 "the xfs_repair -L option to destroy the log and attempt a repair.\n"
 "Note that destroying the log may cause corruption -- please attempt a mount\n"
 "of the filesystem before doing this.\n"), progname);
-		return 0;
+		/* Dirty log would normally result in "return 0".
+		   Return 1 if "-r" (read-only) option is used so
+		   blockget can proceed. */
+		return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0;
 	}
 	/* Log is clean */
 	return 1;
-- 
1.8.3.1




[PATCH OPTION #2]
---
 db/check.c | 5 ++++-
 db/sb.c    | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/db/check.c b/db/check.c
index 2f8dee5..885e84a 100644
--- a/db/check.c
+++ b/db/check.c
@@ -1849,6 +1849,7 @@ init(
 	xfs_fsblock_t	bno;
 	int		c;
 	xfs_ino_t	ino;
+	int		lc;
 	int		rt;
 
 	serious_error = 0;
@@ -1858,7 +1859,9 @@ init(
 		serious_error = 1;
 		return 0;
 	}
-	if (!sb_logcheck())
+	lc = sb_logcheck();
+	/* abort on error or if log is dirty and "-r" flag not used */
+	if (lc < 0 || (lc == 0 && !(x.isreadonly & LIBXFS_ISREADONLY)))
 		return 0;
 	rt = mp->m_sb.sb_rextents != 0;
 	dbmap = xmalloc((mp->m_sb.sb_agcount + rt) * sizeof(*dbmap));
diff --git a/db/sb.c b/db/sb.c
index c7fbfd6..88e5a94 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -235,13 +235,13 @@ sb_logcheck(void)
 		if (x.logdev && x.logdev != x.ddev) {
 			dbprintf(_("aborting - external log specified for FS "
 				 "with an internal log\n"));
-			return 0;
+			return -1;
 		}
 	} else {
 		if (!x.logdev || (x.logdev == x.ddev)) {
 			dbprintf(_("aborting - no external log specified for FS "
 				 "with an external log\n"));
-			return 0;
+			return -1;
 		}
 	}
 
@@ -250,7 +250,7 @@ sb_logcheck(void)
 	dirty = xlog_is_dirty(mp, mp->m_log, &x, 0);
 	if (dirty == -1) {
 		dbprintf(_("ERROR: cannot find log head/tail, run xfs_repair\n"));
-		return 0;
+		return -1;
 	} else if (dirty == 1) {
 		dbprintf(_(
 "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
@@ -271,7 +271,7 @@ sb_logzero(uuid_t *uuidp)
 	int	cycle = XLOG_INIT_CYCLE;
 	int	error;
 
-	if (!sb_logcheck())
+	if (sb_logcheck() < 1)
 		return 0;
 
 	/*
-- 
1.8.3.1

Re: [PATCH] xfs_db: add -R option

[Eric Sandeen]

On 5/8/18 11:13 AM, hal@deer-run.com wrote:

<snip totally acceptable & legit reasons for needing this> ;)

>> Otherwise I might suggest adding a switch specific to the blockget_f
>> function, which would just skip the sb_logcheck() altogether.  That would
>> be more targeted, and wouldn't be some global switch which affects
>> every current and future caller of sb_logcheck.  A warning about how
>> the log is being ignored and results may be inconsistent could then
>> be added specifically to blockget_f.

> Which sort of brings us back to the conversation that Darrick and I
> have been having. Adding a command-line switch seems wrong-- I'm fully
> convinced of that. But I'd still like to be able to blockget to try
> to work if the file system is dirty but "-r" is used.

Right, but my concern with "-r" or anything implying "readonly" as a
modifier is that check & blockget are /always/ readonly.  A readonly
modifier to a readonly command makes little sense IMHO.

> Darrick came up with one fix, which is the first patch option below.
> After staring at Darrick's suggestion a while, I came up with a
> different fix (second patch) that requires more code changes but I
> think more accurately addresses the problem statement. See previous
> messages in this thread for more detail.

Yup, I have read them.

> Let me know which you prefer and I'll submit the patch in the proper
> format to the list.

Well, I don't really like either patch for the reasons stated above.
xfs_db /can/ write to the filesystem, and -r disables that... but -r
as a modifier to affect the behavior of a read-only command is
unintuitive.

"It is only necessary to omit this flag if a command that changes data
(write, blocktrash, crc) is to be used."

What are the objections to a blockget modifier option?  That seemed like
the most direct & obvious solution to me.

"blockget/check -L : attempt to perform the blockget and check functions
even if the log contains unreplayed metadata," or something like that?

Thanks,
-Eric

> Cheers!
> 
> --Hal
> 
> 

Re: [PATCH] xfs_db: add -R option

[hal]

> Well, I don't really like either patch for the reasons stated above.
> xfs_db /can/ write to the filesystem, and -r disables that... but -r
> as a modifier to affect the behavior of a read-only command is
> unintuitive.
> 
> "It is only necessary to omit this flag if a command that changes data
> (write, blocktrash, crc) is to be used."
> 
> What are the objections to a blockget modifier option?  That seemed like
> the most direct & obvious solution to me.
> 
> "blockget/check -L : attempt to perform the blockget and check functions
> even if the log contains unreplayed metadata," or something like that?

Ah, OK. I'm getting what you're saying now. Let me poke around with
the code some and produce a patch that does what you're suggesting.

--Hal

Re: [PATCH] xfs_db: add -R option

[Darrick J. Wong]

On Tue, May 08, 2018 at 11:43:40AM -0500, hal@deer-run.com wrote:
> > Well, I don't really like either patch for the reasons stated above.
> > xfs_db /can/ write to the filesystem, and -r disables that... but -r
> > as a modifier to affect the behavior of a read-only command is
> > unintuitive.
> > 
> > "It is only necessary to omit this flag if a command that changes data
> > (write, blocktrash, crc) is to be used."
> > 
> > What are the objections to a blockget modifier option?  That seemed like
> > the most direct & obvious solution to me.
> > 
> > "blockget/check -L : attempt to perform the blockget and check functions
> > even if the log contains unreplayed metadata," or something like that?

That also seems fine to me. :)

> Ah, OK. I'm getting what you're saying now. Let me poke around with
> the code some and produce a patch that does what you're suggesting.

<nod>

--D

> --Hal
> 
> --
> 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] xfs_db: add -R option

[Dave Chinner]

On Tue, May 08, 2018 at 11:13:53AM -0500, hal@deer-run.com wrote:
> > Hm, so this probably makes it possible that blockget will wander off
> > into inconsistencies and fail; there's a /reason/ for the check.  So
> > if anything this is probably best-effort.  I doubt you can rely on
> > blockget even completing without error, let alone being correct.  In
> > that case, is it still useful to you?
> 
> I've been testing a modified version of xfs_db on live file systems
> and dirty image files, and I have yet to have blockget fail. I agree
> that it's certainly possible for blockget to blow up under these
> circumstances, but it's working often enough to be useful.

That's a slippery slope.

IMO, the only thing worse than not having a forensic tool for a
specific job is having a forensic tool provided by a trusted toolkit
whose results are unreliable and cannot be trusted....

> > What sort of information do you hope to gather after running
> > blockget without a replay?
> 
> Suppose I can match a string or magic number for a file of interest
> at a specific byte offset in the file system image. A little arithmetic
> and I have a daddr value. xfs_db let's me convert that to an fsblock
> and then call blockuse -n to get the inode and file path. I've tried
> it, and it works with my modifed xfs_db.

There are so many ways that can go wrong on a live filesystem.
Regardless of whether you've personally seen it go wrong, it's still
not a reliable method of information extraction. And we know that
there's every chance that xfs_db will stumble on something
unexpected in a live filesystem traversal and just crash.

FWIW, blockuse -n also requires a full filesystem scan to find the
file path (i.e. via blockget -n). That can take a long time, affect
anything that is running on the machine at the time. And by the time
it's all done, there's no guarantee the path it comes up will be
valid or correct....

What you are trying to do - offset/block to owner path lookups on
live filesystems - is something that the FSMAP ioctl and the
upcoming parent pointer functionality will solve.....

> Until we get XFS support into libsleuthkit (which I'm working towards,
> but it's going to be a while), having this functionality lets me
> use xfs_db as a forensic tool. And it also lets me use xfs_db to
> validate other forensic tools.

I'd suggest, in that case, that you limit it's use to off-line or
read-only snapshots of online filesystems? This would mean that the
results of xfs_db operations (while eceedingly slow) will be
reliable.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com