[PATCH] biosdisk / open_device() messing up offsets

[PATCH] biosdisk / open_device() messing up offsets

[Robert Millan]

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


It seems that open_device() in biosdisk is messing up offsets when
accessing partitions.  For example if you try:

grub> hexdump (hd0,1)

in grub-emu, you'll get a message saying lseek failed.  The problem is that
it substracts to the sector offset, so all accesses get to wrong data and
accessing sector 0 results in underflow.

I would think those lines (see patch) are plainly wrong, but they appear to
be very old, and it is strange that this wasn't noticed earlier.  Maybe we
have changed behaviour around partition/disk offsets, causing this breakage
without noticing?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: partition.diff --]
[-- Type: text/x-diff, Size: 549 bytes --]

diff -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/biosdisk.c ./util/biosdisk.c
--- ../grub2/util/biosdisk.c	2008-06-04 16:00:30.000000000 +0200
+++ ./util/biosdisk.c	2008-06-05 01:17:00.000000000 +0200
@@ -298,9 +298,6 @@ open_device (const grub_disk_t disk, gru
 
     /* Make the buffer cache consistent with the physical disk.  */
     ioctl (fd, BLKFLSBUF, 0);
-    
-    if (is_partition)
-      sector -= disk->partition->start;
   }
 #else /* ! __linux__ */
   fd = open (map[disk->id].device, flags);

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Thu, 2008-06-05 at 01:35 +0200, Robert Millan wrote:
> It seems that open_device() in biosdisk is messing up offsets when
> accessing partitions.  For example if you try:
> 
> grub> hexdump (hd0,1)
> 
> in grub-emu, you'll get a message saying lseek failed.  The problem is that
> it substracts to the sector offset, so all accesses get to wrong data and
> accessing sector 0 results in underflow.
> 
> I would think those lines (see patch) are plainly wrong, but they appear to
> be very old, and it is strange that this wasn't noticed earlier.  Maybe we
> have changed behaviour around partition/disk offsets, causing this breakage
> without noticing?

The question whether sector is relative to the partition or to the whole
disk.  From what I see in disk/memdisk.c or it disk/i386/pc/biosdisk.c,
the sector is relative to the whole disk.  There is no compensation for
partition offsets.  That's low-level code that doesn't know about
partitions, and util/biosdisk.c correctly tries to emulate that.

I guess grub-emu gets it wrong somewhere.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Robert Millan]

On Wed, Jun 04, 2008 at 09:47:17PM -0400, Pavel Roskin wrote:
> On Thu, 2008-06-05 at 01:35 +0200, Robert Millan wrote:
> > It seems that open_device() in biosdisk is messing up offsets when
> > accessing partitions.  For example if you try:
> > 
> > grub> hexdump (hd0,1)
> > 
> > in grub-emu, you'll get a message saying lseek failed.  The problem is that
> > it substracts to the sector offset, so all accesses get to wrong data and
> > accessing sector 0 results in underflow.
> > 
> > I would think those lines (see patch) are plainly wrong, but they appear to
> > be very old, and it is strange that this wasn't noticed earlier.  Maybe we
> > have changed behaviour around partition/disk offsets, causing this breakage
> > without noticing?
> 
> The question whether sector is relative to the partition or to the whole
> disk.  From what I see in disk/memdisk.c or it disk/i386/pc/biosdisk.c,
> the sector is relative to the whole disk.  There is no compensation for
> partition offsets.  That's low-level code that doesn't know about
> partitions, and util/biosdisk.c correctly tries to emulate that.
> 
> I guess grub-emu gets it wrong somewhere.

grub-emu itself does nothing about this AFAIK, it just relies on
util/biosdisk.c to do the right thing.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] biosdisk / open_device() messing up offsets

[Robert Millan]

Committed.

On Thu, Jun 05, 2008 at 01:35:36AM +0200, Robert Millan wrote:
> 
> It seems that open_device() in biosdisk is messing up offsets when
> accessing partitions.  For example if you try:
> 
> grub> hexdump (hd0,1)
> 
> in grub-emu, you'll get a message saying lseek failed.  The problem is that
> it substracts to the sector offset, so all accesses get to wrong data and
> accessing sector 0 results in underflow.
> 
> I would think those lines (see patch) are plainly wrong, but they appear to
> be very old, and it is strange that this wasn't noticed earlier.  Maybe we
> have changed behaviour around partition/disk offsets, causing this breakage
> without noticing?
> 
> -- 
> Robert Millan
> 
> <GPLv2> I know my rights; I want my phone call!
> <DRM> What good is a phone call… if you are unable to speak?
> (as seen on /.)

> diff -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/biosdisk.c ./util/biosdisk.c
> --- ../grub2/util/biosdisk.c	2008-06-04 16:00:30.000000000 +0200
> +++ ./util/biosdisk.c	2008-06-05 01:17:00.000000000 +0200
> @@ -298,9 +298,6 @@ open_device (const grub_disk_t disk, gru
>  
>      /* Make the buffer cache consistent with the physical disk.  */
>      ioctl (fd, BLKFLSBUF, 0);
> -    
> -    if (is_partition)
> -      sector -= disk->partition->start;
>    }
>  #else /* ! __linux__ */
>    fd = open (map[disk->id].device, flags);

> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Fri, 2008-06-06 at 17:56 +0200, Robert Millan wrote:
> Committed.

I believe it's wrong.  util/biosdisk.c is a low level disk driver.  It's
supposed to read data relative to the disk, like other drivers do.

Besides, is_partition becomes write-only.  It means that the result of
linux_find_partition() is ignored.  Please note that
linux_find_partition() modifies its first argument on success.  It
rewrites the disk device (e.g. /dev/sda) with the partition device
(e.g. /dev/sda1).

If the result of linux_find_partition() is not checked, nobody knows
whether fd is a handle to the partition or to the whole disk.  Yet the
sector variable is used to seek on that device.  Needless to say that
different values would be obtained.

Also, grub-emu is not the only user of util/biosdisk.c.  It is also used
by grub-setup, which is working correctly,  as far as I can tell.

By the way, the current GRUB just failed to install on my test system.
After I rebooted it, GRUB hung on startup after showing "GRUB".

It may be something else.  It was an unclean reboot after a panic for
unrelated reasons, so the hanging problem may be an issue with ext3.
Anyway, extreme caution is required with the current code!

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Robert Millan]

On Fri, Jun 06, 2008 at 06:53:52PM -0400, Pavel Roskin wrote:
> On Fri, 2008-06-06 at 17:56 +0200, Robert Millan wrote:
> > Committed.
> 
> I believe it's wrong.

You mean that the lines I removed were wrong, or that my commit is wrong?

I thought you agreed that those lines were bogus.  If not, then we'd have
to figure out why sector 0 of the partition can be accessed directly via
hexdump.

> By the way, the current GRUB just failed to install on my test system.
> After I rebooted it, GRUB hung on startup after showing "GRUB".

Please run a regression test, if you can.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> On Fri, Jun 06, 2008 at 06:53:52PM -0400, Pavel Roskin wrote:
>> On Fri, 2008-06-06 at 17:56 +0200, Robert Millan wrote:
>> > Committed.
>>
>> I believe it's wrong.
>
> You mean that the lines I removed were wrong, or that my commit is wrong?

The commit is wrong.

> I thought you agreed that those lines were bogus.

No, I explained why they were needed.  Sorry, I must have been unclear.

> If not, then we'd have
> to figure out why sector 0 of the partition can be accessed directly via
> hexdump.

Absolutely.

>> By the way, the current GRUB just failed to install on my test system.
>> After I rebooted it, GRUB hung on startup after showing "GRUB".
>
> Please run a regression test, if you can.

I could boot from the rescue CD, so it's not that bad.

