[PATCH] xfs_db: update sector size when type is set

[PATCH] xfs_db: update sector size when type is set

[Bill O'Donnell]

xfs_db doesn't take sector size into account when setting type.
This can result in an errant crc. For example, with a sector size
of 4096:

xfs_db> agi 0
xfs_db> p crc
crc = 0xab85043e (correct)
xfs_db> daddr
current daddr is 16
xfs_db> daddr 42
xfs_db> daddr 16
xfs_db> type agi
Metadata CRC error detected at xfs_agi block 0x10/0x200
xfs_db> p crc
crc = 0xab85043e (bad)

When xfs_db sets the new daddr in daddr_f, it does so with one
BBSIZE sector (512). Changing the type doesn't change the size
of the current buffer in iocur_top, so the checksum is calculated
on the wrong length for the type (when the actual sector size > BBSIZE (512).

For types with fields, reread the buffer to pick up the correct size for
the new type when it gets set. Facilitate the reread by setting the cursor
with set_cur().

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 db/io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/db/io.c b/db/io.c
index 9918a51..7e6d330 100644
--- a/db/io.c
+++ b/db/io.c
@@ -616,6 +616,12 @@ set_iocur_type(
 {
 	struct xfs_buf	*bp = iocur_top->bp;
 
+	if (t->fields)
+		set_cur(t,
+			iocur_top->bb,
+			fsize(t->fields, iocur_top->data,
+			      0, 0) / mp->m_sb.sb_blocksize,
+			DB_RING_IGN, NULL);
 	iocur_top->typ = t;
 
 	/* verify the buffer if the type has one. */
-- 
2.9.4

Re: [PATCH] xfs_db: update sector size when type is set

[Darrick J. Wong]

On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> xfs_db doesn't take sector size into account when setting type.
> This can result in an errant crc. For example, with a sector size
> of 4096:
> 
> xfs_db> agi 0
> xfs_db> p crc
> crc = 0xab85043e (correct)
> xfs_db> daddr
> current daddr is 16
> xfs_db> daddr 42
> xfs_db> daddr 16
> xfs_db> type agi
> Metadata CRC error detected at xfs_agi block 0x10/0x200
> xfs_db> p crc
> crc = 0xab85043e (bad)
> 
> When xfs_db sets the new daddr in daddr_f, it does so with one
> BBSIZE sector (512). Changing the type doesn't change the size
> of the current buffer in iocur_top, so the checksum is calculated
> on the wrong length for the type (when the actual sector size > BBSIZE (512).
> 
> For types with fields, reread the buffer to pick up the correct size for
> the new type when it gets set. Facilitate the reread by setting the cursor
> with set_cur().
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  db/io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/db/io.c b/db/io.c
> index 9918a51..7e6d330 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -616,6 +616,12 @@ set_iocur_type(
>  {
>  	struct xfs_buf	*bp = iocur_top->bp;
>  
> +	if (t->fields)
> +		set_cur(t,
> +			iocur_top->bb,
> +			fsize(t->fields, iocur_top->data,
> +			      0, 0) / mp->m_sb.sb_blocksize,

I thought the third parameter to set_cur (which is fed as the third
parameter to libxfs_readbuf) was expressed in units of basic blocks
(i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
think?), so dividing by sb_blocksize renders units of fs blocks, not
basic blocks.

<shrug> the naming isn't helpful at all:

void
set_cur(
	const typ_t	*t,
	int64_t		d,
	int		c,
...
	iocur_top->len = BBTOB(c);

<codeconfused>

--D

> +			DB_RING_IGN, NULL);
>  	iocur_top->typ = t;
>  
>  	/* verify the buffer if the type has one. */
> -- 
> 2.9.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] xfs_db: update sector size when type is set

[Eric Sandeen]

On 6/22/17 2:47 PM, Darrick J. Wong wrote:
> On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
>> xfs_db doesn't take sector size into account when setting type.
>> This can result in an errant crc. For example, with a sector size
>> of 4096:
>>
>> xfs_db> agi 0
>> xfs_db> p crc
>> crc = 0xab85043e (correct)
>> xfs_db> daddr
>> current daddr is 16
>> xfs_db> daddr 42
>> xfs_db> daddr 16
>> xfs_db> type agi
>> Metadata CRC error detected at xfs_agi block 0x10/0x200
>> xfs_db> p crc
>> crc = 0xab85043e (bad)
>>
>> When xfs_db sets the new daddr in daddr_f, it does so with one
>> BBSIZE sector (512). Changing the type doesn't change the size
>> of the current buffer in iocur_top, so the checksum is calculated
>> on the wrong length for the type (when the actual sector size > BBSIZE (512).
>>
>> For types with fields, reread the buffer to pick up the correct size for
>> the new type when it gets set. Facilitate the reread by setting the cursor
>> with set_cur().
>>
>> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
>> ---
>>  db/io.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/db/io.c b/db/io.c
>> index 9918a51..7e6d330 100644
>> --- a/db/io.c
>> +++ b/db/io.c
>> @@ -616,6 +616,12 @@ set_iocur_type(
>>  {
>>  	struct xfs_buf	*bp = iocur_top->bp;
>>  

a comment about why only this "if" case is there would be good.

>> +	if (t->fields)
>> +		set_cur(t,
>> +			iocur_top->bb,
>> +			fsize(t->fields, iocur_top->data,
>> +			      0, 0) / mp->m_sb.sb_blocksize,
> 
> I thought the third parameter to set_cur (which is fed as the third
> parameter to libxfs_readbuf) was expressed in units of basic blocks
> (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> think?), 

nope, bits :)

> so dividing by sb_blocksize renders units of fs blocks, not
> basic blocks.

As Darrick just pointed out to me, you treated bits as bytes and
daddr as fsblock so for 4k blocks, 8/8 came out right ;)

I think what you want for the size is something like:

BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));

It might be a little nicer to stash that in a temp var so you
don't have all that gunk in the call to set_cur.
 
> <shrug> the naming isn't helpful at all:
> 
> void
> set_cur(
> 	const typ_t	*t,
> 	int64_t		d,
> 	int		c,
> ...
> 	iocur_top->len = BBTOB(c);
> 
> <codeconfused>

yup isn't xfs_db fun?

> 
> --D
> 
>> +			DB_RING_IGN, NULL);
>>  	iocur_top->typ = t;
>>  
>>  	/* verify the buffer if the type has one. */
>> -- 
>> 2.9.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] xfs_db: update sector size when type is set

[Bill O'Donnell]

On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote:
> On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> > xfs_db doesn't take sector size into account when setting type.
> > This can result in an errant crc. For example, with a sector size
> > of 4096:
> > 
> > xfs_db> agi 0
> > xfs_db> p crc
> > crc = 0xab85043e (correct)
> > xfs_db> daddr
> > current daddr is 16
> > xfs_db> daddr 42
> > xfs_db> daddr 16
> > xfs_db> type agi
> > Metadata CRC error detected at xfs_agi block 0x10/0x200
> > xfs_db> p crc
> > crc = 0xab85043e (bad)
> > 
> > When xfs_db sets the new daddr in daddr_f, it does so with one
> > BBSIZE sector (512). Changing the type doesn't change the size
> > of the current buffer in iocur_top, so the checksum is calculated
> > on the wrong length for the type (when the actual sector size > BBSIZE (512).
> > 
> > For types with fields, reread the buffer to pick up the correct size for
> > the new type when it gets set. Facilitate the reread by setting the cursor
> > with set_cur().
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  db/io.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/db/io.c b/db/io.c
> > index 9918a51..7e6d330 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -616,6 +616,12 @@ set_iocur_type(
> >  {
> >  	struct xfs_buf	*bp = iocur_top->bp;
> >  
> > +	if (t->fields)
> > +		set_cur(t,
> > +			iocur_top->bb,
> > +			fsize(t->fields, iocur_top->data,
> > +			      0, 0) / mp->m_sb.sb_blocksize,
> 
> I thought the third parameter to set_cur (which is fed as the third
> parameter to libxfs_readbuf) was expressed in units of basic blocks
> (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> think?), so dividing by sb_blocksize renders units of fs blocks, not
> basic blocks.
> 

fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks.

> <shrug> the naming isn't helpful at all:

I also found the naming through the call stack to be terrible and damned
confusing (c, d ? really??), but left that battle for another day, another
patch. ;)

Thanks-
Bill

> 
> void
> set_cur(
> 	const typ_t	*t,
> 	int64_t		d,
> 	int		c,
> ...
> 	iocur_top->len = BBTOB(c);
> 
> <codeconfused>
> 
> --D
> 
> > +			DB_RING_IGN, NULL);
> >  	iocur_top->typ = t;
> >  
> >  	/* verify the buffer if the type has one. */
> > -- 
> > 2.9.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] xfs_db: update sector size when type is set

[Bill O'Donnell]

On Thu, Jun 22, 2017 at 03:16:27PM -0500, Eric Sandeen wrote:
> 
> 
> On 6/22/17 2:47 PM, Darrick J. Wong wrote:
> > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> >> xfs_db doesn't take sector size into account when setting type.
> >> This can result in an errant crc. For example, with a sector size
> >> of 4096:
> >>
> >> xfs_db> agi 0
> >> xfs_db> p crc
> >> crc = 0xab85043e (correct)
> >> xfs_db> daddr
> >> current daddr is 16
> >> xfs_db> daddr 42
> >> xfs_db> daddr 16
> >> xfs_db> type agi
> >> Metadata CRC error detected at xfs_agi block 0x10/0x200
> >> xfs_db> p crc
> >> crc = 0xab85043e (bad)
> >>
> >> When xfs_db sets the new daddr in daddr_f, it does so with one
> >> BBSIZE sector (512). Changing the type doesn't change the size
> >> of the current buffer in iocur_top, so the checksum is calculated
> >> on the wrong length for the type (when the actual sector size > BBSIZE (512).
> >>
> >> For types with fields, reread the buffer to pick up the correct size for
> >> the new type when it gets set. Facilitate the reread by setting the cursor
> >> with set_cur().
> >>
> >> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> >> ---
> >>  db/io.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/db/io.c b/db/io.c
> >> index 9918a51..7e6d330 100644
> >> --- a/db/io.c
> >> +++ b/db/io.c
> >> @@ -616,6 +616,12 @@ set_iocur_type(
> >>  {
> >>  	struct xfs_buf	*bp = iocur_top->bp;
> >>  
> 
> a comment about why only this "if" case is there would be good.

agreed.

> 
> >> +	if (t->fields)
> >> +		set_cur(t,
> >> +			iocur_top->bb,
> >> +			fsize(t->fields, iocur_top->data,
> >> +			      0, 0) / mp->m_sb.sb_blocksize,
> > 
> > I thought the third parameter to set_cur (which is fed as the third
> > parameter to libxfs_readbuf) was expressed in units of basic blocks
> > (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> > think?), 
> 
> nope, bits :)
> 
> > so dividing by sb_blocksize renders units of fs blocks, not
> > basic blocks.
> 
> As Darrick just pointed out to me, you treated bits as bytes and
> daddr as fsblock so for 4k blocks, 8/8 came out right ;)

Yeah. Doh! I tested with sector sizes 512 and 4096 and it all just worked,
since mp->m_sb.sb_blocksize returned 1 and 8, respectively.

> 
> I think what you want for the size is something like:
> 
> BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> 
> It might be a little nicer to stash that in a temp var so you
> don't have all that gunk in the call to set_cur.

Yep.


> > <shrug> the naming isn't helpful at all:
> > 
> > void
> > set_cur(
> > 	const typ_t	*t,
> > 	int64_t		d,
> > 	int		c,
> > ...
> > 	iocur_top->len = BBTOB(c);
> > 
> > <codeconfused>
> 
> yup isn't xfs_db fun?
> 
> > 
> > --D
> > 
> >> +			DB_RING_IGN, NULL);
> >>  	iocur_top->typ = t;
> >>  
> >>  	/* verify the buffer if the type has one. */
> >> -- 
> >> 2.9.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] xfs_db: update sector size when type is set

[Bill O'Donnell]

On Thu, Jun 22, 2017 at 03:31:08PM -0500, Bill O'Donnell wrote:
> On Thu, Jun 22, 2017 at 03:16:27PM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 6/22/17 2:47 PM, Darrick J. Wong wrote:
> > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> > >> xfs_db doesn't take sector size into account when setting type.
> > >> This can result in an errant crc. For example, with a sector size
> > >> of 4096:
> > >>
> > >> xfs_db> agi 0
> > >> xfs_db> p crc
> > >> crc = 0xab85043e (correct)
> > >> xfs_db> daddr
> > >> current daddr is 16
> > >> xfs_db> daddr 42
> > >> xfs_db> daddr 16
> > >> xfs_db> type agi
> > >> Metadata CRC error detected at xfs_agi block 0x10/0x200
> > >> xfs_db> p crc
> > >> crc = 0xab85043e (bad)
> > >>
> > >> When xfs_db sets the new daddr in daddr_f, it does so with one
> > >> BBSIZE sector (512). Changing the type doesn't change the size
> > >> of the current buffer in iocur_top, so the checksum is calculated
> > >> on the wrong length for the type (when the actual sector size > BBSIZE (512).
> > >>
> > >> For types with fields, reread the buffer to pick up the correct size for
> > >> the new type when it gets set. Facilitate the reread by setting the cursor
> > >> with set_cur().
> > >>
> > >> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > >> ---
> > >>  db/io.c | 6 ++++++
> > >>  1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/db/io.c b/db/io.c
> > >> index 9918a51..7e6d330 100644
> > >> --- a/db/io.c
> > >> +++ b/db/io.c
> > >> @@ -616,6 +616,12 @@ set_iocur_type(
> > >>  {
> > >>  	struct xfs_buf	*bp = iocur_top->bp;
> > >>  
> > 
> > a comment about why only this "if" case is there would be good.
> 
> agreed.
> 
> > 
> > >> +	if (t->fields)
> > >> +		set_cur(t,
> > >> +			iocur_top->bb,
> > >> +			fsize(t->fields, iocur_top->data,
> > >> +			      0, 0) / mp->m_sb.sb_blocksize,
> > > 
> > > I thought the third parameter to set_cur (which is fed as the third
> > > parameter to libxfs_readbuf) was expressed in units of basic blocks
> > > (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> > > think?), 
> > 
> > nope, bits :)
> > 
> > > so dividing by sb_blocksize renders units of fs blocks, not
> > > basic blocks.
> > 
> > As Darrick just pointed out to me, you treated bits as bytes and
> > daddr as fsblock so for 4k blocks, 8/8 came out right ;)
> 
> Yeah. Doh! I tested with sector sizes 512 and 4096 and it all just worked,
> since mp->m_sb.sb_blocksize returned 1 and 8, respectively.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Correction: _dividing by mp->m_sb.sb_blocksize_ gave 1 and 8, respectively.                         


> 
> > 
> > I think what you want for the size is something like:
> > 
> > BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> > 
> > It might be a little nicer to stash that in a temp var so you
> > don't have all that gunk in the call to set_cur.
> 
> Yep.
> 
> 
> > > <shrug> the naming isn't helpful at all:
> > > 
> > > void
> > > set_cur(
> > > 	const typ_t	*t,
> > > 	int64_t		d,
> > > 	int		c,
> > > ...
> > > 	iocur_top->len = BBTOB(c);
> > > 
> > > <codeconfused>
> > 
> > yup isn't xfs_db fun?
> > 
> > > 
> > > --D
> > > 
> > >> +			DB_RING_IGN, NULL);
> > >>  	iocur_top->typ = t;
> > >>  
> > >>  	/* verify the buffer if the type has one. */
> > >> -- 
> > >> 2.9.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] xfs_db: update sector size when type is set

[Darrick J. Wong]

On Thu, Jun 22, 2017 at 03:23:22PM -0500, Bill O'Donnell wrote:
> On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> > > xfs_db doesn't take sector size into account when setting type.
> > > This can result in an errant crc. For example, with a sector size
> > > of 4096:
> > > 
> > > xfs_db> agi 0
> > > xfs_db> p crc
> > > crc = 0xab85043e (correct)
> > > xfs_db> daddr
> > > current daddr is 16
> > > xfs_db> daddr 42
> > > xfs_db> daddr 16
> > > xfs_db> type agi
> > > Metadata CRC error detected at xfs_agi block 0x10/0x200
> > > xfs_db> p crc
> > > crc = 0xab85043e (bad)
> > > 
> > > When xfs_db sets the new daddr in daddr_f, it does so with one
> > > BBSIZE sector (512). Changing the type doesn't change the size
> > > of the current buffer in iocur_top, so the checksum is calculated
> > > on the wrong length for the type (when the actual sector size > BBSIZE (512).
> > > 
> > > For types with fields, reread the buffer to pick up the correct size for
> > > the new type when it gets set. Facilitate the reread by setting the cursor
> > > with set_cur().
> > > 
> > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > ---
> > >  db/io.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/db/io.c b/db/io.c
> > > index 9918a51..7e6d330 100644
> > > --- a/db/io.c
> > > +++ b/db/io.c
> > > @@ -616,6 +616,12 @@ set_iocur_type(
> > >  {
> > >  	struct xfs_buf	*bp = iocur_top->bp;
> > >  
> > > +	if (t->fields)
> > > +		set_cur(t,
> > > +			iocur_top->bb,
> > > +			fsize(t->fields, iocur_top->data,
> > > +			      0, 0) / mp->m_sb.sb_blocksize,
> > 
> > I thought the third parameter to set_cur (which is fed as the third
> > parameter to libxfs_readbuf) was expressed in units of basic blocks
> > (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> > think?), so dividing by sb_blocksize renders units of fs blocks, not
> > basic blocks.
> > 
> 
> fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks.

Yes, the subtlety that fsize() returns bits and sb_blocksize is usually
4096 / 512 == 8 means that the conversion works for the default case,
but that leaves us open to bugs.  Consider if sb_blocksize == 65536.

There are so many different unit types in XFS that it is crucial for
auditing to make analyzing unit conversions as easy as possible.  That
means that we must show all intermediate steps in converting from one
unit system to another, and we must not short circuit those hops.  This
helps the reviewers find bugs.

For example, consider Eric's suggested replacement for the third parameter:

BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));

We know that the units of the third parameter are in basic blocks from
flailing around in the set_cur code due to unhelpful parameter names.
BTOBB converts bytes to basic blocks, so we theorize that the input must
be in units of bytes.  We can then unpack the next level:

byteize(fsize(...))

Starting from the premise that this expression is in units of bytes, we
apply the knowledge that byteize converts bit counts to byte counts, so
we now also theorize that the input must be in units of bits.  Unpack
the next level:

fsize(...)

To finish checking the expression value, we simply have to confirm that
fsize() returns units of bits.  Therefore, the expression is correct.
The unit analysis is straightforward and does not require knowledge of
subtleties.

--D

> 
> > <shrug> the naming isn't helpful at all:
> 
> I also found the naming through the call stack to be terrible and damned
> confusing (c, d ? really??), but left that battle for another day, another
> patch. ;)
> 
> Thanks-
> Bill
> 
> > 
> > void
> > set_cur(
> > 	const typ_t	*t,
> > 	int64_t		d,
> > 	int		c,
> > ...
> > 	iocur_top->len = BBTOB(c);
> > 
> > <codeconfused>
> > 
> > --D
> > 
> > > +			DB_RING_IGN, NULL);
> > >  	iocur_top->typ = t;
> > >  
> > >  	/* verify the buffer if the type has one. */
> > > -- 
> > > 2.9.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] xfs_db: update sector size when type is set

[Bill O'Donnell]

On Thu, Jun 22, 2017 at 01:39:41PM -0700, Darrick J. Wong wrote:
> On Thu, Jun 22, 2017 at 03:23:22PM -0500, Bill O'Donnell wrote:
> > On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote:
> > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote:
> > > > xfs_db doesn't take sector size into account when setting type.
> > > > This can result in an errant crc. For example, with a sector size
> > > > of 4096:
> > > > 
> > > > xfs_db> agi 0
> > > > xfs_db> p crc
> > > > crc = 0xab85043e (correct)
> > > > xfs_db> daddr
> > > > current daddr is 16
> > > > xfs_db> daddr 42
> > > > xfs_db> daddr 16
> > > > xfs_db> type agi
> > > > Metadata CRC error detected at xfs_agi block 0x10/0x200
> > > > xfs_db> p crc
> > > > crc = 0xab85043e (bad)
> > > > 
> > > > When xfs_db sets the new daddr in daddr_f, it does so with one
> > > > BBSIZE sector (512). Changing the type doesn't change the size
> > > > of the current buffer in iocur_top, so the checksum is calculated
> > > > on the wrong length for the type (when the actual sector size > BBSIZE (512).
> > > > 
> > > > For types with fields, reread the buffer to pick up the correct size for
> > > > the new type when it gets set. Facilitate the reread by setting the cursor
> > > > with set_cur().
> > > > 
> > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > > ---
> > > >  db/io.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/db/io.c b/db/io.c
> > > > index 9918a51..7e6d330 100644
> > > > --- a/db/io.c
> > > > +++ b/db/io.c
> > > > @@ -616,6 +616,12 @@ set_iocur_type(
> > > >  {
> > > >  	struct xfs_buf	*bp = iocur_top->bp;
> > > >  
> > > > +	if (t->fields)
> > > > +		set_cur(t,
> > > > +			iocur_top->bb,
> > > > +			fsize(t->fields, iocur_top->data,
> > > > +			      0, 0) / mp->m_sb.sb_blocksize,
> > > 
> > > I thought the third parameter to set_cur (which is fed as the third
> > > parameter to libxfs_readbuf) was expressed in units of basic blocks
> > > (i.e. 512 bytes)?  fsize returns the size of the field in bytes (I
> > > think?), so dividing by sb_blocksize renders units of fs blocks, not
> > > basic blocks.
> > > 
> > 
> > fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks.
> 
> Yes, the subtlety that fsize() returns bits and sb_blocksize is usually
> 4096 / 512 == 8 means that the conversion works for the default case,
> but that leaves us open to bugs.  Consider if sb_blocksize == 65536.
> 
> There are so many different unit types in XFS that it is crucial for
> auditing to make analyzing unit conversions as easy as possible.  That
> means that we must show all intermediate steps in converting from one
> unit system to another, and we must not short circuit those hops.  This
> helps the reviewers find bugs.
> 
> For example, consider Eric's suggested replacement for the third parameter:
> 
> BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> 
> We know that the units of the third parameter are in basic blocks from
> flailing around in the set_cur code due to unhelpful parameter names.
> BTOBB converts bytes to basic blocks, so we theorize that the input must
> be in units of bytes.  We can then unpack the next level:
> 
> byteize(fsize(...))
> 
> Starting from the premise that this expression is in units of bytes, we
> apply the knowledge that byteize converts bit counts to byte counts, so
> we now also theorize that the input must be in units of bits.  Unpack
> the next level:
> 
> fsize(...)
> 
> To finish checking the expression value, we simply have to confirm that
> fsize() returns units of bits.  Therefore, the expression is correct.
> The unit analysis is straightforward and does not require knowledge of
> subtleties.

Excellent analysis, thanks!
Eric suggested I come up with a (robust) test case, and as you point out,
the need is fairly obvious. 

Thanks-
Bill

> 
> --D
> 
> > 
> > > <shrug> the naming isn't helpful at all:
> > 
> > I also found the naming through the call stack to be terrible and damned
> > confusing (c, d ? really??), but left that battle for another day, another
> > patch. ;)
> > 
> > Thanks-
> > Bill
> > 
> > > 
> > > void
> > > set_cur(
> > > 	const typ_t	*t,
> > > 	int64_t		d,
> > > 	int		c,
> > > ...
> > > 	iocur_top->len = BBTOB(c);
> > > 
> > > <codeconfused>
> > > 
> > > --D
> > > 
> > > > +			DB_RING_IGN, NULL);
> > > >  	iocur_top->typ = t;
> > > >  
> > > >  	/* verify the buffer if the type has one. */
> > > > -- 
> > > > 2.9.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

[PATCH v2] xfs_db: update sector size when type is set

[Bill O'Donnell]

xfs_db doesn't take sector size into account when setting type.
This can result in an errant crc. For example, with a sector size
of 4096:

xfs_db> agi 0
xfs_db> p crc
crc = 0xab85043e (correct)
xfs_db> daddr
current daddr is 16
xfs_db> daddr 42
xfs_db> daddr 16
xfs_db> type agi
Metadata CRC error detected at xfs_agi block 0x10/0x200
xfs_db> p crc
crc = 0xab85043e (bad)

When xfs_db sets the new daddr in daddr_f, it does so with one
BBSIZE sector (512). Changing the type doesn't change the size
of the current buffer in iocur_top, so the checksum is calculated
on the wrong length for the type (when the actual sector size > BBSIZE (512)).

For types with fields, reread the buffer to pick up the correct size for
the new type when it gets set. Facilitate the reread by setting the cursor
with set_cur().

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v2: correction to argument 3 in call to set_cur: compute and pass basic block count;
    add comment regarding conditional call to set_cur.
    

 db/io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/db/io.c b/db/io.c
index 9918a51..792cc0a 100644
--- a/db/io.c
+++ b/db/io.c
@@ -615,7 +615,13 @@ set_iocur_type(
 	const typ_t	*t)
 {
 	struct xfs_buf	*bp = iocur_top->bp;
+	int bb_count;
 
+	/* adjust cursor for types that contain fields */
+	if (t->fields) {
+		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
+		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
+	}
 	iocur_top->typ = t;
 
 	/* verify the buffer if the type has one. */
-- 
2.9.4

Re: [PATCH v2] xfs_db: update sector size when type is set

[Carlos Maiolino]

Hey Bill,

> diff --git a/db/io.c b/db/io.c
> index 9918a51..792cc0a 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -615,7 +615,13 @@ set_iocur_type(
>  	const typ_t	*t)
>  {
>  	struct xfs_buf	*bp = iocur_top->bp;
> +	int bb_count;
>  
> +	/* adjust cursor for types that contain fields */
> +	if (t->fields) {
> +		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));

Doesn't byteize() requires:

#include "bit.h"

?

I can't build xfsprogs with this patch without including bit.h

In file included from ../include/xfs.h:68:0,
                 from ../include/libxfs.h:24,
                 from io.c:19:
io.c: In function ‘set_iocur_type’:
io.c:600:20: warning: implicit declaration of function ‘byteize’
[-Wimplicit-function-declaration]
   bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
                    ^


Although, after including it, the patch works fine, fixes the problem with
different sector sizes.

After fixing the include above, you can add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


Cheers


> +		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> +	}
>  	iocur_top->typ = t;
>  
>  	/* verify the buffer if the type has one. */
> -- 
> 2.9.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

-- 
Carlos

Re: [PATCH v2] xfs_db: update sector size when type is set

[Bill O'Donnell]

On Fri, Jun 23, 2017 at 12:16:02PM +0200, Carlos Maiolino wrote:
> Hey Bill,
> 
> > diff --git a/db/io.c b/db/io.c
> > index 9918a51..792cc0a 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -615,7 +615,13 @@ set_iocur_type(
> >  	const typ_t	*t)
> >  {
> >  	struct xfs_buf	*bp = iocur_top->bp;
> > +	int bb_count;
> >  
> > +	/* adjust cursor for types that contain fields */
> > +	if (t->fields) {
> > +		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> 
> Doesn't byteize() requires:
> 
> #include "bit.h"
> 
> ?

Gah! Yes it's required. I had it on my test box, but missed it
in my v2 submit. I'll fix it.

Thanks!
Bill


> 
> I can't build xfsprogs with this patch without including bit.h
> 
> In file included from ../include/xfs.h:68:0,
>                  from ../include/libxfs.h:24,
>                  from io.c:19:
> io.c: In function ‘set_iocur_type’:
> io.c:600:20: warning: implicit declaration of function ‘byteize’
> [-Wimplicit-function-declaration]
>    bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
>                     ^
> 
> 
> Although, after including it, the patch works fine, fixes the problem with
> different sector sizes.
> 
> After fixing the include above, you can add:
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 
> Cheers
> 
> 
> > +		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> > +	}
> >  	iocur_top->typ = t;
> >  
> >  	/* verify the buffer if the type has one. */
> > -- 
> > 2.9.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
> 
> -- 
> Carlos

[PATCH v3] xfs_db: update sector size when type is set

[Bill O'Donnell]

xfs_db doesn't take sector size into account when setting type.
This can result in an errant crc. For example, with a sector size
of 4096:

xfs_db> agi 0
xfs_db> p crc
crc = 0xab85043e (correct)
xfs_db> daddr
current daddr is 16
xfs_db> daddr 42
xfs_db> daddr 16
xfs_db> type agi
Metadata CRC error detected at xfs_agi block 0x10/0x200
xfs_db> p crc
crc = 0xab85043e (bad)

When xfs_db sets the new daddr in daddr_f, it does so with one
BBSIZE sector (512). Changing the type doesn't change the size
of the current buffer in iocur_top, so the checksum is calculated
on the wrong length for the type (when the actual sector size > BBSIZE (512).

For types with fields, reread the buffer to pick up the correct size for
the new type when it gets set. Facilitate the reread by setting the cursor
with set_cur().

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---

v2: correction to argument 3 in call to set_cur: compute and pass basic block count;
    add comment regarding conditional call to set_cur.
v3: include bit.h for byteize function

 db/io.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/db/io.c b/db/io.c
index 9918a51..b97b710 100644
--- a/db/io.c
+++ b/db/io.c
@@ -28,6 +28,7 @@
 #include "init.h"
 #include "malloc.h"
 #include "crc.h"
+#include "bit.h"
 
 static int	pop_f(int argc, char **argv);
 static void     pop_help(void);
@@ -615,7 +616,13 @@ set_iocur_type(
 	const typ_t	*t)
 {
 	struct xfs_buf	*bp = iocur_top->bp;
+	int bb_count;
 
+	/* adjust cursor for types that contain fields */
+	if (t->fields) {
+		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
+		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
+	}
 	iocur_top->typ = t;
 
 	/* verify the buffer if the type has one. */
-- 
2.9.4

Re: [PATCH v3] xfs_db: update sector size when type is set

[Carlos Maiolino]

On Fri, Jun 23, 2017 at 05:34:05AM -0500, Bill O'Donnell wrote:
> xfs_db doesn't take sector size into account when setting type.
> This can result in an errant crc. For example, with a sector size
> of 4096:
> 
> xfs_db> agi 0
> xfs_db> p crc
> crc = 0xab85043e (correct)
> xfs_db> daddr
> current daddr is 16
> xfs_db> daddr 42
> xfs_db> daddr 16
> xfs_db> type agi
> Metadata CRC error detected at xfs_agi block 0x10/0x200
> xfs_db> p crc
> crc = 0xab85043e (bad)
> 
> When xfs_db sets the new daddr in daddr_f, it does so with one
> BBSIZE sector (512). Changing the type doesn't change the size
> of the current buffer in iocur_top, so the checksum is calculated
> on the wrong length for the type (when the actual sector size > BBSIZE (512).
> 
> For types with fields, reread the buffer to pick up the correct size for
> the new type when it gets set. Facilitate the reread by setting the cursor
> with set_cur().
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> 
> v2: correction to argument 3 in call to set_cur: compute and pass basic block count;
>     add comment regarding conditional call to set_cur.
> v3: include bit.h for byteize function
> 
>  db/io.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/db/io.c b/db/io.c
> index 9918a51..b97b710 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -28,6 +28,7 @@
>  #include "init.h"
>  #include "malloc.h"
>  #include "crc.h"
> +#include "bit.h"
>  
Thanks!!

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


>  static int	pop_f(int argc, char **argv);
>  static void     pop_help(void);
> @@ -615,7 +616,13 @@ set_iocur_type(
>  	const typ_t	*t)
>  {
>  	struct xfs_buf	*bp = iocur_top->bp;
> +	int bb_count;
>  
> +	/* adjust cursor for types that contain fields */
> +	if (t->fields) {
> +		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> +		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> +	}
>  	iocur_top->typ = t;
>  
>  	/* verify the buffer if the type has one. */
> -- 
> 2.9.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

-- 
Carlos

Re: [PATCH v3] xfs_db: update sector size when type is set

[Darrick J. Wong]

On Fri, Jun 23, 2017 at 05:34:05AM -0500, Bill O'Donnell wrote:
> xfs_db doesn't take sector size into account when setting type.
> This can result in an errant crc. For example, with a sector size
> of 4096:
> 
> xfs_db> agi 0
> xfs_db> p crc
> crc = 0xab85043e (correct)
> xfs_db> daddr
> current daddr is 16
> xfs_db> daddr 42
> xfs_db> daddr 16
> xfs_db> type agi
> Metadata CRC error detected at xfs_agi block 0x10/0x200
> xfs_db> p crc
> crc = 0xab85043e (bad)
> 
> When xfs_db sets the new daddr in daddr_f, it does so with one
> BBSIZE sector (512). Changing the type doesn't change the size
> of the current buffer in iocur_top, so the checksum is calculated
> on the wrong length for the type (when the actual sector size > BBSIZE (512).
> 
> For types with fields, reread the buffer to pick up the correct size for
> the new type when it gets set. Facilitate the reread by setting the cursor
> with set_cur().
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2: correction to argument 3 in call to set_cur: compute and pass basic block count;
>     add comment regarding conditional call to set_cur.
> v3: include bit.h for byteize function
> 
>  db/io.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/db/io.c b/db/io.c
> index 9918a51..b97b710 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -28,6 +28,7 @@
>  #include "init.h"
>  #include "malloc.h"
>  #include "crc.h"
> +#include "bit.h"
>  
>  static int	pop_f(int argc, char **argv);
>  static void     pop_help(void);
> @@ -615,7 +616,13 @@ set_iocur_type(
>  	const typ_t	*t)
>  {
>  	struct xfs_buf	*bp = iocur_top->bp;
> +	int bb_count;
>  
> +	/* adjust cursor for types that contain fields */
> +	if (t->fields) {
> +		bb_count = BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0)));
> +		set_cur(t, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> +	}
>  	iocur_top->typ = t;
>  
>  	/* verify the buffer if the type has one. */
> -- 
> 2.9.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