RE: [PATCH] imsm: fix: correct calculation of UUID

-----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 [off-list ref]:

quoted

On Wed, Jan 4, 2012 at 11:05 AM, Dan Williams [off-list ref]

wrote:

quoted

quoted

On Wed, Jan 4, 2012 at 1:40 AM, Lukasz Dorau [off-list ref]
wrote:

quoted

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.

quoted

quoted

quoted

For example it can sometimes happen when degraded raid is being rebuilt in

OROM.

quoted

Such accidental change of UUID causes that system installed on raid
does not boot due to not recognized booting device (raid with changed

UUID).

quoted

quoted

quoted

Signed-off-by: Lukasz Dorau <redacted>
---
 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

quoted

quoted

*st, int uuid[4])

quoted

       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])

quoted

       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?

quoted

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")

quoted

6. Fail one of the "raid10" volume's disks. The "raid10" volume is degraded

now.

quoted

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".

quoted

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.

quoted

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