I'm looking at the problem now.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> If not, then we'd have
> to figure out why sector 0 of the partition can be accessed directly via
> hexdump.

As I understand, "(hd0,1)" doesn't refer to an extent of data, i.e. it  
not a file or a block list that can be opened like a file.   
"(hd0,1)+1" is the first sector of the partition.  But "(hd0,1)" is  
not all sectors of the partition.  It the partition name.  It can be  
used in the "ls" command, whereas "(hd0,1)+1" cannot.

>> By the way, the current GRUB just failed to install on my test system.
>> After I rebooted it, GRUB hung on startup after showing "GRUB".
>
> Please run a regression test, if you can.

The installation was broken by that patch, so I reverted it.  It's  
easy to check the installation by doing it on a mounted flash drive,  
so that the real boot device is not at risk.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Sat, Jun 7, 2008 at 2:33 PM, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Robert Millan <rmh@aybabtu.com>:
>
>> If not, then we'd have
>> to figure out why sector 0 of the partition can be accessed directly via
>> hexdump.
>
> As I understand, "(hd0,1)" doesn't refer to an extent of data, i.e. it not a
> file or a block list that can be opened like a file.  "(hd0,1)+1" is the
> first sector of the partition.  But "(hd0,1)" is not all sectors of the
> partition.  It the partition name.  It can be used in the "ls" command,
> whereas "(hd0,1)+1" cannot.
>
>>> By the way, the current GRUB just failed to install on my test system.
>>> After I rebooted it, GRUB hung on startup after showing "GRUB".
>>
>> Please run a regression test, if you can.
>
> The installation was broken by that patch, so I reverted it.  It's easy to
> check the installation by doing it on a mounted flash drive, so that the
> real boot device is not at risk.

Hi,

I believe the problem is with hexdump. It open the device with
grub_disk_open, which returns the disk object related to the beginning
of partition. However, it read it using disk->dev->read, which is a
low level api that use absolute address. It should be using
grub_disk_read instead.

BTW, I also find an ingesting situation when writing the load_env
command. The read hook return absolute address, so grub_disk_read
would not work. In that case, we have to use the low level
disk->dev->read.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Sat, Jun 7, 2008 at 6:53 AM, Pavel Roskin <proski@gnu.org> wrote:
>
> It may be something else.  It was an unclean reboot after a panic for
> unrelated reasons, so the hanging problem may be an issue with ext3.
> Anyway, extreme caution is required with the current code!

btw, any chance your cpu is big endian ? The code might have portable
issue in big endian machine.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Sat, 2008-06-07 at 16:37 +0800, Bean wrote:
> On Sat, Jun 7, 2008 at 6:53 AM, Pavel Roskin <proski@gnu.org> wrote:
> >
> > It may be something else.  It was an unclean reboot after a panic for
> > unrelated reasons, so the hanging problem may be an issue with ext3.
> > Anyway, extreme caution is required with the current code!
> 
> btw, any chance your cpu is big endian ? The code might have portable
> issue in big endian machine.

The test machine is i386.  The problems I'm seeing in qemu are observed
on x86_64.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Sat, 2008-06-07 at 15:48 +0800, Bean wrote:

> I believe the problem is with hexdump. It open the device with
> grub_disk_open, which returns the disk object related to the beginning
> of partition. However, it read it using disk->dev->read, which is a
> low level api that use absolute address. It should be using
> grub_disk_read instead.

Nice catch!  Indeed, hexdump has special handling for the whole
partitions.  And it actually tries to satisfy the low level API by
converting offset to the sector number and skipping the remainder.  I
guess it could be simplified if grub_disk_read() is used.

> BTW, I also find an ingesting situation when writing the load_env
> command. The read hook return absolute address, so grub_disk_read
> would not work. In that case, we have to use the low level
> disk->dev->read.

In that case it may be justified.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Sun, Jun 8, 2008 at 2:00 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Sat, 2008-06-07 at 15:48 +0800, Bean wrote:
>
>> I believe the problem is with hexdump. It open the device with
>> grub_disk_open, which returns the disk object related to the beginning
>> of partition. However, it read it using disk->dev->read, which is a
>> low level api that use absolute address. It should be using
>> grub_disk_read instead.
>
> Nice catch!  Indeed, hexdump has special handling for the whole
> partitions.  And it actually tries to satisfy the low level API by
> converting offset to the sector number and skipping the remainder.  I
> guess it could be simplified if grub_disk_read() is used.

The reason I add device support for hexdump is to debug the nand
driver. I need to go around the disk cache and call the underlying
disk driver directly, so I use disk->dev->read. For (nand), there is
just one partition, so I didn't notice the problem then.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Sun, 2008-06-08 at 14:44 +0800, Bean wrote:
> On Sun, Jun 8, 2008 at 2:00 PM, Pavel Roskin <proski@gnu.org> wrote:
> > On Sat, 2008-06-07 at 15:48 +0800, Bean wrote:
> >
> >> I believe the problem is with hexdump. It open the device with
> >> grub_disk_open, which returns the disk object related to the beginning
> >> of partition. However, it read it using disk->dev->read, which is a
> >> low level api that use absolute address. It should be using
> >> grub_disk_read instead.
> >
> > Nice catch!  Indeed, hexdump has special handling for the whole
> > partitions.  And it actually tries to satisfy the low level API by
> > converting offset to the sector number and skipping the remainder.  I
> > guess it could be simplified if grub_disk_read() is used.
> 
> The reason I add device support for hexdump is to debug the nand
> driver. I need to go around the disk cache and call the underlying
> disk driver directly, so I use disk->dev->read. For (nand), there is
> just one partition, so I didn't notice the problem then.

Here's the patch.  Everything seems to be OK.  "--skip=N" is not
recognized, but it's something in the option parsing code.  "-s N" is
working.

Please feel free to apply.

