[PATCH 0/2] patches addresses problem with memory corruption

[PATCH 0/2] patches addresses problem with memory corruption

[Adam Kwolek]

When for raid0 metadata is updated outside mdmon it can happen that
memory corruption occurs due to too amount of memory is allocated 
for imsm device structure.

1. imsm: FIX: mdmon crash during 2 raid0 arrays expansion
  this patch is almost the same to sent earlier except 2 changes:
   a) compilation problem
   b) size should affect anchor size also
      (added calculation changes extends space_needed variable also
       this variable is used for anchor allocation)
  so it replaces previous one.
2.  imsm: FIX: sizeof_imsm_dev() can return too small value
	size returned by sizeof_imsm_dev() describes minimum size of device
        This is correct when both device maps has the same length
        When device expands or shrinks we should return size that allows
        for storing larger device information to avoid memory corruption.

BR
Adam

---

Adam Kwolek (2):
      imsm: FIX: sizeof_imsm_dev() can return too small value
      imsm: FIX: mdmon crash during 2 raid0 arrays expansion


 super-intel.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

-- 
Signature

[PATCH 1/2] imsm: FIX: mdmon crash during 2 raid0 arrays expansion

[Adam Kwolek]

When expansion is run on 2 raid0 arrays in container no update
is sent to mdmon because mdmon is off (mdadm performs update)
Memory size for first reshaped array is allocated to satisfy memory
requirements for expanded maps.
Memory for second device is allocated using old disks number, as in metadata
there is no information about this array reshape.
When mdmon initiates second array reshape it overwrites internal structures
and crashes). There is no place to keep expanded maps.
To avoid this situation during loading metadata, allocated memory should be performed
using the maximum used disks number in particular container.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 3e163ec..0c988d6 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2483,6 +2483,7 @@ static int parse_raid_devices(struct intel_super *super)
 	int i;
 	struct imsm_dev *dev_new;
 	size_t len, len_migr;
+	size_t max_len = 0;
 	size_t space_needed = 0;
 	struct imsm_super *mpb = super->anchor;
 
@@ -2498,7 +2499,11 @@ static int parse_raid_devices(struct intel_super *super)
 		dv = malloc(sizeof(*dv));
 		if (!dv)
 			return 1;
-		dev_new = malloc(len_migr);
+		if (max_len < len_migr)
+			max_len = len_migr;
+		if (max_len > len_migr)
+			space_needed += max_len - len_migr;
+		dev_new = malloc(max_len);
 		if (!dev_new) {
 			free(dv);
 			return 1;

[PATCH 2/2] imsm: FIX: sizeof_imsm_dev() can return too small value

[Adam Kwolek]

sizeof_imsm_dev() should return value that can satisfy map operation
for 2 maps of size equal to bigger one.
If function reports too small value copy of bigger map can overwrite
other data in memory.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 0c988d6..3c969c3 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -584,12 +584,17 @@ static size_t sizeof_imsm_dev(struct imsm_dev *dev, int migr_state)
 {
 	size_t size = sizeof(*dev) - sizeof(struct imsm_map) +
 		      sizeof_imsm_map(get_imsm_map(dev, 0));
+	int map_size = sizeof_imsm_map(get_imsm_map(dev, 0));
+
+	if (dev->vol.migr_state) {
+		int map1_size = sizeof_imsm_map(get_imsm_map(dev, 1));
+		if (map1_size > map_size)
+			map_size = map1_size;
+	}
 
 	/* migrating means an additional map */
-	if (dev->vol.migr_state)
-		size += sizeof_imsm_map(get_imsm_map(dev, 1));
-	else if (migr_state)
-		size += sizeof_imsm_map(get_imsm_map(dev, 0));
+	if ((dev->vol.migr_state) || (migr_state))
+		size += map_size;
 
 	return size;
 }

RE: [PATCH 0/2] patches addresses problem with memory corruption

[Kwolek, Adam]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Adam Kwolek
> Sent: Monday, January 31, 2011 4:25 PM
> To: neilb@suse.de
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: [PATCH 0/2] patches addresses problem with memory corruption
> 
> When for raid0 metadata is updated outside mdmon it can happen that
> memory corruption occurs due to too amount of memory is allocated
> for imsm device structure.
> 
> 1. imsm: FIX: mdmon crash during 2 raid0 arrays expansion
>   this patch is almost the same to sent earlier except 2 changes:
>    a) compilation problem
>    b) size should affect anchor size also
>       (added calculation changes extends space_needed variable also
>        this variable is used for anchor allocation)
>   so it replaces previous one.
> 2.  imsm: FIX: sizeof_imsm_dev() can return too small value
> 	size returned by sizeof_imsm_dev() describes minimum size of
> device
>         This is correct when both device maps has the same length
>         When device expands or shrinks we should return size that
> allows
>         for storing larger device information to avoid memory
> corruption.
> 
> BR
> Adam

