Hi Qu,

On 04/09/2018 11:19 AM, Qu Wenruo wrote:
> When manually patching super blocks, current validation check is pretty
> weak (limited to magic number and csum) and doesn't provide extra check
> for some obvious corruption (like invalid sectorsize).
[...]
> 
> Signed-off-by: Qu Wenruo <w...@suse.com>
> ---
> changelog:
> v2:
>   Generate tab based indent string in helper macro instead of passing spacer
>   string from outside, suggested by Nikolay.
>   (In fact, if using %*s it would be much easier, however it needs extra
>    rework for existing code as they still use tab as indent)
> ---
>  cmds-inspect-dump-super.c | 206 +++++++++++++++++++++++++-------------
>  1 file changed, 137 insertions(+), 69 deletions(-)
> 
> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
> index 24588c87cce6..68d6f59ad727 100644
> --- a/cmds-inspect-dump-super.c
> +++ b/cmds-inspect-dump-super.c
> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag)
>                                    super_flags_num, BTRFS_SUPER_FLAG_SUPP);
>  }
>  
> +#define INDENT_BUFFER_LEN    80
> +#define TAB_LEN                      8
> +#define SUPER_INDENT         24
> +
> +/* helper to generate tab based indent string */
> +static void generate_tab_indent(char *buf, unsigned int indent)
> +{
> +     buf[0] = '\0';
> +     for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN)
> +             strncat(buf, "\t", INDENT_BUFFER_LEN);
> +}
> +
> +/* Helper to print member in %llu */
> +#define print_super(sb, member)                                              
> \
> +({                                                                   \
> +     int indent = SUPER_INDENT - strlen(#member);                    \
> +     char indent_str[INDENT_BUFFER_LEN];                             \
> +                                                                     \
> +     generate_tab_indent(indent_str, indent);                        \
> +     printf("%s%s%llu\n", #member, indent_str,                       \
> +            (u64) btrfs_super_##member(sb));                         \

Why not something like:

            static const char tabs[] = "\t\t\t\t";        \
            int i = (sizeof(#member)+6)/8;                \
            if (i > sizeof(tabs)-1)                       \
                i = sizeof(tabs-1);                       \
            u64 value = (u64)btrfs_super_##member(sb);    \
            printf("%s%s" format,                         \
                #member, tabs+i, value);  


so to get rid  of generate_tab_indent and indent_str

> +})
> +
> +/* Helper to print member in %llx */
> +#define print_super_hex(sb, member)                                  \
> +({                                                                   \
> +     int indent = SUPER_INDENT - strlen(#member);                    \
> +     char indent_str[INDENT_BUFFER_LEN];                             \
> +                                                                     \
> +     generate_tab_indent(indent_str, indent);                        \
> +     printf("%s%s0x%llx\n", #member, indent_str,                     \
> +            (u64) btrfs_super_##member(sb));                         \
> +})
> +
> +/*
> + * Helper macro to wrap member checker
> + *
> + * Only support %llu output for member.
> + */
> +#define print_check_super(sb, member, bad_condition)                 \
> +({                                                                   \
> +     int indent = SUPER_INDENT - strlen(#member);                    \
> +     char indent_str[INDENT_BUFFER_LEN];                             \
> +     u64 value;                                                      \
> +                                                                     \
> +     generate_tab_indent(indent_str, indent);                        \
> +     value = btrfs_super_##member(sb);                               \
> +     printf("%s%s%llu", #member, indent_str, (u64) value);           \
> +     if (bad_condition)                                              \
> +             printf(" [INVALID]");                                   \

What about printing also the condition: something like

+       if (bad_condition)                                              \
+               printf(" [INVALID '%s']", #bad_condition);              \

it would be even better a "good_condition":

so instead of:
+       print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
do
+       print_check_super(sb, bytenr, (IS_ALIGNED(value, SZ_4K)));

and

+       if (!good_condition)                                            \
+               printf(" [ERROR: required '%s']", #good_condition);     \


All above functions could be written as:

  #define __print_and_check(sb, member, format, expected)   \
        do{                                               \
            static const char tabs[] = "\t\t\t\t";        \
            int i = (sizeof(#member)+6)/8;                \
            if (i > sizeof(tabs)-1)                       \
                i = sizeof(tabs-1);                       \
            u64 value = (u64)btrfs_super_##member(sb);    \
            printf("%s%s" format,                         \
                #member, tabs+i, value);                  \
            if (!(expected))                              \
                printf(" [ERROR: expected '%s']", #expected);    \
            printf("\n");                           \
         } while(0)
         
  #define print_super(sb, member) \
    __print_and_check(sb, member, "%llu", 1);

  #define print_super_hex(sb, member) \
    __print_and_check(sb, member, "0x%llx", 1);

  #define print_check_super(sb, member, condition) \
    __print_and_check(sb, member, "0x%llx", condition);


And the value should be printed as (I removed the !):

  print_check_super(sb, root, (IS_ALIGNED(value, SZ_4K)));

In case of error:

test                        12 [ERROR: expected 'IS_ALIGNED(value, SZ_4K)']




> +     printf("\n");                                                   \
> +})
> +
> +#define DEV_INDENT   (SUPER_INDENT - strlen("dev_item."))
> +BUILD_ASSERT(DEV_INDENT > 0);
> +
> +/* Helper to print sb->dev_item members */
> +#define print_super_dev(sb, member)                                  \
> +({                                                                   \
> +     int indent = DEV_INDENT - strlen(#member);                      \
> +     char indent_str[INDENT_BUFFER_LEN];                             \
> +                                                                     \
> +     generate_tab_indent(indent_str, indent);                        \
> +     printf("dev_item.%s%s%llu\n", #member, indent_str,              \
> +            (u64) btrfs_stack_device_##member(&sb->dev_item));       \
> +})
> +
>  static void dump_superblock(struct btrfs_super_block *sb, int full)
>  {
>       int i;
>       char *s, buf[BTRFS_UUID_UNPARSED_SIZE];
> +     int max_level = BTRFS_MAX_LEVEL; /* to save several chars */
>       u8 *p;
> +     u64 super_gen;
>       u32 csum_size;
>       u16 csum_type;
>  
> @@ -348,10 +417,16 @@ static void dump_superblock(struct btrfs_super_block 
> *sb, int full)
>               printf(" [DON'T MATCH]");
>       putchar('\n');
>  
> -     printf("bytenr\t\t\t%llu\n",
> -             (unsigned long long)btrfs_super_bytenr(sb));
> -     printf("flags\t\t\t0x%llx\n",
> -             (unsigned long long)btrfs_super_flags(sb));
> +     /*
> +      * Use btrfs minimal sector size as basic check for bytenr, since we
> +      * can't trust sector size from super block.
> +      * This 4K check should works fine for most architecture, and will be
> +      * just a little loose for 64K page system.
> +      *
> +      * And such 4K check will be used for other members too.
> +      */
> +     print_check_super(sb, bytenr, (!IS_ALIGNED(value, SZ_4K)));
> +     print_super_hex(sb, flags);
>       print_readable_super_flag(btrfs_super_flags(sb));
>  
>       printf("magic\t\t\t");
> @@ -372,53 +447,56 @@ static void dump_superblock(struct btrfs_super_block 
> *sb, int full)
>               putchar(isprint(s[i]) ? s[i] : '.');
>       putchar('\n');
>  
> -     printf("generation\t\t%llu\n",
> -            (unsigned long long)btrfs_super_generation(sb));
> -     printf("root\t\t\t%llu\n", (unsigned long long)btrfs_super_root(sb));
> -     printf("sys_array_size\t\t%llu\n",
> -            (unsigned long long)btrfs_super_sys_array_size(sb));
> -     printf("chunk_root_generation\t%llu\n",
> -            (unsigned long long)btrfs_super_chunk_root_generation(sb));
> -     printf("root_level\t\t%llu\n",
> -            (unsigned long long)btrfs_super_root_level(sb));
> -     printf("chunk_root\t\t%llu\n",
> -            (unsigned long long)btrfs_super_chunk_root(sb));
> -     printf("chunk_root_level\t%llu\n",
> -            (unsigned long long)btrfs_super_chunk_root_level(sb));
> -     printf("log_root\t\t%llu\n",
> -            (unsigned long long)btrfs_super_log_root(sb));
> -     printf("log_root_transid\t%llu\n",
> -            (unsigned long long)btrfs_super_log_root_transid(sb));
> -     printf("log_root_level\t\t%llu\n",
> -            (unsigned long long)btrfs_super_log_root_level(sb));
> -     printf("total_bytes\t\t%llu\n",
> -            (unsigned long long)btrfs_super_total_bytes(sb));
> -     printf("bytes_used\t\t%llu\n",
> -            (unsigned long long)btrfs_super_bytes_used(sb));
> -     printf("sectorsize\t\t%llu\n",
> -            (unsigned long long)btrfs_super_sectorsize(sb));
> -     printf("nodesize\t\t%llu\n",
> -            (unsigned long long)btrfs_super_nodesize(sb));
> +     print_check_super(sb, sys_array_size,
> +                       (value > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE));
> +
> +     super_gen = btrfs_super_generation(sb);
> +     print_check_super(sb, root, (!IS_ALIGNED(value, SZ_4K)));
> +     print_check_super(sb, root_level, (value >= max_level));
> +     print_super(sb, generation);
> +
> +     print_check_super(sb, chunk_root, (!IS_ALIGNED(value, SZ_4K)));
> +     print_check_super(sb, chunk_root_level, (value >= max_level));
> +     /*
> +      * Here we trust super generation, and use it as checker for other
> +      * tree roots. Applies to all other trees.
> +      */
> +     print_check_super(sb, chunk_root_generation, (value > super_gen));
> +
> +     print_check_super(sb, log_root, (!IS_ALIGNED(value, SZ_4K)));
> +     print_check_super(sb, log_root_level, (value >= max_level));
> +     print_check_super(sb, log_root_transid, (value > super_gen + 1));
> +
> +     /*
> +      * For total bytes, it's possible that old kernel is using unaligned
> +      * size, not critical so don't do 4K check here.
> +      */
> +     print_super(sb, total_bytes);
> +
> +     /* Used bytes must be aligned to 4K */
> +     print_check_super(sb, bytes_used, (!IS_ALIGNED(value, SZ_4K)));
> +     print_check_super(sb, sectorsize,
> +                       (!(is_power_of_2(value) && value >= SZ_4K &&
> +                          value <= SZ_64K)));
> +     print_check_super(sb, nodesize,
> +                       (!(is_power_of_2(value) && value >= SZ_4K &&
> +                          value <= SZ_64K &&
> +                          value > btrfs_super_sectorsize(sb))));
>       printf("leafsize (deprecated)\t%u\n",
>              le32_to_cpu(sb->__unused_leafsize));
> -     printf("stripesize\t\t%llu\n",
> -            (unsigned long long)btrfs_super_stripesize(sb));
> -     printf("root_dir\t\t%llu\n",
> -            (unsigned long long)btrfs_super_root_dir(sb));
> -     printf("num_devices\t\t%llu\n",
> -            (unsigned long long)btrfs_super_num_devices(sb));
> -     printf("compat_flags\t\t0x%llx\n",
> -            (unsigned long long)btrfs_super_compat_flags(sb));
> -     printf("compat_ro_flags\t\t0x%llx\n",
> -            (unsigned long long)btrfs_super_compat_ro_flags(sb));
> +
> +     /* Not really used, just check it the same way as kernel */
> +     print_check_super(sb, stripesize, (!is_power_of_2(value)));
> +     print_super(sb, root_dir);
> +     print_super(sb, num_devices);
> +     print_super_hex(sb, compat_flags);
> +     print_super_hex(sb, compat_ro_flags);
>       print_readable_compat_ro_flag(btrfs_super_compat_ro_flags(sb));
> -     printf("incompat_flags\t\t0x%llx\n",
> -            (unsigned long long)btrfs_super_incompat_flags(sb));
> +     print_super_hex(sb, incompat_flags);
>       print_readable_incompat_flag(btrfs_super_incompat_flags(sb));
> -     printf("cache_generation\t%llu\n",
> -            (unsigned long long)btrfs_super_cache_generation(sb));
> -     printf("uuid_tree_generation\t%llu\n",
> -            (unsigned long long)btrfs_super_uuid_tree_generation(sb));
> +     print_check_super(sb, cache_generation,
> +                       (value > super_gen && value != (u64)-1));
> +     print_check_super(sb, uuid_tree_generation, (value > super_gen));
>  
>       uuid_unparse(sb->dev_item.uuid, buf);
>       printf("dev_item.uuid\t\t%s\n", buf);
> @@ -428,28 +506,18 @@ static void dump_superblock(struct btrfs_super_block 
> *sb, int full)
>               !memcmp(sb->dev_item.fsid, sb->fsid, BTRFS_FSID_SIZE) ?
>                       "[match]" : "[DON'T MATCH]");
>  
> -     printf("dev_item.type\t\t%llu\n", (unsigned long long)
> -            btrfs_stack_device_type(&sb->dev_item));
> -     printf("dev_item.total_bytes\t%llu\n", (unsigned long long)
> -            btrfs_stack_device_total_bytes(&sb->dev_item));
> -     printf("dev_item.bytes_used\t%llu\n", (unsigned long long)
> -            btrfs_stack_device_bytes_used(&sb->dev_item));
> -     printf("dev_item.io_align\t%u\n", (unsigned int)
> -            btrfs_stack_device_io_align(&sb->dev_item));
> -     printf("dev_item.io_width\t%u\n", (unsigned int)
> -            btrfs_stack_device_io_width(&sb->dev_item));
> -     printf("dev_item.sector_size\t%u\n", (unsigned int)
> -            btrfs_stack_device_sector_size(&sb->dev_item));
> -     printf("dev_item.devid\t\t%llu\n",
> -            btrfs_stack_device_id(&sb->dev_item));
> -     printf("dev_item.dev_group\t%u\n", (unsigned int)
> -            btrfs_stack_device_group(&sb->dev_item));
> -     printf("dev_item.seek_speed\t%u\n", (unsigned int)
> -            btrfs_stack_device_seek_speed(&sb->dev_item));
> -     printf("dev_item.bandwidth\t%u\n", (unsigned int)
> -            btrfs_stack_device_bandwidth(&sb->dev_item));
> -     printf("dev_item.generation\t%llu\n", (unsigned long long)
> -            btrfs_stack_device_generation(&sb->dev_item));
> +     /* For embedded device item, don't do extra check, just like kernel */
> +     print_super_dev(sb, type);
> +     print_super_dev(sb, total_bytes);
> +     print_super_dev(sb, bytes_used);
> +     print_super_dev(sb, io_align);
> +     print_super_dev(sb, io_width);
> +     print_super_dev(sb, sector_size);
> +     print_super_dev(sb, id);
> +     print_super_dev(sb, group);
> +     print_super_dev(sb, seek_speed);
> +     print_super_dev(sb, bandwidth);
> +     print_super_dev(sb, generation);
>       if (full) {
>               printf("sys_chunk_array[%d]:\n", BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
>               print_sys_chunk_array(sb);
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to