* [PATCH] imsm: fix: correct calculation of UUID
@ 2012-01-04 9:40 Lukasz Dorau
2012-01-04 10:05 ` Dan Williams
0 siblings, 1 reply; 5+ messages in thread
From: Lukasz Dorau @ 2012-01-04 9:40 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, dan.j.williams, marcin.labun, ed.ciechanowski
The UUID is calculated from two numbers and two strings stored in arrays.
Whole arrays of chars are taken to calculation (not only the used part).
This is wrong, because the unused part of arrays can be changed accidentally.
For example it can sometimes happen when degraded raid is being rebuilt in OROM.
Such accidental change of UUID causes that system installed on raid
does not boot due to not recognized booting device (raid with changed UUID).
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
---
super-intel.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/super-intel.c b/super-intel.c
index 0e9269f..7f34542 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1820,6 +1820,7 @@ static void uuid_from_super_imsm(struct supertype *st, int uuid[4])
struct sha1_ctx ctx;
struct imsm_dev *dev = NULL;
__u32 family_num;
+ size_t len;
/* some mdadm versions failed to set ->orig_family_num, in which
* case fall back to ->family_num. orig_family_num will be
@@ -1829,14 +1830,16 @@ static void uuid_from_super_imsm(struct supertype *st, int uuid[4])
if (family_num == 0)
family_num = super->anchor->family_num;
sha1_init_ctx(&ctx);
- sha1_process_bytes(super->anchor->sig, MPB_SIG_LEN, &ctx);
+ len = strnlen((const char *)super->anchor->sig, MPB_SIG_LEN);
+ sha1_process_bytes(super->anchor->sig, len, &ctx);
sha1_process_bytes(&family_num, sizeof(__u32), &ctx);
if (super->current_vol >= 0)
dev = get_imsm_dev(super, super->current_vol);
if (dev) {
__u32 vol = super->current_vol;
sha1_process_bytes(&vol, sizeof(vol), &ctx);
- sha1_process_bytes(dev->volume, MAX_RAID_SERIAL_LEN, &ctx);
+ len = strnlen((const char *)dev->volume, MAX_RAID_SERIAL_LEN);
+ sha1_process_bytes(dev->volume, len, &ctx);
}
sha1_finish_ctx(&ctx, buf);
memcpy(uuid, buf, 4*4);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] imsm: fix: correct calculation of UUID
2012-01-04 9:40 [PATCH] imsm: fix: correct calculation of UUID Lukasz Dorau
@ 2012-01-04 10:05 ` Dan Williams
2012-01-04 12:13 ` Dorau, Lukasz
0 siblings, 1 reply; 5+ messages in thread
From: Dan Williams @ 2012-01-04 10:05 UTC (permalink / raw)
To: Lukasz Dorau; +Cc: neilb, linux-raid, marcin.labun, ed.ciechanowski
On Wed, Jan 4, 2012 at 1:40 AM, Lukasz Dorau <lukasz.dorau@intel.com> wrote:
> The UUID is calculated from two numbers and two strings stored in arrays.
> Whole arrays of chars are taken to calculation (not only the used part).
> This is wrong, because the unused part of arrays can be changed accidentally.
> For example it can sometimes happen when degraded raid is being rebuilt in OROM.
> Such accidental change of UUID causes that system installed on raid
> does not boot due to not recognized booting device (raid with changed UUID).
>
> Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
> ---
> super-intel.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 0e9269f..7f34542 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1820,6 +1820,7 @@ static void uuid_from_super_imsm(struct supertype *st, int uuid[4])
> struct sha1_ctx ctx;
> struct imsm_dev *dev = NULL;
> __u32 family_num;
> + size_t len;
>
> /* some mdadm versions failed to set ->orig_family_num, in which
> * case fall back to ->family_num. orig_family_num will be
> @@ -1829,14 +1830,16 @@ static void uuid_from_super_imsm(struct supertype *st, int uuid[4])
> if (family_num == 0)
> family_num = super->anchor->family_num;
> sha1_init_ctx(&ctx);
> - sha1_process_bytes(super->anchor->sig, MPB_SIG_LEN, &ctx);
> + len = strnlen((const char *)super->anchor->sig, MPB_SIG_LEN);
> + sha1_process_bytes(super->anchor->sig, len, &ctx);
> sha1_process_bytes(&family_num, sizeof(__u32), &ctx);
> if (super->current_vol >= 0)
> dev = get_imsm_dev(super, super->current_vol);
> if (dev) {
> __u32 vol = super->current_vol;
> sha1_process_bytes(&vol, sizeof(vol), &ctx);
> - sha1_process_bytes(dev->volume, MAX_RAID_SERIAL_LEN, &ctx);
> + len = strnlen((const char *)dev->volume, MAX_RAID_SERIAL_LEN);
> + sha1_process_bytes(dev->volume, len, &ctx);
Can't do this. It will break the UUIDs of existing arrays in the field.
When can the unused portion of anchor->sig and dev->volume be "changed
accidentally", as you mention, during a rebuild? That is what needs
to be fixed.
--
Dan
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] imsm: fix: correct calculation of UUID
2012-01-04 10:05 ` Dan Williams
@ 2012-01-04 12:13 ` Dorau, Lukasz
2012-01-04 16:41 ` Dan Williams
0 siblings, 1 reply; 5+ messages in thread
From: Dorau, Lukasz @ 2012-01-04 12:13 UTC (permalink / raw)
To: Williams, Dan J; +Cc: neilb, linux-raid, Labun, Marcin, Ciechanowski, Ed
On Wed, Jan 4, 2012 at 11:05 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Jan 4, 2012 at 1:40 AM, Lukasz Dorau <lukasz.dorau@intel.com>
> wrote:
> > The UUID is calculated from two numbers and two strings stored in arrays.
> > Whole arrays of chars are taken to calculation (not only the used part).
> > This is wrong, because the unused part of arrays can be changed accidentally.
> > For example it can sometimes happen when degraded raid is being rebuilt in
> OROM.
> > Such accidental change of UUID causes that system installed on raid
> > does not boot due to not recognized booting device (raid with changed UUID).
> >
> > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
> > ---
> > super-intel.c | 7 +++++--
> > 1 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 0e9269f..7f34542 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -1820,6 +1820,7 @@ static void uuid_from_super_imsm(struct supertype
> *st, int uuid[4])
> > struct sha1_ctx ctx;
> > struct imsm_dev *dev = NULL;
> > __u32 family_num;
> > + size_t len;
> >
> > /* some mdadm versions failed to set ->orig_family_num, in which
> > * case fall back to ->family_num. orig_family_num will be
> > @@ -1829,14 +1830,16 @@ static void uuid_from_super_imsm(struct
> supertype *st, int uuid[4])
> > if (family_num == 0)
> > family_num = super->anchor->family_num;
> > sha1_init_ctx(&ctx);
> > - sha1_process_bytes(super->anchor->sig, MPB_SIG_LEN, &ctx);
> > + len = strnlen((const char *)super->anchor->sig, MPB_SIG_LEN);
> > + sha1_process_bytes(super->anchor->sig, len, &ctx);
> > sha1_process_bytes(&family_num, sizeof(__u32), &ctx);
> > if (super->current_vol >= 0)
> > dev = get_imsm_dev(super, super->current_vol);
> > if (dev) {
> > __u32 vol = super->current_vol;
> > sha1_process_bytes(&vol, sizeof(vol), &ctx);
> > - sha1_process_bytes(dev->volume, MAX_RAID_SERIAL_LEN, &ctx);
> > + len = strnlen((const char *)dev->volume, MAX_RAID_SERIAL_LEN);
> > + sha1_process_bytes(dev->volume, len, &ctx);
>
> Can't do this. It will break the UUIDs of existing arrays in the field.
>
So we can't fill the unused part of these arrays with '\000' character
before calculating the UUID either, can we?
> When can the unused portion of anchor->sig and dev->volume be "changed
> accidentally", as you mention, during a rebuild?
>
Please consider the following scenario:
1. Create RAID 10 volume of name "r10d4n0s64" in OROM.
2. Delete the volume "r10d4n0s64" in OROM.
3. Create RAID 10 volume of name "raid10" in OROM.
4. Boot the system.
5. Run mdadm, contents of dev->volume array is "raid10\000s64\000\000..." (8th, 9th, 10th bytes are "s64")
6. Fail one of the "raid10" volume's disks. The "raid10" volume is degraded now.
7. Restart the computer and enter OROM.
8. Add a new empty disk to the volume in OROM. The status of the volume is changed to "Rebuild".
9. Boot the system.
10. Run mdadm, contents of dev->volume array is "raid10\000\000\000\000\000..." (8th, 9th, 10th bytes are "\000\000\000").
Now the UUID is different than it was in point 5th (above). If the operating system were installed on that volume, it would not boot due to not recognized booting device (raid with changed UUID).
Regards,
Lukasz
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] imsm: fix: correct calculation of UUID
2012-01-04 12:13 ` Dorau, Lukasz
@ 2012-01-04 16:41 ` Dan Williams
2012-01-05 13:30 ` Dorau, Lukasz
0 siblings, 1 reply; 5+ messages in thread
From: Dan Williams @ 2012-01-04 16:41 UTC (permalink / raw)
To: Dorau, Lukasz; +Cc: neilb, linux-raid, Labun, Marcin, Ciechanowski, Ed
2012/1/4 Dorau, Lukasz <lukasz.dorau@intel.com>:
> On Wed, Jan 4, 2012 at 11:05 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>>
>> On Wed, Jan 4, 2012 at 1:40 AM, Lukasz Dorau <lukasz.dorau@intel.com>
>> wrote:
>> > The UUID is calculated from two numbers and two strings stored in arrays.
>> > Whole arrays of chars are taken to calculation (not only the used part).
>> > This is wrong, because the unused part of arrays can be changed accidentally.
>> > For example it can sometimes happen when degraded raid is being rebuilt in
>> OROM.
>> > Such accidental change of UUID causes that system installed on raid
>> > does not boot due to not recognized booting device (raid with changed UUID).
>> >
>> > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
>> > ---
>> > super-intel.c | 7 +++++--
>> > 1 files changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/super-intel.c b/super-intel.c
>> > index 0e9269f..7f34542 100644
>> > --- a/super-intel.c
>> > +++ b/super-intel.c
>> > @@ -1820,6 +1820,7 @@ static void uuid_from_super_imsm(struct supertype
>> *st, int uuid[4])
>> > struct sha1_ctx ctx;
>> > struct imsm_dev *dev = NULL;
>> > __u32 family_num;
>> > + size_t len;
>> >
>> > /* some mdadm versions failed to set ->orig_family_num, in which
>> > * case fall back to ->family_num. orig_family_num will be
>> > @@ -1829,14 +1830,16 @@ static void uuid_from_super_imsm(struct
>> supertype *st, int uuid[4])
>> > if (family_num == 0)
>> > family_num = super->anchor->family_num;
>> > sha1_init_ctx(&ctx);
>> > - sha1_process_bytes(super->anchor->sig, MPB_SIG_LEN, &ctx);
>> > + len = strnlen((const char *)super->anchor->sig, MPB_SIG_LEN);
>> > + sha1_process_bytes(super->anchor->sig, len, &ctx);
>> > sha1_process_bytes(&family_num, sizeof(__u32), &ctx);
>> > if (super->current_vol >= 0)
>> > dev = get_imsm_dev(super, super->current_vol);
>> > if (dev) {
>> > __u32 vol = super->current_vol;
>> > sha1_process_bytes(&vol, sizeof(vol), &ctx);
>> > - sha1_process_bytes(dev->volume, MAX_RAID_SERIAL_LEN, &ctx);
>> > + len = strnlen((const char *)dev->volume, MAX_RAID_SERIAL_LEN);
>> > + sha1_process_bytes(dev->volume, len, &ctx);
>>
>> Can't do this. It will break the UUIDs of existing arrays in the field.
>>
> So we can't fill the unused part of these arrays with '\000' character
> before calculating the UUID either, can we?
>
>> When can the unused portion of anchor->sig and dev->volume be "changed
>> accidentally", as you mention, during a rebuild?
>>
> Please consider the following scenario:
> 1. Create RAID 10 volume of name "r10d4n0s64" in OROM.
> 2. Delete the volume "r10d4n0s64" in OROM.
> 3. Create RAID 10 volume of name "raid10" in OROM.
> 4. Boot the system.
> 5. Run mdadm, contents of dev->volume array is "raid10\000s64\000\000..." (8th, 9th, 10th bytes are "s64")
> 6. Fail one of the "raid10" volume's disks. The "raid10" volume is degraded now.
> 7. Restart the computer and enter OROM.
> 8. Add a new empty disk to the volume in OROM. The status of the volume is changed to "Rebuild".
> 9. Boot the system.
> 10. Run mdadm, contents of dev->volume array is "raid10\000\000\000\000\000..." (8th, 9th, 10th bytes are "\000\000\000").
On all disks or just the newly added spare?
> Now the UUID is different than it was in point 5th (above). If the operating system were installed on that volume, it would not boot due to not recognized booting device (raid with changed UUID).
...and you can only reproduce this when adding the disk via orom?
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] imsm: fix: correct calculation of UUID
2012-01-04 16:41 ` Dan Williams
@ 2012-01-05 13:30 ` Dorau, Lukasz
0 siblings, 0 replies; 5+ messages in thread
From: Dorau, Lukasz @ 2012-01-05 13:30 UTC (permalink / raw)
To: Williams, Dan J; +Cc: neilb, linux-raid, Labun, Marcin, Ciechanowski, Ed
> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Wednesday, January 04, 2012 5:41 PM
> To: Dorau, Lukasz
> Cc: neilb@suse.de; linux-raid@vger.kernel.org; Labun, Marcin; Ciechanowski, Ed
> Subject: Re: [PATCH] imsm: fix: correct calculation of UUID
>
> 2012/1/4 Dorau, Lukasz <lukasz.dorau@intel.com>:
> > On Wed, Jan 4, 2012 at 11:05 AM, Dan Williams <dan.j.williams@intel.com>
> wrote:
> >>
> >> On Wed, Jan 4, 2012 at 1:40 AM, Lukasz Dorau <lukasz.dorau@intel.com>
> >> wrote:
> >> > The UUID is calculated from two numbers and two strings stored in arrays.
> >> > Whole arrays of chars are taken to calculation (not only the used part).
> >> > This is wrong, because the unused part of arrays can be changed
> accidentally.
> >> > For example it can sometimes happen when degraded raid is being rebuilt in
> >> OROM.
> >> > Such accidental change of UUID causes that system installed on raid
> >> > does not boot due to not recognized booting device (raid with changed
> UUID).
> >> >
> >> > Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
> >> > ---
> >> > super-intel.c | 7 +++++--
> >> > 1 files changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/super-intel.c b/super-intel.c
> >> > index 0e9269f..7f34542 100644
> >> > --- a/super-intel.c
> >> > +++ b/super-intel.c
> >> > @@ -1820,6 +1820,7 @@ static void uuid_from_super_imsm(struct
> supertype
> >> *st, int uuid[4])
> >> > struct sha1_ctx ctx;
> >> > struct imsm_dev *dev = NULL;
> >> > __u32 family_num;
> >> > + size_t len;
> >> >
> >> > /* some mdadm versions failed to set ->orig_family_num, in which
> >> > * case fall back to ->family_num. orig_family_num will be
> >> > @@ -1829,14 +1830,16 @@ static void uuid_from_super_imsm(struct
> >> supertype *st, int uuid[4])
> >> > if (family_num == 0)
> >> > family_num = super->anchor->family_num;
> >> > sha1_init_ctx(&ctx);
> >> > - sha1_process_bytes(super->anchor->sig, MPB_SIG_LEN, &ctx);
> >> > + len = strnlen((const char *)super->anchor->sig, MPB_SIG_LEN);
> >> > + sha1_process_bytes(super->anchor->sig, len, &ctx);
> >> > sha1_process_bytes(&family_num, sizeof(__u32), &ctx);
> >> > if (super->current_vol >= 0)
> >> > dev = get_imsm_dev(super, super->current_vol);
> >> > if (dev) {
> >> > __u32 vol = super->current_vol;
> >> > sha1_process_bytes(&vol, sizeof(vol), &ctx);
> >> > - sha1_process_bytes(dev->volume, MAX_RAID_SERIAL_LEN, &ctx);
> >> > + len = strnlen((const char *)dev->volume, MAX_RAID_SERIAL_LEN);
> >> > + sha1_process_bytes(dev->volume, len, &ctx);
> >>
> >> Can't do this. It will break the UUIDs of existing arrays in the field.
> >>
> > So we can't fill the unused part of these arrays with '\000' character
> > before calculating the UUID either, can we?
> >
> >> When can the unused portion of anchor->sig and dev->volume be "changed
> >> accidentally", as you mention, during a rebuild?
> >>
> > Please consider the following scenario:
> > 1. Create RAID 10 volume of name "r10d4n0s64" in OROM.
> > 2. Delete the volume "r10d4n0s64" in OROM.
> > 3. Create RAID 10 volume of name "raid10" in OROM.
> > 4. Boot the system.
> > 5. Run mdadm, contents of dev->volume array is "raid10\000s64\000\000..."
> (8th, 9th, 10th bytes are "s64")
> > 6. Fail one of the "raid10" volume's disks. The "raid10" volume is degraded
> now.
> > 7. Restart the computer and enter OROM.
> > 8. Add a new empty disk to the volume in OROM. The status of the volume is
> changed to "Rebuild".
> > 9. Boot the system.
> > 10. Run mdadm, contents of dev->volume array is
> "raid10\000\000\000\000\000..." (8th, 9th, 10th bytes are "\000\000\000").
>
> On all disks or just the newly added spare?
>
On all disks.
> > Now the UUID is different than it was in point 5th (above). If the operating
> system were installed on that volume, it would not boot due to not recognized
> booting device (raid with changed UUID).
>
> ...and you can only reproduce this when adding the disk via orom?
>
Yes, only when adding the disk via OROM.
Regards,
Lukasz
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-05 13:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-04 9:40 [PATCH] imsm: fix: correct calculation of UUID Lukasz Dorau
2012-01-04 10:05 ` Dan Williams
2012-01-04 12:13 ` Dorau, Lukasz
2012-01-04 16:41 ` Dan Williams
2012-01-05 13:30 ` Dorau, Lukasz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.