Thread (5 messages) 5 messages, 4 authors, 2020-10-03

Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

From: Dan Carpenter <hidden>
Date: 2020-08-04 11:15:49
Also in: kernel-janitors

On Tue, Aug 04, 2020 at 12:40:18PM +0200, Guoqing Jiang wrote:
quoted hunk ↗ jump to hunk

On 8/4/20 12:16 PM, Dan Carpenter wrote:
quoted
The error handling calls md_bitmap_free(bitmap) which checks for NULL
but will Oops if we pass an error pointer.  Let's set "bitmap" to NULL
on this error path.

Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape")
Signed-off-by: Dan Carpenter <redacted>
---
  drivers/md/md-cluster.c | 1 +
  1 file changed, 1 insertion(+)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 73fd50e77975..d50737ec4039 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz
  		bitmap = get_bitmap_from_slot(mddev, i);
  		if (IS_ERR(bitmap)) {
  			pr_err("can't get bitmap from slot %d\n", i);
+			bitmap = NULL;
  			goto out;
  		}
  		counts = &bitmap->counts;
Thanks for the catch, Reviewed-by: Guoqing Jiang
[off-list ref]

BTW, seems there could be memory leak in the function since it keeps
allocate bitmap
in the loop ..., will send a format patch.

diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 73fd50e77975..89d7b32489d8 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -1165,6 +1165,8 @@ static int resize_bitmaps(struct mddev *mddev,
sector_t newsize, sector_t oldsiz
                         * can't resize bitmap
                         */
                        goto out;
+
+               md_bitmap_free(bitmap);
Hm...  I'm now not at all certain my patch is correct.  Although it's
obviously harmless and fixes an Oops.  I had thought that that the call
to update_bitmap_size(mddev, oldsize) would free the rest of the loop.

I really suspect adding a free like you're suggesting will break the
success path.

I'm not familiar with this code at all.

regards,
dan carpenter
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help