imsm: FIX: sizeof_imsm_dev() can return too small value
	This should be addressed in different way (fix in different place), I have to think about it...
	please review/apply first patch only (imsm: FIX: mdmon crash during 2 raid0 arrays expansion)

BR
Adam

> ---
> 
> Adam Kwolek (2):
>       imsm: FIX: sizeof_imsm_dev() can return too small value
>       imsm: FIX: mdmon crash during 2 raid0 arrays expansion
> 
> 
>  super-intel.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> --
> Signature
> --
> 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 0/2] patches addresses problem with memory corruption

[NeilBrown]

On Mon, 31 Jan 2011 16:25:08 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> When for raid0 metadata is updated outside mdmon it can happen that
> memory corruption occurs due to too amount of memory is allocated 
> for imsm device structure.
> 
> 1. imsm: FIX: mdmon crash during 2 raid0 arrays expansion
>   this patch is almost the same to sent earlier except 2 changes:
>    a) compilation problem
>    b) size should affect anchor size also
>       (added calculation changes extends space_needed variable also
>        this variable is used for anchor allocation)
>   so it replaces previous one.
> 2.  imsm: FIX: sizeof_imsm_dev() can return too small value
> 	size returned by sizeof_imsm_dev() describes minimum size of device
>         This is correct when both device maps has the same length
>         When device expands or shrinks we should return size that allows
>         for storing larger device information to avoid memory corruption.
> 

Both applied (replacing the previous one)..

Thanks.

NeilBrown


> BR
> Adam
> 
> ---
> 
> Adam Kwolek (2):
>       imsm: FIX: sizeof_imsm_dev() can return too small value
>       imsm: FIX: mdmon crash during 2 raid0 arrays expansion
> 
> 
>  super-intel.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 

Re: [PATCH 0/2] patches addresses problem with memory corruption

[NeilBrown]

On Mon, 31 Jan 2011 15:59:24 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of Adam Kwolek
> > Sent: Monday, January 31, 2011 4:25 PM
> > To: neilb@suse.de
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: [PATCH 0/2] patches addresses problem with memory corruption
> > 
> > When for raid0 metadata is updated outside mdmon it can happen that
> > memory corruption occurs due to too amount of memory is allocated
> > for imsm device structure.
> > 
> > 1. imsm: FIX: mdmon crash during 2 raid0 arrays expansion
> >   this patch is almost the same to sent earlier except 2 changes:
> >    a) compilation problem
> >    b) size should affect anchor size also
> >       (added calculation changes extends space_needed variable also
> >        this variable is used for anchor allocation)
> >   so it replaces previous one.
> > 2.  imsm: FIX: sizeof_imsm_dev() can return too small value
> > 	size returned by sizeof_imsm_dev() describes minimum size of
> > device
> >         This is correct when both device maps has the same length
> >         When device expands or shrinks we should return size that
> > allows
> >         for storing larger device information to avoid memory
> > corruption.
> > 
> > BR
> > Adam
> 
> imsm: FIX: sizeof_imsm_dev() can return too small value
> 	This should be addressed in different way (fix in different place), I have to think about it...
> 	please review/apply first patch only (imsm: FIX: mdmon crash during 2 raid0 arrays expansion)
> 

ahhh.. ok.  Yes, I see your point.
The size adjustment has to happen at the point when we malloc.

I suspect we should get ride of the idea of always allocating enough space to
hold the migration info as well, as we don't really know in advance how big
that is going to be.  Maybe we shoul always allocated (or re-allocated)
exactly how much we know we need now...

I had applied this patch, but I have now reverted it.
Thanks,
NeilBrown