[PATCH 0/2] md: memleak fixes

[PATCH 0/2] md: memleak fixes

[Zdenek Kabelac]

Patchset tries to address several kmeleaks visible when lvm2 is using
raid target based on md-core.

1st. patch as quite easy to validate
2nd. please do careful review as bitmap.c is hard to follow

Zdenek Kabelac (2):
  md: release allocated bitset sync_set
  md: free unused memory after bitmap resize

 drivers/md/bitmap.c | 9 +++++++++
 drivers/md/md.c     | 8 +++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.15.0

[PATCH 1/2] md: release allocated bitset sync_set

[Zdenek Kabelac]

Patch fixes kmemleak on md_stop() path used likely only by dm-raid
wrapper. Code of md is using  mddev_put() where both  bitsets
are released however this bitmap freeing is not shared.

Also set NULL to bio_set and sync_set pointers just like mddev_put is
doing.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 drivers/md/md.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0ff1bbf6c90e..635ad7c38f67 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev)
 	 * This is called from dm-raid
 	 */
 	__md_stop(mddev);
-	if (mddev->bio_set)
+	if (mddev->bio_set) {
 		bioset_free(mddev->bio_set);
+		mddev->bio_set = NULL;
+	}
+	if (mddev->sync_set) {
+		bioset_free(mddev->sync_set);
+		mddev->sync_set = NULL;
+	}
 }
 
 EXPORT_SYMBOL_GPL(md_stop);
-- 
2.15.0

[PATCH 2/2] md: free unused memory after bitmap resize

[Zdenek Kabelac]

When bitmap is resized, the old kalloced chunks just are not released
once the resized bitmap starts to use new space.

This fixes in particular kmemleak reports like this one:

unreferenced object 0xffff8f4311e9c000 (size 4096):
  comm "lvm", pid 19333, jiffies 4295263268 (age 528.265s)
  hex dump (first 32 bytes):
    02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80  ................
    02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80  ................
  backtrace:
    [<ffffffffa69471ca>] kmemleak_alloc+0x4a/0xa0
    [<ffffffffa628c10e>] kmem_cache_alloc_trace+0x14e/0x2e0
    [<ffffffffa676cfec>] bitmap_checkpage+0x7c/0x110
    [<ffffffffa676d0c5>] bitmap_get_counter+0x45/0xd0
    [<ffffffffa676d6b3>] bitmap_set_memory_bits+0x43/0xe0
    [<ffffffffa676e41c>] bitmap_init_from_disk+0x23c/0x530
    [<ffffffffa676f1ae>] bitmap_load+0xbe/0x160
    [<ffffffffc04c47d3>] raid_preresume+0x203/0x2f0 [dm_raid]
    [<ffffffffa677762f>] dm_table_resume_targets+0x4f/0xe0
    [<ffffffffa6774b52>] dm_resume+0x122/0x140
    [<ffffffffa6779b9f>] dev_suspend+0x18f/0x290
    [<ffffffffa677a3a7>] ctl_ioctl+0x287/0x560
    [<ffffffffa677a693>] dm_ctl_ioctl+0x13/0x20
    [<ffffffffa62d6b46>] do_vfs_ioctl+0xa6/0x750
    [<ffffffffa62d7269>] SyS_ioctl+0x79/0x90
    [<ffffffffa6956d41>] entry_SYSCALL_64_fastpath+0x1f/0xc2

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 drivers/md/bitmap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index d2121637b4ab..58ee21027709 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -2152,6 +2152,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
 				for (k = 0; k < page; k++) {
 					kfree(new_bp[k].map);
 				}
+				kfree(new_bp);
 
 				/* restore some fields from old_counts */
 				bitmap->counts.bp = old_counts.bp;
@@ -2202,6 +2203,14 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
 		block += old_blocks;
 	}
 
