btrfs-progs: receive: fix a silent data loss bug with encoded writes

From: Filipe Manana <[email protected]>

When using a v2 stream, with encoded writes, if the receiver fails to perform an encoded write, it fallbacks to decompressing the data and then write it using regular buffered IO. However that logic is currenty buggy, and it can result in writing less data than expected or no data at all. This results in a silent data loss.

Patch 3/3 fixes that bug and has all the details about how/why it happens, while previous patches just added debug messages to the callbacks for encoded writes and fallocate, which are currently missing and are very useful for debugging.

Filipe Manana (3):
btrfs-progs: receive: add debug messages when processing encoded writes
btrfs-progs: receive: add debug messages when processing fallocate
btrfs-progs: receive: fix silent data loss after fall back from encoded write

 cmds/receive.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

2.35.1

From: Filipe Manana <[email protected]>

Unlike for commands from the v1 stream, we have no debug messages logged when processing encoded write commands, which makes it harder to debug issues.

So add log messages, when the log verbosity level is >= 3, for encoded write commands, mentioning the value of all fields and also log when we fallback from an encoded write to the decompress and write approach.

The log messages look like this:

encoded_write f3 - offset=33792, len=4096, unencoded_offset=33792, unencoded_file_len=31744, unencoded_len=65536, compression=1, encryption=0 encoded_write f3 - falling back to decompress and write due to errno 22 ("Invalid argument")

---
 cmds/receive.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

index 6f475544..b75a5634 100644
@@ -1246,6 +1246,13 @@ static int process_encoded_write(const char *path, const void *data, u64 offset,
 		.encryption = encryption,
 	};
 
