[PATCH] btrfs-progs: Fix checksum output for "checksum verify failed"

[PATCH] btrfs-progs: Fix checksum output for "checksum verify failed"

[Dāvis Mosāns]

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(-)

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 {
-- 
2.30.1

Re: [PATCH] btrfs-progs: Fix checksum output for "checksum verify failed"

[David Sterba]

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

Re: [PATCH] btrfs-progs: Fix checksum output for "checksum verify failed"

[Dāvis Mosāns]

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...

Re: [PATCH] btrfs-progs: Fix checksum output for "checksum verify failed"

[David Sterba]

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.

[PATCH v2] btrfs-progs: Fix checksum output for "checksum verify failed"

[Dāvis Mosāns]

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 | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 6f584986..10b2421e 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -160,10 +160,30 @@ int btrfs_csum_data(u16 csum_type, const u8 *data, u8 *out, size_t len)
 	return -1;
 }
 
+int btrfs_format_csum(u16 csum_type, u16 csum_size, const char *data, char *output)
+{
+	int i;
+	int position = 0;
+	int direction = 1;
+	if (csum_type == BTRFS_CSUM_TYPE_CRC32 ||
+		csum_type == BTRFS_CSUM_TYPE_XXHASH) {
+		position = csum_size - 1;
+		direction = -1;
+	}
+
+	for (i = 0; i < csum_size; i++) {
+		sprintf(output + i*2, "%02X", data[position + i*direction] & 0xFF);
+	}
+
+	return csum_size;
+}
+
 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 wanted[BTRFS_CSUM_SIZE * 2 + 1];
 	u32 len;
 
 	len = buf->len - BTRFS_CSUM_SIZE;
@@ -172,12 +192,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, csum_size, (char *)result, found);
+				btrfs_format_csum(csum_type, csum_size, buf->data, wanted);
+				printk("checksum verify failed on %llu wanted %s found %s\n",
 				       (unsigned long long)buf->start,
-				       result[0],
-				       buf->data[0]);
+				       wanted,
+				       found);
+			}
 			return 1;
 		}
 	} else {
-- 
2.30.1

Re: [PATCH v2] btrfs-progs: Fix checksum output for "checksum verify failed"

[David Sterba]

On Wed, Mar 03, 2021 at 09:18:44PM +0200, Dāvis Mosāns wrote:
> 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 | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
> index 6f584986..10b2421e 100644
> --- a/kernel-shared/disk-io.c
> +++ b/kernel-shared/disk-io.c
> @@ -160,10 +160,30 @@ int btrfs_csum_data(u16 csum_type, const u8 *data, u8 *out, size_t len)
>  	return -1;
>  }
>  
> +int btrfs_format_csum(u16 csum_type, u16 csum_size, const char *data, char *output)
> +{
> +	int i;
> +	int position = 0;
> +	int direction = 1;
> +	if (csum_type == BTRFS_CSUM_TYPE_CRC32 ||
> +		csum_type == BTRFS_CSUM_TYPE_XXHASH) {
> +		position = csum_size - 1;
> +		direction = -1;
> +	}

Per the discussion, I've dropped the direction variable and added the
"0x" prefix. The csum_size does not need to be passed, though it's
available in the caller's context. The type should be enough and then
the function can find the size independently.

Updated patch added to devel, thanks.