diff --git a/commands/hexdump.c b/commands/hexdump.c
index 6d97fe4..e459b88 100644
--- a/commands/hexdump.c
+++ b/commands/hexdump.c
@@ -99,35 +99,27 @@ grub_cmd_hexdump (struct grub_arg_list *state, int argc, char **args)
   else if ((args[0][0] == '(') && (args[0][namelen - 1] == ')'))
     {
       grub_disk_t disk;
-      grub_disk_addr_t sector;
-      grub_size_t ofs;
 
       args[0][namelen - 1] = 0;
       disk = grub_disk_open (&args[0][1]);
       if (! disk)
         return 0;
 
-      sector = (skip >> (GRUB_DISK_SECTOR_BITS + 2)) * 4;
-      ofs = skip & (GRUB_DISK_SECTOR_SIZE * 4 - 1);
       while (length)
         {
-          grub_size_t len, n;
+          grub_size_t len;
 
           len = length;
-          if (ofs + len > sizeof (buf))
-            len = sizeof (buf) - ofs;
+          if (len > sizeof (buf))
+            len = sizeof (buf);
 
-          n = ((ofs + len + GRUB_DISK_SECTOR_SIZE - 1)
-               >> GRUB_DISK_SECTOR_BITS);
-          if (disk->dev->read (disk, sector, n, buf))
+          if (grub_disk_read (disk, 0, skip, len, buf))
             break;
 
-          hexdump (skip, &buf[ofs], len);
+          hexdump (skip, buf, len);
 
-          ofs = 0;
           skip += len;
           length -= len;
-          sector += 4;
         }
 
       grub_disk_close (disk);


-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

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

On Sun, Jun 8, 2008 at 2:52 PM, Pavel Roskin <proski@gnu.org> wrote:
>>
>> The reason I add device support for hexdump is to debug the nand
>> driver. I need to go around the disk cache and call the underlying
>> disk driver directly, so I use disk->dev->read. For (nand), there is
>> just one partition, so I didn't notice the problem then.
>
> Here's the patch.  Everything seems to be OK.  "--skip=N" is not
> recognized, but it's something in the option parsing code.  "-s N" is
> working.
>
> Please feel free to apply.

Perhaps we should keep the low level api, just fix the offset. Hexdump
is normally used for debugging, and the disk cache is quite annoying.

btw, please try the patch, it dumps the journal information. There
could be quite some output, you can use grub-fstest to capture it.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e1.diff --]
[-- Type: text/x-diff; name=e1.diff, Size: 2729 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..4e76097 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -443,14 +443,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
   log->last_block = grub_be_to_cpu32 (jsb->maxlen);
   log->start_block = grub_be_to_cpu32 (jsb->start);
 
+  grub_printf ("header: %d %d %d\n", log->first_block, log->last_block, log->start_block);
+
   last_num = num = 0;
   block = log->start_block;
   seq = grub_be_to_cpu32 (jsb->sequence);
+  grub_printf ("seq: %d\n", seq);
 
   while (1)
     {
       struct grub_ext3_journal_header *jh;
 
+      grub_printf ("block: %d\n", block);
+
       if (grub_fshelp_read_file (data->disk, &data->logfile, 0,
                                  block << (log2bs + 9), sizeof (buf),
                                  buf, grub_ext2_read_block,
@@ -476,6 +481,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
             struct grub_ext3_journal_block_tag *tag;
             int ofs, flags;
 
+            grub_printf ("dblock: ");
             ofs = sizeof (struct grub_ext3_journal_header);
 
             do
@@ -491,15 +497,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
                   ofs += 16;
 
                 log->mapping[num++] = grub_be_to_cpu32 (tag->block);
+                grub_printf ("%d ", grub_be_to_cpu32 (tag->block));
                 next_block();
               }
             while (! (flags & EXT3_JOURNAL_FLAG_LAST_TAG));
 
+            grub_printf ("\n");
+
             continue;
           }
 
         case EXT3_JOURNAL_COMMIT_BLOCK:
           {
+            grub_printf ("cblock: %d\n", seq);
             seq++;
             last_num = num - 1;
             continue;
@@ -510,6 +520,8 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
             struct grub_ext3_journal_revoke_header *jrh;
             grub_uint32_t i;
 
+            grub_printf ("rblock: %d ", jrh->count);
+
             jrh = (struct grub_ext3_journal_revoke_header *) jh;
 
             for (i = 0; i < grub_be_to_cpu32 (jrh->count); i++)
@@ -518,11 +530,12 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
                 grub_uint32_t map;
 
                 map = grub_be_to_cpu32 (jrh->data[i]);
+                grub_printf ("%d ", map);
                 for (j = 0; j < num; j++)
                   if (log->mapping[j] == map)
                     log->mapping[j] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
               }
-
+            grub_printf ("\n");
             continue;
           }
         default:
@@ -538,6 +551,7 @@ quit:
     {
       int size;
 
+      grub_printf ("num: %d\n", last_num);
       size = sizeof (struct grub_fshelp_journal) +
             last_num * sizeof (grub_disk_addr_t);
 

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Sun, 2008-06-08 at 15:11 +0800, Bean wrote:
> On Sun, Jun 8, 2008 at 2:52 PM, Pavel Roskin <proski@gnu.org> wrote:
> >>
> >> The reason I add device support for hexdump is to debug the nand
> >> driver. I need to go around the disk cache and call the underlying
> >> disk driver directly, so I use disk->dev->read. For (nand), there is
> >> just one partition, so I didn't notice the problem then.
> >
> > Here's the patch.  Everything seems to be OK.  "--skip=N" is not
> > recognized, but it's something in the option parsing code.  "-s N" is
> > working.
> >
> > Please feel free to apply.
> 
> Perhaps we should keep the low level api, just fix the offset. Hexdump
> is normally used for debugging, and the disk cache is quite annoying.

OK, it's your choice.

> btw, please try the patch, it dumps the journal information. There
> could be quite some output, you can use grub-fstest to capture it.

It breaks installation:

# grub-install /dev/sda
grub-mkimage: error: cannot stat /usr/local/lib/grub/i386-pc/header:.mod

"header:" must be from the patch.

I cannot generate a short image easily because loopback devices are not
supported.  Perhaps I'll try making a real partition initialized with
zeroes or something like that.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Sun, Jun 8, 2008 at 3:29 PM, Pavel Roskin <proski@gnu.org> wrote:
>> btw, please try the patch, it dumps the journal information. There
>> could be quite some output, you can use grub-fstest to capture it.
>
> It breaks installation:
>
> # grub-install /dev/sda
> grub-mkimage: error: cannot stat /usr/local/lib/grub/i386-pc/header:.mod
>
> "header:" must be from the patch.
>
> I cannot generate a short image easily because loopback devices are not
> supported.  Perhaps I'll try making a real partition initialized with
> zeroes or something like that.

Can you use grub-fstest ?

Also, the header is from

+  grub_printf ("header: %d %d %d\n", log->first_block,
log->last_block, log->start_block);

I don't know why the number doesn't show, unless log is invalid
pointer. Perhaps the size is invalid, try to add a debug line to show
the allocated size of log:

  log = grub_malloc (sizeof (struct grub_fshelp_journal) +
                     grub_be_to_cpu32 (jsb->maxlen) * sizeof
(grub_disk_addr_t));




-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

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

On Sun, 2008-06-08 at 19:49 +0800, Bean wrote:

> Can you use grub-fstest ?
> 
> Also, the header is from
> 
> +  grub_printf ("header: %d %d %d\n", log->first_block,
> log->last_block, log->start_block);
> 
> I don't know why the number doesn't show, unless log is invalid
> pointer. Perhaps the size is invalid, try to add a debug line to show
> the allocated size of log:
> 
>   log = grub_malloc (sizeof (struct grub_fshelp_journal) +
>                      grub_be_to_cpu32 (jsb->maxlen) * sizeof
> (grub_disk_addr_t));

The debug output must be getting split somewhere.  If I replace colons
and spaces with underscores, I get some numbers.  Anyway, the
installation with the patch is unnecessary.

I made a short partition and installed grub on it, but there are no
problems with it in qemu.

But I can reproduce the problem with grub-fstest on the root partition.

[root@dv grub2]# grub-fstest /dev/sda1 ls /    
lost+found/ bin/ boot/ data/ dev/ etc/ home/ lib/ lib64/ media/ mnt/
opt/ proc/ root/ sbin/ scratchbox/ selinux/ srv/ sys/ tftpboot/ tmp/
usr/ var/ debug/ tftpboot;4834754b 
[root@dv grub2]# grub-fstest /dev/sda1 ls /boot

[root@dv grub2]# ls /   
bin   debug  home   lost+found  opt   sbin        srv
tftpboot;4834754b  var
boot  dev    lib    media       proc  scratchbox  sys       tmp
data  etc    lib64  mnt         root  selinux     tftpboot  usr
[root@dv grub2]# ls /boot
System.map                    grub                      vmlinuz
System.map-2.6.26-rc4-wl      initrd-2.6.26-rc4-wl.img
vmlinuz-2.6.26-rc4-wl
System.map-2.6.26-rc4-wl.old  memtest86+-2.01
vmlinuz-2.6.26-rc4-wl.old
[root@dv grub2]#

As you can see, "grub-fstest /dev/sda1 ls /boot" fails to find any
files.  Then I patched grub with your patch.  The output of the same
command is attached (compressed).

-- 
Regards,
Pavel Roskin

[-- Attachment #2: Log-boot.bz2 --]
[-- Type: application/x-bzip, Size: 14355 bytes --]

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Mon, Jun 9, 2008 at 2:42 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Sun, 2008-06-08 at 19:49 +0800, Bean wrote:
>
>> Can you use grub-fstest ?
>>
>> Also, the header is from
>>
>> +  grub_printf ("header: %d %d %d\n", log->first_block,
>> log->last_block, log->start_block);
>>
>> I don't know why the number doesn't show, unless log is invalid
>> pointer. Perhaps the size is invalid, try to add a debug line to show
>> the allocated size of log:
>>
>>   log = grub_malloc (sizeof (struct grub_fshelp_journal) +
>>                      grub_be_to_cpu32 (jsb->maxlen) * sizeof
>> (grub_disk_addr_t));
>
> The debug output must be getting split somewhere.  If I replace colons
> and spaces with underscores, I get some numbers.  Anyway, the
> installation with the patch is unnecessary.
>
> I made a short partition and installed grub on it, but there are no
> problems with it in qemu.
>
> But I can reproduce the problem with grub-fstest on the root partition.
>
> [root@dv grub2]# grub-fstest /dev/sda1 ls /
> lost+found/ bin/ boot/ data/ dev/ etc/ home/ lib/ lib64/ media/ mnt/
> opt/ proc/ root/ sbin/ scratchbox/ selinux/ srv/ sys/ tftpboot/ tmp/
> usr/ var/ debug/ tftpboot;4834754b
> [root@dv grub2]# grub-fstest /dev/sda1 ls /boot
>
> [root@dv grub2]# ls /
> bin   debug  home   lost+found  opt   sbin        srv
> tftpboot;4834754b  var
> boot  dev    lib    media       proc  scratchbox  sys       tmp
> data  etc    lib64  mnt         root  selinux     tftpboot  usr
> [root@dv grub2]# ls /boot
> System.map                    grub                      vmlinuz
> System.map-2.6.26-rc4-wl      initrd-2.6.26-rc4-wl.img
> vmlinuz-2.6.26-rc4-wl
> System.map-2.6.26-rc4-wl.old  memtest86+-2.01
> vmlinuz-2.6.26-rc4-wl.old
> [root@dv grub2]#
>
> As you can see, "grub-fstest /dev/sda1 ls /boot" fails to find any
> files.  Then I patched grub with your patch.  The output of the same
> command is attached (compressed).

Hi,

Thanks, can you also give me the output of dumpe2fs ?

dumpe2fs /dev/sda1

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Robert Millan]

On Fri, Jun 06, 2008 at 10:58:43PM -0400, Pavel Roskin wrote:
> Quoting Robert Millan <rmh@aybabtu.com>:
> 
> >On Fri, Jun 06, 2008 at 06:53:52PM -0400, Pavel Roskin wrote:
> >>On Fri, 2008-06-06 at 17:56 +0200, Robert Millan wrote:
> >>> Committed.
> >>
> >>I believe it's wrong.
> >
> >You mean that the lines I removed were wrong, or that my commit is wrong?
> 
> The commit is wrong.

Woops, sorry for the confusion then.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Mon, 2008-06-09 at 02:57 +0800, Bean wrote:

> Thanks, can you also give me the output of dumpe2fs ?
> 
> dumpe2fs /dev/sda1

I'll send it privately, as it won't compress much.

I was trying to find out which change breaks the ext3 support.  It turns
out grub-fstest starts working if I comment out this line in fs/ext2.c:

log->mapping[num++] = grub_be_to_cpu32 (tag->block);

Alternatively, I could replace it with

log->mapping[num++] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;

and grub-fstest would list /boot properly.  I added some debug prints,
to see which states it goes through, and here's the end of the output:

descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
revoke
descriptor
commit
num = 15628, last_num = 15627
grub/ System.map vmlinuz-2.6.26-rc4-wl.old vmlinuz
System.map-2.6.26-rc4-wl.old vmlinuz-2.6.26-rc4-wl
System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img memtest86+-2.01 

The number of the blocks is quite large, but we are never hitting the
"default" case.  "revoke" wasn't present in subsequent runs.  The
numbers (num and last_num) keep growing every time by about 20.

I also tried to see what happens on the fshelp.c side.  So I applied
this patch:

diff --git a/fs/fshelp.c b/fs/fshelp.c
index faec1f7..027b579 100644
--- a/fs/fshelp.c
+++ b/fs/fshelp.c
@@ -329,12 +329,18 @@ grub_fshelp_map_block (grub_fshelp_journal_t log, grub_disk_addr_t block)
   if (map_block < 0)
     return block;
 
+  grub_printf("map_block = %ld, start_block = %ld, first_block = %ld, last_block = %d\n",
+	      map_block, log->start_block, log->first_block, log->last_block);
   map_block += log->start_block;
   if (map_block >= log->last_block)
     map_block -= log->last_block - log->first_block;
 
   if (log->type == GRUB_FSHELP_JOURNAL_TYPE_BLOCK)
     return log->blkno + map_block;
-  else
-    return log->get_block (log->node, map_block);
+  else {
+    grub_disk_addr_t ret;
+    ret = log->get_block (log->node, map_block);
+    grub_printf("map_block = %ld, block = %ld, ret = %ld\n", map_block, block, ret);
+    return ret;
+  }
 }

That's what I get:

# ./grub-fstest /dev/sda1 ls /boot
map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9345, block = 1, ret = 10901
map_block = 18780, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9137, block = 1027, ret = 10692
map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9345, block = 1, ret = 10901
map_block = 18780, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9137, block = 1027, ret = 10692
map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9345, block = 1, ret = 10901

But if I change "return ret;" to "return block;" to ignore the lookup, I
get this:

# ./grub-fstest /dev/sda1 ls /boot
map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9730, block = 1, ret = 11286
map_block = 19209, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9566, block = 1027, ret = 11122
map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9730, block = 1, ret = 11286
map_block = 19209, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9566, block = 1027, ret = 11122
map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9730, block = 1, ret = 11286
map_block = 12763, start_block = 23124, first_block = 1, last_block = 32768
map_block = 3120, block = 15237122, ret = 4670
grub/ System.map vmlinuz-2.6.26-rc4-wl.old vmlinuz
System.map-2.6.26-rc4-wl.old vmlinuz-2.6.26-rc4-wl
System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img memtest86+-2.01 

One thing that surprises me is that the value we are supposed to return
is always below 12000.

Then I created file /boot/foo, ran sync several times, and that's what I
get if "block" is returned:

# ./grub-fstest /dev/sda1 ls /boot
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351
map_block = 19630, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9987, block = 1027, ret = 11543
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351
map_block = 19630, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9987, block = 1027, ret = 11543
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351
map_block = 19627, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9984, block = 15237122, ret = 11540
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
grub/ map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
System.map map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
vmlinuz-2.6.26-rc4-wl.old map_block = 19628, start_block = 23124, first_block = 1, last_block = 
32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
vmlinuz map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
System.map-2.6.26-rc4-wl.old map_block = 19628, start_block = 23124, first_block = 1, last_block
 = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
vmlinuz-2.6.26-rc4-wl map_block = 19628, start_block = 23124, first_block = 1, last_block = 3276
8
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
System.map-2.6.26-rc4-wl map_block = 19628, start_block = 23124, first_block = 1, last_block = 3
2768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
foo map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
initrd-2.6.26-rc4-wl.img map_block = 19628, start_block = 23124, first_block = 1, last_block = 3
2768
map_block = 9985, block = 15239168, ret = 11541
map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9985, block = 15239168, ret = 11541
memtest86+-2.01 

But I get this if "ret" is returned:

# ./grub-fstest /dev/sda1 ls /boot
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351
map_block = 19656, start_block = 23124, first_block = 1, last_block = 32768
map_block = 10013, block = 1027, ret = 11569
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351
map_block = 19656, start_block = 23124, first_block = 1, last_block = 32768
map_block = 10013, block = 1027, ret = 11569
map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768
map_block = 9795, block = 1, ret = 11351

"ret" is still in the same region.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

Hi,

It's a little strange, the journal seems normal, I wonder where does
it go wrong.

btw, can you use debugfs to gather more information. You can start it with

debugfs /dev/sda1

Then type in the command

logdump -a output_file

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

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

Hi,

Ok, I fix some bugs, please see if the new patch works.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e2.diff --]
[-- Type: text/x-diff; name=e2.diff, Size: 3425 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..4d1e3e1 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -443,20 +443,25 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
   log->last_block = grub_be_to_cpu32 (jsb->maxlen);
   log->start_block = grub_be_to_cpu32 (jsb->start);
 
+  grub_printf ("header: %d %d %d\n", log->first_block, log->last_block, log->start_block);
+
   last_num = num = 0;
   block = log->start_block;
   seq = grub_be_to_cpu32 (jsb->sequence);
+  grub_printf ("seq: %d\n", seq);
 
   while (1)
     {
       struct grub_ext3_journal_header *jh;
 
-      if (grub_fshelp_read_file (data->disk, &data->logfile, 0,
-                                 block << (log2bs + 9), sizeof (buf),
-                                 buf, grub_ext2_read_block,
-                                 log->last_block << (log2bs + 9),
-                                 log2bs) !=
-          (int) sizeof (buf))
+      grub_printf ("block: %d\n", block);
+
+      grub_fshelp_read_file (data->disk, &data->logfile, 0,
+                             block << (log2bs + 9), sizeof (buf),
+                             buf, grub_ext2_read_block,
+                             log->last_block << (log2bs + 9),
+                             log2bs);
+      if (grub_errno)
         break;
 
       jh = (struct grub_ext3_journal_header *) &buf[0];
@@ -476,6 +481,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
             struct grub_ext3_journal_block_tag *tag;
             int ofs, flags;
 
+            grub_printf ("dblock: ");
             ofs = sizeof (struct grub_ext3_journal_header);
 
             do
@@ -491,15 +497,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
                   ofs += 16;
 
                 log->mapping[num++] = grub_be_to_cpu32 (tag->block);
+                grub_printf ("%d ", grub_be_to_cpu32 (tag->block));
                 next_block();
               }
             while (! (flags & EXT3_JOURNAL_FLAG_LAST_TAG));
 
+            grub_printf ("\n");
+
             continue;
           }
 
         case EXT3_JOURNAL_COMMIT_BLOCK:
           {
+            grub_printf ("cblock: %d\n", seq);
             seq++;
             last_num = num - 1;
             continue;
@@ -508,21 +518,24 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
         case EXT3_JOURNAL_REVOKE_BLOCK:
           {
             struct grub_ext3_journal_revoke_header *jrh;
-            grub_uint32_t i;
+            grub_uint32_t i, cnt;
 
             jrh = (struct grub_ext3_journal_revoke_header *) jh;
+            cnt = (grub_be_to_cpu32 (jrh->count) -
+                   sizeof (struct grub_ext3_journal_revoke_header)) >> 2;
 
-            for (i = 0; i < grub_be_to_cpu32 (jrh->count); i++)
+            for (i = 0; i < cnt; i++)
               {
                 int j;
                 grub_uint32_t map;
 
                 map = grub_be_to_cpu32 (jrh->data[i]);
+                grub_printf ("%d ", map);
                 for (j = 0; j < num; j++)
                   if (log->mapping[j] == map)
                     log->mapping[j] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
               }
-
+            grub_printf ("\n");
             continue;
           }
         default:
@@ -538,6 +551,7 @@ quit:
     {
       int size;
 
+      grub_printf ("num: %d\n", last_num);
       size = sizeof (struct grub_fshelp_journal) +
             last_num * sizeof (grub_disk_addr_t);
 

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Tue, 2008-06-10 at 15:16 +0800, Bean wrote:
> Hi,
> 
> Ok, I fix some bugs, please see if the new patch works.

It doesn't work.  i sent the logs privately.  I also checked the logs
and it looks like that GRUB is reading the log properly.  Then I guess
we are not using the log correctly.

I changed fs/ext2.c to see how the results would differ if
grub_fshelp_map_block() is bypassed:

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..7b4bfd6 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -257,12 +257,24 @@ inline static grub_err_t
 grub_ext2_blockgroup (struct grub_ext2_data *data, int group, 
 		      struct grub_ext2_block_group *blkgrp)
 {
-  return grub_disk_read (data->disk,
-			 (grub_fshelp_map_block (data->journal,
-                                                 grub_le_to_cpu32 (data->sblock.first_data_block) + 1)
-			  << LOG2_EXT2_BLOCK_SIZE (data)),
+  grub_err_t ret;
+  int block;
+
+  block = grub_le_to_cpu32 (data->sblock.first_data_block) + 1;
+  ret = grub_disk_read (data->disk,
+			 block << LOG2_EXT2_BLOCK_SIZE (data),
+			 group * sizeof (struct grub_ext2_block_group), 
+			 sizeof (struct grub_ext2_block_group), (char *) blkgrp);
+  grub_printf("direct:  ret = %d, block = %d, inode_table_id = %d\n", ret, block,
+	      blkgrp->inode_table_id);
+  block = grub_fshelp_map_block(data->journal, block);
+  ret = grub_disk_read (data->disk,
+			 block << LOG2_EXT2_BLOCK_SIZE (data),
 			 group * sizeof (struct grub_ext2_block_group), 
 			 sizeof (struct grub_ext2_block_group), (char *) blkgrp);
+  grub_printf("journal: ret = %d, block = %d, inode_table_id = %d\n", ret, block,
+	      blkgrp->inode_table_id);
+  return ret;
 }
 
 


That's the output:

direct:  ret = 0, block = 1, inode_table_id = 1027
journal: ret = 0, block = 1, inode_table_id = 1027
direct:  ret = 0, block = 1, inode_table_id = 1027
journal: ret = 0, block = 17707, inode_table_id = 1027
direct:  ret = 0, block = 1, inode_table_id = 1027
journal: ret = 0, block = 1, inode_table_id = 1027
direct:  ret = 0, block = 1, inode_table_id = 1027
journal: ret = 0, block = 17707, inode_table_id = 1027
direct:  ret = 0, block = 1, inode_table_id = 15237122
journal: ret = 0, block = 17707, inode_table_id = 6895416


So, originally we didn't have the journal opened, so the mapping was
trivial, then 1 became mapped to 17707.  But at some point the data in
that block became different from what it used to be and different in the
journaled and non-journaled blocks.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

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

Hi,

I spot another bug, please try the new patch.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e3.diff --]
[-- Type: text/x-diff; name=e3.diff, Size: 4328 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..093ce7b 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -257,12 +257,18 @@ inline static grub_err_t
 grub_ext2_blockgroup (struct grub_ext2_data *data, int group, 
 		      struct grub_ext2_block_group *blkgrp)
 {
+  int blkno, blkoff;
+
+  group *= sizeof (struct grub_ext2_block_group);
+  blkno = group >> LOG2_EXT2_BLOCK_SIZE (data);
+  blkoff = group  - (blkno << LOG2_EXT2_BLOCK_SIZE (data));
+
   return grub_disk_read (data->disk,
 			 (grub_fshelp_map_block (data->journal,
-                                                 grub_le_to_cpu32 (data->sblock.first_data_block) + 1)
+                                                 grub_le_to_cpu32 (data->sblock.first_data_block) + 1 + blkno)
 			  << LOG2_EXT2_BLOCK_SIZE (data)),
-			 group * sizeof (struct grub_ext2_block_group), 
-			 sizeof (struct grub_ext2_block_group), (char *) blkgrp);
+			 blkoff,
+                         sizeof (struct grub_ext2_block_group), (char *) blkgrp);
 }
 
 
@@ -443,20 +449,25 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
   log->last_block = grub_be_to_cpu32 (jsb->maxlen);
   log->start_block = grub_be_to_cpu32 (jsb->start);
 
+  grub_printf ("header: %d %d %d\n", log->first_block, log->last_block, log->start_block);
+
   last_num = num = 0;
   block = log->start_block;
   seq = grub_be_to_cpu32 (jsb->sequence);
+  grub_printf ("seq: %d\n", seq);
 
   while (1)
     {
       struct grub_ext3_journal_header *jh;
 
-      if (grub_fshelp_read_file (data->disk, &data->logfile, 0,
-                                 block << (log2bs + 9), sizeof (buf),
-                                 buf, grub_ext2_read_block,
-                                 log->last_block << (log2bs + 9),
-                                 log2bs) !=
-          (int) sizeof (buf))
+      grub_printf ("block: %d\n", block);
+
+      grub_fshelp_read_file (data->disk, &data->logfile, 0,
+                             block << (log2bs + 9), sizeof (buf),
+                             buf, grub_ext2_read_block,
+                             log->last_block << (log2bs + 9),
+                             log2bs);
+      if (grub_errno)
         break;
 
       jh = (struct grub_ext3_journal_header *) &buf[0];
@@ -476,6 +487,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
             struct grub_ext3_journal_block_tag *tag;
             int ofs, flags;
 
+            grub_printf ("dblock: ");
             ofs = sizeof (struct grub_ext3_journal_header);
 
             do
@@ -491,15 +503,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
                   ofs += 16;
 
                 log->mapping[num++] = grub_be_to_cpu32 (tag->block);
+                grub_printf ("%d ", grub_be_to_cpu32 (tag->block));
                 next_block();
               }
             while (! (flags & EXT3_JOURNAL_FLAG_LAST_TAG));
 
+            grub_printf ("\n");
+
             continue;
           }
 
         case EXT3_JOURNAL_COMMIT_BLOCK:
           {
+            grub_printf ("cblock: %d\n", seq);
             seq++;
             last_num = num - 1;
             continue;
@@ -508,21 +524,24 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
         case EXT3_JOURNAL_REVOKE_BLOCK:
           {
             struct grub_ext3_journal_revoke_header *jrh;
-            grub_uint32_t i;
+            grub_uint32_t i, cnt;
 
             jrh = (struct grub_ext3_journal_revoke_header *) jh;
+            cnt = (grub_be_to_cpu32 (jrh->count) -
+                   sizeof (struct grub_ext3_journal_revoke_header)) >> 2;
 
-            for (i = 0; i < grub_be_to_cpu32 (jrh->count); i++)
+            for (i = 0; i < cnt; i++)
               {
                 int j;
                 grub_uint32_t map;
 
                 map = grub_be_to_cpu32 (jrh->data[i]);
+                grub_printf ("%d ", map);
                 for (j = 0; j < num; j++)
                   if (log->mapping[j] == map)
                     log->mapping[j] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
               }
-
+            grub_printf ("\n");
             continue;
           }
         default:
@@ -538,6 +557,7 @@ quit:
     {
       int size;
 
+      grub_printf ("num: %d\n", last_num);
       size = sizeof (struct grub_fshelp_journal) +
             last_num * sizeof (grub_disk_addr_t);
 

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

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

On Wed, 2008-06-11 at 02:51 +0800, Bean wrote:
> Hi,
> 
> I spot another bug, please try the new patch.

That didn't work.  Moreover, grub-fstest failed to read /boot on
read-only filesystem.

Thanks to remounting the filesystem and using lzma, I could reduce the
compressed logs to a reasonable size, so I'm posting them here together
with the script that produced them.  lzma should be available in all
modern distros.

-- 
Regards,
Pavel Roskin

[-- Attachment #2: grub-test.tar.lzma --]
[-- Type: application/x-lzma-compressed-tar, Size: 96478 bytes --]

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Wed, 2008-06-11 at 02:51 +0800, Bean wrote: 
> Hi,
> 
> I spot another bug, please try the new patch.

You probably meant to use (LOG2_EXT2_BLOCK_SIZE (data) + 9) in your
patch because the block size is in 512 bytes units, unlike sizeof:

diff --git a/fs/ext2.c b/fs/ext2.c
index e4ce47b..fb13a80 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -260,8 +260,8 @@ grub_ext2_blockgroup (struct grub_ext2_data *data, int group,
   int blkno, blkoff;
 
   group *= sizeof (struct grub_ext2_block_group);
-  blkno = group >> LOG2_EXT2_BLOCK_SIZE (data);
-  blkoff = group  - (blkno << LOG2_EXT2_BLOCK_SIZE (data));
+  blkno = group >> (LOG2_EXT2_BLOCK_SIZE (data) + 9);
+  blkoff = group - (blkno << (LOG2_EXT2_BLOCK_SIZE (data) + 9));
 
   return grub_disk_read (data->disk,
 			 (grub_fshelp_map_block (data->journal,

That appears to fix everything!!!  grub-fstest is loading, GRUB is
working in qemu on the live filesystem, and the normal boot is still
working.

I think we should consider having a journaling layer above
grub_disk_read() that would do the substitution and avoid such bugs in
the future.

-- 
Regards,
Pavel Roskin

[RFC PATCH] Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Hello!

Here's the patch with some cleanups and nice wrapped lines.  I leaving
out other fixes from e3.diff, they should be applied separately and
don't appear to be critical for my setup.

Changelog:

	* fs/ext2.c (grub_ext2_blockgroup): Call grub_fshelp_map_block()
	for the block that contains data for the requested group.

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..f8642bb 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -257,12 +257,18 @@ inline static grub_err_t
 grub_ext2_blockgroup (struct grub_ext2_data *data, int group, 
 		      struct grub_ext2_block_group *blkgrp)
 {
+  int blkno, blkoff;
+
+  blkoff = group * sizeof (struct grub_ext2_block_group);
+  blkno = blkoff >> (LOG2_EXT2_BLOCK_SIZE (data) + 9);
+  blkoff -= (blkno << (LOG2_EXT2_BLOCK_SIZE (data) + 9));
+  blkno += grub_le_to_cpu32 (data->sblock.first_data_block) + 1;
+
   return grub_disk_read (data->disk,
-			 (grub_fshelp_map_block (data->journal,
-                                                 grub_le_to_cpu32 (data->sblock.first_data_block) + 1)
+			 (grub_fshelp_map_block (data->journal, blkno)
 			  << LOG2_EXT2_BLOCK_SIZE (data)),
-			 group * sizeof (struct grub_ext2_block_group), 
-			 sizeof (struct grub_ext2_block_group), (char *) blkgrp);
+			 blkoff, sizeof (struct grub_ext2_block_group),
+			 (char *) blkgrp);
 }
 
 

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Wed, Jun 11, 2008 at 6:58 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Wed, 2008-06-11 at 02:51 +0800, Bean wrote:
>> Hi,
>>
>> I spot another bug, please try the new patch.
>
> You probably meant to use (LOG2_EXT2_BLOCK_SIZE (data) + 9) in your
> patch because the block size is in 512 bytes units, unlike sizeof:
>
> diff --git a/fs/ext2.c b/fs/ext2.c
> index e4ce47b..fb13a80 100644
> --- a/fs/ext2.c
> +++ b/fs/ext2.c
> @@ -260,8 +260,8 @@ grub_ext2_blockgroup (struct grub_ext2_data *data, int group,
>   int blkno, blkoff;
>
>   group *= sizeof (struct grub_ext2_block_group);
> -  blkno = group >> LOG2_EXT2_BLOCK_SIZE (data);
> -  blkoff = group  - (blkno << LOG2_EXT2_BLOCK_SIZE (data));
> +  blkno = group >> (LOG2_EXT2_BLOCK_SIZE (data) + 9);
> +  blkoff = group - (blkno << (LOG2_EXT2_BLOCK_SIZE (data) + 9));
>
>   return grub_disk_read (data->disk,
>                         (grub_fshelp_map_block (data->journal,
>
> That appears to fix everything!!!  grub-fstest is loading, GRUB is
> working in qemu on the live filesystem, and the normal boot is still
> working.
>

Oh, nice catch, I make a silly mistake again, :)

> I think we should consider having a journaling layer above
> grub_disk_read() that would do the substitution and avoid such bugs in
> the future.

This should be all right, but we need to move the journal support from
fshelp.c to disk.c, anyone objects ?

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Quoting Bean <bean123ch@gmail.com>:

>> I think we should consider having a journaling layer above
>> grub_disk_read() that would do the substitution and avoid such bugs in
>> the future.
>
> This should be all right, but we need to move the journal support from
> fshelp.c to disk.c, anyone objects ?

No objection from me.  But if it's complicated, we can start with an  
ext2 specific function that would translate offset and size to block  
numbers, apply journal mappings and read the data.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

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

Quoting Bean <bean123ch@gmail.com>:

> This should be all right, but we need to move the journal support from
> fshelp.c to disk.c, anyone objects ?

I've converted ext2.c.  The patch is attached.  The interface should  
be OK for reiserfs.  It looks like the call to grub_fshelp_map_block()  
from grub_ext2_read_inode() was also wrong.  I'm afraid there are some  
wrong places in reiserfs as well.

I would like to commit the patch as soon as possible.  It's a serious  
issue, and we don't want to scare away potential testers and  
contributors.

ChangeLog:
         * fs/fshelp.c (grub_fshelp_read): New function.  Implement linear
         disk read with journal translation.
         * fs/ext2.c: Use grub_fshelp_read() instead of grub_disk_read().
         * include/grub/fshelp.h: Declare grub_fshelp_read().

-- 
Regards,
Pavel Roskin


[-- Attachment #2: 01-ext2-blockread.patch --]
[-- Type: text/plain, Size: 5651 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ffe9e33..999bd80 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -257,12 +257,11 @@ inline static grub_err_t
 grub_ext2_blockgroup (struct grub_ext2_data *data, int group, 
 		      struct grub_ext2_block_group *blkgrp)
 {
-  return grub_disk_read (data->disk,
-			 (grub_fshelp_map_block (data->journal,
-                                                 grub_le_to_cpu32 (data->sblock.first_data_block) + 1)
-			  << LOG2_EXT2_BLOCK_SIZE (data)),
-			 group * sizeof (struct grub_ext2_block_group), 
-			 sizeof (struct grub_ext2_block_group), (char *) blkgrp);
+  return grub_fshelp_read (data->disk, data->journal,
+			   grub_le_to_cpu32 (data->sblock.first_data_block) + 1,
+			   group * sizeof (struct grub_ext2_block_group),
+			   sizeof (struct grub_ext2_block_group),
+			   (char *) blkgrp, LOG2_EXT2_BLOCK_SIZE (data));
 }
 
 
@@ -283,11 +282,9 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
     {
       grub_uint32_t indir[blksz / 4];
 
-      if (grub_disk_read (data->disk, 
-			  grub_fshelp_map_block(data->journal,
-                                                grub_le_to_cpu32 (inode->blocks.indir_block))
-			  << log2_blksz,
-			  0, blksz, (char *) indir))
+      if (grub_fshelp_read (data->disk, data->journal,
+			    grub_le_to_cpu32 (inode->blocks.indir_block),
+			    0, blksz, (char *) indir, log2_blksz))
 	return grub_errno;
 	  
       blknr = grub_le_to_cpu32 (indir[fileblock - INDIRECT_BLOCKS]);
@@ -300,18 +297,14 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 					 + blksz / 4);
       grub_uint32_t indir[blksz / 4];
 
-      if (grub_disk_read (data->disk, 
-			  grub_fshelp_map_block(data->journal,
-                                                grub_le_to_cpu32 (inode->blocks.double_indir_block))
-			  << log2_blksz,
-			  0, blksz, (char *) indir))
+      if (grub_fshelp_read (data->disk, data->journal,
+			    grub_le_to_cpu32 (inode->blocks.double_indir_block),
+			    0, blksz, (char *) indir, log2_blksz))
 	return grub_errno;
 
-      if (grub_disk_read (data->disk,
-			  grub_fshelp_map_block(data->journal,
-                                                grub_le_to_cpu32 (indir[rblock / perblock]))
-			  << log2_blksz,
-			  0, blksz, (char *) indir))
+      if (grub_fshelp_read (data->disk, data->journal,
+			    grub_le_to_cpu32 (indir[rblock / perblock]),
+			    0, blksz, (char *) indir, log2_blksz))
 	return grub_errno;
 
       
@@ -372,12 +365,11 @@ grub_ext2_read_inode (struct grub_ext2_data *data,
     % inodes_per_block;
   
   /* Read the inode.  */
-  if (grub_disk_read (data->disk, 
-		      grub_fshelp_map_block(data->journal,
-                                            grub_le_to_cpu32 (blkgrp.inode_table_id) + blkno)
-                      << LOG2_EXT2_BLOCK_SIZE (data),
-		      EXT2_INODE_SIZE (data) * blkoff,
-		      sizeof (struct grub_ext2_inode), (char *) inode))
+  if (grub_fshelp_read (data->disk, data->journal,
+			grub_le_to_cpu32 (blkgrp.inode_table_id) + blkno,
+			EXT2_INODE_SIZE (data) * blkoff,
+			sizeof (struct grub_ext2_inode), (char *) inode,
+			LOG2_EXT2_BLOCK_SIZE (data)))
     return grub_errno;
   
   return 0;
diff --git a/fs/fshelp.c b/fs/fshelp.c
index faec1f7..52131b0 100644
--- a/fs/fshelp.c
+++ b/fs/fshelp.c
@@ -215,6 +215,45 @@ grub_fshelp_find_file (const char *path, grub_fshelp_node_t rootnode,
 }
 
 
+/* Read LEN bytes from the block BLOCK on disk DISK into the buffer BUF,
+   beginning with the block POS.  Apply mappings from LOG.  The blocks
+   have a size of LOG2BLOCKSIZE (in log2).  */
+grub_err_t grub_fshelp_read (grub_disk_t disk, grub_fshelp_journal_t log,
+			     grub_disk_addr_t block, grub_off_t pos,
+			     grub_size_t len, char *buf, int log2blocksize)
+{
+  grub_off_t relblk;
+
+  relblk = pos >> (log2blocksize + GRUB_DISK_SECTOR_BITS);
+  block += relblk;
+  pos -= relblk << (log2blocksize + GRUB_DISK_SECTOR_BITS);
+
+  while (len > 0)
+    {
+      grub_err_t ret;
+      grub_size_t size;
+
+      size = (GRUB_DISK_SECTOR_SIZE << log2blocksize) - pos;
+      if (size > len)
+	size = len;
+
+      ret = grub_disk_read (disk,
+			    grub_fshelp_map_block (log, block) << log2blocksize,
+			    pos, size, buf);
+
+      if (ret)
+	return ret;
+
+      block++;
+      pos = 0;
+      buf += size;
+      len -= size;
+    }
+
+  return 0;
+}
+
+
 /* Read LEN bytes from the file NODE on disk DISK into the buffer BUF,
    beginning with the block POS.  READ_HOOK should be set before
    reading a block from the file.  GET_BLOCK is used to translate file
diff --git a/include/grub/fshelp.h b/include/grub/fshelp.h
index 0120cbf..847f78e 100644
--- a/include/grub/fshelp.h
+++ b/include/grub/fshelp.h
@@ -84,6 +84,15 @@ EXPORT_FUNC(grub_fshelp_find_file) (const char *path,
 				    enum grub_fshelp_filetype expect);
 
 
+/* Read LEN bytes from the block BLOCK on disk DISK into the buffer BUF,
+   beginning with the block POS.  Apply mappings from LOG.  The blocks
+   have a size of LOG2BLOCKSIZE (in log2).  */
+grub_err_t
+EXPORT_FUNC(grub_fshelp_read) (grub_disk_t disk, grub_fshelp_journal_t log,
+			       grub_disk_addr_t block, grub_off_t pos,
+			       grub_size_t len, char *buf, int log2blocksize);
+
+
 /* Read LEN bytes from the file NODE on disk DISK into the buffer BUF,
    beginning with the block POS.  READ_HOOK should be set before
    reading a block from the file.  GET_BLOCK is used to translate file

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Thu, Jun 12, 2008 at 2:22 PM, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Bean <bean123ch@gmail.com>:
>
>> This should be all right, but we need to move the journal support from
>> fshelp.c to disk.c, anyone objects ?
>
> I've converted ext2.c.  The patch is attached.  The interface should be OK
> for reiserfs.  It looks like the call to grub_fshelp_map_block() from
> grub_ext2_read_inode() was also wrong.  I'm afraid there are some wrong
> places in reiserfs as well.

The call in grub_ext2_read_inode should be ok, blkoff would limit the
read access to within a block.

>
> I would like to commit the patch as soon as possible.  It's a serious issue,
> and we don't want to scare away potential testers and contributors.
>
> ChangeLog:
>        * fs/fshelp.c (grub_fshelp_read): New function.  Implement linear
>        disk read with journal translation.
>        * fs/ext2.c: Use grub_fshelp_read() instead of grub_disk_read().
>        * include/grub/fshelp.h: Declare grub_fshelp_read().

Seems fine to me. grub_ext3_get_journal still have some issue, but I
can fix it after you commit it.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

On Thu, 2008-06-12 at 17:51 +0800, Bean wrote:

> The call in grub_ext2_read_inode should be ok, blkoff would limit the
> read access to within a block.

You are right.  Now that we have grub_fshelp_read(), we don't need that
code, so I'm removing it.  The block normalization should be done in one
place.

> Seems fine to me.

Committed.

>  grub_ext3_get_journal still have some issue, but I
> can fix it after you commit it.

Please go ahead.  And please take care of reiserfs if you can.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Fri, Jun 13, 2008 at 12:25 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Thu, 2008-06-12 at 17:51 +0800, Bean wrote:
>
>> The call in grub_ext2_read_inode should be ok, blkoff would limit the
>> read access to within a block.
>
> You are right.  Now that we have grub_fshelp_read(), we don't need that
> code, so I'm removing it.  The block normalization should be done in one
> place.
>
>> Seems fine to me.
>
> Committed.
>
>>  grub_ext3_get_journal still have some issue, but I
>> can fix it after you commit it.
>
> Please go ahead.  And please take care of reiserfs if you can.

Hi,

After more thoughts, I think the current method still have problem.
grub_disk_read reads up to 8192 bytes at a time, which can be larger
than the block size. Unless all or none of the sectors in the same
block is mapped, we end up reading the wrong sector. So we need to do
the mapping inside grub_disk_read.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Quoting Bean <bean123ch@gmail.com>:

> After more thoughts, I think the current method still have problem.
> grub_disk_read reads up to 8192 bytes at a time, which can be larger
> than the block size.

But we never ask grub_disk_read() to read across the block boundary in  
grub_fshelp_read().  The code takes care of it.  I assume  
grub_disk_read() would not write more than requested to the buffer.

> Unless all or none of the sectors in the same
> block is mapped, we end up reading the wrong sector. So we need to do
> the mapping inside grub_disk_read.

But mapping is done for blocks, not for sectors.   
grub_fshelp_map_block() clearly assumes that for every filesystem.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Fri, Jun 13, 2008 at 12:31 PM, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Bean <bean123ch@gmail.com>:
>
>> After more thoughts, I think the current method still have problem.
>> grub_disk_read reads up to 8192 bytes at a time, which can be larger
>> than the block size.
>
> But we never ask grub_disk_read() to read across the block boundary in
> grub_fshelp_read().  The code takes care of it.  I assume grub_disk_read()
> would not write more than requested to the buffer.

grub_disk_read would attempt to read up to GRUB_DISK_CACHE_SIZE and
put the result in a cache, then copy the necessary part to output.

>
>> Unless all or none of the sectors in the same
>> block is mapped, we end up reading the wrong sector. So we need to do
>> the mapping inside grub_disk_read.
>
> But mapping is done for blocks, not for sectors.  grub_fshelp_map_block()
> clearly assumes that for every filesystem.

block size is fs related, but most are less than 8192 bytes.

-- 
Bean

Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin]

Quoting Bean <bean123ch@gmail.com>:

> On Fri, Jun 13, 2008 at 12:31 PM, Pavel Roskin <proski@gnu.org> wrote:
>> Quoting Bean <bean123ch@gmail.com>:
>>
>>> After more thoughts, I think the current method still have problem.
>>> grub_disk_read reads up to 8192 bytes at a time, which can be larger
>>> than the block size.
>>
>> But we never ask grub_disk_read() to read across the block boundary in
>> grub_fshelp_read().  The code takes care of it.  I assume grub_disk_read()
>> would not write more than requested to the buffer.
>
> grub_disk_read would attempt to read up to GRUB_DISK_CACHE_SIZE and
> put the result in a cache, then copy the necessary part to output.

Then I don't see any problem with it.  Maybe you could give an example?

>>> Unless all or none of the sectors in the same
>>> block is mapped, we end up reading the wrong sector. So we need to do
>>> the mapping inside grub_disk_read.
>>
>> But mapping is done for blocks, not for sectors.  grub_fshelp_map_block()
>> clearly assumes that for every filesystem.
>
> block size is fs related, but most are less than 8192 bytes.

I don't understand why caching would have any effect.  It should be  
transparent for higher layers, like caching inside the hard drive.  If  
caching is not transparent, then it's broken.

-- 
Regards,
Pavel Roskin

Re: [PATCH] biosdisk / open_device() messing up offsets

[Bean]

On Fri, Jun 13, 2008 at 1:00 PM, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Bean <bean123ch@gmail.com>:
>
>> On Fri, Jun 13, 2008 at 12:31 PM, Pavel Roskin <proski@gnu.org> wrote:
>>>
>>> Quoting Bean <bean123ch@gmail.com>:
>>>
>>>> After more thoughts, I think the current method still have problem.
>>>> grub_disk_read reads up to 8192 bytes at a time, which can be larger
>>>> than the block size.
>>>
>>> But we never ask grub_disk_read() to read across the block boundary in
>>> grub_fshelp_read().  The code takes care of it.  I assume
>>> grub_disk_read()
>>> would not write more than requested to the buffer.
>>
>> grub_disk_read would attempt to read up to GRUB_DISK_CACHE_SIZE and
>> put the result in a cache, then copy the necessary part to output.
>
> Then I don't see any problem with it.  Maybe you could give an example?
>
>>>> Unless all or none of the sectors in the same
>>>> block is mapped, we end up reading the wrong sector. So we need to do
>>>> the mapping inside grub_disk_read.
>>>
>>> But mapping is done for blocks, not for sectors.  grub_fshelp_map_block()
>>> clearly assumes that for every filesystem.
>>
>> block size is fs related, but most are less than 8192 bytes.
>
> I don't understand why caching would have any effect.  It should be
> transparent for higher layers, like caching inside the hard drive.  If
> caching is not transparent, then it's broken.

Hi,

I'm sorry I got a little confused. Your method does work, no need to
worry about it now.

-- 
Bean