Fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

Fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

[sean finney]

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

okay, I think the attached patch should fix the problem.

I haven't tested it thoroughly, though my system does boot.  It seems
there may be a seperate issue with os-prober that results in some junk
entries being added to grub.conf if the snapshot volumes happen to be
root filesystems, but that probably needs to be taken up seperately and
the critical aspect of the bug is fixed anyway.


	sean

[-- Attachment #1.2: 574863.patch.3.diff --]
[-- Type: text/x-diff, Size: 2907 bytes --]

Author: Sean Finney <seanius@debian.org>
Description: Fix for lvm2 parsing failures with snapshot logical volumes

This patch prevents the lvm2 parsing code from prematurely aborting
when encountering LV and segment metadata related to snapshot volumes.
Instead, the parser will now skip over these as if it never saw them,
which is probably the safest thing to do without a major injection of
lvm2 support code.

Bug-Debian: #574863
--- disk/lvm.c	2010-04-27 15:25:12 +0000
+++ disk/lvm.c	2010-06-02 22:12:58 +0000
@@ -420,9 +420,11 @@
 	  /* And add all the lvs to the volume group. */
 	  while (1)
 	    {
-	      int s;
+	      int s, skip_lv = 0, status_visible = 0;
 	      struct grub_lvm_lv *lv;
 	      struct grub_lvm_segment *seg;
+	      char *status = NULL, *status_end = NULL;
+	      grub_size_t status_len = 0;
 
 	      while (grub_isspace (*p))
 		p++;
@@ -431,6 +433,8 @@
 		break;
 
 	      lv = grub_malloc (sizeof (*lv));
+		skip_lv = 0; /*Flag to skip snapshots */
+		status_visible = 0; /*Flag to skip non-visible LV's */
 
 	      q = p;
 	      while (*q != ' ')
@@ -445,6 +449,25 @@
 
 	      lv->size = 0;
 
+		/* read LV status and ignore ones not listed as "VISIBLE" */
+		p = grub_strstr (p, "status = ");
+		if (p == NULL)
+			goto lvs_fail;
+		status_end = grub_strchr(p, ']');
+		if (status_end == NULL)
+			goto lvs_fail;
+		status_len = (status_end - p) + 1;
+		status = grub_malloc(status_len + 1);
+		if (status == NULL)
+			goto lvs_fail;
+		grub_memcpy(status, p, status_len);
+		status[status_len] = '\0';
+		if (grub_strstr(status, "VISIBLE") != NULL)
+			status_visible = 1;
+		grub_free(status);
+		if (!status_visible)
+			goto lv_parsed;  /* don't bother parsing this one */
+
 	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
 	      if (p == NULL)
 		goto lvs_fail;
@@ -465,6 +488,18 @@
 		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
+
+			/* Skip LV's that have snapshot segments */
+			p = grub_strstr (p, "type = ");
+			if (p == NULL)
+				goto lvs_segment_fail;
+			p += sizeof("type = ") - 1;
+			if (!grub_strncmp(p, "\"snapshot\"", 10)) {
+				/* Found a snapshot, give up and move on */
+				skip_lv=1;
+				break;
+			}
+
 		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
@@ -531,12 +566,19 @@
 		  goto fail4;
 		}
 
+	    lv_parsed: /* all done with parsing this LV, seek to the end */
 	      if (p != NULL)
 		p = grub_strchr (p, '}');
 	      if (p == NULL)
 		goto lvs_fail;
 	      p += 3;
 
+		if (skip_lv || ! status_visible) {
+			grub_free (lv->name);
+			grub_free (lv);
+			continue;
+		}
+
 	      lv->number = lv_count++;
 	      lv->vg = vg;
 	      lv->next = vg->lvs;


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] Updated fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

[sean finney]

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

and here is a slightly updated patch.  there is no functional change in the
code, i have only reformatted the whitespace etc so that the code matches
with the style of the surrounding code.

i have also tested this now on my primary testing system and haven't
noticed any problems.


	sean

