[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH]: grub: Fix ofdisk disk cache corruption.
From: |
Pavel Roskin |
Subject: |
Re: [PATCH]: grub: Fix ofdisk disk cache corruption. |
Date: |
Sun, 12 Apr 2009 02:29:15 -0400 |
On Sat, 2009-04-11 at 01:08 -0700, David Miller wrote:
> The ieee1275 ofdisk driver doesn't use a unique value for
> disk->id so it's really easy to get disk corruption. I was
> able to see such corruption by simply booting grub from one
> disk and booting a Linux kernel from another, both of which
> were on the same disk controller.
I hope you mean disk cache corruption, as in the subject, not disk
corruption. GRUB only writes to disks to save environment variables,
and it's done very carefully.
> +#define OFDISK_HASH_SZ 8
> +static struct ofdisk_hash_ent *ofdisk_hash[OFDISK_HASH_SZ];
> +
> +static int
> +ofdisk_hash_fn (const char *devpath)
> +{
> + int hash = 0;
> + while (*devpath)
> + hash ^= *devpath++;
> + return (hash & (OFDISK_HASH_SZ - 1));
> +}
That's a 3 bit hash. The risk of collisions is very high. I would
understand if you had 8 entries for the hash values, but the hash values
themselves should be reasonably unique.
> +static struct ofdisk_hash_ent *
> +ofdisk_hash_add (char *devpath)
> +{
> + struct ofdisk_hash_ent **head = &ofdisk_hash[ofdisk_hash_fn(devpath)];
> + struct ofdisk_hash_ent *p = grub_malloc(sizeof (*p));
> +
> + if (p)
> + {
> + p->devpath = devpath;
If you can save the device names, then there is no point in using
hashes. You can use (long)devpath.
> + if (!op)
> + op = ofdisk_hash_add (devpath);
>
> - grub_ieee1275_open (devpath, &dev_ihandle);
> + grub_free (devpath);
But if you free the device names, then they are bad IDs. The
probability of the same memory being reused for another name is high.
Perhaps I misunderstand something.
--
Regards,
Pavel Roskin