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