[PATCH v2 0/2] btrfs-corrupt-block: btree data corruption

[PATCH v2 0/2] btrfs-corrupt-block: btree data corruption

[Boris Burkov]

Add some more generic corruption to btrfs-corrupt-block which allows
corrupting the data in metadata items.

Motivated by testing fsverity which requires rather specific corruption
of the metadata.

The first patch adds corrupting arbitrary regions of item data with -I.
The second patch adds corrupting holes and prealloc in extent data.

--
v2: minor cleanups from rebasing after a year

Boris Burkov (2):
  btrfs-progs: corrupt generic item data with btrfs-corrupt-block
  btrfs-progs: expand corrupt_file_extent in btrfs-corrupt-block

 btrfs-corrupt-block.c | 93 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 8 deletions(-)

-- 
2.37.1

[PATCH v2 1/2] btrfs-progs: corrupt generic item data with btrfs-corrupt-block

[Boris Burkov]

btrfs-corrupt-block already has a mix of generic and specific corruption
options, but currently lacks the capacity for totally arbitrary
corruption in item data.

There is already a flag for corruption size (bytes/-b), so add a flag
for an offset and a value to memset the item with. Exercise the new
flags with a new variant for -I (item) corruption. Look up the item as
before, but instead of corrupting a field in the item struct, corrupt an
offset/size in the item data.

The motivating example for this is that in testing fsverity with btrfs,
we need to corrupt the generated Merkle tree--metadata item data which
is an opaque blob to btrfs.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 btrfs-corrupt-block.c | 71 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index e961255d5..5c39459db 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -98,12 +98,14 @@ static void print_usage(int ret)
 	printf("\t-m   The metadata block to corrupt (must also specify -f for the field to corrupt)\n");
 	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field and optionally -r for the root)\n");
 	printf("\t-f   The field in the item to corrupt\n");
-	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
+	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field, or bytes, offset, and value to corrupt and root for the item)\n");
 	printf("\t-D <u64,u8,u64> Corrupt a dir item corresponding to the passed key triplet, must also specify a field\n");
 	printf("\t-d <u64,u8,u64> Delete item corresponding to passed key triplet\n");
 	printf("\t-r   Operate on this root\n");
 	printf("\t-C   Delete a csum for the specified bytenr.  When used with -b it'll delete that many bytes, otherwise it's just sectorsize\n");
 	printf("\t--block-group OFFSET  corrupt the given block group\n");
+	printf("\t-v   Value to use for corrupting item data\n");
+	printf("\t-o   Offset to use for corrupting item data\n");
 	exit(ret);
 }
 
@@ -974,6 +976,50 @@ out:
 	return ret;
 }
 