+	if (bitmap->counts.bp != old_counts.bp) {
+		unsigned long k;
+		for (k = 0; k < old_counts.pages; k++)
+			if (!old_counts.bp[k].hijacked)
+				kfree(old_counts.bp[k].map);
+		kfree(old_counts.bp);
+	}
+
 	if (!init) {
 		int i;
 		while (block < (chunks << chunkshift)) {
-- 
2.15.0

Re: [PATCH 2/2] md: free unused memory after bitmap resize

[Guoqing Jiang]

On 11/08/2017 08:44 PM, Zdenek Kabelac wrote:
> When bitmap is resized, the old kalloced chunks just are not released
> once the resized bitmap starts to use new space.
>
> This fixes in particular kmemleak reports like this one:
>
> unreferenced object 0xffff8f4311e9c000 (size 4096):
>    comm "lvm", pid 19333, jiffies 4295263268 (age 528.265s)
>    hex dump (first 32 bytes):
>      02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80  ................
>      02 80 02 80 02 80 02 80 02 80 02 80 02 80 02 80  ................
>    backtrace:
>      [<ffffffffa69471ca>] kmemleak_alloc+0x4a/0xa0
>      [<ffffffffa628c10e>] kmem_cache_alloc_trace+0x14e/0x2e0
>      [<ffffffffa676cfec>] bitmap_checkpage+0x7c/0x110
>      [<ffffffffa676d0c5>] bitmap_get_counter+0x45/0xd0
>      [<ffffffffa676d6b3>] bitmap_set_memory_bits+0x43/0xe0
>      [<ffffffffa676e41c>] bitmap_init_from_disk+0x23c/0x530
>      [<ffffffffa676f1ae>] bitmap_load+0xbe/0x160
>      [<ffffffffc04c47d3>] raid_preresume+0x203/0x2f0 [dm_raid]
>      [<ffffffffa677762f>] dm_table_resume_targets+0x4f/0xe0
>      [<ffffffffa6774b52>] dm_resume+0x122/0x140
>      [<ffffffffa6779b9f>] dev_suspend+0x18f/0x290
>      [<ffffffffa677a3a7>] ctl_ioctl+0x287/0x560
>      [<ffffffffa677a693>] dm_ctl_ioctl+0x13/0x20
>      [<ffffffffa62d6b46>] do_vfs_ioctl+0xa6/0x750
>      [<ffffffffa62d7269>] SyS_ioctl+0x79/0x90
>      [<ffffffffa6956d41>] entry_SYSCALL_64_fastpath+0x1f/0xc2
>
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>   drivers/md/bitmap.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index d2121637b4ab..58ee21027709 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c

The bitmap.c is renamed to md-bitmap.c now.

> @@ -2152,6 +2152,7 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
>   				for (k = 0; k < page; k++) {
>   					kfree(new_bp[k].map);
>   				}
> +				kfree(new_bp);

Agreed.

>   
>   				/* restore some fields from old_counts */
>   				bitmap->counts.bp = old_counts.bp;
> @@ -2202,6 +2203,14 @@ int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
>   		block += old_blocks;
>   	}
>   
> +	if (bitmap->counts.bp != old_counts.bp) {
> +		unsigned long k;

Just nit-picking, maybe you can move belows to

> +		for (k = 0; k < old_counts.pages; k++)
> +			if (!old_counts.bp[k].hijacked)
> +				kfree(old_counts.bp[k].map);
> +		kfree(old_counts.bp);
> +	}
> +
>   	if (!init) {
>   		int i;

here and reuse the 'i'.

Thanks,
Guoqing

Re: [PATCH 1/2] md: release allocated bitset sync_set

[Shaohua Li]

On Wed, Nov 08, 2017 at 01:44:55PM +0100, Zdenek Kabelac wrote:
> Patch fixes kmemleak on md_stop() path used likely only by dm-raid
> wrapper. Code of md is using  mddev_put() where both  bitsets
> are released however this bitmap freeing is not shared.

what did you mean 'bitmap freeing is not shared' ? The patch doesn't handle
bitmap either.

> Also set NULL to bio_set and sync_set pointers just like mddev_put is
> doing.
> 
> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  drivers/md/md.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0ff1bbf6c90e..635ad7c38f67 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev)
>  	 * This is called from dm-raid
>  	 */
>  	__md_stop(mddev);
> -	if (mddev->bio_set)
> +	if (mddev->bio_set) {
>  		bioset_free(mddev->bio_set);
> +		mddev->bio_set = NULL;
> +	}
> +	if (mddev->sync_set) {
> +		bioset_free(mddev->sync_set);
> +		mddev->sync_set = NULL;
> +	}
>  }
>  
>  EXPORT_SYMBOL_GPL(md_stop);
> -- 
> 2.15.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] md: release allocated bitset sync_set

[Zdenek Kabelac]

Dne 10.11.2017 v 00:32 Shaohua Li napsal(a):
> On Wed, Nov 08, 2017 at 01:44:55PM +0100, Zdenek Kabelac wrote:
>> Patch fixes kmemleak on md_stop() path used likely only by dm-raid
>> wrapper. Code of md is using  mddev_put() where both  bitsets
>> are released however this bitmap freeing is not shared.
> 
> what did you mean 'bitmap freeing is not shared' ? The patch doesn't handle
> bitmap either.
> 

It only means there is some code to teardown md device which is not shared
between 'md'  and  'dm' wrapper - thus whenever mddev_put() is updated
in some way (adding freeing another object)  same code should be
duplicated into  md_stop()  which is used only by 'dm' (from my
understanding of comment in the source).

IMHO this is not ideal - the code for freeing resources should be sharing more 
code lines...

But as of now - patch works and does exactly what is needed...

>> Also set NULL to bio_set and sync_set pointers just like mddev_put is
>> doing.
>>
>> Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
>> ---
>>   drivers/md/md.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 0ff1bbf6c90e..635ad7c38f67 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5834,8 +5834,14 @@ void md_stop(struct mddev *mddev)
>>   	 * This is called from dm-raid
>>   	 */
>>   	__md_stop(mddev);
>> -	if (mddev->bio_set)
>> +	if (mddev->bio_set) {
>>   		bioset_free(mddev->bio_set);
>> +		mddev->bio_set = NULL;
>> +	}
>> +	if (mddev->sync_set) {
>> +		bioset_free(mddev->sync_set);
>> +		mddev->sync_set = NULL;
>> +	}
>>   }
>>   
>>   EXPORT_SYMBOL_GPL(md_stop);
>> -- 
>> 2.15.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html