[PATCH] Refuse to install on XFS destroying its superblock

[PATCH] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

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


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 


[-- Attachment #2: xfs.diff --]
[-- Type: text/x-patch, Size: 929 bytes --]

diff --git a/ChangeLog b/ChangeLog
index b0864a9..a67fdfd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
+
 2009-10-15  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* loader/i386/pc/xnu.c (grub_xnu_set_video): Fix loading splash image.
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index ccfbd1d..5181e58 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -205,6 +205,9 @@ setup (const char *dir,
   boot_img = grub_util_read_image (boot_path);
   free (boot_path);
 
+  if (memcmp (boot_img, "XFSB", 4) == 0)
+    grub_util_error ("Can't install on XFS.");
+
   /* Set the addresses of variables in the boot image.  */
   boot_drive = (grub_uint8_t *) (boot_img + GRUB_BOOT_MACHINE_BOOT_DRIVE);
   kernel_sector = (grub_disk_addr_t *) (boot_img

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

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

Sorry, patch had a problem
Vladimir 'phcoder' Serbinenko wrote:

-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 


[-- Attachment #2: xfs.diff --]
[-- Type: text/x-patch, Size: 924 bytes --]

diff --git a/ChangeLog b/ChangeLog
index b0864a9..a67fdfd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
+
 2009-10-15  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* loader/i386/pc/xnu.c (grub_xnu_set_video): Fix loading splash image.
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index ccfbd1d..b3f2736 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -251,6 +251,9 @@ setup (const char *dir,
   if (grub_disk_read (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, tmp_img))
     grub_util_error ("%s", grub_errmsg);
 
+  if (memcmp (tmp_img, "XFSB", 4) == 0)
+    grub_util_error ("Can't install on XFS.");
+
   /* Copy the possible DOS BPB.  */
   memcpy (boot_img + GRUB_BOOT_MACHINE_BPB_START,
 	  tmp_img + GRUB_BOOT_MACHINE_BPB_START,

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Jordi Mallach]

On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
> +    grub_util_error ("Can't install on XFS.");

Can this error message give some more detail on what the problem is?

Jordi
-- 
Jordi Mallach Pérez  --  Debian developer     http://www.debian.org/
jordi@sindominio.net     jordi@debian.org     http://www.sindominio.net/
GnuPG public key information available at http://oskuro.net/

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > +  if (memcmp (tmp_img, "XFSB", 4) == 0)
> > +    grub_util_error ("Can't install on XFS.");
> 
> Can this error message give some more detail on what the problem is?

I suggest something like:

  grub_util_warn ("Refusing to overwrite XFS meta-data.");

This is more informative, and with grub_util_warn() user has an opportunity to
override it if she knows what she's doing.

-- 
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] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
>   
>> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>     
>>> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
>>> +    grub_util_error ("Can't install on XFS.");
>>>       
>> Can this error message give some more detail on what the problem is?
>>     
>
> I suggest something like:
>
>   grub_util_warn ("Refusing to overwrite XFS meta-data.");
>
> This is more informative, and with grub_util_warn() user has an opportunity to
> override it if she knows what she's doing.
>
>   
Installing with blocklists/to partition is considered
backward-compatibility feature. We never supported a config with XFS why
we would want bw-compat for it?


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Fri, Oct 16, 2009 at 10:09:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
> >   
> >> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >>     
> >>> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
> >>> +    grub_util_error ("Can't install on XFS.");
> >>>       
> >> Can this error message give some more detail on what the problem is?
> >>     
> >
> > I suggest something like:
> >
> >   grub_util_warn ("Refusing to overwrite XFS meta-data.");
> >
> > This is more informative, and with grub_util_warn() user has an opportunity to
> > override it if she knows what she's doing.
> >
> >   
> Installing with blocklists/to partition is considered
> backward-compatibility feature. We never supported a config with XFS why
> we would want bw-compat for it?

Because we can't reliably tell if it's a config with XFS, only the user can.
This is an issue for both MBR or PBR installs.

Maybe "XFSB" is only a remnant from one of this disk / partition former
lifes.  Maybe it's a valid XFS but user no longer cares about it.  Or
maybe a DOS-style label was created on top of it, without overwriting the first
440 bytes.  Or maybe another filesystem had overwritten most XFS metadata
but preserved the first block (this is conceivable since other filesystems
tend to avoid using the first block).

If user has to workaround GRUB heuristics by dd'ing zeros into a partition
before running grub-install, this is a sign GRUB isn't doing the right thing.

-- 
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] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Fri, Oct 16, 2009 at 10:09:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
>>>   
>>>       
>>>> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>>     
>>>>         
>>>>> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
>>>>> +    grub_util_error ("Can't install on XFS.");
>>>>>       
>>>>>           
>>>> Can this error message give some more detail on what the problem is?
>>>>     
>>>>         
>>> I suggest something like:
>>>
>>>   grub_util_warn ("Refusing to overwrite XFS meta-data.");
>>>
>>> This is more informative, and with grub_util_warn() user has an opportunity to
>>> override it if she knows what she's doing.
>>>
>>>   
>>>       
>> Installing with blocklists/to partition is considered
>> backward-compatibility feature. We never supported a config with XFS why
>> we would want bw-compat for it?
>>     
>
> Because we can't reliably tell if it's a config with XFS, only the user can.
> This is an issue for both MBR or PBR installs.
>
> Maybe "XFSB" is only a remnant from one of this disk / partition former
> lifes.  Maybe it's a valid XFS but user no longer cares about it.  Or
> maybe a DOS-style label was created on top of it, without overwriting the first
> 440 bytes.  Or maybe another filesystem had overwritten most XFS metadata
> but preserved the first block (this is conceivable since other filesystems
> tend to avoid using the first block).
>
> If user has to workaround GRUB heuristics by dd'ing zeros into a partition
> before running grub-install, this is a sign GRUB isn't doing the right thing.
>
>   
Well, ok. But then I would ask to use a separate --force e.g.
--force-destroy-xfs since users and distributions tend to use --force
too much

-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Fri, Oct 16, 2009 at 11:08:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Fri, Oct 16, 2009 at 10:09:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >   
> >> Robert Millan wrote:
> >>     
> >>> On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
> >>>   
> >>>       
> >>>> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >>>>     
> >>>>         
> >>>>> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
> >>>>> +    grub_util_error ("Can't install on XFS.");
> >>>>>       
> >>>>>           
> >>>> Can this error message give some more detail on what the problem is?
> >>>>     
> >>>>         
> >>> I suggest something like:
> >>>
> >>>   grub_util_warn ("Refusing to overwrite XFS meta-data.");
> >>>
> >>> This is more informative, and with grub_util_warn() user has an opportunity to
> >>> override it if she knows what she's doing.
> >>>
> >>>   
> >>>       
> >> Installing with blocklists/to partition is considered
> >> backward-compatibility feature. We never supported a config with XFS why
> >> we would want bw-compat for it?
> >>     
> >
> > Because we can't reliably tell if it's a config with XFS, only the user can.
> > This is an issue for both MBR or PBR installs.
> >
> > Maybe "XFSB" is only a remnant from one of this disk / partition former
> > lifes.  Maybe it's a valid XFS but user no longer cares about it.  Or
> > maybe a DOS-style label was created on top of it, without overwriting the first
> > 440 bytes.  Or maybe another filesystem had overwritten most XFS metadata
> > but preserved the first block (this is conceivable since other filesystems
> > tend to avoid using the first block).
> >
> > If user has to workaround GRUB heuristics by dd'ing zeros into a partition
> > before running grub-install, this is a sign GRUB isn't doing the right thing.
> >
> >   
> Well, ok. But then I would ask to use a separate --force e.g.
> --force-destroy-xfs since users and distributions tend to use --force
> too much

Ok.

-- 
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] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

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

Robert Millan wrote:
> On Fri, Oct 16, 2009 at 11:08:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Fri, Oct 16, 2009 at 10:09:41PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>   
>>>       
>>>> Robert Millan wrote:
>>>>     
>>>>         
>>>>> On Fri, Oct 16, 2009 at 06:01:56PM +0200, Jordi Mallach wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> On Fri, Oct 16, 2009 at 04:03:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> +  if (memcmp (tmp_img, "XFSB", 4) == 0)
>>>>>>> +    grub_util_error ("Can't install on XFS.");
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Can this error message give some more detail on what the problem is?
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> I suggest something like:
>>>>>
>>>>>   grub_util_warn ("Refusing to overwrite XFS meta-data.");
>>>>>
>>>>> This is more informative, and with grub_util_warn() user has an opportunity to
>>>>> override it if she knows what she's doing.
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> Installing with blocklists/to partition is considered
>>>> backward-compatibility feature. We never supported a config with XFS why
>>>> we would want bw-compat for it?
>>>>     
>>>>         
>>> Because we can't reliably tell if it's a config with XFS, only the user can.
>>> This is an issue for both MBR or PBR installs.
>>>
>>> Maybe "XFSB" is only a remnant from one of this disk / partition former
>>> lifes.  Maybe it's a valid XFS but user no longer cares about it.  Or
>>> maybe a DOS-style label was created on top of it, without overwriting the first
>>> 440 bytes.  Or maybe another filesystem had overwritten most XFS metadata
>>> but preserved the first block (this is conceivable since other filesystems
>>> tend to avoid using the first block).
>>>
>>> If user has to workaround GRUB heuristics by dd'ing zeros into a partition
>>> before running grub-install, this is a sign GRUB isn't doing the right thing.
>>>
>>>   
>>>       
>> Well, ok. But then I would ask to use a separate --force e.g.
>> --force-destroy-xfs since users and distributions tend to use --force
>> too much
>>     
>
> Ok.
>
>   


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 


[-- Attachment #2: xfs.diff --]
[-- Type: text/x-patch, Size: 3415 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 960fc06..cc225a7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
 
+	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
+	(options): New option --destroy-xfs.
+	(main): Handle --destroy-xfs.
+
+2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
+
 	Let user specify OpenBSD root device.
 
 	* loader/i386/bsd.c (openbsd_root): New variable.
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index ccfbd1d..4ef746b 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -86,7 +86,8 @@ grub_refresh (void)
 static void
 setup (const char *dir,
        const char *boot_file, const char *core_file,
-       const char *root, const char *dest, int must_embed, int force)
+       const char *root, const char *dest, int must_embed, int force,
+       int destroy_xfs)
 {
   char *boot_path, *core_path, *core_path_dev;
   char *boot_img, *core_img;
@@ -251,6 +252,13 @@ setup (const char *dir,
   if (grub_disk_read (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, tmp_img))
     grub_util_error ("%s", grub_errmsg);
 
+  if (memcmp (tmp_img, "XFSB", 4) == 0)
+    {
+      grub_util_warn ("This install would require destroying XFS.");
+      if (! destroy_xfs)
+	grub_util_error ("If you really want to destroy it use --destroy-xfs.");
+    }
+
   /* Copy the possible DOS BPB.  */
   memcpy (boot_img + GRUB_BOOT_MACHINE_BPB_START,
 	  tmp_img + GRUB_BOOT_MACHINE_BPB_START,
@@ -556,6 +564,7 @@ static struct option options[] =
     {"device-map", required_argument, 0, 'm'},
     {"root-device", required_argument, 0, 'r'},
     {"force", no_argument, 0, 'f'},
+    {"destroy-xfs", no_argument, 0, 'X'},
     {"help", no_argument, 0, 'h'},
     {"version", no_argument, 0, 'V'},
     {"verbose", no_argument, 0, 'v'},
@@ -580,6 +589,7 @@ DEVICE must be a GRUB device (e.g. ``(hd0,1)'').\n\
   -m, --device-map=FILE   use FILE as the device map [default=%s]\n\
   -r, --root-device=DEV   use DEV as the root device [default=guessed]\n\
   -f, --force             install even if problems are detected\n\
+  --destroy-xfs           install even destroying XFS superblock\n\
   -h, --help              display this message and exit\n\
   -V, --version           print version information and exit\n\
   -v, --verbose           print verbose messages\n\
@@ -614,6 +624,7 @@ main (int argc, char *argv[])
   char *root_dev = 0;
   char *dest_dev;
   int must_embed = 0, force = 0;
+  int destroy_xfs = 0;
 
   progname = "grub-setup";
 
@@ -666,6 +677,10 @@ main (int argc, char *argv[])
 	    force = 1;
 	    break;
 
+	  case 'X':
+	    destroy_xfs = 1;
+	    break;
+
 	  case 'h':
 	    usage (0);
 	    break;
@@ -767,7 +782,8 @@ main (int argc, char *argv[])
 	  setup (dir ? : DEFAULT_DIRECTORY,
 		 boot_file ? : DEFAULT_BOOT_FILE,
 		 core_file ? : DEFAULT_CORE_FILE,
-		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
+		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force,
+		 destroy_xfs);
 	}
     }
   else
@@ -776,7 +792,7 @@ main (int argc, char *argv[])
     setup (dir ? : DEFAULT_DIRECTORY,
 	   boot_file ? : DEFAULT_BOOT_FILE,
 	   core_file ? : DEFAULT_CORE_FILE,
-	   root_dev, dest_dev, must_embed, force);
+	   root_dev, dest_dev, must_embed, force, destroy_xfs);
 
   /* Free resources.  */
   grub_fini_all ();

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

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

On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
>  
> +	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
> +	(options): New option --destroy-xfs.
> +	(main): Handle --destroy-xfs.

I gave this some more thought, and I think this could be less ad-hoc.  We're
treating XFS as if it were a "weird", unique thing just because it isn't biased
towards DOS-style boot like most filesystems are.

Instead, I've done something more generic, using our standard filesystem
probing engine which should be more reliable than a single memcmp.

I propose the attached patch.

-- 
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."

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

=== modified file 'ChangeLog'
--- ChangeLog	2009-10-16 20:21:12 +0000
+++ ChangeLog	2009-10-17 11:19:35 +0000
@@ -1,3 +1,13 @@
+2009-10-17  Robert Millan  <rmh.grub@aybabtu.com>
+
+	* include/grub/fs.h (struct grub_fs): Add `begins_at_first_sector'
+	member.
+	* fs/xfs.c (grub_xfs_fs): Initialize `begins_at_first_sector' to 1.
+	* util/i386/pc/grub-setup.c (setup): Add safety check that probes for
+	filesystems which begin at first sector.
+	(options): New option --skip-fs-probe.
+	(main): Handle --skip-fs-probe and pass it to setup().
+
 2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Let user specify OpenBSD root device.

=== modified file 'fs/xfs.c'
--- fs/xfs.c	2009-08-26 14:17:34 +0000
+++ fs/xfs.c	2009-10-17 11:19:35 +0000
@@ -805,6 +805,9 @@
     .close = grub_xfs_close,
     .label = grub_xfs_label,
     .uuid = grub_xfs_uuid,
+#ifdef GRUB_UTIL
+    .begins_at_first_sector = 1,
+#endif
     .next = 0
   };
 

=== modified file 'include/grub/fs.h'
--- include/grub/fs.h	2009-06-10 21:04:23 +0000
+++ include/grub/fs.h	2009-10-17 11:19:35 +0000
@@ -68,6 +68,12 @@
   /* Get writing time of filesystem. */
   grub_err_t (*mtime) (grub_device_t device, grub_int32_t *timebuf);
 
+#ifdef GRUB_UTIL
+  /* Whether this filesystem begins at first sector or reserves it for
+     DOS-style boot.  */
+  int begins_at_first_sector;
+#endif
+
   /* The next filesystem.  */
   struct grub_fs *next;
 };

=== modified file 'util/i386/pc/grub-setup.c'
--- util/i386/pc/grub-setup.c	2009-08-25 08:28:13 +0000
+++ util/i386/pc/grub-setup.c	2009-10-17 11:19:35 +0000
@@ -86,7 +86,7 @@
 static void
 setup (const char *dir,
        const char *boot_file, const char *core_file,
-       const char *root, const char *dest, int must_embed, int force)
+       const char *root, const char *dest, int must_embed, int force, int fs_probe)
 {
   char *boot_path, *core_path, *core_path_dev;
   char *boot_img, *core_img;
@@ -251,6 +251,16 @@
   if (grub_disk_read (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, tmp_img))
     grub_util_error ("%s", grub_errmsg);
 
+  if (fs_probe)
+    {
+      grub_fs_t fs;
+      fs = grub_fs_probe (dest_dev);
+      if (fs && fs->begins_at_first_sector)
+	grub_util_error ("%s appears to contain a %s filesystem, which would be "
+			 "overwritten by grub-setup (--skip-fs-probe disables this "
+			 "check, use at your own risk).", dest_dev->disk->name, fs->name);
+    }
+
   /* Copy the possible DOS BPB.  */
   memcpy (boot_img + GRUB_BOOT_MACHINE_BPB_START,
 	  tmp_img + GRUB_BOOT_MACHINE_BPB_START,
@@ -556,6 +566,7 @@
     {"device-map", required_argument, 0, 'm'},
     {"root-device", required_argument, 0, 'r'},
     {"force", no_argument, 0, 'f'},
+    {"skip-fs-probe", no_argument, 0, 's'},
     {"help", no_argument, 0, 'h'},
     {"version", no_argument, 0, 'V'},
     {"verbose", no_argument, 0, 'v'},
@@ -580,6 +591,7 @@
   -m, --device-map=FILE   use FILE as the device map [default=%s]\n\
   -r, --root-device=DEV   use DEV as the root device [default=guessed]\n\
   -f, --force             install even if problems are detected\n\
+  -s, --skip-fs-probe     do not probe for filesystems in DEVICE\n\
   -h, --help              display this message and exit\n\
   -V, --version           print version information and exit\n\
   -v, --verbose           print verbose messages\n\
@@ -613,7 +625,7 @@
   char *dev_map = 0;
   char *root_dev = 0;
   char *dest_dev;
-  int must_embed = 0, force = 0;
+  int must_embed = 0, force = 0, fs_probe = 1;
 
   progname = "grub-setup";
 
@@ -666,6 +678,10 @@
 	    force = 1;
 	    break;
 
+	  case 's':
+	    fs_probe = 0;
+	    break;
+
 	  case 'h':
 	    usage (0);
 	    break;
@@ -767,7 +783,7 @@
 	  setup (dir ? : DEFAULT_DIRECTORY,
 		 boot_file ? : DEFAULT_BOOT_FILE,
 		 core_file ? : DEFAULT_CORE_FILE,
-		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
+		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force, fs_probe);
 	}
     }
   else
