Re: Btrfs: add support for multiple csum algorithms

[Liu Bo <bo.li.liu@oracle.com>]

On Mon, Jan 25, 2016 at 01:31:34PM +0300, Dan Carpenter wrote:
> 
> Hello Josef Bacik,
> 
> The patch 607d432da054: "Btrfs: add support for multiple csum
> algorithms" from Dec 2, 2008, leads to the following static checker
> warning:
> 
> 	fs/btrfs/disk-io.c:320 csum_tree_block()
> 	error: __memcpy() '&found' too small (4 vs 9)
> 
> fs/btrfs/disk-io.c
>    278  static int csum_tree_block(struct btrfs_fs_info *fs_info,
>    279                             struct extent_buffer *buf,
>    280                             int verify)
>    281  {
>    282          u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> 
> The problem here is that we wrote some incomplete stub code 8 years ago
> an never used it.  Can we delete this stuff?
> 
> btrfs_super_csum_type() always returns 0 and thus
> btrfs_super_csum_size() always returns 4.  There is a
> btrfs_set_super_csum_type() function but it is never called otherwise
> we would hit several bugs.

Hmm..since we can only choose one type from btrfs_super_csum_type, this btrfs_set_super_csum_type() is called only when we do mkfs.btrfs.

Thanks,

-liubo
> 
>    283          char *result = NULL;
>    284          unsigned long len;
>    285          unsigned long cur_len;
>    286          unsigned long offset = BTRFS_CSUM_SIZE;
>    287          char *kaddr;
>    288          unsigned long map_start;
>    289          unsigned long map_len;
>    290          int err;
>    291          u32 crc = ~(u32)0;
>    292          unsigned long inline_result;
> 
> Let's assume this is 64 bit system and inline_result is 8 bytes.
> 
>    293  
>    294          len = buf->len - offset;
>    295          while (len > 0) {
>    296                  err = map_private_extent_buffer(buf, offset, 32,
>    297                                          &kaddr, &map_start, &map_len);
>    298                  if (err)
>    299                          return 1;
>    300                  cur_len = min(len, map_len - (offset - map_start));
>    301                  crc = btrfs_csum_data(kaddr + offset - map_start,
>    302                                        crc, cur_len);
>    303                  len -= cur_len;
>    304                  offset += cur_len;
>    305          }
>    306          if (csum_size > sizeof(inline_result)) {
>    307                  result = kzalloc(csum_size, GFP_NOFS);
> 
> In the future code we would allocate 9+ bytes.
> 
>    308                  if (!result)
>    309                          return 1;
>    310          } else {
>    311                  result = (char *)&inline_result;
>    312          }
>    313  
>    314          btrfs_csum_final(crc, result);
> 
> We only ever use the first 4 bytes.  We need to add a size option to
> this function to support future code.
> 
>    315  
>    316          if (verify) {
>    317                  if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
>    318                          u32 val;
>    319                          u32 found = 0;
>    320                          memcpy(&found, result, csum_size);
> 
> If csum_size were more than 4 this would corrupt memory.
> 
>    321  
>    322                          read_extent_buffer(buf, &val, 0, csum_size);
>    323                          btrfs_warn_rl(fs_info,
>    324                                  "%s checksum verify failed on %llu wanted %X found %X "
>    325                                  "level %d",
>    326                                  fs_info->sb->s_id, buf->start,
>    327                                  val, found, btrfs_header_level(buf));
>    328                          if (result != (char *)&inline_result)
>    329                                  kfree(result);
>    330                          return 1;
>    331                  }
>    332          } else {
>    333                  write_extent_buffer(buf, result, 0, csum_size);
>    334          }
>    335          if (result != (char *)&inline_result)
>    336                  kfree(result);
>    337          return 0;
> 
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html