[PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the support
of bigtime by adding the some features in inodes V3.

This change extended grub_xfs_inode struct by 76 bytes but also changed the
computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE.

Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit it's 84 bytes.
XFS_V2_INODE_SIZE becomes 16 bytes too small.

As a result, the data structure aren't properly aligned and generates
"attempt to read or write outside of partition" errors when trying to
read the filesystem.

                             GNU GRUB  version 2.11
	....
	grub> set debug=efi,gpt,xfs
	grub> insmod part_gpt
	grub> ls (hd0,gpt1)/
	partmap/gpt.c:93: Read a valid GPT header
	partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
	fs/xfs.c:931: Reading sb
	fs/xfs.c:270: Validating superblock
	fs/xfs.c:295: XFS v4 superblock detected
	fs/xfs.c:962: Reading root ino 128
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:515: Reading inode (739521961424144223) - 344365866970255880, 3840
	error: attempt to read or write outside of partition.

This commit change the XFS_V2_INODE_SIZE computation by substracting 76
instead of 92 from the actual size of grub_xfs_inode.
This 76 value is coming from the added :
	20 uint8   unused5
	 1 uint64  flags2
        48 uint8   unused6

This patch explicit the split between the v2 and v3 parts of structure.
The unused4 is still ending to the v2 structures and the v3 starts at unused5.
This will avoid future corruption of v2 or v3.

The XFS_V2_INODE_SIZE is returning to its expected size and the
filesystem is back to a readable state.

                      GNU GRUB  version 2.11
	....
	grub> set debug=efi,gpt,xfs
	grub> insmod part_gpt
	grub> ls (hd0,gpt1)/
	partmap/gpt.c:93: Read a valid GPT header
	partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
	fs/xfs.c:931: Reading sb
	fs/xfs.c:270: Validating superblock
	fs/xfs.c:295: XFS v4 superblock detected
	fs/xfs.c:962: Reading root ino 128
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:931: Reading sb
	fs/xfs.c:270: Validating superblock
	fs/xfs.c:295: XFS v4 superblock detected
	fs/xfs.c:962: Reading root ino 128
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:515: Reading inode (128) - 64, 0
	fs/xfs.c:515: Reading inode (131) - 64, 768
	efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0
	grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024
	grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816
	grub>

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 grub-core/fs/xfs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index 0f524c3a8a61..becea5bcdf61 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -192,6 +192,7 @@ struct grub_xfs_time_legacy
   grub_uint32_t nanosec;
 } GRUB_PACKED;
 
+// From https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_filesystem_structure.pdf about 'struct xfs_dinode_core'
 struct grub_xfs_inode
 {
   grub_uint8_t magic[2];
@@ -208,14 +209,15 @@ struct grub_xfs_inode
   grub_uint32_t nextents;
   grub_uint16_t unused3;
   grub_uint8_t fork_offset;
-  grub_uint8_t unused4[37];
+  grub_uint8_t unused4[17]; // Last member of inode v2
+  grub_uint8_t unused5[20]; // First member of inode v3
   grub_uint64_t flags2;
-  grub_uint8_t unused5[48];
+  grub_uint8_t unused6[48]; // Last member of inode v3
 } GRUB_PACKED;
 
 #define XFS_V3_INODE_SIZE	sizeof(struct grub_xfs_inode)
-/* Size of struct grub_xfs_inode until fork_offset (included). */
-#define XFS_V2_INODE_SIZE	(XFS_V3_INODE_SIZE - 92)
+/* Size of struct grub_xfs_inode v2, up to unused4 included */
+#define XFS_V2_INODE_SIZE	(XFS_V3_INODE_SIZE - 76)
 
 struct grub_xfs_dirblock_tail
 {
-- 
2.25.1

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

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

I should have cc Carlos Maiolino who authored this patch.
This is now done.


Le mer. 25 août 2021 à 15:32, Erwan Velu <erwanaliasr1@gmail.com> a écrit :

> Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the support
> of bigtime by adding the some features in inodes V3.
>
> This change extended grub_xfs_inode struct by 76 bytes but also changed the
> computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE.
>
> Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit it's
> 84 bytes.
> XFS_V2_INODE_SIZE becomes 16 bytes too small.
>
> As a result, the data structure aren't properly aligned and generates
> "attempt to read or write outside of partition" errors when trying to
> read the filesystem.
>
>                              GNU GRUB  version 2.11
>         ....
>         grub> set debug=efi,gpt,xfs
>         grub> insmod part_gpt
>         grub> ls (hd0,gpt1)/
>         partmap/gpt.c:93: Read a valid GPT header
>         partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>         fs/xfs.c:931: Reading sb
>         fs/xfs.c:270: Validating superblock
>         fs/xfs.c:295: XFS v4 superblock detected
>         fs/xfs.c:962: Reading root ino 128
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:515: Reading inode (739521961424144223) -
> 344365866970255880, 3840
>         error: attempt to read or write outside of partition.
>
> This commit change the XFS_V2_INODE_SIZE computation by substracting 76
> instead of 92 from the actual size of grub_xfs_inode.
> This 76 value is coming from the added :
>         20 uint8   unused5
>          1 uint64  flags2
>         48 uint8   unused6
>
> This patch explicit the split between the v2 and v3 parts of structure.
> The unused4 is still ending to the v2 structures and the v3 starts at
> unused5.
> This will avoid future corruption of v2 or v3.
>
> The XFS_V2_INODE_SIZE is returning to its expected size and the
> filesystem is back to a readable state.
>
>                       GNU GRUB  version 2.11
>         ....
>         grub> set debug=efi,gpt,xfs
>         grub> insmod part_gpt
>         grub> ls (hd0,gpt1)/
>         partmap/gpt.c:93: Read a valid GPT header
>         partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>         fs/xfs.c:931: Reading sb
>         fs/xfs.c:270: Validating superblock
>         fs/xfs.c:295: XFS v4 superblock detected
>         fs/xfs.c:962: Reading root ino 128
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:931: Reading sb
>         fs/xfs.c:270: Validating superblock
>         fs/xfs.c:295: XFS v4 superblock detected
>         fs/xfs.c:962: Reading root ino 128
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:515: Reading inode (128) - 64, 0
>         fs/xfs.c:515: Reading inode (131) - 64, 768
>         efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0
>         grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024
>         grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816
>         grub>
>
> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  grub-core/fs/xfs.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 0f524c3a8a61..becea5bcdf61 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -192,6 +192,7 @@ struct grub_xfs_time_legacy
>    grub_uint32_t nanosec;
>  } GRUB_PACKED;
>
> +// From
> https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_filesystem_structure.pdf
> about 'struct xfs_dinode_core'
>  struct grub_xfs_inode
>  {
>    grub_uint8_t magic[2];
> @@ -208,14 +209,15 @@ struct grub_xfs_inode
>    grub_uint32_t nextents;
>    grub_uint16_t unused3;
>    grub_uint8_t fork_offset;
> -  grub_uint8_t unused4[37];
> +  grub_uint8_t unused4[17]; // Last member of inode v2
> +  grub_uint8_t unused5[20]; // First member of inode v3
>    grub_uint64_t flags2;
> -  grub_uint8_t unused5[48];
> +  grub_uint8_t unused6[48]; // Last member of inode v3
>  } GRUB_PACKED;
>
>  #define XFS_V3_INODE_SIZE      sizeof(struct grub_xfs_inode)
> -/* Size of struct grub_xfs_inode until fork_offset (included). */
> -#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 92)
> +/* Size of struct grub_xfs_inode v2, up to unused4 included */
> +#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 76)
>
>  struct grub_xfs_dirblock_tail
>  {
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 5640 bytes --]

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Carlos Maiolino]