+static int corrupt_btrfs_item_data(struct btrfs_root *root,
+				   struct btrfs_key *key,
+				   u64 bogus_offset, u64 bogus_size,
+				   char bogus_value)
+{
+	struct btrfs_trans_handle *trans;
+	struct btrfs_path *path;
+	int ret;
+	void *data;
+	struct extent_buffer *leaf;
+	int slot;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		fprintf(stderr, "Couldn't start transaction %ld\n",
+			PTR_ERR(trans));
+		ret = PTR_ERR(trans);
+		goto free_path;
+	}
+
+	ret = btrfs_search_slot(trans, root, key, path, 0, 1);
+	if (ret != 0) {
+		fprintf(stderr, "Error searching to node %d\n", ret);
+		goto commit_txn;
+	}
+	leaf = path->nodes[0];
+	slot = path->slots[0];
+	data = btrfs_item_ptr(leaf, slot, void);
+	// TODO: check offset/size legitimacy
+	data += bogus_offset;
+	memset_extent_buffer(leaf, bogus_value, (unsigned long)data, bogus_size);
+	btrfs_mark_buffer_dirty(leaf);
+
+commit_txn:
+	btrfs_commit_transaction(trans, root);
+free_path:
+	btrfs_free_path(path);
+	return ret;
+}
+
 static int delete_item(struct btrfs_root *root, struct btrfs_key *key)
 {
 	struct btrfs_trans_handle *trans;
@@ -1230,6 +1276,8 @@ int main(int argc, char **argv)
 	u64 csum_bytenr = 0;
 	u64 block_group = 0;
 	char field[FIELD_BUF_LEN];
+	u64 bogus_value = (u64)-1;
+	u64 bogus_offset = (u64)-1;
 
 	field[0] = '\0';
 	memset(&key, 0, sizeof(key));
@@ -1258,11 +1306,13 @@ int main(int argc, char **argv)
 			{ "root", no_argument, NULL, 'r'},
 			{ "csum", required_argument, NULL, 'C'},
 			{ "block-group", required_argument, NULL, GETOPT_VAL_BLOCK_GROUP},
+			{ "value", required_argument, NULL, 'v'},
+			{ "offset", required_argument, NULL, 'o'},
 			{ "help", no_argument, NULL, GETOPT_VAL_HELP},
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:v:o:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1328,6 +1378,12 @@ int main(int argc, char **argv)
 			case GETOPT_VAL_BLOCK_GROUP:
 				block_group = arg_strtou64(optarg);
 				break;
+			case 'v':
+				bogus_value = arg_strtou64(optarg);
+				break;
+			case 'o':
+				bogus_offset = arg_strtou64(optarg);
+				break;
 			case GETOPT_VAL_HELP:
 			default:
 				print_usage(c != GETOPT_VAL_HELP);
@@ -1454,7 +1510,16 @@ int main(int argc, char **argv)
 		if (!root_objectid)
 			print_usage(1);
 
-		ret = corrupt_btrfs_item(target_root, &key, field);
+		if (*field != 0)
+			ret = corrupt_btrfs_item(target_root, &key, field);
+		else if (bogus_offset != (u64)-1 &&
+			 bytes != (u64)-1 &&
+			 bogus_value != (u64)-1)
+			ret = corrupt_btrfs_item_data(target_root, &key,
+						      bogus_offset, bytes,
+						      bogus_value);
+		else
+			print_usage(1);
 		goto out_close;
 	}
 	if (delete) {
-- 
2.37.1

Re: [PATCH v2 1/2] btrfs-progs: corrupt generic item data with btrfs-corrupt-block

[Sweet Tea Dorminy]

I thought I replied with this, but I can't find any evidence that the 
message actually sent, so: apologies if this is a duplication of a 
previous message...


>> -	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the
>> passed key triplet (must also specify the field to corrupt and root
>> for the item)\n");
>> +	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the
>> passed key triplet (must also specify the field, or bytes, offset, and
>> value to corrupt and root for the item)\n");
> 
> I'd find it a little more understandable, even though I know it's not
> intended to be read often, as:
> +	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the
> passed key triplet (must also specify the field, or (bytes, offset,
> value) tuple, to corrupt, and root for the item)\n");
> 
> 
> 
>> +	data = btrfs_item_ptr(leaf, slot, void);
>> +	// TODO: check offset/size legitimacy
> 
> Is it worth fixing this todo?
> 
> I'd prefer if there was a check that the existing data at the offset
> isn't the same as the new value, so as to ensure we notice if we're
> failing to corrupt.
> 
>> +			case 'v':
>> +				bogus_value = arg_strtou64(optarg);
>> +				break;
> 
> You're parsing, and storing here, a u64; meanwhile
> corrupt_btrfs_item_data() takes a char bogus_value. I think it
> probably makes sense to only parse and store a char, but
> 
>> +		else if (bogus_offset != (u64)-1 &&
>> +			 bytes != (u64)-1 &&
>> +			 bogus_value != (u64)-1)
> Might be worth calling out (u64)-1 as #define UNSET_VALUE for easier
> readability?
> 
>> +			ret = corrupt_btrfs_item_data(target_root, &key,
>> +						      bogus_offset, bytes,
>> +						      bogus_value);
>> +		else
>> +			print_usage(1);
> Maybe add an extra message here to say that either field or all of
> offset, bytes, and value have to be set?
> 
> Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Re: [PATCH v2 1/2] btrfs-progs: corrupt generic item data with btrfs-corrupt-block

[Nikolay Borisov]

On 16.07.22 г. 0:22 ч., Boris Burkov wrote:
> btrfs-corrupt-block already has a mix of generic and specific corruption
> options, but currently lacks the capacity for totally arbitrary
> corruption in item data.
> 
> There is already a flag for corruption size (bytes/-b), so add a flag
> for an offset and a value to memset the item with. Exercise the new
> flags with a new variant for -I (item) corruption. Look up the item as
> before, but instead of corrupting a field in the item struct, corrupt an
> offset/size in the item data.
> 
> The motivating example for this is that in testing fsverity with btrfs,
> we need to corrupt the generated Merkle tree--metadata item data which
> is an opaque blob to btrfs.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   btrfs-corrupt-block.c | 71 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
> index e961255d5..5c39459db 100644
> --- a/btrfs-corrupt-block.c
> +++ b/btrfs-corrupt-block.c
> @@ -98,12 +98,14 @@ static void print_usage(int ret)
>   	printf("\t-m   The metadata block to corrupt (must also specify -f for the field to corrupt)\n");
>   	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field and optionally -r for the root)\n");
>   	printf("\t-f   The field in the item to corrupt\n");
> -	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
> +	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field, or bytes, offset, and value to corrupt and root for the item)\n");
>   	printf("\t-D <u64,u8,u64> Corrupt a dir item corresponding to the passed key triplet, must also specify a field\n");
>   	printf("\t-d <u64,u8,u64> Delete item corresponding to passed key triplet\n");
>   	printf("\t-r   Operate on this root\n");
>   	printf("\t-C   Delete a csum for the specified bytenr.  When used with -b it'll delete that many bytes, otherwise it's just sectorsize\n");
>   	printf("\t--block-group OFFSET  corrupt the given block group\n");
> +	printf("\t-v   Value to use for corrupting item data\n");
> +	printf("\t-o   Offset to use for corrupting item data\n");
>   	exit(ret);
>   }
>   
> @@ -974,6 +976,50 @@ out:
>   	return ret;
>   }
>   
> +static int corrupt_btrfs_item_data(struct btrfs_root *root,
> +				   struct btrfs_key *key,
> +				   u64 bogus_offset, u64 bogus_size,
> +				   char bogus_value)
> +{
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_path *path;
> +	int ret;
> +	void *data;
> +	struct extent_buffer *leaf;
> +	int slot;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	trans = btrfs_start_transaction(root, 1);
> +	if (IS_ERR(trans)) {
> +		fprintf(stderr, "Couldn't start transaction %ld\n",
> +			PTR_ERR(trans));
> +		ret = PTR_ERR(trans);
> +		goto free_path;
> +	}
> +
> +	ret = btrfs_search_slot(trans, root, key, path, 0, 1);
> +	if (ret != 0) {
> +		fprintf(stderr, "Error searching to node %d\n", ret);
> +		goto commit_txn;
> +	}
> +	leaf = path->nodes[0];
> +	slot = path->slots[0];
> +	data = btrfs_item_ptr(leaf, slot, void);
> +	// TODO: check offset/size legitimacy
> +	data += bogus_offset;
> +	memset_extent_buffer(leaf, bogus_value, (unsigned long)data, bogus_size);
> +	btrfs_mark_buffer_dirty(leaf);
> +
> +commit_txn:
> +	btrfs_commit_transaction(trans, root);
> +free_path:
> +	btrfs_free_path(path);
> +	return ret;
> +}
> +
>   static int delete_item(struct btrfs_root *root, struct btrfs_key *key)
>   {
>   	struct btrfs_trans_handle *trans;
> @@ -1230,6 +1276,8 @@ int main(int argc, char **argv)
>   	u64 csum_bytenr = 0;
>   	u64 block_group = 0;
>   	char field[FIELD_BUF_LEN];
> +	u64 bogus_value = (u64)-1;
> +	u64 bogus_offset = (u64)-1;

nit: Why the casts, according to 
http://port70.net/~nsz/c/c99/n1256.html#6.3.1.3 :

2 Otherwise, if the new type is unsigned, the value is converted by 
repeatedly adding or subtracting one more than the maximum value that 
can be represented in the new type until the value is in the range of 
the new type.49)

