[PATCH] Bug fix for ext2.c

[PATCH] Bug fix for ext2.c

[Bean]

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

Hi,

I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
function must return GRUB_ERR_BAD_FS if something goes wrong, because
grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
error, thus preventing other fs driver from detecting the correct fs
type. This patch fixes the problem.

-- 
Bean

[-- Attachment #2: ext2.diff --]
[-- Type: application/octet-stream, Size: 943 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ac0757e..aad1384 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -548,17 +548,14 @@ grub_ext2_mount (grub_disk_t disk)
 
   /* Make sure this is an ext2 filesystem.  */
   if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
-    {
-      grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
-      goto fail;
-    }
+    goto fail;
   
   /* Check the FS doesn't have feature bits enabled that we don't support. */
   if (grub_le_to_cpu32 (data->sblock.feature_incompat)
         & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
     {
       grub_error (GRUB_ERR_BAD_FS, "filesystem has unsupported incompatible features");
-      goto fail;
+      goto fail_1;
     }
     
   
@@ -576,7 +573,10 @@ grub_ext2_mount (grub_disk_t disk)
   
   return data;
 
- fail:
+fail:
+  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+ 
+fail_1:
   grub_free (data);
   return 0;
 }

Re: [PATCH] Bug fix for ext2.c

[Robert Millan]

On Sun, Mar 15, 2009 at 12:16:23AM +0800, Bean wrote:
> Hi,
> 
> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
> function must return GRUB_ERR_BAD_FS if something goes wrong, because
> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
> error, thus preventing other fs driver from detecting the correct fs
> type. This patch fixes the problem.

I think current behaviour is correct.  If a failure is triggered by
grub_disk_read(), from grub_fs_probe perspective it means something is
fucked up other than just "this is not the FS we're looking for", so it
should be aware of the difference.

Or is grub_ext2_read_inode() failure the one that's causing trouble for
you?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Bug fix for ext2.c

[Bean]

On Sun, Mar 15, 2009 at 12:37 AM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sun, Mar 15, 2009 at 12:16:23AM +0800, Bean wrote:
>> Hi,
>>
>> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
>> function must return GRUB_ERR_BAD_FS if something goes wrong, because
>> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
>> error, thus preventing other fs driver from detecting the correct fs
>> type. This patch fixes the problem.
>
> I think current behaviour is correct.  If a failure is triggered by
> grub_disk_read(), from grub_fs_probe perspective it means something is
> fucked up other than just "this is not the FS we're looking for", so it
> should be aware of the difference.
>
> Or is grub_ext2_read_inode() failure the one that's causing trouble for
> you?

Hi,

ext2 only reads the first two sectors in mount, which is normally ok ,
but there are exceptions. For example, cpio filesystem could be less
one sector. Also, hostfs always return error in its read function,
which would cause ext2 to fail. The effect can be seen in grub-fstest.
hostfs is the first fs driver to register, and the last to query. When
accessing the (host) device, ext2 cause it to fail before hostfs has a
chance to see it.

-- 
Bean

Re: [PATCH] Bug fix for ext2.c

[Robert Millan]

On Sun, Mar 15, 2009 at 12:56:26AM +0800, Bean wrote:
> On Sun, Mar 15, 2009 at 12:37 AM, Robert Millan <rmh@aybabtu.com> wrote:
> > On Sun, Mar 15, 2009 at 12:16:23AM +0800, Bean wrote:
> >> Hi,
> >>
> >> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
> >> function must return GRUB_ERR_BAD_FS if something goes wrong, because
> >> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
> >> error, thus preventing other fs driver from detecting the correct fs
> >> type. This patch fixes the problem.
> >
> > I think current behaviour is correct.  If a failure is triggered by
> > grub_disk_read(), from grub_fs_probe perspective it means something is
> > fucked up other than just "this is not the FS we're looking for", so it
> > should be aware of the difference.
> >
> > Or is grub_ext2_read_inode() failure the one that's causing trouble for
> > you?
> 
> Hi,
> 
> ext2 only reads the first two sectors in mount, which is normally ok ,
> but there are exceptions. For example, cpio filesystem could be less
> one sector. 

I don't understand what you mean here.  If grub_disk_read failed, this
indicates something's broken down in the disk layer doesn't it?  How is
the number of sectors related to this?

> Also, hostfs always return error in its read function,
> which would cause ext2 to fail. The effect can be seen in grub-fstest.
> hostfs is the first fs driver to register, and the last to query. When
> accessing the (host) device, ext2 cause it to fail before hostfs has a
> chance to see it.

Why does hostfs fail?  Is this intentional?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Bug fix for ext2.c

[Bean]

On Sun, Mar 15, 2009 at 1:35 AM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sun, Mar 15, 2009 at 12:56:26AM +0800, Bean wrote:
>> On Sun, Mar 15, 2009 at 12:37 AM, Robert Millan <rmh@aybabtu.com> wrote:
>> > On Sun, Mar 15, 2009 at 12:16:23AM +0800, Bean wrote:
>> >> Hi,
>> >>
>> >> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
>> >> function must return GRUB_ERR_BAD_FS if something goes wrong, because
>> >> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
>> >> error, thus preventing other fs driver from detecting the correct fs
>> >> type. This patch fixes the problem.
>> >
>> > I think current behaviour is correct.  If a failure is triggered by
>> > grub_disk_read(), from grub_fs_probe perspective it means something is
>> > fucked up other than just "this is not the FS we're looking for", so it
>> > should be aware of the difference.
>> >
>> > Or is grub_ext2_read_inode() failure the one that's causing trouble for
>> > you?
>>
>> Hi,
>>
>> ext2 only reads the first two sectors in mount, which is normally ok ,
>> but there are exceptions. For example, cpio filesystem could be less
>> one sector.
>
> I don't understand what you mean here.  If grub_disk_read failed, this
> indicates something's broken down in the disk layer doesn't it?  How is
> the number of sectors related to this?
>

Hi,

grub_disk_read checks for disk limit. For example, cpio file system
can be one sector long, trying to read two sectors would cause a out
of range error.

>> Also, hostfs always return error in its read function,
>> which would cause ext2 to fail. The effect can be seen in grub-fstest.
>> hostfs is the first fs driver to register, and the last to query. When
>> accessing the (host) device, ext2 cause it to fail before hostfs has a
>> chance to see it.
>
> Why does hostfs fail?  Is this intentional?

Yes, it's intentional. Reading raw sectors from (host) doesn't make
much sense. The (host) device is a place holder for hostfs filesystem.

-- 
Bean

Re: [PATCH] Bug fix for ext2.c

[Yoshinori K. Okuji]

On Sunday 15 March 2009 01:16:23 Bean wrote:
> Hi,
>
> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
> function must return GRUB_ERR_BAD_FS if something goes wrong, because
> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
> error, thus preventing other fs driver from detecting the correct fs
> type. This patch fixes the problem.

This patch ignores any kind of failure. That's a problem. If you only convert 
GRUB_ERR_OUT_OF_RANGE to GRUB_ERR_BAD_FS, I agree.

Regards,
Okuji

Re: [PATCH] Bug fix for ext2.c

[Bean]

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

On Sun, Mar 15, 2009 at 5:27 AM, Yoshinori K. Okuji <okuji@enbug.org> wrote:
> On Sunday 15 March 2009 01:16:23 Bean wrote:
>> Hi,
>>
>> I've discovered a bug in ext2.c, inside grub_ext2_mount. The mount
>> function must return GRUB_ERR_BAD_FS if something goes wrong, because
>> grub_fs_probe would stop as soon as it sees a non-GRUB_ERR_BAD_FS
>> error, thus preventing other fs driver from detecting the correct fs
>> type. This patch fixes the problem.
>
> This patch ignores any kind of failure. That's a problem. If you only convert
> GRUB_ERR_OUT_OF_RANGE to GRUB_ERR_BAD_FS, I agree.

Hi,

Oh right, this one should be ok.

-- 
Bean

[-- Attachment #2: ext2.diff --]
[-- Type: application/octet-stream, Size: 321 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index ac0757e..1c29885 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -577,6 +577,9 @@ grub_ext2_mount (grub_disk_t disk)
   return data;
 
  fail:
+  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+    grub_error (GRUB_ERR_BAD_FS, "not an xfs filesystem");
+
   grub_free (data);
   return 0;
 }

Re: [PATCH] Bug fix for ext2.c

[Felix Zielcke]

Am Sonntag, den 15.03.2009, 14:00 +0800 schrieb Bean:

> Hi,

Hi Bean,

> Oh right, this one should be ok.

+    grub_error (GRUB_ERR_BAD_FS, "not an xfs filesystem");
This should probable be `not an ext2 filesystem' :)
-- 
Felix Zielcke

Re: [PATCH] Bug fix for ext2.c

[Robert Millan]

On Sun, Mar 15, 2009 at 11:33:18AM +0100, Felix Zielcke wrote:
> Am Sonntag, den 15.03.2009, 14:00 +0800 schrieb Bean:
> 
> > Hi,
> 
> Hi Bean,
> 
> > Oh right, this one should be ok.
> 
> +    grub_error (GRUB_ERR_BAD_FS, "not an xfs filesystem");
> This should probable be `not an ext2 filesystem' :)

Why override grub_errmsg anyway?  "not ext2" isn't going to be more useful
than a "read out of range" text when displayed to the user.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Bug fix for ext2.c

[Bean]

On Sun, Mar 15, 2009 at 11:37 PM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sun, Mar 15, 2009 at 11:33:18AM +0100, Felix Zielcke wrote:
>> Am Sonntag, den 15.03.2009, 14:00 +0800 schrieb Bean:
>>
>> > Hi,
>>
>> Hi Bean,
>>
>> > Oh right, this one should be ok.
>>
>> +    grub_error (GRUB_ERR_BAD_FS, "not an xfs filesystem");
>> This should probable be `not an ext2 filesystem' :)
>
> Why override grub_errmsg anyway?  "not ext2" isn't going to be more useful
> than a "read out of range" text when displayed to the user.

Hi,

I think the ext2 error message is better. The "read out of range"
error is such a general message, it tells us little about where it
goes wrong, while with "not ext2 filesystem",. we know that the error
is generated in grub_ext2_mount.

Although, a more informative way is to write it like this:

  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
    grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem (out of range)");

But that seems a little overdo for me. Besides, fs drivers such as
xfs.c, hfsplus.c use simple message for out of range error.

-- 
Bean

Re: [PATCH] Bug fix for ext2.c

[Bean]

Committed.

-- 
Bean