On Thu, Aug 26, 2021 at 09:54:59AM +0200, Erwan Velu wrote:
>    I should have cc Carlos Maiolino who authored this patch.
>    This is now done.
> 
>    Le mer. 25 août 2021 à 15:32, Erwan Velu <[1]erwanaliasr1@gmail.com> a
>    écrit :
> 
>      Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the
>      support
>      of bigtime by adding the some features in inodes V3.
>      This change extended grub_xfs_inode struct by 76 bytes but also
>      changed the
>      computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE.
>      Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit
>      it's 84 bytes.
>      XFS_V2_INODE_SIZE becomes 16 bytes too small.
>      As a result, the data structure aren't properly aligned and
>      generates
>      "attempt to read or write outside of partition" errors when trying
>      to
>      read the filesystem.

Thanks for spotting this!

>                                   GNU GRUB  version 2.11
>              ....
>              grub> set debug=efi,gpt,xfs
>              grub> insmod part_gpt
>              grub> ls (hd0,gpt1)/
>              partmap/gpt.c:93: Read a valid GPT header
>              partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (739521961424144223) -
>      344365866970255880, 3840
>              error: attempt to read or write outside of partition.
>      This commit change the XFS_V2_INODE_SIZE computation by substracting
>      76
>      instead of 92 from the actual size of grub_xfs_inode.
>      This 76 value is coming from the added :
>              20 uint8   unused5
>               1 uint64  flags2
>              48 uint8   unused6
>      This patch explicit the split between the v2 and v3 parts of
>      structure.
>      The unused4 is still ending to the v2 structures and the v3 starts
>      at unused5.
>      This will avoid future corruption of v2 or v3.
>      The XFS_V2_INODE_SIZE is returning to its expected size and the
>      filesystem is back to a readable state.
>                            GNU GRUB  version 2.11
>              ....
>              grub> set debug=efi,gpt,xfs
>              grub> insmod part_gpt
>              grub> ls (hd0,gpt1)/
>              partmap/gpt.c:93: Read a valid GPT header
>              partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (131) - 64, 768
>              efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0
>              grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024
>              grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816
>              grub>
>      Signed-off-by: Erwan Velu <[2]e.velu@criteo.com>
>      ---
>       grub-core/fs/xfs.c | 10 ++++++----
>       1 file changed, 6 insertions(+), 4 deletions(-)
>      diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
>      index 0f524c3a8a61..becea5bcdf61 100644
>      --- a/grub-core/fs/xfs.c
>      +++ b/grub-core/fs/xfs.c
>      @@ -192,6 +192,7 @@ struct grub_xfs_time_legacy
>         grub_uint32_t nanosec;
>       } GRUB_PACKED;
>      +// From
>      [3]https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_f
>      ilesystem_structure.pdf about 'struct xfs_dinode_core'
>       struct grub_xfs_inode
>       {
>         grub_uint8_t magic[2];
>      @@ -208,14 +209,15 @@ struct grub_xfs_inode
>         grub_uint32_t nextents;
>         grub_uint16_t unused3;
>         grub_uint8_t fork_offset;
>      -  grub_uint8_t unused4[37];
>      +  grub_uint8_t unused4[17]; // Last member of inode v2
>      +  grub_uint8_t unused5[20]; // First member of inode v3
>         grub_uint64_t flags2;
>      -  grub_uint8_t unused5[48];
>      +  grub_uint8_t unused6[48]; // Last member of inode v3
>       } GRUB_PACKED;
>       #define XFS_V3_INODE_SIZE      sizeof(struct grub_xfs_inode)
>      -/* Size of struct grub_xfs_inode until fork_offset (included). */
>      -#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 92)
>      +/* Size of struct grub_xfs_inode v2, up to unused4 included */
>      +#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 76)
>       struct grub_xfs_dirblock_tail
>       {
>      --
>      2.25.1
> 
> References
> 
>    1. mailto:erwanaliasr1@gmail.com
>    2. mailto:e.velu@criteo.com
>    3. https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_filesystem_structure.pdf

-- 
Carlos

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

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

>
> [...]
> Thanks for spotting this!
>

We got hit in production with it, that's why we investigate this issue.
It's deployed on our infra & works fine on the affected servers but works
fine too on the servers running V5 fs.

I don't know what is the policy of this project, but a point release would
make sense to avoid breaking servers that have /boot in an XFS v4.
2.06 is still young and few bumped it + reboot their machines.

Erwan,

[-- Attachment #2: Type: text/html, Size: 784 bytes --]

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

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

Good day list,

Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <cmaiolino@redhat.com> a
écrit :

> [..]
>
> Thanks for spotting this!
>
>
I'm adding the maintainers in CC. Carlos who commit the patch I'm fixing,
agreed on the content.
This patch is important as it prevents the 2.06 version to turn XFS v4
system into an unbootable state.

[-- Attachment #2: Type: text/html, Size: 703 bytes --]

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Carlos Maiolino]

Hi.
On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
>    Good day list,
>    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiolino@redhat.com>
>    a écrit :
> 
>      [..]
>      Thanks for spotting this!
> 
>    I'm adding the maintainers in CC. Carlos who commit the patch I'm
>    fixing, agreed on the content.

I didn't test the patch itself yet, but I've reproduced the issue. I was quite
sure I had tested this patch on a V4 fs, but looks like I miscalculated the
sizing. Thanks again. I'll try to test the patch here asap.


>    This patch is important as it prevents the 2.06 version to turn XFS v4
>    system into an unbootable state.
> 
> References
> 
>    1. mailto:cmaiolino@redhat.com

-- 
Carlos

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Daniel Kiper]

CC-ing Javier...

On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> Hi.
> On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> >    Good day list,
> >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiolino@redhat.com>
> >    a écrit :
> >
> >      [..]
> >      Thanks for spotting this!
> >
> >    I'm adding the maintainers in CC. Carlos who commit the patch I'm
> >    fixing, agreed on the content.
>
> I didn't test the patch itself yet, but I've reproduced the issue. I was quite
> sure I had tested this patch on a V4 fs, but looks like I miscalculated the
> sizing. Thanks again. I'll try to test the patch here asap.

Did you test this patch? If yes may I add your Tested-by to it?

Daniel

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Carlos Maiolino]

