[PATCH] imsm: FIX: crash during getting map

When get_imsm_map() is called with second_map parameter == '-1'
and array is not in migration state NULL pointer is returned.
This is wrong. '-1' means return map as migration record points.

'-1' can be passed to get_imsm_map() from imsm_num_data_members().
imsm_num_data_members() is called to get current map members based
on migr_state information

Signed-off-by: Adam Kwolek
---

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

diff --git a/super-intel.c b/super-intel.c
index 84ab47b..ee0d9c4 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -567,15 +567,16 @@ struct imsm_map *get_imsm_map(struct imsm_dev *dev, int second_map)
{
struct imsm_map *map = &dev->vol.map[0];

- if (second_map && !dev->vol.migr_state)
+ if ((second_map == 1) && !dev->vol.migr_state)
return NULL;
- else if (second_map) {
+ else if ((second_map == 1) ||
+ ((second_map < 0) && (dev->vol.migr_state))) {
void *ptr = map;

return ptr + sizeof_imsm_map(map);
} else
return map;
-
+
}

/* return the size of the device.

--
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] imsm: FIX: crash during getting map

On Tue, 01 Feb 2011 08:57:37 +0100 Adam Kwolek wrote:

> When get_imsm_map() is called with second_map parameter == '-1'
> and array is not in migration state NULL pointer is returned.
> This is wrong. '-1' means return map as migration record points.
>
> '-1' can be passed to get_imsm_map() from imsm_num_data_members().
> imsm_num_data_members() is called to get current map members based
> on migr_state information
>
> Signed-off-by: Adam Kwolek
> ---
>
> super-intel.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 84ab47b..ee0d9c4 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -567,15 +567,16 @@ struct imsm_map *get_imsm_map(struct imsm_dev *dev, int second_map)
> {
> struct imsm_map *map = &dev->vol.map[0];
>
> - if (second_map && !dev->vol.migr_state)
> + if ((second_map == 1) && !dev->vol.migr_state)
> return NULL;
> - else if (second_map) {
> + else if ((second_map == 1) ||
> + ((second_map < 0) && (dev->vol.migr_state))) {
> void *ptr = map;
>
> return ptr + sizeof_imsm_map(map);
> } else
> return map;
> -
> +
> }
>
> /* return the size of the device.

Thanks.
I added some comments and took the opportunity to simplify
get_imsm_ord_tbl_ent. See below.

NeilBrown

commit 5e7b0330669594ee79201d19ff45a7850fa0f951
Author: Adam Kwolek
Date: Thu Feb 3 17:02:39 2011 +1100

imsm: FIX: crash during getting map

When get_imsm_map() is called with second_map parameter == '-1'
and array is not in migration state NULL pointer is returned.
This is wrong. '-1' means return map as migration record points.

'-1' can be passed to get_imsm_map() from imsm_num_data_members().
imsm_num_data_members() is called to get current map members based
on migr_state information

Signed-off-by: Adam Kwolek
Signed-off-by: NeilBrown

diff --git a/super-intel.c b/super-intel.c
index 84ab47b..4081071 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -565,17 +565,24 @@ static size_t sizeof_imsm_map(struct imsm_map *map)

struct imsm_map *get_imsm_map(struct imsm_dev *dev, int second_map)
{
+ /* A device can have 2 maps if it is in the middle of a migration.
+ * If second_map is:
+ * 0 - we return the first map
+ * 1 - we return the second map if it exists, else NULL
+ * -1 - we return the second map if it exists, else the first
+ */
struct imsm_map *map = &dev->vol.map[0];

- if (second_map && !dev->vol.migr_state)
+ if (second_map == 1 && !dev->vol.migr_state)
return NULL;
- else if (second_map) {
+ else if (second_map == 1 ||
+ (second_map < 0 && dev->vol.migr_state)) {
void *ptr = map;

return ptr + sizeof_imsm_map(map);
} else
return map;
-
+
}

/* return the size of the device.
@@ -654,14 +661,7 @@ static __u32 get_imsm_ord_tbl_ent(struct imsm_dev *dev,
{
struct imsm_map *map;

- if (second_map == -1) {
- if (dev->vol.migr_state)
- map = get_imsm_map(dev, 1);
- else
- map = get_imsm_map(dev, 0);
- } else {
- map = get_imsm_map(dev, second_map);
- }
+ map = get_imsm_map(dev, second_map);

/* top byte identifies disk under rebuild */
return __le32_to_cpu(map->disk_ord_tbl[slot]);
--
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] imsm: FIX: crash during getting map

