Re: [BUG] grub-install fails on devices with high minor numbers

Re: [BUG] grub-install fails on devices with high minor numbers

[Andrei Borzenkov]

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

06.11.2015 19:08, Tim Walberg пишет:
> On some relatively large systems that are used for virtualization,
> often containing upwards of 100 virtual machines, we've started running
> into issues with GRUB not being able to install on a new virtual machine
> (using the typical process of mounting all the VM file systems on the host
> and running grub-install under chroot). We've tracked the issue down to this
> code, which is clearly erroneous - device minor numbers have been more than
> 8 bits for quite some time now - in grub-core/osdep/devmapper/getroot.c:
>
>      char *
>      grub_util_devmapper_part_to_disk (struct stat *st,
>                                    int *is_part, const char *path)
>      {
>        int major, minor;
>
>        if (grub_util_get_dm_node_linear_info (st->st_rdev,
>                                           &major, &minor, 0))
>          {
>            *is_part = 1;
>            return grub_find_device ("/dev",
>                                     (major << 8) | minor);         <<<<< --------- ERROR!
>          }
>        *is_part = 0;
>        return xstrdup (path);
>      }
>
> When we have enough device-mapper devices (including all their partitions)
> on a host that the next newly-added set of devices ends up with minor numbers
> outside the 8-bit range, this code fails, with the result that grub-install
> can't find the devices that it needs to complete the install.
>
> There might be other places in the code where similar assumptions are made.
> Someone more familiar with the code would probably be better for tracking
> those down. But, this one at least needs to be fixed.
>


Does attached patch help?

[-- Attachment #2: devmapper-getroot.patch --]
[-- Type: text/x-patch, Size: 886 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] devmapper/getroot: use makedev instead of direct shift

Fixes device detection with large number of devices.

Reported by Tim Wallberg <twalberg@comcast.net>

---
 grub-core/osdep/devmapper/getroot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 0a77a04..64419f6 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -208,8 +208,7 @@ grub_util_devmapper_part_to_disk (struct stat *st,
 					 &major, &minor, 0))
     {
       *is_part = 1;
-      return grub_find_device ("/dev",
-			       (major << 8) | minor);
+      return grub_find_device ("/dev", makedev (major, minor));
     }
   *is_part = 0;
   return xstrdup (path);
-- 
tg: (cd6d79c..) u/makedev (depends on: master)

Re: [BUG] grub-install fails on devices with high minor numbers

[Tim Walberg]

On 11/06/2015 21:26 +0300, Andrei Borzenkov wrote:
>>	06.11.2015 19:08, Tim Walberg ??????????:
>>	>On some relatively large systems that are used for virtualization,
>>	>often containing upwards of 100 virtual machines, we've started running
>>	>into issues with GRUB not being able to install on a new virtual machine
>>	>(using the typical process of mounting all the VM file systems on the host
>>	>and running grub-install under chroot). We've tracked the issue down to this
>>	>code, which is clearly erroneous - device minor numbers have been more than
>>	>8 bits for quite some time now - in grub-core/osdep/devmapper/getroot.c:
>>	>
>>	>     char *
>>	>     grub_util_devmapper_part_to_disk (struct stat *st,
>>	>                                   int *is_part, const char *path)
>>	>     {
>>	>       int major, minor;
>>	>
>>	>       if (grub_util_get_dm_node_linear_info (st->st_rdev,
>>	>                                          &major, &minor, 0))
>>	>         {
>>	>           *is_part = 1;
>>	>           return grub_find_device ("/dev",
>>	>                                    (major << 8) | minor);         <<<<< --------- ERROR!
>>	>         }
>>	>       *is_part = 0;
>>	>       return xstrdup (path);
>>	>     }
>>	>
>>	>When we have enough device-mapper devices (including all their partitions)
>>	>on a host that the next newly-added set of devices ends up with minor numbers
>>	>outside the 8-bit range, this code fails, with the result that grub-install
>>	>can't find the devices that it needs to complete the install.
>>	>
>>	>There might be other places in the code where similar assumptions are made.
>>	>Someone more familiar with the code would probably be better for tracking
>>	>those down. But, this one at least needs to be fixed.
>>	>
>>	
>>	
>>	Does attached patch help?