On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> CC-ing Javier...
> 
> On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > Hi.
> > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > >    Good day list,
> > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiolino@redhat.com>
> > >    a écrit :
> > >
> > >      [..]
> > >      Thanks for spotting this!
> > >
> > >    I'm adding the maintainers in CC. Carlos who commit the patch I'm
> > >    fixing, agreed on the content.
> >
> > I didn't test the patch itself yet, but I've reproduced the issue. I was quite
> > sure I had tested this patch on a V4 fs, but looks like I miscalculated the
> > sizing. Thanks again. I'll try to test the patch here asap.
> 
> Did you test this patch? If yes may I add your Tested-by to it?

Not yet, I may be able to test it today, I let you know.

> 
> Daniel
> 

-- 
Carlos

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Carlos Maiolino]

On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> CC-ing Javier...
> 
> On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > Hi.
> > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > >    Good day list,
> > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiolino@redhat.com>
> > >    a écrit :
> > >
> > >      [..]
> > >      Thanks for spotting this!
> > >
> > >    I'm adding the maintainers in CC. Carlos who commit the patch I'm
> > >    fixing, agreed on the content.
> >
> > I didn't test the patch itself yet, but I've reproduced the issue. I was quite
> > sure I had tested this patch on a V4 fs, but looks like I miscalculated the
> > sizing. Thanks again. I'll try to test the patch here asap.
> 
> Did you test this patch? If yes may I add your Tested-by to it?

Yup, patch works fine, just finished testing it, I was just trying to understand
where/why I miscalculated the inode size on V4 filesystems, and the reason was
the same why Erwan split the last/first members of inode v2/v3 in two different
unused structs.

Feel free to add to the patch:

Tested-by: Carlos Maiolino <cmaiolino@redhat.com>


> 
> Daniel
> 

-- 
Carlos

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Daniel Kiper]

On Thu, Sep 02, 2021 at 10:56:49AM +0200, Carlos Maiolino wrote:
> On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > CC-ing Javier...
> >
> > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > Hi.
> > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > >    Good day list,
> > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino <[1]cmaiolino@redhat.com>
> > > >    a écrit :
> > > >
> > > >      [..]
> > > >      Thanks for spotting this!
> > > >
> > > >    I'm adding the maintainers in CC. Carlos who commit the patch I'm
> > > >    fixing, agreed on the content.
> > >
> > > I didn't test the patch itself yet, but I've reproduced the issue. I was quite
> > > sure I had tested this patch on a V4 fs, but looks like I miscalculated the
> > > sizing. Thanks again. I'll try to test the patch here asap.
> >
> > Did you test this patch? If yes may I add your Tested-by to it?
>
> Yup, patch works fine, just finished testing it, I was just trying to understand
> where/why I miscalculated the inode size on V4 filesystems, and the reason was
> the same why Erwan split the last/first members of inode v2/v3 in two different
> unused structs.
>
> Feel free to add to the patch:
>
> Tested-by: Carlos Maiolino <cmaiolino@redhat.com>

Cool! Thanks a lot!

Daniel

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

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

>
> Cool! Thanks a lot!
>
> I have a question about this. Considering the impact of this issue, does
this trigger a point-release of GRUB ?

[-- Attachment #2: Type: text/html, Size: 354 bytes --]

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Daniel Kiper]

On Thu, Sep 02, 2021 at 01:39:43PM +0200, Erwan Velu wrote:
>      Cool! Thanks a lot!
>
> I have a question about this. Considering the impact of this issue, does this
> trigger a point-release of GRUB ?

Nope...

Daniel

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

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

[...]

> Nope...
>
> I'm pretty new at contributing to see project but I really wonder what's
the rational being this.
I mean, 2.06 is very recent, debuting its life cycle and its now known as
totally broken on XFS V4 and we have here a very simple patch.
How can we prevent users from installing this release ?
Can't we get a point release or even maybe a 2.07 and a statement reporting
that 2.06 is not suitable for XFS v4 based system.
I'm very uncomfortable knowing the release is broken on some config
(including mine) and new people will be caught in the same trap.
Sorry to ask maybe a question that got asked zillion times, but that sounds
important to me that we can project users that didn't reboot yet with this
release.

[-- Attachment #2: Type: text/html, Size: 1011 bytes --]

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Daniel Kiper]