@@ -776,7 +792,7 @@
     setup (dir ? : DEFAULT_DIRECTORY,
 	   boot_file ? : DEFAULT_BOOT_FILE,
 	   core_file ? : DEFAULT_CORE_FILE,
-	   root_dev, dest_dev, must_embed, force);
+	   root_dev, dest_dev, must_embed, force, fs_probe);
 
   /* Free resources.  */
   grub_fini_all ();

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
>>  
>> +	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
>> +	(options): New option --destroy-xfs.
>> +	(main): Handle --destroy-xfs.
>>     
>
> I gave this some more thought, and I think this could be less ad-hoc.  We're
> treating XFS as if it were a "weird", unique thing just because it isn't biased
> towards DOS-style boot like most filesystems are.
>
> Instead, I've done something more generic, using our standard filesystem
> probing engine which should be more reliable than a single memcmp.
>
>   
The danger is that fs_probe may reject filesystem as valid just because
it's newer than expected.
> I propose the attached patch.
>
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Sat, Oct 17, 2009 at 01:43:37PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >   
> >>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
> >>  
> >> +	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
> >> +	(options): New option --destroy-xfs.
> >> +	(main): Handle --destroy-xfs.
> >>     
> >
> > I gave this some more thought, and I think this could be less ad-hoc.  We're
> > treating XFS as if it were a "weird", unique thing just because it isn't biased
> > towards DOS-style boot like most filesystems are.
> >
> > Instead, I've done something more generic, using our standard filesystem
> > probing engine which should be more reliable than a single memcmp.
> >
> >   
> The danger is that fs_probe may reject filesystem as valid just because
> it's newer than expected.

