Message ID | 1499781351-3156-1-git-send-email-nborisov@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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. > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> -- 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
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 ? > >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > -- 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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index c95f018d4a1e..6959177154b0 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -5180,12 +5180,13 @@ unsigned long btrfs_full_stripe_len(struct btrfs_fs_info *fs_info, unsigned long len = fs_info->sectorsize; em = get_chunk_map(fs_info, logical, len); - WARN_ON(IS_ERR(em)); - map = em->map_lookup; - if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) - len = map->stripe_len * nr_data_stripes(map); - free_extent_map(em); + if (!WARN_ON(IS_ERR(em))) { + map = em->map_lookup; + if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) + len = map->stripe_len * nr_data_stripes(map); + free_extent_map(em); + } return len; } @@ -5197,12 +5198,13 @@ int btrfs_is_parity_mirror(struct btrfs_fs_info *fs_info, int ret = 0; em = get_chunk_map(fs_info, logical, len); - WARN_ON(IS_ERR(em)); - map = em->map_lookup; - if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) - ret = 1; - free_extent_map(em); + if(!WARN_ON(IS_ERR(em))) { + map = em->map_lookup; + if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) + ret = 1; + free_extent_map(em); + } return ret; }
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. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/volumes.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)