| Message ID | 20210227200702.11977-1-davispuh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: Fix checksum output for "checksum verify failed" | expand |
On Sat, Feb 27, 2021 at 10:07:02PM +0200, Dāvis Mosāns wrote: > Currently only single checksum byte is outputted. > This fixes it so that full checksum is outputted. Thanks, that really needs fixing. > Signed-off-by: Dāvis Mosāns <davispuh@gmail.com> > --- > kernel-shared/disk-io.c | 47 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 42 insertions(+), 5 deletions(-) > > diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c > index 6f584986..8773eed7 100644 > --- a/kernel-shared/disk-io.c > +++ b/kernel-shared/disk-io.c > @@ -160,10 +160,45 @@ int btrfs_csum_data(u16 csum_type, const u8 *data, u8 *out, size_t len) > return -1; > } > > +int btrfs_format_csum(u16 csum_type, const char *data, char *output) > +{ > + int i; > + int csum_len = 0; > + int position = 0; > + int direction = 1; > + switch (csum_type) { > + case BTRFS_CSUM_TYPE_CRC32: > + csum_len = 4; This duplicates the per-csum size, you could use btrfs_csum_type_size > + position = csum_len - 1; > + direction = -1; > + break; > + case BTRFS_CSUM_TYPE_XXHASH: > + csum_len = 8; > + position = csum_len - 1; > + direction = -1; So the direction says if it's big endian or little endian, right, so for xxhash it's bigendian but the crc32c above it's little. In kernel the format is 0x%*phN, which translates to 'hexadecimal with variable length', so all the work is hidden inside printk. But still there are no changes in the 'direction'. I haven't actually checked if the printed format matches what kernel does but would think that there should be no direction adjustment needed. > + break; > + case BTRFS_CSUM_TYPE_SHA256: > + case BTRFS_CSUM_TYPE_BLAKE2: > + csum_len = 32; > + break; > + default: > + fprintf(stderr, "ERROR: unknown csum type: %d\n", csum_type); > + ASSERT(0); > + } > + > + for (i = 0; i < csum_len; i++) { > + sprintf(output + i*2, "%02X", data[position + i*direction] & 0xFF); > + } > + > + return csum_len; > +} > + > static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size, > int verify, int silent, u16 csum_type) > { > u8 result[BTRFS_CSUM_SIZE]; > + char found[BTRFS_CSUM_SIZE * 2 + 1]; // 2 hex chars for each byte + null > + char expected[BTRFS_CSUM_SIZE * 2 + 1]; > u32 len; > > len = buf->len - BTRFS_CSUM_SIZE; > @@ -172,12 +207,14 @@ static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size, > > if (verify) { > if (memcmp_extent_buffer(buf, result, 0, csum_size)) { > - /* FIXME: format */ > - if (!silent) > - printk("checksum verify failed on %llu found %08X wanted %08X\n", > + if (!silent) { > + btrfs_format_csum(csum_type, (char *)result, found); > + btrfs_format_csum(csum_type, buf->data, expected); > + printk("checksum verify failed on %llu found %s wanted %s\n", > (unsigned long long)buf->start, > - result[0], > - buf->data[0]); > + found, > + expected); > + } > return 1; > } > } else { > -- > 2.30.1
otrd., 2021. g. 2. marts, plkst. 16:18 — lietotājs David Sterba (<dsterba@suse.cz>) rakstīja: > > On Sat, Feb 27, 2021 at 10:07:02PM +0200, Dāvis Mosāns wrote: > > Currently only single checksum byte is outputted. > > This fixes it so that full checksum is outputted. > > Thanks, that really needs fixing. > > > Signed-off-by: Dāvis Mosāns <davispuh@gmail.com> > > --- > > kernel-shared/disk-io.c | 47 ++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 42 insertions(+), 5 deletions(-) > > > > diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c > > index 6f584986..8773eed7 100644 > > --- a/kernel-shared/disk-io.c > > +++ b/kernel-shared/disk-io.c > > @@ -160,10 +160,45 @@ int btrfs_csum_data(u16 csum_type, const u8 *data, u8 *out, size_t len) > > return -1; > > } > > > > +int btrfs_format_csum(u16 csum_type, const char *data, char *output) > > +{ > > + int i; > > + int csum_len = 0; > > + int position = 0; > > + int direction = 1; > > + switch (csum_type) { > > + case BTRFS_CSUM_TYPE_CRC32: > > + csum_len = 4; > > This duplicates the per-csum size, you could use btrfs_csum_type_size > Didn't notice this, fixed in v2. > > + position = csum_len - 1; > > + direction = -1; > > + break; > > + case BTRFS_CSUM_TYPE_XXHASH: > > + csum_len = 8; > > + position = csum_len - 1; > > + direction = -1; > > So the direction says if it's big endian or little endian, right, so for > xxhash it's bigendian but the crc32c above it's little. > The problem is that both crc and xxhash uses native CPU endianess - they're 32-bit and 64-bit numbers. But sha256 and blake2 always uses big endian when displayed. So here I implemented this difference. > In kernel the format is 0x%*phN, which translates to 'hexadecimal with > variable length', so all the work is hidden inside printk. But still > there are no changes in the 'direction'. I wasn't aware of such format specifier, but unfortunately here in btrfs-progs printk is just alias to fprintf which doesn't support this format. So not sure if there's any better way to implement this. > I haven't actually checked if the printed format matches what kernel > does but would think that there should be no direction adjustment > needed. I looked at kernel's implementation and it always outputs in big endian and that's why there's no such direction. kernel output: > checksum verify failed on 21057101103104 wanted 0x753cdd5f found 0x9c0ba035 level 0 this patch's output: > checksum verify failed on 21057101103104 wanted 5FDD3C75 found 35A00B9C As you can see for crc32c they're reversed. This isn't really big problem but it can be confusing as most tools output crc as number which converted to hex won't match kernel's output. Not sure if we should stick to how kernel is outputting for sake of consistency or if this would be better...
On Wed, Mar 03, 2021 at 09:39:09PM +0200, Dāvis Mosāns wrote: > otrd., 2021. g. 2. marts, plkst. 16:18 — lietotājs David Sterba > (<dsterba@suse.cz>) rakstīja: > > So the direction says if it's big endian or little endian, right, so for > > xxhash it's bigendian but the crc32c above it's little. > > The problem is that both crc and xxhash uses native CPU endianess - > they're 32-bit and 64-bit numbers. But sha256 and blake2 always uses > big endian when displayed. > So here I implemented this difference. That would be right if we really used just the integer types to show the number, but with the additional algorithms the checksum is a byte buffer. > > In kernel the format is 0x%*phN, which translates to 'hexadecimal with > > variable length', so all the work is hidden inside printk. But still > > there are no changes in the 'direction'. > > I wasn't aware of such format specifier, but unfortunately here in > btrfs-progs printk is just alias to fprintf which doesn't support this > format. So not sure if there's any better way to implement this. Potentially we could add own printf format (https://www.gnu.org/software/libc/manual/html_node/Customizing-Printf.html) but it's not part of standard libc API. > > I haven't actually checked if the printed format matches what kernel > > does but would think that there should be no direction adjustment > > needed. > > I looked at kernel's implementation and it always outputs in big > endian and that's why there's no such direction. > > kernel output: > > checksum verify failed on 21057101103104 wanted 0x753cdd5f found 0x9c0ba035 level 0 > > this patch's output: > > checksum verify failed on 21057101103104 wanted 5FDD3C75 found 35A00B9C > > As you can see for crc32c they're reversed. This isn't really big > problem but it can be confusing as most tools output crc as number > which converted to hex won't match kernel's output. > Not sure if we should stick to how kernel is outputting for sake of > consistency or if this would be better... I think the consistency needs to be preserved, it's too confusing when the same filesystem would get two different checksums reported. Reporting the bytes in hex "as they're stored in order", which corresponds to the big-endian ordering.
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c index 6f584986..8773eed7 100644 --- a/kernel-shared/disk-io.c +++ b/kernel-shared/disk-io.c @@ -160,10 +160,45 @@ int btrfs_csum_data(u16 csum_type, const u8 *data, u8 *out, size_t len) return -1; } +int btrfs_format_csum(u16 csum_type, const char *data, char *output) +{ + int i; + int csum_len = 0; + int position = 0; + int direction = 1; + switch (csum_type) { + case BTRFS_CSUM_TYPE_CRC32: + csum_len = 4; + position = csum_len - 1; + direction = -1; + break; + case BTRFS_CSUM_TYPE_XXHASH: + csum_len = 8; + position = csum_len - 1; + direction = -1; + break; + case BTRFS_CSUM_TYPE_SHA256: + case BTRFS_CSUM_TYPE_BLAKE2: + csum_len = 32; + break; + default: + fprintf(stderr, "ERROR: unknown csum type: %d\n", csum_type); + ASSERT(0); + } + + for (i = 0; i < csum_len; i++) { + sprintf(output + i*2, "%02X", data[position + i*direction] & 0xFF); + } + + return csum_len; +} + static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size, int verify, int silent, u16 csum_type) { u8 result[BTRFS_CSUM_SIZE]; + char found[BTRFS_CSUM_SIZE * 2 + 1]; // 2 hex chars for each byte + null + char expected[BTRFS_CSUM_SIZE * 2 + 1]; u32 len; len = buf->len - BTRFS_CSUM_SIZE; @@ -172,12 +207,14 @@ static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size, if (verify) { if (memcmp_extent_buffer(buf, result, 0, csum_size)) { - /* FIXME: format */ - if (!silent) - printk("checksum verify failed on %llu found %08X wanted %08X\n", + if (!silent) { + btrfs_format_csum(csum_type, (char *)result, found); + btrfs_format_csum(csum_type, buf->data, expected); + printk("checksum verify failed on %llu found %s wanted %s\n", (unsigned long long)buf->start, - result[0], - buf->data[0]); + found, + expected); + } return 1; } } else {
Currently only single checksum byte is outputted. This fixes it so that full checksum is outputted. Signed-off-by: Dāvis Mosāns <davispuh@gmail.com> --- kernel-shared/disk-io.c | 47 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-)