What do you mean with "reject filesystem as valid"?

-- 
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] Refuse to install on XFS destroying its superblock

[Felix Zielcke]

Am Samstag, den 17.10.2009, 14:00 +0200 schrieb Robert Millan:
> On Sat, Oct 17, 2009 at 01:43:37PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Robert Millan wrote:
> > > On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > >   
> > >>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
> > >>  
> > >> +	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
> > >> +	(options): New option --destroy-xfs.
> > >> +	(main): Handle --destroy-xfs.
> > >>     
> > >
> > > I gave this some more thought, and I think this could be less ad-hoc.  We're
> > > treating XFS as if it were a "weird", unique thing just because it isn't biased
> > > towards DOS-style boot like most filesystems are.
> > >
> > > Instead, I've done something more generic, using our standard filesystem
> > > probing engine which should be more reliable than a single memcmp.
> > >
> > >   
> > The danger is that fs_probe may reject filesystem as valid just because
> > it's newer than expected.
> 
> What do you mean with "reject filesystem as valid"?

For example with the extN filesystems we reject them as valid if they
use INCOMPAT flags we don't support.
For example external ext3/4 journal devices, which caused a reboot or
segfault or something like that, before we commited Javier's patch for
it.
In that case it doestn't matter because the first sector is still unused
but for other filesystems this could maybe be a problem.



