xfs_db bug

xfs_db bug

[Tom Samstag]

Hi!

I'm encountering what I believe to be a bug in xfs_db with some code
that worked previously for me. I have some code that uses xfs_db to
copy some specific data out of an XFS disk image based on an inode
number. Basically, it does similar to:

inode [inodenum]
dblock 0
p
dblock 1
p
dblock 2
p
[etc]

This worked on older versions of xfs_db but is now resulting in
corrupted output. I believe I've traced the issue to the code
introduced in https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/commit/?id=b05a31722f5d4c5e071238cbedf491d5b109f278
to support realtime files.

Specifically, the use of a `dblock` command when the previous command
was not an `inode` command looks to lead to the data in
iocur_top->data to no longer contain inode data as expected in the
line
if (is_rtfile(iocur_top->data))

I don't know the code well enough to submit a fix, but some quick
experimentation suggested it may be sufficient to move the check for
an rtfile to inside the push/pop context above after the call to
set_cur_inode(iocur_top->ino).

Hopefully this describes the issue sufficiently but please let me know
if you need anything else from me.

-Tom Samstag
NetRise.io

Re: xfs_db bug

[Darrick J. Wong]

On Thu, Jan 02, 2025 at 02:14:45PM -0800, Tom Samstag wrote:
> Hi!
> 
> I'm encountering what I believe to be a bug in xfs_db with some code
> that worked previously for me. I have some code that uses xfs_db to
> copy some specific data out of an XFS disk image based on an inode
> number. Basically, it does similar to:
> 
> inode [inodenum]
> dblock 0
> p
> dblock 1
> p
> dblock 2
> p
> [etc]
> 
> This worked on older versions of xfs_db but is now resulting in
> corrupted output. I believe I've traced the issue to the code
> introduced in https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/commit/?id=b05a31722f5d4c5e071238cbedf491d5b109f278
> to support realtime files.
> 
> Specifically, the use of a `dblock` command when the previous command
> was not an `inode` command looks to lead to the data in
> iocur_top->data to no longer contain inode data as expected in the
> line
> if (is_rtfile(iocur_top->data))
> 
> I don't know the code well enough to submit a fix, but some quick
> experimentation suggested it may be sufficient to move the check for
> an rtfile to inside the push/pop context above after the call to
> set_cur_inode(iocur_top->ino).
> 
> Hopefully this describes the issue sufficiently but please let me know
> if you need anything else from me.

Oh, yeah, that's definitely a bug.  Thank you for isolating the cause!
Does the following patch fix it for you?

--D

From: Darrick J. Wong <djwong@kernel.org>
Subject: [PATCH] xfs_db: fix multiple dblock commands

Tom Samstag reported that running the following sequence of commands no
longer works quite right:

> inode [inodenum]
> dblock 0
> p
> dblock 1
> p
> dblock 2
> p
> [etc]

Mr. Samstag looked into the source code and discovered that the
dblock_f is incorrectly accessing iocur_top->data outside of the
push_cur -> set_cur_inode -> pop_cur sequence that this function uses to
compute the type of the file data.  In other words, it's using
whatever's on top of the stack at the start of the function.  For the
"dblock 0" case above this is the inode, but for the "dblock 1" case
this is the contents of file data block 0, not an inode.

Fix this by relocating the check to the correct place.

