• Nikos Tsironis's avatar
    dm clone: Fix handling of partial region discards · 4b514290
    Nikos Tsironis authored
    There is a bug in the way dm-clone handles discards, which can lead to
    discarding the wrong blocks or trying to discard blocks beyond the end
    of the device.
    
    This could lead to data corruption, if the destination device indeed
    discards the underlying blocks, i.e., if the discard operation results
    in the original contents of a block to be lost.
    
    The root of the problem is the code that calculates the range of regions
    covered by a discard request and decides which regions to discard.
    
    Since dm-clone handles the device in units of regions, we don't discard
    parts of a region, only whole regions.
    
    The range is calculated as:
    
        rs = dm_sector_div_up(bio->bi_iter.bi_sector, clone->region_size);
        re = bio_end_sector(bio) >> clone->region_shift;
    
    , where 'rs' is the first region to discard and (re - rs) is the number
    of regions to discard.
    
    The bug manifests when we try to discard part of a single region, i.e.,
    when we try to discard a block with size < region_size, and the discard
    request both starts at an offset with respect to the beginning of that
    region and ends before the end of the region.
    
    The root cause is the following comparison:
    
      if (rs == re)
        // skip discard and complete original bio immediately
    
    , which doesn't take into account that 'rs' might be greater than 're'.
    
    Thus, we then issue a discard request for the wrong blocks, instead of
    skipping the discard all together.
    
    Fix the check to also take into account the above case, so we don't end
    up discarding the wrong blocks.
    
    Also, add some range checks to dm_clone_set_region_hydrated() and
    dm_clone_cond_set_range(), which update dm-clone's region bitmap.
    
    Note that the aforementioned bug doesn't cause invalid memory accesses,
    because dm_clone_is_range_hydrated() returns True for this case, so the
    checks are just precautionary.
    
    Fixes: 7431b783 ("dm: add clone target")
    Cc: stable@vger.kernel.org # v5.4+
    Signed-off-by: default avatarNikos Tsironis <ntsironis@arrikto.com>
    Signed-off-by: default avatarMike Snitzer <snitzer@redhat.com>
    4b514290
dm-clone-target.c 55.2 KB