On Thu, Sep 02, 2021 at 03:05:13PM +0200, Erwan Velu wrote:
> [...]
>      Nope...
>
> I'm pretty new at contributing to see project but I really wonder what's the
> rational being this.
> I mean, 2.06 is very recent, debuting its life cycle and its now known as
> totally broken on XFS V4 and we have here a very simple patch.
> How can we prevent users from installing this release ?
> Can't we get a point release or even maybe a 2.07 and a statement reporting
> that 2.06 is not suitable for XFS v4 based system.
> I'm very uncomfortable knowing the release is broken on some config (including
> mine) and new people will be caught in the same trap.
> Sorry to ask maybe a question that got asked zillion times, but that sounds
> important to me that we can project users that didn't reboot yet with this
> release.

Making a release is a big hassle for the project right now. I hope it
will change in the future but it is what it is. I think distros will
take this patch immediately. If that helps I can announce here in
separate thread when patch lands in the git repository.

Daniel

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Erwan Velu]

Le 02/09/2021 à 15:31, Daniel Kiper a écrit :
> Making a release is a big hassle for the project right now. I hope it
> will change in the future but it is what it is. I think distros will
> take this patch immediately. If that helps I can announce here in
> separate thread when patch lands in the git repository.

Yeah, if we can at least point this to distro maintainers that would be 
a great workaround if releasing is too complex.

Thanks,

Erwan

Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Daniel Kiper]

On Wed, Aug 25, 2021 at 03:31:52PM +0200, Erwan Velu wrote:
> Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the support
> of bigtime by adding the some features in inodes V3.
>
> This change extended grub_xfs_inode struct by 76 bytes but also changed the
> computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE.
>
> Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit it's 84 bytes.
> XFS_V2_INODE_SIZE becomes 16 bytes too small.
>
> As a result, the data structure aren't properly aligned and generates
> "attempt to read or write outside of partition" errors when trying to
> read the filesystem.
>
>                              GNU GRUB  version 2.11
> 	....
> 	grub> set debug=efi,gpt,xfs
> 	grub> insmod part_gpt
> 	grub> ls (hd0,gpt1)/
> 	partmap/gpt.c:93: Read a valid GPT header
> 	partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
> 	fs/xfs.c:931: Reading sb
> 	fs/xfs.c:270: Validating superblock
> 	fs/xfs.c:295: XFS v4 superblock detected
> 	fs/xfs.c:962: Reading root ino 128
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:515: Reading inode (739521961424144223) - 344365866970255880, 3840
> 	error: attempt to read or write outside of partition.
>
> This commit change the XFS_V2_INODE_SIZE computation by substracting 76
> instead of 92 from the actual size of grub_xfs_inode.
> This 76 value is coming from the added :
> 	20 uint8   unused5
> 	 1 uint64  flags2
>         48 uint8   unused6
>
> This patch explicit the split between the v2 and v3 parts of structure.
> The unused4 is still ending to the v2 structures and the v3 starts at unused5.
> This will avoid future corruption of v2 or v3.
>
> The XFS_V2_INODE_SIZE is returning to its expected size and the
> filesystem is back to a readable state.
>
>                       GNU GRUB  version 2.11
> 	....
> 	grub> set debug=efi,gpt,xfs
> 	grub> insmod part_gpt
> 	grub> ls (hd0,gpt1)/
> 	partmap/gpt.c:93: Read a valid GPT header
> 	partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
> 	fs/xfs.c:931: Reading sb
> 	fs/xfs.c:270: Validating superblock
> 	fs/xfs.c:295: XFS v4 superblock detected
> 	fs/xfs.c:962: Reading root ino 128
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:931: Reading sb
> 	fs/xfs.c:270: Validating superblock
> 	fs/xfs.c:295: XFS v4 superblock detected
> 	fs/xfs.c:962: Reading root ino 128
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:515: Reading inode (128) - 64, 0
> 	fs/xfs.c:515: Reading inode (131) - 64, 768
> 	efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0
> 	grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024
> 	grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816
> 	grub>
>
> Signed-off-by: Erwan Velu <e.velu@criteo.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Glenn Washburn]

On Thu, 2 Sep 2021 10:56:49 +0200
Carlos Maiolino <cmaiolino@redhat.com> wrote:

> On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > CC-ing Javier...
> > 
> > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > Hi.
> > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > >    Good day list,
> > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino
> > > > <[1]cmaiolino@redhat.com> a écrit :
> > > >
> > > >      [..]
> > > >      Thanks for spotting this!
> > > >
> > > >    I'm adding the maintainers in CC. Carlos who commit the
> > > > patch I'm fixing, agreed on the content.
> > >
> > > I didn't test the patch itself yet, but I've reproduced the
> > > issue. I was quite sure I had tested this patch on a V4 fs, but
> > > looks like I miscalculated the sizing. Thanks again. I'll try to
> > > test the patch here asap.
> > 
> > Did you test this patch? If yes may I add your Tested-by to it?
> 
> Yup, patch works fine, just finished testing it, I was just trying to
> understand where/why I miscalculated the inode size on V4
> filesystems, and the reason was the same why Erwan split the
> last/first members of inode v2/v3 in two different unused structs.
> 
> Feel free to add to the patch:
> 
> Tested-by: Carlos Maiolino <cmaiolino@redhat.com>

It looks like the xfs_test test succeeds with tag grub-2.06-rc1a, fails
with tag grub-2.06, and succeeds with current master. Yes, as expected.
However, what this tells me is that no "make check" tests were done
before finalizing the 2.06 release. I was under the impression that that
was part of the release procedure. If its not, it would seem that we're
not using the tests at a time when they would have the most impact.

It is my understanding that we have travis-ci tests that get run (at
some point?), however they are only build tests and so would not have
caught this. It was precisely this scenario that I hoped to avoid by
doing more thorough continuous integration, which runs the extensive
array of "make check" tests, when I submitted the Gitlab-CI patch
series (which would've caught this automatically if it had been merged).
To me this scenario is the poster child for taking action on this
front. Can we make this a priority?

Glenn

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Erwan Velu]

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

Le mer. 8 sept. 2021 à 03:24, Glenn Washburn <development@efficientek.com>
a écrit :