On Thu, Jun 03, 2010 at 12:41:32AM +0200, sean finney wrote:
> okay, I think the attached patch should fix the problem.
> 
> I haven't tested it thoroughly, though my system does boot.  It seems
> there may be a seperate issue with os-prober that results in some junk
> entries being added to grub.conf if the snapshot volumes happen to be
> root filesystems, but that probably needs to be taken up seperately and
> the critical aspect of the bug is fixed anyway.
> 
> 
> 	sean

> Author: Sean Finney <seanius@debian.org>
> Description: Fix for lvm2 parsing failures with snapshot logical volumes
> 
> This patch prevents the lvm2 parsing code from prematurely aborting
> when encountering LV and segment metadata related to snapshot volumes.
> Instead, the parser will now skip over these as if it never saw them,
> which is probably the safest thing to do without a major injection of
> lvm2 support code.
> 
> Bug-Debian: #574863
> --- disk/lvm.c	2010-04-27 15:25:12 +0000
> +++ disk/lvm.c	2010-06-02 22:12:58 +0000
> @@ -420,9 +420,11 @@
>  	  /* And add all the lvs to the volume group. */
>  	  while (1)
>  	    {
> -	      int s;
> +	      int s, skip_lv = 0, status_visible = 0;
>  	      struct grub_lvm_lv *lv;
>  	      struct grub_lvm_segment *seg;
> +	      char *status = NULL, *status_end = NULL;
> +	      grub_size_t status_len = 0;
>  
>  	      while (grub_isspace (*p))
>  		p++;
> @@ -431,6 +433,8 @@
>  		break;
>  
>  	      lv = grub_malloc (sizeof (*lv));
> +		skip_lv = 0; /*Flag to skip snapshots */
> +		status_visible = 0; /*Flag to skip non-visible LV's */
>  
>  	      q = p;
>  	      while (*q != ' ')
> @@ -445,6 +449,25 @@
>  
>  	      lv->size = 0;
>  
> +		/* read LV status and ignore ones not listed as "VISIBLE" */
> +		p = grub_strstr (p, "status = ");
> +		if (p == NULL)
> +			goto lvs_fail;
> +		status_end = grub_strchr(p, ']');
> +		if (status_end == NULL)
> +			goto lvs_fail;
> +		status_len = (status_end - p) + 1;
> +		status = grub_malloc(status_len + 1);
> +		if (status == NULL)
> +			goto lvs_fail;
> +		grub_memcpy(status, p, status_len);
> +		status[status_len] = '\0';
> +		if (grub_strstr(status, "VISIBLE") != NULL)
> +			status_visible = 1;
> +		grub_free(status);
> +		if (!status_visible)
> +			goto lv_parsed;  /* don't bother parsing this one */
> +
>  	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
>  	      if (p == NULL)
>  		goto lvs_fail;
> @@ -465,6 +488,18 @@
>  		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
>  		  if (p == NULL)
>  		    goto lvs_segment_fail;
> +
> +			/* Skip LV's that have snapshot segments */
> +			p = grub_strstr (p, "type = ");
> +			if (p == NULL)
> +				goto lvs_segment_fail;
> +			p += sizeof("type = ") - 1;
> +			if (!grub_strncmp(p, "\"snapshot\"", 10)) {
> +				/* Found a snapshot, give up and move on */
> +				skip_lv=1;
> +				break;
> +			}
> +
>  		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
>  		  if (p == NULL)
>  		    goto lvs_segment_fail;
> @@ -531,12 +566,19 @@
>  		  goto fail4;
>  		}
>  
> +	    lv_parsed: /* all done with parsing this LV, seek to the end */
>  	      if (p != NULL)
>  		p = grub_strchr (p, '}');
>  	      if (p == NULL)
>  		goto lvs_fail;
>  	      p += 3;
>  
> +		if (skip_lv || ! status_visible) {
> +			grub_free (lv->name);
> +			grub_free (lv);
> +			continue;
> +		}
> +
>  	      lv->number = lv_count++;
>  	      lv->vg = vg;
>  	      lv->next = vg->lvs;
> 




-- 

