|
From: | phcoder |
Subject: | Re: [PATCH]: grub: Fix ofdisk disk cache corruption. |
Date: | Sun, 12 Apr 2009 12:44:10 +0200 |
User-agent: | Thunderbird 2.0.0.21 (X11/20090318) |
David Miller wrote:
This is so called "survivorship bias". If this hash generated a collision for you you would already have changed itFrom: Pavel Roskin <address@hidden> Date: Sun, 12 Apr 2009 02:29:15 -0400On 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.In my testing there weren't many collisions.
I think fixing disk cache corruption is more important than arguing over the distribution properties of the hash function I have choosen.
Yes, but weak hash has exactly the same problem, just on other systems
You can maintain a table of devpathes in cache and use the index in this table as id. This way is the safestIf you can save the device names, then there is no point in using hashes. You can use (long)devpath.Sure we need the hash, to find path entries we've saved beforehand.
+ 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.The path we use is dup'd into the hash entries we create, and the hash entry path string is the one we use. Therefore "devpath" is only needed across the ofdisk_hash_add() call. _______________________________________________ Grub-devel mailing list address@hidden http://lists.gnu.org/mailman/listinfo/grub-devel
-- Regards Vladimir 'phcoder' Serbinenko
[Prev in Thread] | Current Thread | [Next in Thread] |