Could you give me branch name were the changes can be visible?

Thanks
Adam


> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> Sent: Thursday, February 03, 2011 7:04 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH] imsm: FIX: crash during getting map
>
> On Tue, 01 Feb 2011 08:57:37 +0100 Adam Kwolek
> wrote:
>
> > When get_imsm_map() is called with second_map parameter == '-1'
> > and array is not in migration state NULL pointer is returned.
> > This is wrong. '-1' means return map as migration record points.
> >
> > '-1' can be passed to get_imsm_map() from imsm_num_data_members().
> > imsm_num_data_members() is called to get current map members based
> > on migr_state information
> >
> > Signed-off-by: Adam Kwolek
> > ---
> >
> > super-intel.c | 7 ++++---
> > 1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 84ab47b..ee0d9c4 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -567,15 +567,16 @@ struct imsm_map *get_imsm_map(struct imsm_dev
> *dev, int second_map)
> > {
> > struct imsm_map *map = &dev->vol.map[0];
> >
> > - if (second_map && !dev->vol.migr_state)
> > + if ((second_map == 1) && !dev->vol.migr_state)
> > return NULL;
> > - else if (second_map) {
> > + else if ((second_map == 1) ||
> > + ((second_map < 0) && (dev->vol.migr_state))) {
> > void *ptr = map;
> >
> > return ptr + sizeof_imsm_map(map);
> > } else
> > return map;
> > -
> > +
> > }
> >
> > /* return the size of the device.
>
> Thanks.
> I added some comments and took the opportunity to simplify
> get_imsm_ord_tbl_ent. See below.
>
> NeilBrown
>
> commit 5e7b0330669594ee79201d19ff45a7850fa0f951
> Author: Adam Kwolek
> Date: Thu Feb 3 17:02:39 2011 +1100
>
> imsm: FIX: crash during getting map
>
> When get_imsm_map() is called with second_map parameter == '-1'
> and array is not in migration state NULL pointer is returned.
> This is wrong. '-1' means return map as migration record points.
>
> '-1' can be passed to get_imsm_map() from imsm_num_data_members().
> imsm_num_data_members() is called to get current map members based
> on migr_state information
>
> Signed-off-by: Adam Kwolek
> Signed-off-by: NeilBrown
>
> diff --git a/super-intel.c b/super-intel.c
> index 84ab47b..4081071 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -565,17 +565,24 @@ static size_t sizeof_imsm_map(struct imsm_map
> *map)
>
> struct imsm_map *get_imsm_map(struct imsm_dev *dev, int second_map)
> {
> + /* A device can have 2 maps if it is in the middle of a
> migration.
> + * If second_map is:
> + * 0 - we return the first map
> + * 1 - we return the second map if it exists, else NULL
> + * -1 - we return the second map if it exists, else the first
> + */
> struct imsm_map *map = &dev->vol.map[0];
>
> - if (second_map && !dev->vol.migr_state)
> + if (second_map == 1 && !dev->vol.migr_state)
> return NULL;
> - else if (second_map) {
> + else if (second_map == 1 ||
> + (second_map < 0 && dev->vol.migr_state)) {
> void *ptr = map;
>
> return ptr + sizeof_imsm_map(map);
> } else
> return map;
> -
> +
> }
>
> /* return the size of the device.
> @@ -654,14 +661,7 @@ static __u32 get_imsm_ord_tbl_ent(struct imsm_dev
> *dev,
> {
> struct imsm_map *map;
>
> - if (second_map == -1) {
> - if (dev->vol.migr_state)
> - map = get_imsm_map(dev, 1);
> - else
> - map = get_imsm_map(dev, 0);
> - } else {
> - map = get_imsm_map(dev, second_map);
> - }
> + map = get_imsm_map(dev, second_map);
>
> /* top byte identifies disk under rebuild */
> return __le32_to_cpu(map->disk_ord_tbl[slot]);
> --
> 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
--
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