On Tue, Jul 11, 2017 at 10:29:49PM +0300, Nikolay Borisov wrote: > > > On 11.07.2017 20:24, David Sterba wrote: > > On Tue, Jul 11, 2017 at 04:55:51PM +0300, Nikolay Borisov wrote: > >> In btrfs_full_stripe_len/btrfs_is_parity_mirror we have similar code which > >> gets the chunk map for a particular range via get_chunk_map. However, > >> get_chunk_map can return an ERR_PTR value and while the 2 callers do catch > >> this with a WARN_ON they then proceed to indiscriminately dereference the > >> extent map. This of course leads to a crash. Fix the offenders by making the > >> dereference conditional on IS_ERR. > > > > While the code makes it better, the whole callchain should be fixed. The > > WARN_ON used to be a BUG_ON and the error handling was absent, and still > > is. Although it's unlikely to see the warnings from that, I'd rather see > > it fixed properly. The direct caller of btrfs_full_stripe_len will be > > able to handle it. > > What should be returned in case we can't find the chunk_map -EINVAL ? Returning what get_chunk_map seems ok in btrfs_full_stripe_len (compared to other callers of get_chunk_map that may interpret a failure in a different way). But EINVAL is IMO wrong as it's more like the ENOENT when the mapping is missing or EUCLEAN when the mapping looks incorrect. The failure of btrfs_full_stripe_len in btrfs_create_block_group_cache could be best handled if we return ERR_PTR instead of assuming ENOMEM in all of its callers (btrfs_read_block_groups and btrfs_make_block_group). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