[-- Attachment #1.2: 574863.patch.4.diff --]
[-- Type: text/x-diff, Size: 3116 bytes --]

Author: Sean Finney <seanius@debian.org>
Description: Fix for lvm2 parsing failures with snapshot logical volumes
Forwarded: yes

This patch prevents the lvm2 parsing code from prematurely aborting
when encountering LV and segment metadata related to snapshot volumes.
Instead, the parser will now skip over these as if it never saw them,
which is probably the safest thing to do without a major injection of
lvm2 support code.

Bug-Debian: #574863
--- a/disk/lvm.c	2010-04-27 15:25:12 +0000
+++ b/disk/lvm.c	2010-06-03 16:06:33 +0000
@@ -420,9 +420,11 @@
 	  /* And add all the lvs to the volume group. */
 	  while (1)
 	    {
-	      int s;
+	      int s, skip_lv = 0, status_visible = 0;
 	      struct grub_lvm_lv *lv;
 	      struct grub_lvm_segment *seg;
+	      char *status = NULL, *status_end = NULL;
+	      grub_size_t status_len = 0;
 
 	      while (grub_isspace (*p))
 		p++;
@@ -431,6 +433,8 @@
 		break;
 
 	      lv = grub_malloc (sizeof (*lv));
+	      skip_lv = 0; /*Flag to skip snapshots */
+	      status_visible = 0; /*Flag to skip non-visible LV's */
 
 	      q = p;
 	      while (*q != ' ')
@@ -445,6 +449,25 @@
 
 	      lv->size = 0;
 
+	      /* read LV status and ignore ones not listed as "VISIBLE" */
+	      p = grub_strstr (p, "status = ");
+	      if (p == NULL)
+	        goto lvs_fail;
+	      status_end = grub_strchr (p, ']');
+	      if (status_end == NULL)
+	        goto lvs_fail;
+	      status_len = (status_end - p) + 1;
+	      status = grub_malloc (status_len + 1);
+	      if (status == NULL)
+	        goto lvs_fail;
+	      grub_memcpy (status, p, status_len);
+	      status[status_len] = '\0';
+	      if (grub_strstr (status, "VISIBLE") != NULL)
+	        status_visible = 1;
+	      grub_free (status);
+	      if (!status_visible)
+	        goto lv_parsed;  /* don't bother parsing this one */
+
 	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
 	      if (p == NULL)
 		goto lvs_fail;
@@ -465,6 +488,19 @@
 		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
+
+		  /* Skip LV's that have snapshot segments */
+		  p = grub_strstr (p, "type = ");
+		  if (p == NULL)
+		    goto lvs_segment_fail;
+		  p += sizeof ("type = ") - 1;
+		  if (!grub_strncmp (p, "\"snapshot\"", 10))
+		    {
+		      /* Found a snapshot, give up and move on */
+		      skip_lv=1;
+		      break;
+		    }
+
 		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
@@ -531,12 +567,20 @@
 		  goto fail4;
 		}
 
+	    lv_parsed: /* all done with parsing this LV, seek to the end */
 	      if (p != NULL)
 		p = grub_strchr (p, '}');
 	      if (p == NULL)
 		goto lvs_fail;
 	      p += 3;
 
+	      if (skip_lv || ! status_visible)
+	        {
+	          grub_free (lv->name);
+	          grub_free (lv);
+	          continue;
+	        }
+
 	      lv->number = lv_count++;
 	      lv->vg = vg;
 	      lv->next = vg->lvs;


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] Updated fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/03/2010 08:41 PM, sean finney wrote:
> and here is a slightly updated patch.  there is no functional change in the
> code, i have only reformatted the whitespace etc so that the code matches
> with the style of the surrounding code.
>
> i have also tested this now on my primary testing system and haven't
> noticed any problems.
>
>   
I've cleaned up your patch using my more generic string parsing
functions. Could you test attached patch?
> 	sean
>
> On Thu, Jun 03, 2010 at 12:41:32AM +0200, sean finney wrote:
>   
>> okay, I think the attached patch should fix the problem.
>>
>> I haven't tested it thoroughly, though my system does boot.  It seems
>> there may be a seperate issue with os-prober that results in some junk
>> entries being added to grub.conf if the snapshot volumes happen to be
>> root filesystems, but that probably needs to be taken up seperately and
>> the critical aspect of the bug is fixed anyway.
>>
>>
>> 	sean
>>     
>   
>> Author: Sean Finney <seanius@debian.org>
>> Description: Fix for lvm2 parsing failures with snapshot logical volumes
>>
>> This patch prevents the lvm2 parsing code from prematurely aborting
>> when encountering LV and segment metadata related to snapshot volumes.
>> Instead, the parser will now skip over these as if it never saw them,
>> which is probably the safest thing to do without a major injection of
>> lvm2 support code.
>>
>> Bug-Debian: #574863
>> --- disk/lvm.c	2010-04-27 15:25:12 +0000
>> +++ disk/lvm.c	2010-06-02 22:12:58 +0000
>> @@ -420,9 +420,11 @@
>>  	  /* And add all the lvs to the volume group. */
>>  	  while (1)
>>  	    {
>> -	      int s;
>> +	      int s, skip_lv = 0, status_visible = 0;
>>  	      struct grub_lvm_lv *lv;
>>  	      struct grub_lvm_segment *seg;
>> +	      char *status = NULL, *status_end = NULL;
>> +	      grub_size_t status_len = 0;
>>  
>>  	      while (grub_isspace (*p))
>>  		p++;
>> @@ -431,6 +433,8 @@
>>  		break;
>>  
>>  	      lv = grub_malloc (sizeof (*lv));
>> +		skip_lv = 0; /*Flag to skip snapshots */
>> +		status_visible = 0; /*Flag to skip non-visible LV's */
>>  
>>  	      q = p;
>>  	      while (*q != ' ')
>> @@ -445,6 +449,25 @@
>>  
>>  	      lv->size = 0;
>>  
>> +		/* read LV status and ignore ones not listed as "VISIBLE" */
>> +		p = grub_strstr (p, "status = ");
>> +		if (p == NULL)
>> +			goto lvs_fail;
>> +		status_end = grub_strchr(p, ']');
>> +		if (status_end == NULL)
>> +			goto lvs_fail;
>> +		status_len = (status_end - p) + 1;
>> +		status = grub_malloc(status_len + 1);
>> +		if (status == NULL)
>> +			goto lvs_fail;
>> +		grub_memcpy(status, p, status_len);
>> +		status[status_len] = '\0';
>> +		if (grub_strstr(status, "VISIBLE") != NULL)
>> +			status_visible = 1;
>> +		grub_free(status);
>> +		if (!status_visible)
>> +			goto lv_parsed;  /* don't bother parsing this one */
>> +
>>  	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
>>  	      if (p == NULL)
>>  		goto lvs_fail;
>> @@ -465,6 +488,18 @@
>>  		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
>>  		  if (p == NULL)
>>  		    goto lvs_segment_fail;
>> +
>> +			/* Skip LV's that have snapshot segments */
>> +			p = grub_strstr (p, "type = ");
>> +			if (p == NULL)
>> +				goto lvs_segment_fail;
>> +			p += sizeof("type = ") - 1;
>> +			if (!grub_strncmp(p, "\"snapshot\"", 10)) {
>> +				/* Found a snapshot, give up and move on */
>> +				skip_lv=1;
>> +				break;
>> +			}
>> +
>>  		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
>>  		  if (p == NULL)
>>  		    goto lvs_segment_fail;
>> @@ -531,12 +566,19 @@
>>  		  goto fail4;
>>  		}
>>  
>> +	    lv_parsed: /* all done with parsing this LV, seek to the end */
>>  	      if (p != NULL)
>>  		p = grub_strchr (p, '}');
>>  	      if (p == NULL)
>>  		goto lvs_fail;
>>  	      p += 3;
>>  
>> +		if (skip_lv || ! status_visible) {
>> +			grub_free (lv->name);
>> +			grub_free (lv);
>> +			continue;
>> +		}
>> +
>>  	      lv->number = lv_count++;
>>  	      lv->vg = vg;
>>  	      lv->next = vg->lvs;
>>
>>     
>
>
>
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: lvm.diff --]
[-- Type: text/x-diff; name="lvm.diff", Size: 2721 bytes --]