Reported-by: tom.samstag@netrise.io
Cc: <linux-xfs@vger.kernel.org> # v6.12.0
Fixes: b05a31722f5d4c ("xfs_db: access realtime file blocks")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 db/block.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/db/block.c b/db/block.c
index 00830a3d57e1df..2f1978c41f3094 100644
--- a/db/block.c
+++ b/db/block.c
@@ -246,6 +246,7 @@ dblock_f(
 	int		nb;
 	xfs_extnum_t	nex;
 	char		*p;
+	bool		isrt;
 	typnm_t		type;
 
 	bno = (xfs_fileoff_t)strtoull(argv[1], &p, 0);
@@ -255,6 +256,7 @@ dblock_f(
 	}
 	push_cur();
 	set_cur_inode(iocur_top->ino);
+	isrt = is_rtfile(iocur_top->data);
 	type = inode_next_type();
 	pop_cur();
 	if (type == TYP_NONE) {
@@ -273,7 +275,7 @@ dblock_f(
 	ASSERT(typtab[type].typnm == type);
 	if (nex > 1)
 		make_bbmap(&bbmap, nex, bmp);
-	if (is_rtfile(iocur_top->data))
+	if (isrt)
 		set_rt_cur(&typtab[type], xfs_rtb_to_daddr(mp, dfsbno),
 				nb * blkbb, DB_RING_ADD,
 				nex > 1 ? &bbmap : NULL);

Re: xfs_db bug

[Tom Samstag]

Tested-by: Tom Samstag <tom.samstag@netrise.io>

On Thu, Jan 2, 2025 at 3:20 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Jan 02, 2025 at 02:14:45PM -0800, Tom Samstag wrote:
> > Hi!
> >
> > I'm encountering what I believe to be a bug in xfs_db with some code
> > that worked previously for me. I have some code that uses xfs_db to
> > copy some specific data out of an XFS disk image based on an inode
> > number. Basically, it does similar to:
> >
> > inode [inodenum]
> > dblock 0
> > p
> > dblock 1
> > p
> > dblock 2
> > p
> > [etc]
> >
> > This worked on older versions of xfs_db but is now resulting in
> > corrupted output. I believe I've traced the issue to the code
> > introduced in https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/commit/?id=b05a31722f5d4c5e071238cbedf491d5b109f278
> > to support realtime files.
> >
> > Specifically, the use of a `dblock` command when the previous command
> > was not an `inode` command looks to lead to the data in
> > iocur_top->data to no longer contain inode data as expected in the
> > line
> > if (is_rtfile(iocur_top->data))
> >
> > I don't know the code well enough to submit a fix, but some quick
> > experimentation suggested it may be sufficient to move the check for
> > an rtfile to inside the push/pop context above after the call to
> > set_cur_inode(iocur_top->ino).
> >
> > Hopefully this describes the issue sufficiently but please let me know
> > if you need anything else from me.
>
> Oh, yeah, that's definitely a bug.  Thank you for isolating the cause!
> Does the following patch fix it for you?
>
> --D
>
> From: Darrick J. Wong <djwong@kernel.org>
> Subject: [PATCH] xfs_db: fix multiple dblock commands
>
> Tom Samstag reported that running the following sequence of commands no
> longer works quite right:
>
> > inode [inodenum]
> > dblock 0
> > p
> > dblock 1
> > p
> > dblock 2
> > p
> > [etc]
>
> Mr. Samstag looked into the source code and discovered that the
> dblock_f is incorrectly accessing iocur_top->data outside of the
> push_cur -> set_cur_inode -> pop_cur sequence that this function uses to
> compute the type of the file data.  In other words, it's using
> whatever's on top of the stack at the start of the function.  For the
> "dblock 0" case above this is the inode, but for the "dblock 1" case
> this is the contents of file data block 0, not an inode.
>
> Fix this by relocating the check to the correct place.
>
> Reported-by: tom.samstag@netrise.io
> Cc: <linux-xfs@vger.kernel.org> # v6.12.0
> Fixes: b05a31722f5d4c ("xfs_db: access realtime file blocks")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  db/block.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/db/block.c b/db/block.c
> index 00830a3d57e1df..2f1978c41f3094 100644
> --- a/db/block.c
> +++ b/db/block.c
> @@ -246,6 +246,7 @@ dblock_f(
>         int             nb;
>         xfs_extnum_t    nex;
>         char            *p;
> +       bool            isrt;
>         typnm_t         type;
>
>         bno = (xfs_fileoff_t)strtoull(argv[1], &p, 0);
> @@ -255,6 +256,7 @@ dblock_f(
>         }
>         push_cur();
>         set_cur_inode(iocur_top->ino);
> +       isrt = is_rtfile(iocur_top->data);
>         type = inode_next_type();
>         pop_cur();
>         if (type == TYP_NONE) {
> @@ -273,7 +275,7 @@ dblock_f(
>         ASSERT(typtab[type].typnm == type);
>         if (nex > 1)
>                 make_bbmap(&bbmap, nex, bmp);
> -       if (is_rtfile(iocur_top->data))
> +       if (isrt)
>                 set_rt_cur(&typtab[type], xfs_rtb_to_daddr(mp, dfsbno),
>                                 nb * blkbb, DB_RING_ADD,
>                                 nex > 1 ? &bbmap : NULL);