> On Thu, 2 Sep 2021 10:56:49 +0200
> Carlos Maiolino <cmaiolino@redhat.com> wrote:
>
> > On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > > CC-ing Javier...
> > >
> > > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > > Hi.
> > > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > > >    Good day list,
> > > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino
> > > > > <[1]cmaiolino@redhat.com> a écrit :
> > > > >
> > > > >      [..]
> > > > >      Thanks for spotting this!
> > > > >
> > > > >    I'm adding the maintainers in CC. Carlos who commit the
> > > > > patch I'm fixing, agreed on the content.
> > > >
> > > > I didn't test the patch itself yet, but I've reproduced the
> > > > issue. I was quite sure I had tested this patch on a V4 fs, but
> > > > looks like I miscalculated the sizing. Thanks again. I'll try to
> > > > test the patch here asap.
> > >
> > > Did you test this patch? If yes may I add your Tested-by to it?
> >
> > Yup, patch works fine, just finished testing it, I was just trying to
> > understand where/why I miscalculated the inode size on V4
> > filesystems, and the reason was the same why Erwan split the
> > last/first members of inode v2/v3 in two different unused structs.
> >
> > Feel free to add to the patch:
> >
> > Tested-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> It looks like the xfs_test test succeeds with tag grub-2.06-rc1a, fails
> with tag grub-2.06, and succeeds with current master. Yes, as expected.
> However, what this tells me is that no "make check" tests were done
> before finalizing the 2.06 release. I was under the impression that that
> was part of the release procedure. If its not, it would seem that we're
> not using the tests at a time when they would have the most impact.
>
> It is my understanding that we have travis-ci tests that get run (at
> some point?), however they are only build tests and so would not have
> caught this. It was precisely this scenario that I hoped to avoid by
> doing more thorough continuous integration, which runs the extensive
> array of "make check" tests, when I submitted the Gitlab-CI patch
> series (which would've caught this automatically if it had been merged).
> To me this scenario is the poster child for taking action on this
> front. Can we make this a priority?
>
>
That also triggers to me the question of the difficulty to make a release.
I'm pretty new to this project but from my other experiences, I'm missing
what prevents doing a stable branch per release to perform critical updates
on them.
I'm a big fan of how the Linux kernel manages this: once the commit is
merged in master, it can be backported into the stable branches if the
patch makes sense.
Then, the stable branch can be released whenever it makes sense.
This increases the maintenance burden (but can be delegated to dedicated
people)  but gives users the insurance of having an up to date & secured
product.

This topic was probably already addressed or questioned by the past
soplease receive this as a simple questioning and not a straight criticism
of the project maintenance.

[-- Attachment #2: Type: text/html, Size: 4455 bytes --]

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Daniel Kiper]

On Wed, Sep 08, 2021 at 01:22:20AM +0000, Glenn Washburn wrote:
> On Thu, 2 Sep 2021 10:56:49 +0200
> Carlos Maiolino <cmaiolino@redhat.com> wrote:
>
> > On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > > CC-ing Javier...
> > >
> > > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > > Hi.
> > > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > > >    Good day list,
> > > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino
> > > > > <[1]cmaiolino@redhat.com> a écrit :
> > > > >
> > > > >      [..]
> > > > >      Thanks for spotting this!
> > > > >
> > > > >    I'm adding the maintainers in CC. Carlos who commit the
> > > > > patch I'm fixing, agreed on the content.
> > > >
> > > > I didn't test the patch itself yet, but I've reproduced the
> > > > issue. I was quite sure I had tested this patch on a V4 fs, but
> > > > looks like I miscalculated the sizing. Thanks again. I'll try to
> > > > test the patch here asap.
> > >
> > > Did you test this patch? If yes may I add your Tested-by to it?
> >
> > Yup, patch works fine, just finished testing it, I was just trying to
> > understand where/why I miscalculated the inode size on V4
> > filesystems, and the reason was the same why Erwan split the
> > last/first members of inode v2/v3 in two different unused structs.
> >
> > Feel free to add to the patch:
> >
> > Tested-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> It looks like the xfs_test test succeeds with tag grub-2.06-rc1a, fails
> with tag grub-2.06, and succeeds with current master. Yes, as expected.
> However, what this tells me is that no "make check" tests were done
> before finalizing the 2.06 release. I was under the impression that that
> was part of the release procedure. If its not, it would seem that we're
> not using the tests at a time when they would have the most impact.

Currently I run build tests for all architectures and platforms and
Coverity for x86_64 EFI before every push and release. I know this is
not enough but I tried "make check" at least once and got the impression
that the tests are not reliable. Sadly I have not time to dive deeper
and fix this and that. If someone, you?, wants to verify all tests and
fix broken ones I will be more than happy to start using it (it seems to
me your "[PATCH v2 0/8] Various fixes/improvements for tests" patch set
fixes some issues but I am not sure if all of them).

> It is my understanding that we have travis-ci tests that get run (at
> some point?), however they are only build tests and so would not have
> caught this. It was precisely this scenario that I hoped to avoid by
> doing more thorough continuous integration, which runs the extensive
> array of "make check" tests, when I submitted the Gitlab-CI patch
> series (which would've caught this automatically if it had been merged).
> To me this scenario is the poster child for taking action on this
> front. Can we make this a priority?

I think I can take a closer look at patch set mentioned above in a week
or two. If "make check" works as expected and can be run locally then we
can think about automating the testing.

Daniel

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Daniel Kiper]

On Wed, Sep 08, 2021 at 11:20:08AM +0200, Erwan Velu wrote:
> Le mer. 8 sept. 2021 à 03:24, Glenn Washburn <development@efficientek.com>
> a écrit :
>
> > On Thu, 2 Sep 2021 10:56:49 +0200
> > Carlos Maiolino <cmaiolino@redhat.com> wrote:
> >
> > > On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > > > CC-ing Javier...
> > > >
> > > > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > > > Hi.
> > > > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > > > >    Good day list,
> > > > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino
> > > > > > <[1]cmaiolino@redhat.com> a écrit :
> > > > > >
> > > > > >      [..]
> > > > > >      Thanks for spotting this!
> > > > > >
> > > > > >    I'm adding the maintainers in CC. Carlos who commit the
> > > > > > patch I'm fixing, agreed on the content.
> > > > >
> > > > > I didn't test the patch itself yet, but I've reproduced the
> > > > > issue. I was quite sure I had tested this patch on a V4 fs, but
> > > > > looks like I miscalculated the sizing. Thanks again. I'll try to
> > > > > test the patch here asap.
> > > >
> > > > Did you test this patch? If yes may I add your Tested-by to it?
> > >
> > > Yup, patch works fine, just finished testing it, I was just trying to
> > > understand where/why I miscalculated the inode size on V4
> > > filesystems, and the reason was the same why Erwan split the
> > > last/first members of inode v2/v3 in two different unused structs.
> > >
> > > Feel free to add to the patch:
> > >
> > > Tested-by: Carlos Maiolino <cmaiolino@redhat.com>
> >
> > It looks like the xfs_test test succeeds with tag grub-2.06-rc1a, fails
> > with tag grub-2.06, and succeeds with current master. Yes, as expected.
> > However, what this tells me is that no "make check" tests were done
> > before finalizing the 2.06 release. I was under the impression that that
> > was part of the release procedure. If its not, it would seem that we're
> > not using the tests at a time when they would have the most impact.
> >
> > It is my understanding that we have travis-ci tests that get run (at
> > some point?), however they are only build tests and so would not have
> > caught this. It was precisely this scenario that I hoped to avoid by
> > doing more thorough continuous integration, which runs the extensive
> > array of "make check" tests, when I submitted the Gitlab-CI patch
> > series (which would've caught this automatically if it had been merged).
> > To me this scenario is the poster child for taking action on this
> > front. Can we make this a priority?
> >
> >
> That also triggers to me the question of the difficulty to make a release.
> I'm pretty new to this project but from my other experiences, I'm missing
> what prevents doing a stable branch per release to perform critical updates
> on them.
> I'm a big fan of how the Linux kernel manages this: once the commit is
> merged in master, it can be backported into the stable branches if the
> patch makes sense.
> Then, the stable branch can be released whenever it makes sense.
> This increases the maintenance burden (but can be delegated to dedicated
> people)  but gives users the insurance of having an up to date & secured
> product.
>
> This topic was probably already addressed or questioned by the past
> soplease receive this as a simple questioning and not a straight criticism
> of the project maintenance.

The Linux project is more mature in many areas than the GRUB right now.
Additionally, due to various reasons development of the GRUB almost
stopped for years. Currently we are clearing the backlog and trying to
regain the control on the project. So, we have to focus on most
important things due to limited resources. Though I think at some point
we change and improve the release process too.

Daniel

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Erwan Velu]

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

When this topic will be out, I'd would be glad to help.
Cheers,
Erwan,

Le mer. 8 sept. 2021 à 18:19, Daniel Kiper <dkiper@net-space.pl> a écrit :

> On Wed, Sep 08, 2021 at 11:20:08AM +0200, Erwan Velu wrote:
> > Le mer. 8 sept. 2021 à 03:24, Glenn Washburn <
> development@efficientek.com>
> > a écrit :
> >
> > > On Thu, 2 Sep 2021 10:56:49 +0200
> > > Carlos Maiolino <cmaiolino@redhat.com> wrote:
> > >
> > > > On Wed, Sep 01, 2021 at 02:40:57PM +0200, Daniel Kiper wrote:
> > > > > CC-ing Javier...
> > > > >
> > > > > On Mon, Aug 30, 2021 at 01:48:50PM +0200, Carlos Maiolino wrote:
> > > > > > Hi.
> > > > > > On Mon, Aug 30, 2021 at 11:18:31AM +0200, Erwan Velu wrote:
> > > > > > >    Good day list,
> > > > > > >    Le jeu. 26 août 2021 à 15:26, Carlos Maiolino
> > > > > > > <[1]cmaiolino@redhat.com> a écrit :
> > > > > > >
> > > > > > >      [..]
> > > > > > >      Thanks for spotting this!
> > > > > > >
> > > > > > >    I'm adding the maintainers in CC. Carlos who commit the
> > > > > > > patch I'm fixing, agreed on the content.
> > > > > >
> > > > > > I didn't test the patch itself yet, but I've reproduced the
> > > > > > issue. I was quite sure I had tested this patch on a V4 fs, but
> > > > > > looks like I miscalculated the sizing. Thanks again. I'll try to
> > > > > > test the patch here asap.
> > > > >
> > > > > Did you test this patch? If yes may I add your Tested-by to it?
> > > >
> > > > Yup, patch works fine, just finished testing it, I was just trying to
> > > > understand where/why I miscalculated the inode size on V4
> > > > filesystems, and the reason was the same why Erwan split the
> > > > last/first members of inode v2/v3 in two different unused structs.
> > > >
> > > > Feel free to add to the patch:
> > > >
> > > > Tested-by: Carlos Maiolino <cmaiolino@redhat.com>
> > >
> > > It looks like the xfs_test test succeeds with tag grub-2.06-rc1a, fails
> > > with tag grub-2.06, and succeeds with current master. Yes, as expected.
> > > However, what this tells me is that no "make check" tests were done
> > > before finalizing the 2.06 release. I was under the impression that
> that
> > > was part of the release procedure. If its not, it would seem that we're
> > > not using the tests at a time when they would have the most impact.
> > >
> > > It is my understanding that we have travis-ci tests that get run (at
> > > some point?), however they are only build tests and so would not have
> > > caught this. It was precisely this scenario that I hoped to avoid by
> > > doing more thorough continuous integration, which runs the extensive
> > > array of "make check" tests, when I submitted the Gitlab-CI patch
> > > series (which would've caught this automatically if it had been
> merged).
> > > To me this scenario is the poster child for taking action on this
> > > front. Can we make this a priority?
> > >
> > >
> > That also triggers to me the question of the difficulty to make a
> release.
> > I'm pretty new to this project but from my other experiences, I'm missing
> > what prevents doing a stable branch per release to perform critical
> updates
> > on them.
> > I'm a big fan of how the Linux kernel manages this: once the commit is
> > merged in master, it can be backported into the stable branches if the
> > patch makes sense.
> > Then, the stable branch can be released whenever it makes sense.
> > This increases the maintenance burden (but can be delegated to dedicated
> > people)  but gives users the insurance of having an up to date & secured
> > product.
> >
> > This topic was probably already addressed or questioned by the past
> > soplease receive this as a simple questioning and not a straight
> criticism
> > of the project maintenance.
>
> The Linux project is more mature in many areas than the GRUB right now.
> Additionally, due to various reasons development of the GRUB almost
> stopped for years. Currently we are clearing the backlog and trying to
> regain the control on the project. So, we have to focus on most
> important things due to limited resources. Though I think at some point
> we change and improve the release process too.
>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 5636 bytes --]

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Glenn Washburn]

