grub-devel.gnu.org archive mirror
 help / color / mirror / Atom feed
* Add check when store disk cache
@ 2015-09-18  0:15 Arch Stack
  2015-09-18  4:03 ` Andrei Borzenkov
  0 siblings, 1 reply; 5+ messages in thread
From: Arch Stack @ 2015-09-18  0:15 UTC (permalink / raw)
  To: grub-devel


[-- Attachment #1.1: Type: text/plain, Size: 294 bytes --]

I found that the function *grub_disk_cache_store* didn't check for
*cache->lock* before free *cache->data*, and didn't set *cache->lock*
before memcpy something to *cache->data*. If multi thread handle with the
same cache at the same time, it will cause a fault. I have created a patch
for it.

[-- Attachment #1.2: Type: text/html, Size: 365 bytes --]

[-- Attachment #2: 0001-Add-check-when-store-disk-cache.patch --]
[-- Type: application/octet-stream, Size: 1112 bytes --]

From b31c94a60c841f61572c7f27afda1f688573f4c2 Mon Sep 17 00:00:00 2001
From: ArchStacker <ArchStacker@gmail.com>
Date: Thu, 17 Sep 2015 11:19:53 +0800
Subject: [PATCH] Add check when store disk cache

---
 grub-core/kern/disk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 789f8c0..acd0894 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -124,10 +124,12 @@ grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id,
   cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector);
   cache = grub_disk_cache_table + cache_index;
 
+  if(cache->lock)
+    return GRUB_ERR_NONE;
+
   cache->lock = 1;
   grub_free (cache->data);
   cache->data = 0;
-  cache->lock = 0;
 
   cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
   if (! cache->data)
@@ -138,6 +140,7 @@ grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id,
   cache->dev_id = dev_id;
   cache->disk_id = disk_id;
   cache->sector = sector;
+  cache->lock = 0;
 
   return GRUB_ERR_NONE;
 }
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: Add check when store disk cache
  2015-09-18  0:15 Add check when store disk cache Arch Stack
@ 2015-09-18  4:03 ` Andrei Borzenkov
  2015-09-18  9:07   ` Arch Stack
  0 siblings, 1 reply; 5+ messages in thread
From: Andrei Borzenkov @ 2015-09-18  4:03 UTC (permalink / raw)
  To: The development of GNU GRUB

18.09.2015 03:15, Arch Stack пишет:
> I found that the function *grub_disk_cache_store* didn't check for
> *cache->lock* before free *cache->data*, and didn't set *cache->lock*
> before memcpy something to *cache->data*. If multi thread handle with the
> same cache at the same time, it will cause a fault.

Do you actually observe a problem or it is pure hypothesis? GRUB does 
not run multi-threaded and probably never will.

> I have created a patch
> for it.
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Add check when store disk cache
  2015-09-18  4:03 ` Andrei Borzenkov
@ 2015-09-18  9:07   ` Arch Stack
  2015-09-19  7:00     ` Andrei Borzenkov
  0 siblings, 1 reply; 5+ messages in thread
From: Arch Stack @ 2015-09-18  9:07 UTC (permalink / raw)
  To: The development of GNU GRUB

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

I want to use the part of the filesystem codes in GRUB to read different
filesystems on Windows. I have almost completed it and I will release it in
a few days.
But it crash sometimes because of the write of zero pointer.I debug it and
find why it crashed.When I apply this patch, it won't crash because of this
reason.

On Fri, Sep 18, 2015 at 12:03 PM, Andrei Borzenkov <arvidjaar@gmail.com>
wrote:

> 18.09.2015 03:15, Arch Stack пишет:
>
>> I found that the function *grub_disk_cache_store* didn't check for
>> *cache->lock* before free *cache->data*, and didn't set *cache->lock*
>> before memcpy something to *cache->data*. If multi thread handle with the
>> same cache at the same time, it will cause a fault.
>>
>
> Do you actually observe a problem or it is pure hypothesis? GRUB does not
> run multi-threaded and probably never will.
>
> I have created a patch
>> for it.
>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2189 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Add check when store disk cache
  2015-09-18  9:07   ` Arch Stack
@ 2015-09-19  7:00     ` Andrei Borzenkov
  2015-10-13 22:55       ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 1 reply; 5+ messages in thread