-- 
Felix Zielcke
Proud Debian Maintainer and GNU GRUB developer

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Sat, Oct 17, 2009 at 01:43:37PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>   
>>>       
>>>>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
>>>>  
>>>> +	* util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
>>>> +	(options): New option --destroy-xfs.
>>>> +	(main): Handle --destroy-xfs.
>>>>     
>>>>         
>>> I gave this some more thought, and I think this could be less ad-hoc.  We're
>>> treating XFS as if it were a "weird", unique thing just because it isn't biased
>>> towards DOS-style boot like most filesystems are.
>>>
>>> Instead, I've done something more generic, using our standard filesystem
>>> probing engine which should be more reliable than a single memcmp.
>>>
>>>   
>>>       
>> The danger is that fs_probe may reject filesystem as valid just because
>> it's newer than expected.
>>     
>
> What do you mean with "reject filesystem as valid"?
>
>   
Sorry for being unclear. I just meant that if some XFS structures are
updated then our xfs driver won't recognise it as xfs


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[richardvoigt]

On Sat, Oct 17, 2009 at 7:09 AM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Robert Millan wrote:
>> On Sat, Oct 17, 2009 at 01:43:37PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>
>>> Robert Millan wrote:
>>>
>>>> On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>>
>>>>
>>>>>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
>>>>>
>>>>> +  * util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
>>>>> +  (options): New option --destroy-xfs.
>>>>> +  (main): Handle --destroy-xfs.
>>>>>
>>>>>
>>>> I gave this some more thought, and I think this could be less ad-hoc.  We're
>>>> treating XFS as if it were a "weird", unique thing just because it isn't biased
>>>> towards DOS-style boot like most filesystems are.
>>>>
>>>> Instead, I've done something more generic, using our standard filesystem
>>>> probing engine which should be more reliable than a single memcmp.
>>>>
>>>>
>>>>
>>> The danger is that fs_probe may reject filesystem as valid just because
>>> it's newer than expected.
>>>
>>
>> What do you mean with "reject filesystem as valid"?
>>
>>
> Sorry for being unclear. I just meant that if some XFS structures are
> updated then our xfs driver won't recognise it as xfs