On Wed, 8 Sep 2021 18:03:50 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Wed, Sep 08, 2021 at 01:22:20AM +0000, Glenn Washburn wrote:
> > It looks like the xfs_test test succeeds with tag grub-2.06-rc1a,
> > fails with tag grub-2.06, and succeeds with current master. Yes, as
> > expected. However, what this tells me is that no "make check" tests
> > were done before finalizing the 2.06 release. I was under the
> > impression that that was part of the release procedure. If its not,
> > it would seem that we're not using the tests at a time when they
> > would have the most impact.
> 
> Currently I run build tests for all architectures and platforms and
> Coverity for x86_64 EFI before every push and release. I know this is
> not enough but I tried "make check" at least once and got the
> impression that the tests are not reliable. Sadly I have not time to
> dive deeper and fix this and that. If someone, you?, wants to verify
> all tests and fix broken ones I will be more than happy to start
> using it (it seems to me your "[PATCH v2 0/8] Various
> fixes/improvements for tests" patch set fixes some issues but I am
> not sure if all of them).

I suspect the problem you're running into with the tests is more a
matter of setting up the expected environment than the tests
themselves. From my experience, most of the tests work once everything
is setup right. My gitlab patch shows how to do this on Ubuntu 20.04.

Due to not being able to load kernel modules on the shared gitlab
runners, I was not able to run all the tests either, and there are some
tests that fail. The failures that come to mind are on Sparc and some
of the functional tests, and I've notified the list, but no one came
forward to fix them. So what I've done is have a list of known failing
tests and ignore those in determining whether the test suite as a whole
has succeeded. I'm not convinced that that behavior is something we
should include in the test framework itself.