So in this case the correct thing would happen without the casts.


<snip>

[PATCH v2 2/2] btrfs-progs: expand corrupt_file_extent in btrfs-corrupt-block

[Boris Burkov]

To corrupt holes/prealloc/inline extents, we need to mess with
extent data items. This patch makes it possible to modify
disk_bytenr with a specific value (useful for hole corruptions)
and to modify the type field (useful for prealloc corruptions)

Signed-off-by: Boris Burkov <boris@bur.io>
---
 btrfs-corrupt-block.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 5c39459db..27844b184 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -307,6 +307,7 @@ enum btrfs_inode_field {
 
 enum btrfs_file_extent_field {
 	BTRFS_FILE_EXTENT_DISK_BYTENR,
+	BTRFS_FILE_EXTENT_TYPE,
 	BTRFS_FILE_EXTENT_BAD,
 };
 
@@ -379,6 +380,8 @@ static enum btrfs_file_extent_field convert_file_extent_field(char *field)
 {
 	if (!strncmp(field, "disk_bytenr", FIELD_BUF_LEN))
 		return BTRFS_FILE_EXTENT_DISK_BYTENR;
+	if (!strncmp(field, "type", FIELD_BUF_LEN))
+		return BTRFS_FILE_EXTENT_TYPE;
 	return BTRFS_FILE_EXTENT_BAD;
 }
 
@@ -752,14 +755,14 @@ out:
 
 static int corrupt_file_extent(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root, u64 inode, u64 extent,
-			       char *field)
+			       char *field, u64 bogus)
 {
 	struct btrfs_file_extent_item *fi;
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	enum btrfs_file_extent_field corrupt_field;
-	u64 bogus;
 	u64 orig;
+	u8 bogus_type = bogus;
 	int ret = 0;
 
 	corrupt_field = convert_file_extent_field(field);
@@ -791,9 +794,18 @@ static int corrupt_file_extent(struct btrfs_trans_handle *trans,
 	switch (corrupt_field) {
 	case BTRFS_FILE_EXTENT_DISK_BYTENR:
 		orig = btrfs_file_extent_disk_bytenr(path->nodes[0], fi);
-		bogus = generate_u64(orig);
+		if (bogus == (u64)-1)
+			bogus = generate_u64(orig);
 		btrfs_set_file_extent_disk_bytenr(path->nodes[0], fi, bogus);
 		break;
+	case BTRFS_FILE_EXTENT_TYPE:
+		if (bogus == (u64)-1) {
+			fprintf(stderr, "Specify a new extent type value (-v)\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		btrfs_set_file_extent_type(path->nodes[0], fi, bogus_type);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1480,9 +1492,9 @@ int main(int argc, char **argv)
 			printf("corrupting inode\n");
 			ret = corrupt_inode(trans, root, inode, field);
 		} else {
-			printf("corrupting file extent\n");
 			ret = corrupt_file_extent(trans, root, inode,
-						  file_extent, field);
+						  file_extent, field,
+						  bogus_value);
 		}
 		btrfs_commit_transaction(trans, root);
 		goto out_close;
-- 
2.37.1

Re: [PATCH v2 2/2] btrfs-progs: expand corrupt_file_extent in btrfs-corrupt-block

[Sweet Tea Dorminy]

On 2022-07-15 17:22, Boris Burkov wrote:
> To corrupt holes/prealloc/inline extents, we need to mess with
> extent data items. This patch makes it possible to modify
> disk_bytenr with a specific value (useful for hole corruptions)
> and to modify the type field (useful for prealloc corruptions)
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  btrfs-corrupt-block.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
> index 5c39459db..27844b184 100644
> --- a/btrfs-corrupt-block.c
> +++ b/btrfs-corrupt-block.c
> @@ -307,6 +307,7 @@ enum btrfs_inode_field {
> 
>  enum btrfs_file_extent_field {
>  	BTRFS_FILE_EXTENT_DISK_BYTENR,
> +	BTRFS_FILE_EXTENT_TYPE,
>  	BTRFS_FILE_EXTENT_BAD,
>  };
> 
> @@ -379,6 +380,8 @@ static enum btrfs_file_extent_field
> convert_file_extent_field(char *field)
>  {
>  	if (!strncmp(field, "disk_bytenr", FIELD_BUF_LEN))
>  		return BTRFS_FILE_EXTENT_DISK_BYTENR;
> +	if (!strncmp(field, "type", FIELD_BUF_LEN))
> +		return BTRFS_FILE_EXTENT_TYPE;
>  	return BTRFS_FILE_EXTENT_BAD;
>  }
> 
> @@ -752,14 +755,14 @@ out:
> 
>  static int corrupt_file_extent(struct btrfs_trans_handle *trans,
>  			       struct btrfs_root *root, u64 inode, u64 extent,
> -			       char *field)
> +			       char *field, u64 bogus)
>  {
>  	struct btrfs_file_extent_item *fi;
>  	struct btrfs_path *path;
>  	struct btrfs_key key;
>  	enum btrfs_file_extent_field corrupt_field;
> -	u64 bogus;
>  	u64 orig;
> +	u8 bogus_type = bogus;
>  	int ret = 0;
> 
>  	corrupt_field = convert_file_extent_field(field);
> @@ -791,9 +794,18 @@ static int corrupt_file_extent(struct
> btrfs_trans_handle *trans,
>  	switch (corrupt_field) {
>  	case BTRFS_FILE_EXTENT_DISK_BYTENR:
>  		orig = btrfs_file_extent_disk_bytenr(path->nodes[0], fi);
> -		bogus = generate_u64(orig);
> +		if (bogus == (u64)-1)
> +			bogus = generate_u64(orig);
Personally I like ternaries a little more but whatever.
>  		btrfs_set_file_extent_disk_bytenr(path->nodes[0], fi, bogus);
>  		break;
> +	case BTRFS_FILE_EXTENT_TYPE:
> +		if (bogus == (u64)-1) {
> +			fprintf(stderr, "Specify a new extent type value (-v)\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
Again calling out (u64)-1 as a defined name would be nice.

Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Re: [PATCH v2 2/2] btrfs-progs: expand corrupt_file_extent in btrfs-corrupt-block

[Nikolay Borisov]

On 16.07.22 г. 0:22 ч., Boris Burkov wrote:
> To corrupt holes/prealloc/inline extents, we need to mess with
> extent data items. This patch makes it possible to modify
> disk_bytenr with a specific value (useful for hole corruptions)
> and to modify the type field (useful for prealloc corruptions)
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   btrfs-corrupt-block.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
> index 5c39459db..27844b184 100644
> --- a/btrfs-corrupt-block.c
> +++ b/btrfs-corrupt-block.c
> @@ -307,6 +307,7 @@ enum btrfs_inode_field {
>   
>   enum btrfs_file_extent_field {
>   	BTRFS_FILE_EXTENT_DISK_BYTENR,
> +	BTRFS_FILE_EXTENT_TYPE,
>   	BTRFS_FILE_EXTENT_BAD,
>   };
>   
> @@ -379,6 +380,8 @@ static enum btrfs_file_extent_field convert_file_extent_field(char *field)
>   {
>   	if (!strncmp(field, "disk_bytenr", FIELD_BUF_LEN))
>   		return BTRFS_FILE_EXTENT_DISK_BYTENR;
> +	if (!strncmp(field, "type", FIELD_BUF_LEN))
> +		return BTRFS_FILE_EXTENT_TYPE;
>   	return BTRFS_FILE_EXTENT_BAD;
>   }
>   
> @@ -752,14 +755,14 @@ out:
>   
>   static int corrupt_file_extent(struct btrfs_trans_handle *trans,
>   			       struct btrfs_root *root, u64 inode, u64 extent,
> -			       char *field)
> +			       char *field, u64 bogus)
>   {
>   	struct btrfs_file_extent_item *fi;
>   	struct btrfs_path *path;
>   	struct btrfs_key key;
>   	enum btrfs_file_extent_field corrupt_field;
> -	u64 bogus;
>   	u64 orig;
> +	u8 bogus_type = bogus;

nit: Why do the truncation here, when you can simply pass bogus to 
btrfs_set_file_extent_type and the truncation would be done when the 
value is passed? Or simply cast it when passing bogus to 
btrfs_set_file_extent_type, it makes the code somewhat simpler since we 
now don't have this bogus_type which is really field-specific.