From: Andrei Borzenkov @ 2015-09-19  7:00 UTC (permalink / raw)
  To: The development of GNU GRUB

18.09.2015 12:07, Arch Stack пишет:
> I want to use the part of the filesystem codes in GRUB to read different
> filesystems on Windows. I have almost completed it and I will release it in
> a few days.
> But it crash sometimes because of the write of zero pointer.I debug it and
> find why it crashed.

Please show stack trace of crash. GRUB does not run multithreaded so 
something different must happen.

> When I apply this patch, it won't crash because of this
> reason.
>
> On Fri, Sep 18, 2015 at 12:03 PM, Andrei Borzenkov <arvidjaar@gmail.com>
> wrote:
>
>> 18.09.2015 03:15, Arch Stack пишет:
>>
>>> I found that the function *grub_disk_cache_store* didn't check for
>>> *cache->lock* before free *cache->data*, and didn't set *cache->lock*
>>> before memcpy something to *cache->data*. If multi thread handle with the
>>> same cache at the same time, it will cause a fault.
>>>
>>
>> Do you actually observe a problem or it is pure hypothesis? GRUB does not
>> run multi-threaded and probably never will.
>>
>> I have created a patch
>>> for it.
>>>
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Add check when store disk cache
  2015-09-19  7:00     ` Andrei Borzenkov
@ 2015-10-13 22:55       ` Vladimir 'φ-coder/phcoder' Serbinenko
  0 siblings, 0 replies; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-13 22:55 UTC (permalink / raw)
  To: The development of GNU GRUB

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

On 19.09.2015 09:00, Andrei Borzenkov wrote:
> 18.09.2015 12:07, Arch Stack пишет:
>> I want to use the part of the filesystem codes in GRUB to read different
>> filesystems on Windows. I have almost completed it and I will release
>> it in
>> a few days.
>> But it crash sometimes because of the write of zero pointer.I debug it
>> and
>> find why it crashed.
> 
> Please show stack trace of crash. GRUB does not run multithreaded so
> something different must happen.
> 
Most likely he has just run the grub code multithreaded in his fuse-like
driver. It's probably a bad idea since GRUB is not designed for this and
in fact this lock is more for the cases when one disk code calls another
disk code. I'm not sure it actually can happen i
n current code. Moreover current way of handling ->lock isn't thread-safe
as we first check it and then set rather than using a lock primitive. If you
want to go into making parts of GRUB multi-threaded I suggest going with
giant lock
first and then breaking it down progressively. The locks should fully
disappear when compiling for real GRUB. This will probably require
preprocessor magic which makes variables disappear when GRUB_UTIL is not
defined.


>> When I apply this patch, it won't crash because of this
>> reason.
>>
>> On Fri, Sep 18, 2015 at 12:03 PM, Andrei Borzenkov <arvidjaar@gmail.com>
>> wrote:
>>
>>> 18.09.2015 03:15, Arch Stack пишет:
>>>
>>>> I found that the function *grub_disk_cache_store* didn't check for
>>>> *cache->lock* before free *cache->data*, and didn't set *cache->lock*
>>>> before memcpy something to *cache->data*. If multi thread handle
>>>> with the
>>>> same cache at the same time, it will cause a fault.
>>>>
>>>
>>> Do you actually observe a problem or it is pure hypothesis? GRUB does
>>> not
>>> run multi-threaded and probably never will.
>>>
>>> I have created a patch
>>>> for it.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-10-13 22:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-18  0:15 Add check when store disk cache Arch Stack
2015-09-18  4:03 ` Andrei Borzenkov
2015-09-18  9:07   ` Arch Stack
2015-09-19  7:00     ` Andrei Borzenkov
2015-10-13 22:55       ` Vladimir 'φ-coder/phcoder' Serbinenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).