Re: [PATCH]: grub: Fix ofdisk disk cache corruption.

[phcoder]

David Miller wrote:
> From: Pavel Roskin <address@hidden>
> 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.
>
> In my testing there weren't many collisions.
This is so called "survivorship bias". If this hash generated a 
collision for you you would already have changed it
> 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
> > If 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.
You can maintain a table of devpathes in cache and use the index in this 
table as id. This way is the safest
> > > +  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