Re: grub 1.96 svn 20080813 and circular lvm2 metadata

[Marco Gerards <mgerards@xs4all.nl>]

Hi Hans,

hans.lambermont@newtec.eu (Hans Lambermont) writes:

> Felix Zielcke wrote on 20080828:
>
>> Am Donnerstag, den 28.08.2008, 13:14 +0200 schrieb Hans Lambermont:
>>> My colleague Jan Derk Gerlings found a bug in the way grub 1.96, svn
>>> version 20080813 (and earlier) reads the circular lvm2 metadata
>>> buffer.
>> 
>> Great, on Debian BTS we have 2 bugs [0] related to LVM and I wasn't
>> able yet to trace them down, but that could be the reason for them.
>> 
>>> Shall we attempt to fix this and send a patch ?
>> 
>> Yes please do :)
>
> Please find the patch attached, this was tested with circular metadata
> and the segfault in grub-setup is gone and the system boots fine.

Great!  It is really nice to have this fixed :-)

>> Because GRUB is a GNU project, you have to obey the GNU Coding Standards [1]
>
> Ok, I hope I got it right. If not please let me know.

Okay.

> diff -uwr grub-1.96_svn20080813-org/ChangeLog grub-1.96_svn20080813-new/ChangeLog
> --- grub-1.96_svn20080813-org/ChangeLog	2008-08-13 17:24:36.000000000 +0200
> +++ grub-1.96_svn20080813-new/ChangeLog	2008-08-29 10:33:03.000000000 +0200
> @@ -1,3 +1,8 @@
> +2008-08-28 Hans Lambermont <hans.lambermont@newtec.eu> (tiny change)
> +	   Jan Derk Gerlings <jan.derk.gerlings@newtec.eu> (tiny change)
> +
> +	* disk/lvm.c: Add capability to read circular metadata

Please describe changes in the changelog entry, not the effect.

For example:

	* disk/lvm.c (grub_lvm_scan_device): Allocate buffer space for the
	worst case scenario.
	(grub_lvm_scan_device): ...

Where ... has to be filled in, I have no idea what this code actually
does or what you changed ;-)

The tiny change syntax does not seem familiar to me, AFAIK it is not
from the GCS.  Can you please change that?  Furthermore, if you both
worked on there two parts of the patch, please send in separate
patches.  It will make my life a lot easier... :-)

If only one person worked on this, for example Jan Derk, which Hans
only forwards this patch, please only list Jan Derk as the
contributor.

>  2008-08-12  Robert Millan  <rmh@aybabtu.com>
>  
>  	* loader/i386/pc/multiboot.c (grub_multiboot_load_elf32): Move part
> diff -uwr grub-1.96_svn20080813-org/disk/lvm.c grub-1.96_svn20080813-new/disk/lvm.c
> --- grub-1.96_svn20080813-org/disk/lvm.c	2008-08-28 14:32:53.000000000 +0200
> +++ grub-1.96_svn20080813-new/disk/lvm.c	2008-08-28 18:31:19.000000000 +0200
> @@ -281,7 +281,8 @@
>        goto fail;
>      }
>  
> -  metadatabuf = grub_malloc (mda_size);
> +  /* alloc for circular worst-case scenario */

Nitpick: Please start a sentence with a capital letter and end with a
`.'.  So this will become:

  /* Assume circular buffer in a worst case scenario.  */


> +  metadatabuf = grub_malloc (2*mda_size);

Please use spaces around operators:

2 * md_size

>    if (! metadatabuf)
>      goto fail;
>  
> @@ -300,6 +301,12 @@
>      }
>  
>    rlocn = mdah->raw_locns;
> +  if (rlocn->offset + rlocn->size > mdah->size)

Here rlcon->offset seems to be little endian (64 bits), so please use
grub_le_to_cpu64.  Same for rlocn->size, please check the size of this
member before you use a macro (I couldn't find it immediately...).

> +    {
> +      /* metadata is circular */

Same as above.

> +      grub_memcpy(metadatabuf + mda_size, metadatabuf + mdah->start,
> +		  ((rlocn->offset + rlocn->size) - mdah->size));
> +    }

Please check and correct the endianess.  you use a lot of
parenthesises.  Actually, I think none are requires.

Thanks,
Marco