=== modified file 'disk/lvm.c'
--- disk/lvm.c	2010-04-27 15:25:12 +0000
+++ disk/lvm.c	2010-06-29 05:27:15 +0000
@@ -46,6 +46,52 @@ grub_lvm_getvalue (char **p, char *str)
 }
 
 static int
+grub_lvm_checkvalue (char **p, char *str, char *tmpl)
+{
+  int tmpllen = grub_strlen (tmpl);
+  *p = grub_strstr (*p, str);
+  if (! *p)
+    return 0;
+  *p += grub_strlen (str);
+  if (**p != '"')
+    return 0;
+  return (grub_memcmp (*p + 1, tmpl, tmpllen) == 0 && (*p)[tmpllen + 1] == '"');
+}
+
+static int
+grub_lvm_check_flag (char *p, char *str, char *flag)
+{
+  int len_str = grub_strlen (str), len_flag = grub_strlen (flag);
+  while (1)
+    {
+      char *q;
+      p = grub_strstr (p, str);
+      if (! p)
+	return 0;
+      p += len_str;
+      if (grub_memcmp (p, " = [", sizeof (" = [") - 1) != 0)
+	continue;
+      q = p + sizeof (" = [") - 1;
+      while (1)
+	{
+	  while (grub_isspace (*q))
+	    q++;
+	  if (*q != '"')
+	    return 0;
+	  q++;
+	  if (grub_memcmp (q, flag, len_flag) == 0 && q[len_flag] == '"')
+	    return 1;
+	  while (*q != '"')
+	    q++;
+	  q++;
+	  if (*q == ']')
+	    return 0;
+	  q++;
+	}
+    }
+}
+
+static int
 grub_lvm_iterate (int (*hook) (const char *name))
 {
   struct grub_lvm_vg *vg;
@@ -421,6 +467,7 @@ grub_lvm_scan_device (const char *name)
 	  while (1)
 	    {
 	      int s;
+	      int skip_lv = 0;
 	      struct grub_lvm_lv *lv;
 	      struct grub_lvm_segment *seg;
 
@@ -445,6 +492,12 @@ grub_lvm_scan_device (const char *name)
 
 	      lv->size = 0;
 
+	      if (!grub_lvm_check_flag (p, "status", "VISIBLE"))
+		{
+		  skip_lv = 1;
+		  goto lv_parsed;
+		}
+
 	      lv->segment_count = grub_lvm_getvalue (&p, "segment_count = ");
 	      if (p == NULL)
 		goto lvs_fail;
@@ -465,6 +518,14 @@ grub_lvm_scan_device (const char *name)
 		  seg->extent_count = grub_lvm_getvalue (&p, "extent_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
+
+		  if (grub_lvm_checkvalue (&p, "type = ", "snapshot"))
+		    {
+		      /* Found a snapshot, give up and move on. */
+		      skip_lv = 1;
+		      break;
+		    }
+
 		  seg->stripe_count = grub_lvm_getvalue (&p, "stripe_count = ");
 		  if (p == NULL)
 		    goto lvs_segment_fail;
@@ -531,12 +592,20 @@ grub_lvm_scan_device (const char *name)
 		  goto fail4;
 		}
 
+	    lv_parsed:
 	      if (p != NULL)
 		p = grub_strchr (p, '}');
 	      if (p == NULL)
 		goto lvs_fail;
 	      p += 3;
 
+	      if (skip_lv)
+		{
+		  grub_free (lv->name);
+		  grub_free (lv);
+		  continue;
+		}
+
 	      lv->number = lv_count++;
 	      lv->vg = vg;
 	      lv->next = vg->lvs;


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

Re: [PATCH] Updated fix for Debian Bug#574863: boot failure with lvm2 and snapshot volumes

[sean finney]

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

hi there,

On Tue, Jun 29, 2010 at 07:30:57AM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> I've cleaned up your patch using my more generic string parsing
> functions. Could you test attached patch?

i read over the patch and it looks good.  i've just tested the patch
and grub-install, grub-mkconfig, and all the underlying calls to
grub-probe work without problem, and the system happily boots with active
lvm snapshots.  so i think the patch is good from a testing perspective
too.


	sean

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]