Then instead of blacklisting xfs, why not whitelist filesystems which
are known to have usable blocks for embedding (doesn't the number of
blocks vary with filesystem anyway?) and an override parameter
(--into-unrecognized-fs).

>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

richardvoigt@gmail.com wrote:
> On Sat, Oct 17, 2009 at 7:09 AM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Sat, Oct 17, 2009 at 01:43:37PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>
>>>       
>>>> Robert Millan wrote:
>>>>
>>>>         
>>>>> On Sat, Oct 17, 2009 at 12:18:05AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>>>
>>>>>
>>>>>           
>>>>>>  2009-10-16  Vladimir Serbinenko  <phcoder@gmail.com>
>>>>>>
>>>>>> +  * util/i386/pc/grub-setup.c (setup): Refuse to overwrite XFS superblock.
>>>>>> +  (options): New option --destroy-xfs.
>>>>>> +  (main): Handle --destroy-xfs.
>>>>>>
>>>>>>
>>>>>>             
>>>>> I gave this some more thought, and I think this could be less ad-hoc.  We're
>>>>> treating XFS as if it were a "weird", unique thing just because it isn't biased
>>>>> towards DOS-style boot like most filesystems are.
>>>>>
>>>>> Instead, I've done something more generic, using our standard filesystem
>>>>> probing engine which should be more reliable than a single memcmp.
>>>>>
>>>>>
>>>>>
>>>>>           
>>>> The danger is that fs_probe may reject filesystem as valid just because
>>>> it's newer than expected.
>>>>
>>>>         
>>> What do you mean with "reject filesystem as valid"?
>>>
>>>
>>>       
>> Sorry for being unclear. I just meant that if some XFS structures are
>> updated then our xfs driver won't recognise it as xfs
>>     
>
>
> Then instead of blacklisting xfs, why not whitelist filesystems which
> are known to have usable blocks for embedding (doesn't the number of
> blocks vary with filesystem anyway?) and an override parameter
> (--into-unrecognized-fs).
>
>   
In long term it's the best possibility but in this case we're too near
to a release. What I want is just to avoid XFS pitfall many users may
step into
>> --
>> Regards
>> Vladimir 'phcoder' Serbinenko
>> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>     
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Sat, Oct 17, 2009 at 02:09:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> The danger is that fs_probe may reject filesystem as valid just because
> >> it's newer than expected.
> >>     
> >
> > What do you mean with "reject filesystem as valid"?
> >
> >   
> Sorry for being unclear. I just meant that if some XFS structures are
> updated then our xfs driver won't recognise it as xfs