>>	From: Andrei Borzenkov <arvidjaar@gmail.com>
>>	Subject: [PATCH] devmapper/getroot: use makedev instead of direct shift
>>	
>>	Fixes device detection with large number of devices.
>>	
>>	Reported by Tim Wallberg <twalberg@comcast.net>
>>	
>>	---
>>	 grub-core/osdep/devmapper/getroot.c | 3 +--
>>	 1 file changed, 1 insertion(+), 2 deletions(-)
>>	
>>	diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
>>	index 0a77a04..64419f6 100644
>>	--- a/grub-core/osdep/devmapper/getroot.c
>>	+++ b/grub-core/osdep/devmapper/getroot.c
>>	@@ -208,8 +208,7 @@ grub_util_devmapper_part_to_disk (struct stat *st,
>>	 					 &major, &minor, 0))
>>	     {
>>	       *is_part = 1;
>>	-      return grub_find_device ("/dev",
>>	-			       (major << 8) | minor);
>>	+      return grub_find_device ("/dev", makedev (major, minor));
>>	     }
>>	   *is_part = 0;
>>	   return xstrdup (path);
>>	-- 
>>	tg: (cd6d79c..) u/makedev (depends on: master)

End of included message


Yes, that appears to have resolved the issue. I was already testing essentially
that patch on my own, but wasn't sure if there might be similar assumptions
made elsewhere in the code. This single fix handles at least the use case we
were having issues with.

	    Thanks!
		  tw


-- 
twalberg@gmail.com

Re: [BUG] grub-install fails on devices with high minor numbers

[Andrei Borzenkov]

06.11.2015 22:59, Tim Walberg пишет:
> On 11/06/2015 21:26 +0300, Andrei Borzenkov wrote:
>>> 	06.11.2015 19:08, Tim Walberg ??????????:
>>> 	>On some relatively large systems that are used for virtualization,
>>> 	>often containing upwards of 100 virtual machines, we've started running
>>> 	>into issues with GRUB not being able to install on a new virtual machine
>>> 	>(using the typical process of mounting all the VM file systems on the host
>>> 	>and running grub-install under chroot). We've tracked the issue down to this
>>> 	>code, which is clearly erroneous - device minor numbers have been more than
>>> 	>8 bits for quite some time now - in grub-core/osdep/devmapper/getroot.c:
>>> 	>
>>> 	>     char *
>>> 	>     grub_util_devmapper_part_to_disk (struct stat *st,
>>> 	>                                   int *is_part, const char *path)
>>> 	>     {
>>> 	>       int major, minor;
>>> 	>
>>> 	>       if (grub_util_get_dm_node_linear_info (st->st_rdev,
>>> 	>                                          &major, &minor, 0))
>>> 	>         {
>>> 	>           *is_part = 1;
>>> 	>           return grub_find_device ("/dev",
>>> 	>                                    (major << 8) | minor);         <<<<< --------- ERROR!
>>> 	>         }
>>> 	>       *is_part = 0;
>>> 	>       return xstrdup (path);
>>> 	>     }
>>> 	>
>>> 	>When we have enough device-mapper devices (including all their partitions)
>>> 	>on a host that the next newly-added set of devices ends up with minor numbers
>>> 	>outside the 8-bit range, this code fails, with the result that grub-install
>>> 	>can't find the devices that it needs to complete the install.
>>> 	>
>>> 	>There might be other places in the code where similar assumptions are made.
>>> 	>Someone more familiar with the code would probably be better for tracking
>>> 	>those down. But, this one at least needs to be fixed.
>>> 	>
>>> 	
>>> 	
>>> 	Does attached patch help?
>
>>> 	From: Andrei Borzenkov <arvidjaar@gmail.com>
>>> 	Subject: [PATCH] devmapper/getroot: use makedev instead of direct shift
>>> 	
>>> 	Fixes device detection with large number of devices.
>>> 	
>>> 	Reported by Tim Wallberg <twalberg@comcast.net>
>>> 	
>>> 	---
>>> 	 grub-core/osdep/devmapper/getroot.c | 3 +--
>>> 	 1 file changed, 1 insertion(+), 2 deletions(-)
>>> 	
>>> 	diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
>>> 	index 0a77a04..64419f6 100644
>>> 	--- a/grub-core/osdep/devmapper/getroot.c
>>> 	+++ b/grub-core/osdep/devmapper/getroot.c
>>> 	@@ -208,8 +208,7 @@ grub_util_devmapper_part_to_disk (struct stat *st,
>>> 	 					 &major, &minor, 0))
>>> 	     {
>>> 	       *is_part = 1;
>>> 	-      return grub_find_device ("/dev",
>>> 	-			       (major << 8) | minor);
>>> 	+      return grub_find_device ("/dev", makedev (major, minor));
>>> 	     }
>>> 	   *is_part = 0;
>>> 	   return xstrdup (path);
>>> 	--
>>> 	tg: (cd6d79c..) u/makedev (depends on: master)
>
> End of included message
>
>
> Yes, that appears to have resolved the issue. I was already testing essentially
> that patch on my own, but wasn't sure if there might be similar assumptions
> made elsewhere in the code.

As far as I can tell, other places used makedev() already. Committed. 
Thanks!