So to my mind, I have verified (nearly) all tests on most platforms. I
have fixed the ones I could. I think the biggest low hanging fruit (for
me), is to get the tests running on a system with all the needed kernel
fs drivers loaded. Perhaps to your point, the filesystem tests failing
due to not having a kernel module loaded should be marked as skipped
because the test isn't really run.

Here is an example of a successfully completed pipeline
https://gitlab.com/gwashburn/grub/-/pipelines/258561619

And here is the raw log for the x86_64-efi build and test (search for
"Testsuite summary" and look at the lines above for individual test
pass/fail):
https://storage.googleapis.com/gitlab-gprd-artifacts/0e/0d/0e0d2ccc27a1e9a121b3d4ef831c56b5fd81cc8b565a3dbc5fa09b9a58edbcb7/2021_02_19/1042905700/1137549157/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8&response-content-disposition=inline&GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=ewpRWBqWWorOXak6hFkb5kUEfefU1biAtu6xY2Rtyds3%2BduM6q0kiFIKe4A5%0A8wS%2FPbd8Al3AwF45Q22KcpYyZ87UBkryQHjispAj%2B3tBQrnnyOYSRKGNV%2Bbz%0A4iaKAdYn4onIcJM9Ro4RkJ%2FCAY2tBxWmmLoEZ%2FQzWLvbhH%2BJSmuNqtEZXfuD%0AI1tXD1ZKgoLcYCCLNz8iaMFpgB32NfR2W6sDDuFb24ZFSy0X7H02KRVAMIor%0Akhj5QXdXGqoqFFXXMM9r8ZGv34Kh4GdDbVjMmJ%2FaDhvLPgmZF3UKnhcDYJoB%0AiMj%2BbimYNoiQW0SLg4hHA11PkKNn4KPAFqhLVscsCw%3D%3D&Expires=1631130462

This is all to show that the tests are (mostly) working. There's some
post-processing on the pass/fail status of some tests to ignore
expected failures (eg. zfs_test). Occasionally there are spurious test
failures, I think due to the VM getting starved at times (eg. why
grub_cmd_sleep fails and is ignored).

No as far as you personally using the tests, based on the above what do
we need to get that to happen? I don't know the setup that you plan on
doing the tests on (is it debian based?). Do you want a script that
sets up the environment for you? Perhaps a docker or lxd setup
(probably better in the long run)?

A word about why I haven't verified the fs tests that require kernel
modules not loaded in gitlab shared runner, I could run these on my
system. However, I've had some strange experiences in the past with the
fs tests not cleaning up well and causing some issues with the rest of
the system. So I'm not keen on running all the fs tests on my daily
driver. Perhaps in a container (a virtual machine would work, but I
suspect really slow, because many of the tests use virtual machines for
testing, so there would be VMs in a VM).

I'd be curious to know what specific issues you had with the test, if
you can remember or reproduce them.

> > It is my understanding that we have travis-ci tests that get run (at
> > some point?), however they are only build tests and so would not
> > have caught this. It was precisely this scenario that I hoped to
> > avoid by doing more thorough continuous integration, which runs the
> > extensive array of "make check" tests, when I submitted the
> > Gitlab-CI patch series (which would've caught this automatically if
> > it had been merged). To me this scenario is the poster child for
> > taking action on this front. Can we make this a priority?
> 
> I think I can take a closer look at patch set mentioned above in a
> week or two. If "make check" works as expected and can be run locally
> then we can think about automating the testing.

I think the requirement to "run locally" should be more precisely
defined. Is that all tests for all architectures? Running it reliably
on an local machine might be difficult due to the variability of
configurations. Perhaps in a docker or lxd container would be the best
route for that. Thoughts?

Glenn

Re: Where is the testing? (was: Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock)

[Daniel Kiper]