What do you mean with "updated"?  You mean a new implementation of XFS?  Or
the same instance of XFS that has been modified after use?

-- 
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] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Sat, Oct 17, 2009 at 02:09:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>>>> The danger is that fs_probe may reject filesystem as valid just because
>>>> it's newer than expected.
>>>>     
>>>>         
>>> What do you mean with "reject filesystem as valid"?
>>>
>>>   
>>>       
>> Sorry for being unclear. I just meant that if some XFS structures are
>> updated then our xfs driver won't recognise it as xfs
>>     
>
> What do you mean with "updated"?  You mean a new implementation of XFS?  Or
> the same instance of XFS that has been modified after use?
>
>   
I mean next version on XFS


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

On Sun, Oct 18, 2009 at 06:30:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Sat, Oct 17, 2009 at 02:09:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >   
> >>>> The danger is that fs_probe may reject filesystem as valid just because
> >>>> it's newer than expected.
> >>>>     
> >>>>         
> >>> What do you mean with "reject filesystem as valid"?
> >>>
> >>>   
> >>>       
> >> Sorry for being unclear. I just meant that if some XFS structures are
> >> updated then our xfs driver won't recognise it as xfs
> >>     
> >
> > What do you mean with "updated"?  You mean a new implementation of XFS?  Or
> > the same instance of XFS that has been modified after use?
> >
> >   
> I mean next version on XFS

