From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1LsxAc-0001kl-QA for mharc-grub-devel@gnu.org; Sun, 12 Apr 2009 06:44:14 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LsxAb-0001kg-QN for grub-devel@gnu.org; Sun, 12 Apr 2009 06:44:13 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LsxAW-0001kU-B3 for grub-devel@gnu.org; Sun, 12 Apr 2009 06:44:12 -0400 Received: from [199.232.76.173] (port=34026 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LsxAW-0001kR-5Y for grub-devel@gnu.org; Sun, 12 Apr 2009 06:44:08 -0400 Received: from fg-out-1718.google.com ([72.14.220.152]:5753) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LsxAV-0006HT-Il for grub-devel@gnu.org; Sun, 12 Apr 2009 06:44:07 -0400 Received: by fg-out-1718.google.com with SMTP id l27so222776fgb.7 for ; Sun, 12 Apr 2009 03:44:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=SM9vnjJxZY1+l8cOZOqkn0LUDNAev7pgsuzFhZkR4SU=; b=OZKSrhRgTIhL1C12ZXu5Py+x0DagoyNJOCxv7ht3D97McuAtzeRuG3bUJ+8hnDRpc2 Qb4fHp/EeDB/SK34zdzUxJH3C5KnUv7qarwHbMn7yhdZBGdkngHTwpETZczlq6SiUUHy QmMyn7cA6kt1BXB6gKdE/8zMo6b4h7stP2vfU= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; b=hmISUGzFfzAXCDr5FFTTyocYqC1zClPRgfPeVginr0hJcsS/Ofbur9HMaiTMZBkT+d bgsqbwouz17risiOgdHFTXQ41ZTWmkCpUQAS+tqaTcss6rp6pH3eELPCesQlCVpAhS9y HFT2X/rkrJ3xyxU+FuPQAcmzBZpYwUzz0WvcE= Received: by 10.86.87.13 with SMTP id k13mr653824fgb.45.1239533046455; Sun, 12 Apr 2009 03:44:06 -0700 (PDT) Received: from ?192.168.1.25? (116-145.62-81.cust.bluewin.ch [81.62.145.116]) by mx.google.com with ESMTPS id e20sm5358030fga.24.2009.04.12.03.44.05 (version=SSLv3 cipher=RC4-MD5); Sun, 12 Apr 2009 03:44:06 -0700 (PDT) Message-ID: <49E1C5FA.7080808@gmail.com> Date: Sun, 12 Apr 2009 12:44:10 +0200 From: phcoder User-Agent: Thunderbird 2.0.0.21 (X11/20090318) MIME-Version: 1.0 To: The development of GRUB 2 References: <20090411.010833.154152360.davem@davemloft.net> <1239517755.3887.24.camel@mj> <20090412.010121.183222401.davem@davemloft.net> In-Reply-To: <20090412.010121.183222401.davem@davemloft.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: Re: [PATCH]: grub: Fix ofdisk disk cache corruption. X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 12 Apr 2009 10:44:13 -0000 David Miller wrote: > From: Pavel Roskin > 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 > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel -- Regards Vladimir 'phcoder' Serbinenko