[PATCH] xfs_db: use correct inode to set inode type

[PATCH] xfs_db: use correct inode to set inode type

[Zorro Lang]

A test fails as:
  # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
  current
          byte offset 68096, length 512
          buffer block 128 (fsbno 16), 32 bbs
          inode 133, dir inode -1, type inode
  core.size = 123142
  current
          byte offset 65536, length 512
          buffer block 128 (fsbno 16), 32 bbs
          inode 128, dir inode 128, type inode
  core.size = 42

The "type inode" get wrong inode addr due to it trys to get the
beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
inode type".

We don't need to get the beginning of a chunk in set_iocur_type, due
to set_cur_inode(ino) will help to do all of that and make a proper
verification. We just need to give it a correct inode.

Reported-by: Jianhong Yin <jiyin@redhat.com>
Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 db/io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/db/io.c b/db/io.c
index 6628d061..61940a07 100644
--- a/db/io.c
+++ b/db/io.c
@@ -591,6 +591,7 @@ set_iocur_type(
 	/* Inodes are special; verifier checks all inodes in the chunk */
 	if (type->typnm == TYP_INODE) {
 		xfs_daddr_t	b = iocur_top->bb;
+		int		bo = iocur_top->boff;
 		xfs_ino_t	ino;
 
 		/*
@@ -598,7 +599,7 @@ set_iocur_type(
  		 * which contains the current disk location; daddr may change.
  		 */
 		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
-			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
+			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
 			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
 		set_cur_inode(ino);
 		return;
-- 
2.20.1

Re: [PATCH] xfs_db: use correct inode to set inode type

[Gao Xiang]

Hi,

On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote:
> A test fails as:
>   # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
>   current
>           byte offset 68096, length 512
>           buffer block 128 (fsbno 16), 32 bbs
>           inode 133, dir inode -1, type inode
>   core.size = 123142
>   current
>           byte offset 65536, length 512
>           buffer block 128 (fsbno 16), 32 bbs
>           inode 128, dir inode 128, type inode
>   core.size = 42
> 
> The "type inode" get wrong inode addr due to it trys to get the
> beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
> inode type".

From the kernel side, the prefered way is
commit id ("subject")

> 
> We don't need to get the beginning of a chunk in set_iocur_type, due
> to set_cur_inode(ino) will help to do all of that and make a proper
> verification. We just need to give it a correct inode.
> 
> Reported-by: Jianhong Yin <jiyin@redhat.com>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  db/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 6628d061..61940a07 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -591,6 +591,7 @@ set_iocur_type(
>  	/* Inodes are special; verifier checks all inodes in the chunk */
>  	if (type->typnm == TYP_INODE) {
>  		xfs_daddr_t	b = iocur_top->bb;
> +		int		bo = iocur_top->boff;
>  		xfs_ino_t	ino;
>  
>  		/*
> @@ -598,7 +599,7 @@ set_iocur_type(
>   		 * which contains the current disk location; daddr may change.
>   		 */
>  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
>  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
>  		set_cur_inode(ino);
>  		return;

Not familar with such code, but after looking into a bit, (my premature
thought is that) I'm wondering if we need to reverify original buffer in

if (type->fields) {
	...
	set_cur()
}

iocur_top->typ = type;

/* verify the buffer if the type has one. */
...

since set_cur() already verified the buffer in
set_cur->libxfs_buf_read->...->libxfs_readbuf_verify?

Not related to this patchset but I'm a bit curious about it now...

Thanks,
Gao Xiang

> -- 
> 2.20.1
> 

Re: [PATCH] xfs_db: use correct inode to set inode type

[Darrick J. Wong]

On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote:
> A test fails as:
>   # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
>   current
>           byte offset 68096, length 512
>           buffer block 128 (fsbno 16), 32 bbs
>           inode 133, dir inode -1, type inode
>   core.size = 123142
>   current
>           byte offset 65536, length 512
>           buffer block 128 (fsbno 16), 32 bbs
>           inode 128, dir inode 128, type inode
>   core.size = 42
> 
> The "type inode" get wrong inode addr due to it trys to get the
> beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
> inode type".

It took me a minute to figure out what this was referring to (though it
was obvious from the code change).  Might I suggest something like:

The "type inode" command accidentally moves the io cursor because it
forgets to include the io cursor's buffer offset when it computes the
inode number from the io cursor's location.

Fixes: 533d1d229a88 ("xfs_db: properly set inode type")

> We don't need to get the beginning of a chunk in set_iocur_type, due
> to set_cur_inode(ino) will help to do all of that and make a proper
> verification. We just need to give it a correct inode.
> 
> Reported-by: Jianhong Yin <jiyin@redhat.com>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  db/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 6628d061..61940a07 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -591,6 +591,7 @@ set_iocur_type(
>  	/* Inodes are special; verifier checks all inodes in the chunk */
>  	if (type->typnm == TYP_INODE) {
>  		xfs_daddr_t	b = iocur_top->bb;
> +		int		bo = iocur_top->boff;
>  		xfs_ino_t	ino;
>  
>  		/*
> @@ -598,7 +599,7 @@ set_iocur_type(
>   		 * which contains the current disk location; daddr may change.
>   		 */
>  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
>  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));

/me feels like this whole thing ought to be revised into something
involving XFS_OFFBNO_TO_AGINO to make the unit conversions easier to
read, e.g.:

	xfs_daddr_t	b = iocur_top->bb;
	xfs_agbno_t	agbno;
	xfs_agino_t	agino;

	agbno = xfs_daddr_to_agbno(mp, b);
	agino = XFS_OFFBNO_TO_AGINO(mp, agbno,
			iocur_top->boff / mp->m_sb.sb_inodesize);
	ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);

--D

>  		set_cur_inode(ino);
>  		return;
> -- 
> 2.20.1
> 

Re: [PATCH] xfs_db: use correct inode to set inode type

[Gao Xiang]

On Thu, Aug 13, 2020 at 11:29:05PM +0800, Zorro Lang wrote:

...

> > >  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> > > -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> > > +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
> > >  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
> > >  		set_cur_inode(ino);
> > >  		return;
> > 
> > Not familar with such code, but after looking into a bit, (my premature
> > thought is that) I'm wondering if we need to reverify original buffer in
> > 
> > if (type->fields) {
> > 	...
> > 	set_cur()
> > }
> > 
> > iocur_top->typ = type;
> > 
> > /* verify the buffer if the type has one. */
> > ...
> > 
> > since set_cur() already verified the buffer in
> > set_cur->libxfs_buf_read->...->libxfs_readbuf_verify?
> > 
> > Not related to this patchset but I'm a bit curious about it now...
> 
> I'm not the one who learn about xfsprogs best:) But by looking into
> set_cur_inode() -> set_cur(), I think the set_cur() does the xfs_buf
> verification, so I don't think we need to do that again at the end
> of set_iocur_type().

Nope, I wasn't saying we need to add anything after set_cur_inode().
but commit 55f224baf83d ("xfs_db: update buffer size when new type is set")

In detail, I think it might be

if (type->fields) {
	...
	set_cur()
	return;		<- here
}

iocur_top->typ = type;

/* verify the buffer if the type has one. */
...

Thanks,
Gao Xiang

> 
> Thanks,
> Zorro
> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > -- 
> > > 2.20.1
> > > 
> > 
> 

Re: [PATCH] xfs_db: use correct inode to set inode type

[Zorro Lang]

On Thu, Aug 13, 2020 at 09:53:45PM +0800, Gao Xiang wrote:
> Hi,
> 
> On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote:
> > A test fails as:
> >   # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
> >   current
> >           byte offset 68096, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 133, dir inode -1, type inode
> >   core.size = 123142
> >   current
> >           byte offset 65536, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 128, dir inode 128, type inode
> >   core.size = 42
> > 
> > The "type inode" get wrong inode addr due to it trys to get the
> > beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
> > inode type".
> 
> From the kernel side, the prefered way is
> commit id ("subject")

Hi Xiang,

Thanks for your review. I'll change my commit log.

> 
> > 
> > We don't need to get the beginning of a chunk in set_iocur_type, due
> > to set_cur_inode(ino) will help to do all of that and make a proper
> > verification. We just need to give it a correct inode.
> > 
> > Reported-by: Jianhong Yin <jiyin@redhat.com>
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  db/io.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/db/io.c b/db/io.c
> > index 6628d061..61940a07 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -591,6 +591,7 @@ set_iocur_type(
> >  	/* Inodes are special; verifier checks all inodes in the chunk */
> >  	if (type->typnm == TYP_INODE) {
> >  		xfs_daddr_t	b = iocur_top->bb;
> > +		int		bo = iocur_top->boff;
> >  		xfs_ino_t	ino;
> >  
> >  		/*
> > @@ -598,7 +599,7 @@ set_iocur_type(
> >   		 * which contains the current disk location; daddr may change.
> >   		 */
> >  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> > -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> > +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
> >  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
> >  		set_cur_inode(ino);
> >  		return;
> 
> Not familar with such code, but after looking into a bit, (my premature
> thought is that) I'm wondering if we need to reverify original buffer in
> 
> if (type->fields) {
> 	...
> 	set_cur()
> }
> 
> iocur_top->typ = type;
> 
> /* verify the buffer if the type has one. */
> ...
> 
> since set_cur() already verified the buffer in
> set_cur->libxfs_buf_read->...->libxfs_readbuf_verify?
> 
> Not related to this patchset but I'm a bit curious about it now...

I'm not the one who learn about xfsprogs best:) But by looking into
set_cur_inode() -> set_cur(), I think the set_cur() does the xfs_buf
verification, so I don't think we need to do that again at the end
of set_iocur_type().

Thanks,
Zorro

> 
> Thanks,
> Gao Xiang
> 
> > -- 
> > 2.20.1
> > 
> 

Re: [PATCH] xfs_db: use correct inode to set inode type

[Zorro Lang]

On Thu, Aug 13, 2020 at 07:56:16AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote:
> > A test fails as:
> >   # xfs_db -c "inode 133" -c "addr" -c "p core.size" -c "type inode" -c "addr" -c "p core.size" /dev/sdb1
> >   current
> >           byte offset 68096, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 133, dir inode -1, type inode
> >   core.size = 123142
> >   current
> >           byte offset 65536, length 512
> >           buffer block 128 (fsbno 16), 32 bbs
> >           inode 128, dir inode 128, type inode
> >   core.size = 42
> > 
> > The "type inode" get wrong inode addr due to it trys to get the
> > beginning of an inode chunk, refer to "533d1d229 xfs_db: properly set
> > inode type".
> 
> It took me a minute to figure out what this was referring to (though it

Sorry about that :-p

> was obvious from the code change).  Might I suggest something like:
> 
> The "type inode" command accidentally moves the io cursor because it
> forgets to include the io cursor's buffer offset when it computes the
> inode number from the io cursor's location.
> 
> Fixes: 533d1d229a88 ("xfs_db: properly set inode type")

Sure, thanks for this detailed suggestion.

> 
> > We don't need to get the beginning of a chunk in set_iocur_type, due
> > to set_cur_inode(ino) will help to do all of that and make a proper
> > verification. We just need to give it a correct inode.
> > 
> > Reported-by: Jianhong Yin <jiyin@redhat.com>
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  db/io.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/db/io.c b/db/io.c
> > index 6628d061..61940a07 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -591,6 +591,7 @@ set_iocur_type(
> >  	/* Inodes are special; verifier checks all inodes in the chunk */
> >  	if (type->typnm == TYP_INODE) {
> >  		xfs_daddr_t	b = iocur_top->bb;
> > +		int		bo = iocur_top->boff;
> >  		xfs_ino_t	ino;
> >  
> >  		/*
> > @@ -598,7 +599,7 @@ set_iocur_type(
> >   		 * which contains the current disk location; daddr may change.
> >   		 */
> >  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
> > -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
> > +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
> >  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
> 
> /me feels like this whole thing ought to be revised into something
> involving XFS_OFFBNO_TO_AGINO to make the unit conversions easier to
> read, e.g.:
> 
> 	xfs_daddr_t	b = iocur_top->bb;
> 	xfs_agbno_t	agbno;
> 	xfs_agino_t	agino;
> 
> 	agbno = xfs_daddr_to_agbno(mp, b);
> 	agino = XFS_OFFBNO_TO_AGINO(mp, agbno,
> 			iocur_top->boff / mp->m_sb.sb_inodesize);
> 	ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);

Sure, that looks clearer.

Thanks,
Zorro

> 
> --D
> 
> >  		set_cur_inode(ino);
> >  		return;
> > -- 
> > 2.20.1
> > 
> 

Re: [PATCH] xfs_db: use correct inode to set inode type

[Eric Sandeen]

On 8/13/20 6:53 AM, Gao Xiang wrote:
> Hi,
> 
> On Thu, Aug 13, 2020 at 02:03:24PM +0800, Zorro Lang wrote:

...

>> diff --git a/db/io.c b/db/io.c
>> index 6628d061..61940a07 100644
>> --- a/db/io.c
>> +++ b/db/io.c
>> @@ -591,6 +591,7 @@ set_iocur_type(
>>  	/* Inodes are special; verifier checks all inodes in the chunk */
>>  	if (type->typnm == TYP_INODE) {
>>  		xfs_daddr_t	b = iocur_top->bb;
>> +		int		bo = iocur_top->boff;
>>  		xfs_ino_t	ino;
>>  
>>  		/*
>> @@ -598,7 +599,7 @@ set_iocur_type(
>>   		 * which contains the current disk location; daddr may change.
>>   		 */
>>  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b),
>> -			((b << BBSHIFT) >> mp->m_sb.sb_inodelog) %
>> +			(((b << BBSHIFT) + bo) >> mp->m_sb.sb_inodelog) %
>>  			XFS_AGB_TO_AGINO(mp, mp->m_sb.sb_agblocks));
>>  		set_cur_inode(ino);
>>  		return;
> 
> Not familar with such code, but after looking into a bit, (my premature
> thought is that) I'm wondering if we need to reverify original buffer in
> 
> if (type->fields) {
> 	...
> 	set_cur()
> }
> 
> iocur_top->typ = type;
> 
> /* verify the buffer if the type has one. */
> ...
> 
> since set_cur() already verified the buffer in
> set_cur->libxfs_buf_read->...->libxfs_readbuf_verify?
> 
> Not related to this patchset but I'm a bit curious about it now...

I'm wondering about all this, too.

set_cur_inode() actually calls set_cur, which /does/ get into the verifier.

It definitely seems like a mess; the early return from the 

	if (type->typnm == TYP_INODE) {

block is a little weird, and the explicit verify_read() later in the
function seems like it might be unnecessary?

Agreed that it's unrelated to this bug, though.

-Eric