Sorry, but what did you expect?  You want to prevent PEBCAK using
heuristic.  There's no way we can tell if those 512 bytes are valuable
data, only the user can.  And even if you try to err on the safest side,
there's no garantee that newer versions of XFS, or other filesystems that
don't even exist yet will be detectable by us no matter what we do.

Why don't we just take a backup like someone suggested a while ago?  I
think there was even a patch.  This way if valuable data is lost, user can
restore it (and while at it, learnt his lesson that filesystems and embedded
code aren't really supposed to be mixed in the same place).

-- 
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] Refuse to install on XFS destroying its superblock

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> On Sun, Oct 18, 2009 at 06:30:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Sat, Oct 17, 2009 at 02:09:31PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>   
>>>       
>>>>>> The danger is that fs_probe may reject filesystem as valid just because
>>>>>> it's newer than expected.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> What do you mean with "reject filesystem as valid"?
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> Sorry for being unclear. I just meant that if some XFS structures are
>>>> updated then our xfs driver won't recognise it as xfs
>>>>     
>>>>         
>>> What do you mean with "updated"?  You mean a new implementation of XFS?  Or
>>> the same instance of XFS that has been modified after use?
>>>
>>>   
>>>       
>> I mean next version on XFS
>>     
>
> Sorry, but what did you expect?  You want to prevent PEBCAK using
> heuristic.  There's no way we can tell if those 512 bytes are valuable
> data, only the user can.  And even if you try to err on the safest side,
> there's no garantee that newer versions of XFS, or other filesystems that
> don't even exist yet will be detectable by us no matter what we do.
>
>   
I think best way is to have whitelist of OS which have first sector free
and possible manual override like it was suggested.
> Why don't we just take a backup like someone suggested a while ago?  I
> think there was even a patch.  This way if valuable data is lost, user can
> restore it (and while at it, learnt his lesson that filesystems and embedded
> code aren't really supposed to be mixed in the same place).
>
>   
The backup is inevitably stored on the filesystem itself which becomes
inaccessible once superblock is destroyed.


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Refuse to install on XFS destroying its superblock

[Robert Millan]

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


Ok, let's try this:  We refuse to install on a partition UNLESS:

  - A filesystem can be identified in it.

  - This filesystem is known to reserve the first block for DOS-style
    chainload.

If these conditions aren't met, user will have to override our check.

Patch attached.  Also in people/robertmh/grub-setup-fs-probe.

-- 
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."

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

=== modified file 'ChangeLog'
--- ChangeLog	2009-10-24 23:13:27 +0000
+++ ChangeLog	2009-10-24 23:57:58 +0000
@@ -1,5 +1,15 @@
 2009-10-25  Robert Millan  <rmh.grub@aybabtu.com>
 
+	* include/grub/fs.h (struct grub_fs): Add `reserved_first_sector'
+	member.
+	* fs/ext2.c (grub_ext2_fs): Initialize `reserved_first_sector' to 1.
+	* util/i386/pc/grub-setup.c (setup): Add safety check that probes for
+	filesystems which begin at first sector.
+	(options): New option --skip-fs-probe.
+	(main): Handle --skip-fs-probe and pass it to setup().
+
+2009-10-25  Robert Millan  <rmh.grub@aybabtu.com>
+
 	* fs/cpio.c [MODE_USTAR]: Finish `tar' module instead of
 	`cpio'.
 	[! MODE_USTAR]: Finish `cpio' module instead of `tar'.

=== modified file 'fs/ext2.c'
--- fs/ext2.c	2009-07-19 13:59:21 +0000
+++ fs/ext2.c	2009-10-24 23:57:58 +0000
@@ -924,6 +924,7 @@
     .label = grub_ext2_label,
     .uuid = grub_ext2_uuid,
     .mtime = grub_ext2_mtime,
+    .reserved_first_sector = 1,
     .next = 0
   };
 

