[dm-devel] [PATCH] DM RAID: Fix memory corruption caused by repeated calls to bitmap_load

[NeilBrown]

On Fri, 27 Jan 2012 08:53:53 -0600 Jonathan Brassow <jbrassow at redhat.com>
wrote:

> Neil,
> 
> I came across this bug when testing snapshots of RAID volumes.  The
> snapshot code performs multiple suspend/resume cycles on the underlying
> LV, which is what caused me to notice that 'bitmap_load' was being
> called multiple times and causing memory corruption.  As I explain in
> the patch header, I'm pretty sure that avoiding the repeat call to
> 'bitmap_load' and kicking the MD thread instead is all that is needed.
> (It certainly works in all my tests.)  However, please let me know if
> there is anything I'm not aware of.
> 
> Thanks,
>  brassow

Thanks.  I've applied it and pushed it to my for-next branch, with
a small change to the patch description:

> 
> Prevent DM RAID from loading bitmap twice.
> 
> The life cycle of a device-mapper target is:
> 1) create
> 2) resume
> 3) suspend
> *) possibly repeat from 2
> 4) destroy
> 
> The dm-raid target is unconditionally calling MD's bitmap_load function upon
> every resume.  If steps 2 & 3 above are repeated, bitmap_load is called
> multiple times.  It is only written to be called once; otherwise, it allocates
> new memory for the bitmap (without freeing the old) and incrementing the number
> of pages it thinks it has without zeroing first.  This ultimately leads to
> access beyond allocated memory and lost memory.
> 
> Simply avoiding the bitmap_load call upon resume is not sufficient.  If the
> target was suspended while the initial recovery was only partially complete,
> it needs to be restarted when the target is resumed.  This is why
> 'md_wakeup_thread' is called on the before issuing the 'mddev_resume'.
                               ^^^^^^

I removed "on the "

Thanks,
NeilBrown



> 
> Signed-off-by: Jonathan Brassow <jbrassow at redhat.com>
> 
> Index: linux-upstream/drivers/md/dm-raid.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/dm-raid.c
> +++ linux-upstream/drivers/md/dm-raid.c
> @@ -56,7 +56,8 @@ struct raid_dev {
>  struct raid_set {
>  	struct dm_target *ti;
>  
> -	uint64_t print_flags;
> +	uint32_t bitmap_loaded;
> +	uint32_t print_flags;
>  
>  	struct mddev md;
>  	struct raid_type *raid_type;
> @@ -1135,7 +1136,7 @@ static int raid_status(struct dm_target 
>  				raid_param_cnt += 2;
>  		}
>  
> -		raid_param_cnt += (hweight64(rs->print_flags & ~DMPF_REBUILD) * 2);
> +		raid_param_cnt += (hweight32(rs->print_flags & ~DMPF_REBUILD) * 2);
>  		if (rs->print_flags & (DMPF_SYNC | DMPF_NOSYNC))
>  			raid_param_cnt--;
>  
> @@ -1247,7 +1248,12 @@ static void raid_resume(struct dm_target
>  {
>  	struct raid_set *rs = ti->private;
>  
> -	bitmap_load(&rs->md);
> +	if (!rs->bitmap_loaded) {
> +		bitmap_load(&rs->md);
> +		rs->bitmap_loaded = 1;
> +	} else
> +		md_wakeup_thread(rs->md.thread);
> +
>  	mddev_resume(&rs->md);
>  }
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20120131/e6c92da2/attachment.sig>