+	if (bconf.verbose >= 3)
+		fprintf(stderr,
+"encoded_write %s - offset=%llu, len=%llu, unencoded_offset=%llu, unencoded_file_len=%llu, unencoded_len=%llu, compression=%u, encryption=%u\n",
+			path, offset, len, unencoded_offset, unencoded_file_len,
+			unencoded_len, compression, encryption);
+
+
 	if (encryption) {
 		error("encoded_write: encryption not supported");
 		return -EOPNOTSUPP;

@@ -1271,6 +1278,10 @@ static int process_encoded_write(const char *path, const void *data, u64 offset,
 			error("encoded_write: writing to %s failed: %m", path);
 			return ret;
 		}
+		if (bconf.verbose >= 3)
+			fprintf(stderr,
+"encoded_write %s - falling back to decompress and write due to errno %d (\"%m\")\n",
+				path, errno);
 	}
 
 	return decompress_and_write(rctx, data, offset, len, unencoded_file_len,

2.35.1

From: Filipe Manana <[email protected]>

Unlike for commands from the v1 stream, we have no debug messages logged when processing fallocate commands, which makes it harder to debug issues.

So add log messages, when the log verbosity level is >= 3, for fallocate commands, mentioning the value of all fields.

---
 cmds/receive.c | 5 +++++
 1 file changed, 5 insertions(+)

index b75a5634..e6f1aeab 100644
@@ -1296,6 +1296,11 @@ static int process_fallocate(const char *path, int mode, u64 offset, u64 len,
 	struct btrfs_receive *rctx = user;
 	char full_path[PATH_MAX];
 
+	if (bconf.verbose >= 3)
+		fprintf(stderr,
+			"fallocate %s - offset=%llu, len=%llu, mode=%d\n",
+			path, offset, len, mode);
+
 	ret = path_cat_out(full_path, rctx->full_subvol_path, path);
 	if (ret < 0) {
 		error("fallocate: path invalid: %s", path);

2.35.1

From: Filipe Manana <[email protected]>

When attempting an encoded write, if it fails for some specific reason like -EINVAL (when an offset is not sector size aligned) or -ENOSPC, we then fallback into decompressing the data and writing it using regular buffered IO. This logic however is not correct, one of the reasons is that it assumes the encoded offset is smaller than the unencoded file length and that they can be compared, but one is an offset and the other is a length, not an end offset, so they can't be compared to get correct results. This bad logic will often result in not copying all data, or even no data at all, resulting in a silent data loss. This is easily seen in with the following reproducer:

   $ cat test.sh
   #!/bin/bash

DEV=/dev/sdj
MNT=/mnt/sdj

umount $DEV &> /dev/null
mkfs.btrfs -f $DEV > /dev/null
mount -o compress $DEV $MNT

# File foo has a size of 33K, not aligned to the sector size. xfs_io -f -c "pwrite -S 0xab 0 33K" $MNT/foo

xfs_io -f -c "pwrite -S 0xcd 0 64K" $MNT/bar

# Now clone the first 32K of file bar into foo at offset 0. xfs_io -c "reflink $MNT/bar 0 0 32K" $MNT/foo

# Snapshot the default subvolume and create a full send stream (v2). btrfs subvolume snapshot -r $MNT $MNT/snap

   btrfs send --compressed-data -f /tmp/test.send $MNT/snap

echo -e "\nFile bar in the original filesystem:" od -A d -t x1 $MNT/snap/bar

umount $MNT
mkfs.btrfs -f $DEV > /dev/null
mount $DEV $MNT

echo -e "\nReceiving stream in a new filesystem..." btrfs receive -f /tmp/test.send $MNT

echo -e "\nFile bar in the new filesystem:"
od -A d -t x1 $MNT/snap/bar

umount $MNT

Running the test without this patch:

   $ ./test.sh
   (...)
   File bar in the original filesystem:
   0000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd
   *
   0065536

Receiving stream in a new filesystem...
At subvol snap

File bar in the new filesystem:
0000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd *
0033792

We end up with file bar having less data, and a smaller size, than in the original filesystem.

This happens because when processing file bar, send issues the following commands:

clone bar - source=foo source offset=0 offset=0 length=32768
write bar - offset=32768 length=1024
encoded_write bar - offset=33792, len=4096, unencoded_offset=33792, unencoded_file_len=31744, unencoded_len=65536, compression=1, encryption=0

The first 32K are cloned from file foo, as that file ranged is shared between the files.

Then there's a regular write operation for the file range [32K, 33K[, since file foo has different data from bar for that file range.

Finally for the remainder of file bar, the send side issues an encoded write since the extent is compressed in the source filesystem, for the file offset 33792 (33K), remaining 31K of data. The receiver will try the encoded write, but that fails with -EINVAL since the offset 33K is not sector size aligned, so it will fallback to decompressing the data and writing it using regular buffered writes. However that results in doing no writes at decompress_and_write() because 'pos' is initialized to the value of 33K (unencoded_offset) and unencoded_file_len is 31K, so the while loop has no iterations.

Another case where we can fallback to decompression plus regular buffered writes is when the destination filesystem has a sector size larger then the sector size of the source filesystem (for example when the source filesystem is on x86_64 with a 4K sector size and the destination filesystem is PowerPC with a 64K sector size). In that scenario encoded write attempts wil fail with -EINVAL due to offsets not being aligned with the sector size of the destination filesystem, and the receive will attempt the fallback of decompressing the buffer and writing the decompressed using regular buffered IO.

Fix this by tracking the number of written bytes instead, and increment it, and the unencoded offset, after each write.

---
 cmds/receive.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

index e6f1aeab..0db66ee5 100644
@@ -1155,10 +1155,9 @@ static int decompress_and_write(struct btrfs_receive *rctx,
 				u32 compression)
 {
 	int ret = 0;
-	size_t pos;
-	ssize_t w;
 	char *unencoded_data;
 	int sector_shift = 0;
+	u64 written = 0;
 
 	unencoded_data = calloc(unencoded_len, 1);
 	if (!unencoded_data) {

@@ -1209,17 +1208,19 @@ static int decompress_and_write(struct btrfs_receive *rctx,
 		goto out;
 	}
 
-	pos = unencoded_offset;
-	while (pos < unencoded_file_len) {
-		w = pwrite(rctx->write_fd, unencoded_data + pos,
-			   unencoded_file_len - pos, offset);
+	while (written < unencoded_file_len) {
+		ssize_t w;
+
+		w = pwrite(rctx->write_fd, unencoded_data + unencoded_offset,
+			   unencoded_file_len - written, offset);
 		if (w < 0) {
 			ret = -errno;
 			error("writing unencoded data failed: %m");
 			goto out;
 		}
-		pos += w;
+		written += w;
 		offset += w;
+		unencoded_offset += w;
 	}
 out:
 	free(unencoded_data);

2.35.1

Thank you for catching and fixing this.

Hey.

I recently sent | received a lot of previous data, thus asking:

What exactly does encoded write mean? Is one *only* affected when ones used compression - respectively if one DID NOT do any filesystem compression (i.e. compress mount option)... can one be sure to be safe?

Thanks,
Chris.

[0] https://lore.kernel.org/linux-btrfs/[email protected]/

Right now, it means a write operation where the data is compressed. I.e. there was no decompression at the sender side.

If you haven't used 'btrfs send' with the --compressed-data option or you are sure you don't have any compressed files, then you're fine. Otherwise check your files by comparing them between the sending side and the receiving side.

Thanks a lot... so all good for me. :-)

But still, as I wrote in the other mail,... other people might be affected... and it would be reeeeally nice if there was some good way for them to get alerted about such cases.

Thanks,
Chris.

There will always be users who'll miss such alerts, many don't read all the emails in this list, many are not even subscribed to the mailing list, etc.
Do you have examples of other projects that have an effective alert system?

Sure... and it will never be possible to have a system which really reaches 100% of the desired users.

But this isn't only the case for a something that notices about data corruption issues, it's also the case for any other warning system:
- civil defense via sirens (people may life to far away, wear headphones with loud music, be deaf, sleep with earmuffs) via apps (they often simply fail or are overloaded, people may not have a smartphone at all, it maybe powered off)
- software security advisories (again, people may not be subscribed to such mailing lists, may not run upgrades regularly respectively check for security updates in some automated fashion, or attackers may try to specifically block such information from certain people)
- etc. pp.
- and even *if* the information reaches someone, it's still not guaranteed that he cares about or understands it well enough

I don't think the goal is ever about reaching everyone - the goal is having a simple way of reaching those who care.

Because whether it's storage farm admins who keep important scientific data on btrfs or some end user who values his precious collection of pictures, etc. ... it would be quite good for them to be able to react in cases of (especially silent) data corruption.

And I should emphasis, that this is in no way targeted for lazy people who don't make backups.
I for example do have some rather sophisticated backup strategy, but if there's silent data corruption it's often quite hard to tell whether the data is still valid, even if one does things like storing hash sums of them (and verifying these of some 10TB of data already takes several days).

Well, as I've said, not at the filesystem level. But it's e.g. common practise for security incidents.

Upstream developers put quite some effort into finding and fixing such bugs, which is appreciated, but IMO that is a bit ridiculed by the fact that people then may still loose their data, just because they never take notice about such issues, when they'd still have valid backups.

Cheers,
Chris.

Hey again.

Something more general:

Just as with the most recent (at least as far as I've noticed) possible silent data corruption[0], it's IMO really most unfortunate and a structural problem, that there is no useful way for (interested) end users to take notice them.

If I weren't following the list a bit - and even there only by chance and depending on an alerting commit summary - I'd probably never ever hear about it... yet I might still suffer from it without realising immediately.

Right now people may still have backups respectively the sources they btrfs-sent from... but perhaps not so in a year when they possibly notice the corruption (if at all).

This is really no criticism that such bugs do happen - btrfs is quite actively developed, so I fully understand that such bugs show up.

But for end users its really awful, if especially for silent corruption issues there is no alerting mechanism.

And yes I know, other filesystems don't have one either - doesn't make it better though.

I'd say a simple corruptions announcement mailing list would do:

No other announcements like "new btrfs progs version" (unless that would fix a data corruption issue). Also usually no announcement for issues that were 100% user-visible (like general crash when btrfs tries to mount a fs) OR which have no impact on data consistency (e.g. if a bug would prevent the "compress" mount option to be considered at all).

Only really serious things that could cause data loss (perhaps including confidentiality issues, like with fscrypt).

Per issue, one announcement mail with description on what it is, how likely it happened, if/how one can find out whether one was affected, how one can fix (or if checks against / recovery from backups are needed).

Perhaps other filesystems would even want to join in, then mails should be prefixed with "btrfs: ", "xfs:", "ext4", etc.:

Regards,
Chris.

Added to devel, thanks.