=== modified file 'include/grub/fs.h'
--- include/grub/fs.h	2009-06-10 21:04:23 +0000
+++ include/grub/fs.h	2009-10-24 23:57:58 +0000
@@ -68,6 +68,11 @@
   /* Get writing time of filesystem. */
   grub_err_t (*mtime) (grub_device_t device, grub_int32_t *timebuf);
 
+#ifdef GRUB_UTIL
+  /* Whether this filesystem reserves first sector for DOS-style boot.  */
+  int reserved_first_sector;
+#endif
+
   /* The next filesystem.  */
   struct grub_fs *next;
 };

=== modified file 'util/i386/pc/grub-setup.c'
--- util/i386/pc/grub-setup.c	2009-08-25 08:28:13 +0000
+++ util/i386/pc/grub-setup.c	2009-10-24 23:57:58 +0000
@@ -86,7 +86,7 @@
 static void
 setup (const char *dir,
        const char *boot_file, const char *core_file,
-       const char *root, const char *dest, int must_embed, int force)
+       const char *root, const char *dest, int must_embed, int force, int fs_probe)
 {
   char *boot_path, *core_path, *core_path_dev;
   char *boot_img, *core_img;
@@ -251,6 +251,21 @@
   if (grub_disk_read (dest_dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, tmp_img))
     grub_util_error ("%s", grub_errmsg);
 
+  if (dest_dev->disk->partition && fs_probe)
+    {
+      grub_fs_t fs;
+      fs = grub_fs_probe (dest_dev);
+      if (! fs)
+	grub_util_error ("Unable to identify a filesystem in %s; safety check can't be performed.");
+
+      if (! fs->reserved_first_sector)
+	grub_util_error ("%s appears to contain a %s filesystem which isn't known to "
+			 "reserve space for DOS-style boot.  Installing GRUB there could "
+			 "result in FILESYSTEM DESTRUCTION if valuable data is overwritten "
+			 "by grub-setup (--skip-fs-probe disables this "
+			 "check, use at your own risk).", dest_dev->disk->name, fs->name);
+    }
+
   /* Copy the possible DOS BPB.  */
   memcpy (boot_img + GRUB_BOOT_MACHINE_BPB_START,
 	  tmp_img + GRUB_BOOT_MACHINE_BPB_START,
@@ -556,6 +571,7 @@
     {"device-map", required_argument, 0, 'm'},
     {"root-device", required_argument, 0, 'r'},
     {"force", no_argument, 0, 'f'},
+    {"skip-fs-probe", no_argument, 0, 's'},
     {"help", no_argument, 0, 'h'},
     {"version", no_argument, 0, 'V'},
     {"verbose", no_argument, 0, 'v'},
@@ -580,6 +596,7 @@
   -m, --device-map=FILE   use FILE as the device map [default=%s]\n\
   -r, --root-device=DEV   use DEV as the root device [default=guessed]\n\
   -f, --force             install even if problems are detected\n\
+  -s, --skip-fs-probe     do not probe for filesystems in DEVICE\n\
   -h, --help              display this message and exit\n\
   -V, --version           print version information and exit\n\
   -v, --verbose           print verbose messages\n\
@@ -613,7 +630,7 @@
   char *dev_map = 0;
   char *root_dev = 0;
   char *dest_dev;
-  int must_embed = 0, force = 0;
+  int must_embed = 0, force = 0, fs_probe = 1;
 
   progname = "grub-setup";
 
@@ -666,6 +683,10 @@
 	    force = 1;
 	    break;
 
+	  case 's':
+	    fs_probe = 0;
+	    break;
+
 	  case 'h':
 	    usage (0);
 	    break;
@@ -767,7 +788,7 @@
 	  setup (dir ? : DEFAULT_DIRECTORY,
 		 boot_file ? : DEFAULT_BOOT_FILE,
 		 core_file ? : DEFAULT_CORE_FILE,
-		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force);
+		 root_dev, grub_util_get_grub_dev (devicelist[i]), 1, force, fs_probe);
 	}
     }
   else
@@ -776,7 +797,7 @@
     setup (dir ? : DEFAULT_DIRECTORY,
 	   boot_file ? : DEFAULT_BOOT_FILE,
 	   core_file ? : DEFAULT_CORE_FILE,
-	   root_dev, dest_dev, must_embed, force);
+	   root_dev, dest_dev, must_embed, force, fs_probe);
 
   /* Free resources.  */
   grub_fini_all ();