On Wed, Sep 08, 2021 at 07:57:53PM +0000, Glenn Washburn wrote:
> On Wed, 8 Sep 2021 18:03:50 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Wed, Sep 08, 2021 at 01:22:20AM +0000, Glenn Washburn wrote:
> > > It looks like the xfs_test test succeeds with tag grub-2.06-rc1a,
> > > fails with tag grub-2.06, and succeeds with current master. Yes, as
> > > expected. However, what this tells me is that no "make check" tests
> > > were done before finalizing the 2.06 release. I was under the
> > > impression that that was part of the release procedure. If its not,
> > > it would seem that we're not using the tests at a time when they
> > > would have the most impact.
> >
> > Currently I run build tests for all architectures and platforms and
> > Coverity for x86_64 EFI before every push and release. I know this is
> > not enough but I tried "make check" at least once and got the
> > impression that the tests are not reliable. Sadly I have not time to
> > dive deeper and fix this and that. If someone, you?, wants to verify
> > all tests and fix broken ones I will be more than happy to start
> > using it (it seems to me your "[PATCH v2 0/8] Various
> > fixes/improvements for tests" patch set fixes some issues but I am
> > not sure if all of them).
>
> I suspect the problem you're running into with the tests is more a
> matter of setting up the expected environment than the tests
> themselves. From my experience, most of the tests work once everything
> is setup right. My gitlab patch shows how to do this on Ubuntu 20.04.
>
> Due to not being able to load kernel modules on the shared gitlab
> runners, I was not able to run all the tests either, and there are some
> tests that fail. The failures that come to mind are on Sparc and some
> of the functional tests, and I've notified the list, but no one came
> forward to fix them. So what I've done is have a list of known failing
> tests and ignore those in determining whether the test suite as a whole
> has succeeded. I'm not convinced that that behavior is something we
> should include in the test framework itself.
>
> So to my mind, I have verified (nearly) all tests on most platforms. I
> have fixed the ones I could. I think the biggest low hanging fruit (for
> me), is to get the tests running on a system with all the needed kernel
> fs drivers loaded. Perhaps to your point, the filesystem tests failing
> due to not having a kernel module loaded should be marked as skipped
> because the test isn't really run.
>
> Here is an example of a successfully completed pipeline
> https://gitlab.com/gwashburn/grub/-/pipelines/258561619
>
> And here is the raw log for the x86_64-efi build and test (search for
> "Testsuite summary" and look at the lines above for individual test
> pass/fail):
> https://storage.googleapis.com/gitlab-gprd-artifacts/0e/0d/0e0d2ccc27a1e9a121b3d4ef831c56b5fd81cc8b565a3dbc5fa09b9a58edbcb7/2021_02_19/1042905700/1137549157/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8&response-content-disposition=inline&GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=ewpRWBqWWorOXak6hFkb5kUEfefU1biAtu6xY2Rtyds3%2BduM6q0kiFIKe4A5%0A8wS%2FPbd8Al3AwF45Q22KcpYyZ87UBkryQHjispAj%2B3tBQrnnyOYSRKGNV%2Bbz%0A4iaKAdYn4onIcJM9Ro4RkJ%2FCAY2tBxWmmLoEZ%2FQzWLvbhH%2BJSmuNqtEZXfuD%0AI1tXD1ZKgoLcYCCLNz8iaMFpgB32NfR2W6sDDuFb24ZFSy0X7H02KRVAMIor%0Akhj5QXdXGqoqFFXXMM9r8ZGv34Kh4GdDbVjMmJ%2FaDhvLPgmZF3UKnhcDYJoB%0AiMj%2BbimYNoiQW0SLg4hHA11PkKNn4KPAFqhLVscsCw%3D%3D&Expires=1631130462
>
> This is all to show that the tests are (mostly) working. There's some
> post-processing on the pass/fail status of some tests to ignore
> expected failures (eg. zfs_test). Occasionally there are spurious test
> failures, I think due to the VM getting starved at times (eg. why
> grub_cmd_sleep fails and is ignored).
>
> No as far as you personally using the tests, based on the above what do
> we need to get that to happen? I don't know the setup that you plan on
> doing the tests on (is it debian based?). Do you want a script that
> sets up the environment for you? Perhaps a docker or lxd setup
> (probably better in the long run)?
>
> A word about why I haven't verified the fs tests that require kernel
> modules not loaded in gitlab shared runner, I could run these on my
> system. However, I've had some strange experiences in the past with the
> fs tests not cleaning up well and causing some issues with the rest of
> the system. So I'm not keen on running all the fs tests on my daily
> driver. Perhaps in a container (a virtual machine would work, but I
> suspect really slow, because many of the tests use virtual machines for
> testing, so there would be VMs in a VM).
>
> I'd be curious to know what specific issues you had with the test, if
> you can remember or reproduce them.
>
> > > It is my understanding that we have travis-ci tests that get run (at
> > > some point?), however they are only build tests and so would not
> > > have caught this. It was precisely this scenario that I hoped to
> > > avoid by doing more thorough continuous integration, which runs the
> > > extensive array of "make check" tests, when I submitted the
> > > Gitlab-CI patch series (which would've caught this automatically if
> > > it had been merged). To me this scenario is the poster child for
> > > taking action on this front. Can we make this a priority?
> >
> > I think I can take a closer look at patch set mentioned above in a
> > week or two. If "make check" works as expected and can be run locally
> > then we can think about automating the testing.
>
> I think the requirement to "run locally" should be more precisely
> defined. Is that all tests for all architectures? Running it reliably
> on an local machine might be difficult due to the variability of
> configurations. Perhaps in a docker or lxd container would be the best
> route for that. Thoughts?

First of all, thank you for doing so much experiments and describing the
results. I believe we can use your knowledge to improve the GRUB testing.

I think everybody should be able to run all or at least most tests on
their local machines, baremetal or VM. So, we should have a clear
description how to do that, e.g., in the INSTALL and maybe docs/grub-dev.texi
files. It seems to me the reference config should be based on latest Debian
stable. It should list the packages needed to install, important changes
needed to make tests work, etc. If some binaries needed to build the GRUB
for a given platform or run tests are missing from Debian I am OK with
pointing people to the well known and trusted repositories, e.g. [1] and [2].
If we know some tests may fail in some situations we have to document it.
When we are sure this basic setup works then we can think how to use
containers, if some want to, automate these tests and run them on the
upstream repository.

Daniel

[1] https://mirrors.kernel.org/pub/tools/crosstool/
[2] https://toolchains.bootlin.com/