[PATCH] btrfs-progs: Output proper csum string if csum mismatch

[PATCH] btrfs-progs: Output proper csum string if csum mismatch

[Qu Wenruo]

Introduce a new helper, csum_to_string(), to convert binary csum to
string.

And use it in check_extent_csums(), so that
"btrfs check --check-data-csum" could report the following human
readable output:
  mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625

Other than the original octane one:
  mirror 1 bytenr 13631488 csum 19 expected csum 152

It also has the extra handling for 32 bytes csum (e.g. SHA256).
For such long csum, it needs 66 characters (+2 for "0x") to just output
one hash, so this function would truncate them into the following
format:
 0xaabb...ccdd
    |       \- The tailing 2 bytes
    \--------- The leading 2 bytes

Although this means it's possible to hit cases where both result and
expected csum look the same, but it should be rare since small change in
cryptographic should lead to completely different output.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/check/main.c b/check/main.c
index 74316eab85ec..7db65150048b 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5667,6 +5667,27 @@ static int check_space_cache(struct btrfs_root *root)
 	return error ? -EINVAL : 0;
 }
 
+/*
+ * The buffer size must be large enough to contain:
+ * - "0x": 2
+ * - Hex strings: 2 * 4 (CRC32 csum size)
+ * - "...": 3 (For longer csum size)
+ * - \0: 1
+ */
+#define CSUM_STRING_BUFSIZE (2 + 2 * 4 + 3 + 1)
+
+static void csum_to_string(u16 csum_size, u8 *csum, char *buf)
+{
+	if (csum_size <= btrfs_csum_type_size(BTRFS_CSUM_TYPE_CRC32)) {
+		ASSERT(csum_size == btrfs_csum_type_size(BTRFS_CSUM_TYPE_CRC32));
+		sprintf(buf, "0x%02x%02x%02x%02x", csum[0], csum[1],
+				csum[2], csum[3]);
+	} else {
+		sprintf(buf, "0x%02x%02x...%02x%02x", csum[0], csum[1],
+			csum[csum_size - 2], csum[csum_size - 1]);
+	}
+}
+
 /*
  * Check data checksum for [@bytenr, @bytenr + @num_bytes).
  *
@@ -5719,6 +5740,9 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 			data_checked = 0;
 			/* verify every 4k data's checksum */
 			while (data_checked < read_len) {
+				char result_buf[CSUM_STRING_BUFSIZE];
+				char expect_buf[CSUM_STRING_BUFSIZE];
+
 				tmp = offset + data_checked;
 
 				btrfs_csum_data(csum_type, data + tmp,
@@ -5730,11 +5754,14 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 						   csum_offset, csum_size);
 				if (memcmp(result, csum_expected, csum_size) != 0) {
 					csum_mismatch = true;
-					/* FIXME: format of the checksum value */
+					csum_to_string(csum_size, result,
+							result_buf);
+					csum_to_string(csum_size, csum_expected,
+							expect_buf);
 					fprintf(stderr,
-			"mirror %d bytenr %llu csum %u expected csum %u\n",
+			"mirror %d bytenr %llu csum %s expected csum %s\n",
 						mirror, bytenr + tmp,
-						result[0], csum_expected[0]);
+						result_buf, expect_buf);
 				}
 				data_checked += fs_info->sectorsize;
 			}
-- 
2.24.1

Re: [PATCH] btrfs-progs: Output proper csum string if csum mismatch

[David Sterba]

On Mon, Jan 20, 2020 at 05:32:05PM +0800, Qu Wenruo wrote:
> Introduce a new helper, csum_to_string(), to convert binary csum to
> string.
> 
> And use it in check_extent_csums(), so that
> "btrfs check --check-data-csum" could report the following human
> readable output:
>   mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625
> 
> Other than the original octane one:
>   mirror 1 bytenr 13631488 csum 19 expected csum 152
> 
> It also has the extra handling for 32 bytes csum (e.g. SHA256).
> For such long csum, it needs 66 characters (+2 for "0x") to just output
> one hash, so this function would truncate them into the following
> format:
>  0xaabb...ccdd
>     |       \- The tailing 2 bytes
>     \--------- The leading 2 bytes

Kernel prints the whole checksum and also 2 on one line,
btrfs_print_data_csum_error. The short version is ok for quick comparing
but for consistency do you think it's too bad to print the whole
checksum? Eg. when I see errors, copy&paste the checksum and can cross
check with the kernel messages/logs.

Re: [PATCH] btrfs-progs: Output proper csum string if csum mismatch

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1956 bytes --]



On 2020/3/4 下午10:28, David Sterba wrote:
> On Mon, Jan 20, 2020 at 05:32:05PM +0800, Qu Wenruo wrote:
>> Introduce a new helper, csum_to_string(), to convert binary csum to
>> string.
>>
>> And use it in check_extent_csums(), so that
>> "btrfs check --check-data-csum" could report the following human
>> readable output:
>>   mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625
>>
>> Other than the original octane one:
>>   mirror 1 bytenr 13631488 csum 19 expected csum 152
>>
>> It also has the extra handling for 32 bytes csum (e.g. SHA256).
>> For such long csum, it needs 66 characters (+2 for "0x") to just output
>> one hash, so this function would truncate them into the following
>> format:
>>  0xaabb...ccdd
>>     |       \- The tailing 2 bytes
>>     \--------- The leading 2 bytes
> 
> Kernel prints the whole checksum and also 2 on one line,
> btrfs_print_data_csum_error. The short version is ok for quick comparing
> but for consistency do you think it's too bad to print the whole
> checksum? Eg. when I see errors, copy&paste the checksum and can cross
> check with the kernel messages/logs.
> 

Personally speaking, for kernel I'm in the middle land, leaning a little
more towards shorter csum.
But I'm not confident enough for kernel, thus there is no such patch for
kernel submitted.

For longer csum, unless we have some intentional conflicting algo, it's
really hard to craft csum which makes the leading and tailing 8 bytes.

Furthermore, the only csum human can really use is the csum for all 0
page. Otherwise all csum makes not much sense to human.

But still, full csum may still make sense for certain corner case, and I
don't want even a small chance that user reports csum matches in dmesg
but still causing EIO.
So I hesitate for kernel patch.

As usual, you have the final say, and if kernel patch is needed I'm
pretty happy to submit.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]