[v3] squashfs: Add regression test for sanity check bug

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Adds a regression test for the fixes
c1b2028315 ("squashfs: fix inode lookup sanity checks")
and
8b44ca2b62 ("squashfs: fix xattr id and id lookup sanity checks")

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---

Changes to v2:
 - Rename to squashfs01
 - Add mksquashfs to needs_cmds
 - Use needs_device and mount syscall instead of mount tool
 - Moved test file creation to setup
 - Use tst_cmd instead of tst_system
 - Use flag to call umount conditionally in cleanup

Changes to v1:
 - Implement whole test in c
 - Fixed whitespaces...

 runtest/fs                                |   2 +
 testcases/kernel/fs/squashfs/.gitignore   |   1 +
 testcases/kernel/fs/squashfs/Makefile     |   9 ++
 testcases/kernel/fs/squashfs/squashfs01.c | 121 ++++++++++++++++++++++
 4 files changed, 133 insertions(+)
 create mode 100644 testcases/kernel/fs/squashfs/.gitignore
 create mode 100644 testcases/kernel/fs/squashfs/Makefile
 create mode 100644 testcases/kernel/fs/squashfs/squashfs01.c

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> Adds a regression test for the fixes
> c1b2028315 ("squashfs: fix inode lookup sanity checks")
> and
> 8b44ca2b62 ("squashfs: fix xattr id and id lookup sanity checks")
>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Acked-by: Richard Palethorpe <rpalethorpe@suse.com>

Hi Joery
> From: Joerg Vehlow<joerg.vehlow@aox-tech.de>
>
> Adds a regression test for the fixes
> c1b2028315 ("squashfs: fix inode lookup sanity checks")
> and
> 8b44ca2b62 ("squashfs: fix xattr id and id lookup sanity checks")
>
> Signed-off-by: Joerg Vehlow<joerg.vehlow@aox-tech.de>
> ---
>
> Changes to v2:
>   - Rename to squashfs01
>   - Add mksquashfs to needs_cmds
>   - Use needs_device and mount syscall instead of mount tool
>   - Moved test file creation to setup
>   - Use tst_cmd instead of tst_system
>   - Use flag to call umount conditionally in cleanup
>
> Changes to v1:
>   - Implement whole test in c
>   - Fixed whitespaces...
>
>   runtest/fs                                |   2 +
>   testcases/kernel/fs/squashfs/.gitignore   |   1 +
>   testcases/kernel/fs/squashfs/Makefile     |   9 ++
>   testcases/kernel/fs/squashfs/squashfs01.c | 121 ++++++++++++++++++++++
>   4 files changed, 133 insertions(+)
>   create mode 100644 testcases/kernel/fs/squashfs/.gitignore
>   create mode 100644 testcases/kernel/fs/squashfs/Makefile
>   create mode 100644 testcases/kernel/fs/squashfs/squashfs01.c
>
> diff --git a/runtest/fs b/runtest/fs
> index 17b1415eb..1d753e0dd 100644
> --- a/runtest/fs
> +++ b/runtest/fs
> @@ -85,3 +85,5 @@ fs_fill fs_fill
>
>   binfmt_misc01 binfmt_misc01.sh
>   binfmt_misc02 binfmt_misc02.sh
> +
> +squashfs01 squashfs01
> diff --git a/testcases/kernel/fs/squashfs/.gitignore b/testcases/kernel/fs/squashfs/.gitignore
> new file mode 100644
> index 000000000..d28920fe8
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/.gitignore
> @@ -0,0 +1 @@
> +squashfs01
> diff --git a/testcases/kernel/fs/squashfs/Makefile b/testcases/kernel/fs/squashfs/Makefile
> new file mode 100644
> index 000000000..67021139c
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2009, Cisco Systems Inc.
> +# Ngie Cooper, July 2009
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/fs/squashfs/squashfs01.c b/testcases/kernel/fs/squashfs/squashfs01.c
> new file mode 100644
> index 000000000..f02c91f83
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/squashfs01.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Joerg Vehlow<joerg.vehlow@aox-tech.de>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Kernel commits
> + *
> + * - f37aa4c7366 (squashfs: add more sanity checks in id lookup)
> + * - eabac19e40c (squashfs: add more sanity checks in inode lookup)
> + * - 506220d2ba2 (squashfs: add more sanity checks in xattr id lookup)
> + *
> + * added some sanity checks, that verify the size of
> + * inode lookup, id (uid/gid) and xattr blocks in the squashfs,
> + * but broke mounting filesystems with completely filled blocks.
> + * A block has a max size of 8192.
> + * An inode lookup entry has an uncompressed size of 8 bytes,
> + * an id block 4 bytes and an xattr block 16 bytes.
> + *
> + *
> + * To fill up at least one block for each of the three tables,
> + * 2048 files with unique uid/gid and xattr are created.
> + *
> + *
> + * The bugs are fixed in kernel commits
> + *
> + * - c1b2028315c (squashfs: fix inode lookup sanity checks)
> + * - 8b44ca2b634 (squashfs: fix xattr id and id lookup sanity checks)
> + */
> +
> +#include<stdio.h>
> +#include<sys/mount.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +static const char *MOUNT_DIR = "mnt";
> +static const char *DATA_DIR = "data";
> +
> +static int mounted;
> +
> +static void cleanup(void)
> +{
> +	if (mounted)
> +		SAFE_UMOUNT("mnt");
> +}
> +
> +static void setup(void)
> +{
> +	int i;
> +
> +	SAFE_MKDIR(DATA_DIR, 0777);
> +
> +	for (i = 0; i<  2048; ++i) {
> +		int fd;
> +		char name[20];
> +
> +		sprintf(name, "%s/%d", DATA_DIR, i);
> +		fd = SAFE_OPEN(name, O_CREAT | O_EXCL, 0666);
> +		SAFE_FCHOWN(fd, i, i);
> +
> +		/* This must be either "security", "user" or "trusted" namespace,
> +		 * because squashfs cannot store other namespaces.
> +		 * Since the files are most likely created on a tmpfs,
> +		 * "user" namespace is not possible, because it is not allowed.
> +		 */
> +		SAFE_FSETXATTR(fd, "security.x",&i, sizeof(i), 0);
> +		close(fd);
> +	}
> +
> +	/* Create squashfs without any compression.
> +	 * This allows reasoning about block sizes.
> +	 * Redirect stdout, to get rid of undefined uid messages
> +	 */
> +	const char *argv[] = {
> +		"mksquashfs", DATA_DIR, tst_device->dev,
> +		"-noappend", "-noI", "-noD", "-noX", "-noF", NULL
> +	};
> +	tst_cmd(argv, "/dev/null", NULL, 0);
We have SAFE_CMD api.
> +
> +	SAFE_MKDIR(MOUNT_DIR, 0777);
> +}
> +
> +static void run(void)
> +{
> +	tst_res(TINFO, "Test squashfs sanity check regressions");
> +
> +	if (mount(tst_device->dev, MOUNT_DIR, "squashfs", 0, NULL) != 0)
> +		tst_brk(TFAIL | TERRNO, "Mount failed");
> +	mounted = 1;
> +
> +	SAFE_UMOUNT("mnt");
> +	mounted = 0;
> +
> +	tst_res(TPASS, "Test passed");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.cleanup = cleanup,
> +	.setup = setup,
> +	.needs_root = 1,
> +	.needs_device = 1,
> +	.dev_min_size = 1,
> +	.needs_cmds = (const char *const []) {
> +		"mksquashfs",
> +		NULL
> +	},
> +	.needs_drivers = (const char *const []) {
> +		"squashfs",
> +		NULL
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "c1b2028315c"},
> +		{"linux-git", "8b44ca2b634"},
> +		{}
> +	},
> +	.needs_tmpdir = 1,
needs_device has enabled needs_tmpdir in internal, so we don't need to 
set it here.
> +};

Hi,


On 7/15/2021 10:21 AM, xuyang2018.jy@fujitsu.com wrote:
>
>> +	tst_cmd(argv, "/dev/null", NULL, 0);
> We have SAFE_CMD api.
Yes makes sense. This can be replaced during merge I guess.
-       tst_cmd(argv, "/dev/null", NULL, 0);
+       SAFE_CMD(argv, "/dev/null", NULL);
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.cleanup = cleanup,
>> +	.setup = setup,
>> +	.needs_root = 1,
>> +	.needs_device = 1,
>> +	.dev_min_size = 1,
>> +	.needs_cmds = (const char *const []) {
>> +		"mksquashfs",
>> +		NULL
>> +	},
>> +	.needs_drivers = (const char *const []) {
>> +		"squashfs",
>> +		NULL
>> +	},
>> +	.tags = (const struct tst_tag[]) {
>> +		{"linux-git", "c1b2028315c"},
>> +		{"linux-git", "8b44ca2b634"},
>> +		{}
>> +	},
>> +	.needs_tmpdir = 1,
> needs_device has enabled needs_tmpdir in internal, so we don't need to
> set it here.
Honestly I hate implicitness like that. I think if the test itself needs 
the tmpdir, it should state it and not rely on some other "needs_*" 
stuff to also enable it.
But if whoever merges this agrees with you, he can change it...

Joerg

Hi!
> >> +static struct tst_test test = {
> >> +	.test_all = run,
> >> +	.cleanup = cleanup,
> >> +	.setup = setup,
> >> +	.needs_root = 1,
> >> +	.needs_device = 1,
> >> +	.dev_min_size = 1,
> >> +	.needs_cmds = (const char *const []) {
> >> +		"mksquashfs",
> >> +		NULL
> >> +	},
> >> +	.needs_drivers = (const char *const []) {
> >> +		"squashfs",
> >> +		NULL
> >> +	},
> >> +	.tags = (const struct tst_tag[]) {
> >> +		{"linux-git", "c1b2028315c"},
> >> +		{"linux-git", "8b44ca2b634"},
> >> +		{}
> >> +	},
> >> +	.needs_tmpdir = 1,
> > needs_device has enabled needs_tmpdir in internal, so we don't need to
> > set it here.
> Honestly I hate implicitness like that. I think if the test itself needs 
> the tmpdir, it should state it and not rely on some other "needs_*" 
> stuff to also enable it.
> But if whoever merges this agrees with you, he can change it...

We tend to avoid listing full subtree of dependencies, in this case it's
not that bad, but it tends to get out of hand quickly.

For instance mount_device flag needs implies format_device which implies
needs_device which implies needs_tmpdir.

Also the dev_min_size = 1 does not have any efect here, since it can be
used only to request bigger-than-default size and gets ignored here. I
guess that we can merge this as it is and I will add needs_loopdev to
the tst_test structure later which will just allocate loop device and
pass it down to the test.

Hi!
> > For instance mount_device flag needs implies format_device which implies
> > needs_device which implies needs_tmpdir.
> I agree with that. If needs_tmpdir was only required, because 
> needs_device is required, I wouldn't add it.
> But if needs_device implementation is changed, the test still needs a 
> tmpdir. That's why I would always vote for adding it here.

The .needs_device flag implies .needs_tmpdir that's a part of the test
library API.

> > Also the dev_min_size = 1 does not have any efect here, since it can be
> > used only to request bigger-than-default size and gets ignored here. I
> > guess that we can merge this as it is and I will add needs_loopdev to
> > the tst_test structure later which will just allocate loop device and
> > pass it down to the test.
> This is true, but the test should also specify what it needs. If for 
> whatever reason DEV_SIZE_MB is redefined to a smaller value, the test 
> would still work.

The DEV_SIZE_MB will never go backward, this is actually the minimal
size that is needed in order to create all supported Linux filesystems
and we need to bump the size up every few years. I think that it was
100MB when I started to work on LTP and it got up to 256MB, which is
also the reason the tests cannot know and why this is hidden in the
library.

> To be honest, for "1" it doesn't matter. But it it was bigger, it makes 
> total sense to specify the size if the test knows it...

Also LTP allows to be passed a real block device to the tests, in a case
that you want to test a block driver, in which case the test has no
exact controll on the device size, which is also the reason why we
allow minimal size to be specified but not exact size.

> I don't understand why a lot of developers like implicit definitions so 
> much more over explicit definitions.
> I could understand it for language intrinsic stuff, because that is (or 
> could be) known to all developers.
> But for someone, who rarely works on a project or switches between 
> different projects implicit information is bad!

I think that we can actually make things better by adding all of the
text I've written above as a top level description in the tst_device.c.

Hi Cyril,

On 7/15/2021 11:27 AM, Cyril Hrubis wrote:
> Hi!
>>>> +static struct tst_test test = {
>>>> +	.test_all = run,
>>>> +	.cleanup = cleanup,
>>>> +	.setup = setup,
>>>> +	.needs_root = 1,
>>>> +	.needs_device = 1,
>>>> +	.dev_min_size = 1,
>>>> +	.needs_cmds = (const char *const []) {
>>>> +		"mksquashfs",
>>>> +		NULL
>>>> +	},
>>>> +	.needs_drivers = (const char *const []) {
>>>> +		"squashfs",
>>>> +		NULL
>>>> +	},
>>>> +	.tags = (const struct tst_tag[]) {
>>>> +		{"linux-git", "c1b2028315c"},
>>>> +		{"linux-git", "8b44ca2b634"},
>>>> +		{}
>>>> +	},
>>>> +	.needs_tmpdir = 1,
>>> needs_device has enabled needs_tmpdir in internal, so we don't need to
>>> set it here.
>> Honestly I hate implicitness like that. I think if the test itself needs
>> the tmpdir, it should state it and not rely on some other "needs_*"
>> stuff to also enable it.
>> But if whoever merges this agrees with you, he can change it...
> We tend to avoid listing full subtree of dependencies, in this case it's
> not that bad, but it tends to get out of hand quickly.
>
> For instance mount_device flag needs implies format_device which implies
> needs_device which implies needs_tmpdir.
I agree with that. If needs_tmpdir was only required, because 
needs_device is required, I wouldn't add it.
But if needs_device implementation is changed, the test still needs a 
tmpdir. That's why I would always vote for adding it here.
> Also the dev_min_size = 1 does not have any efect here, since it can be
> used only to request bigger-than-default size and gets ignored here. I
> guess that we can merge this as it is and I will add needs_loopdev to
> the tst_test structure later which will just allocate loop device and
> pass it down to the test.
This is true, but the test should also specify what it needs. If for 
whatever reason DEV_SIZE_MB is redefined to a smaller value, the test 
would still work.
To be honest, for "1" it doesn't matter. But it it was bigger, it makes 
total sense to specify the size if the test knows it...

I don't understand why a lot of developers like implicit definitions so 
much more over explicit definitions.
I could understand it for language intrinsic stuff, because that is (or 
could be) known to all developers.
But for someone, who rarely works on a project or switches between 
different projects implicit information is bad!

Joerg

Hi C

On 7/15/2021 12:09 PM, Cyril Hrubis wrote:
> Hi!
>>> For instance mount_device flag needs implies format_device which implies
>>> needs_device which implies needs_tmpdir.
>> I agree with that. If needs_tmpdir was only required, because
>> needs_device is required, I wouldn't add it.
>> But if needs_device implementation is changed, the test still needs a
>> tmpdir. That's why I would always vote for adding it here.
> The .needs_device flag implies .needs_tmpdir that's a part of the test
> library API.
>
>>> Also the dev_min_size = 1 does not have any efect here, since it can be
>>> used only to request bigger-than-default size and gets ignored here. I
>>> guess that we can merge this as it is and I will add needs_loopdev to
>>> the tst_test structure later which will just allocate loop device and
>>> pass it down to the test.
>> This is true, but the test should also specify what it needs. If for
>> whatever reason DEV_SIZE_MB is redefined to a smaller value, the test
>> would still work.
> The DEV_SIZE_MB will never go backward, this is actually the minimal
> size that is needed in order to create all supported Linux filesystems
> and we need to bump the size up every few years. I think that it was
> 100MB when I started to work on LTP and it got up to 256MB, which is
> also the reason the tests cannot know and why this is hidden in the
> library.
>
>> To be honest, for "1" it doesn't matter. But it it was bigger, it makes
>> total sense to specify the size if the test knows it...
> Also LTP allows to be passed a real block device to the tests, in a case
> that you want to test a block driver, in which case the test has no
> exact controll on the device size, which is also the reason why we
> allow minimal size to be specified but not exact size.
>
>> I don't understand why a lot of developers like implicit definitions so
>> much more over explicit definitions.
>> I could understand it for language intrinsic stuff, because that is (or
>> could be) known to all developers.
>> But for someone, who rarely works on a project or switches between
>> different projects implicit information is bad!
> I think that we can actually make things better by adding all of the
> text I've written above as a top level description in the tst_device.c.
>

I guess we agree to disagree on this matter. It is the same as the 
discussion mailinglist vs. github for patches.
Change the metadata in whatever way you like while merging.

Joerg

Hi!
> > Also the dev_min_size = 1 does not have any efect here, since it can be
> > used only to request bigger-than-default size and gets ignored here. I
> > guess that we can merge this as it is and I will add needs_loopdev to
> > the tst_test structure later which will just allocate loop device and
> > pass it down to the test.
> This is true, but the test should also specify what it needs. If for 
> whatever reason DEV_SIZE_MB is redefined to a smaller value, the test 
> would still work.
> To be honest, for "1" it doesn't matter. But it it was bigger, it makes 
> total sense to specify the size if the test knows it...

I was thinking about it and we can easily allow the test to request
smaller than the default size with a pretty minimal change:

diff --git a/lib/tst_device.c b/lib/tst_device.c
index c91c6cd55..4ef802c41 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -300,7 +300,7 @@ const char *tst_acquire_device__(unsigned int size)
        unsigned int acq_dev_size;
        uint64_t ltp_dev_size;

-       acq_dev_size = MAX(size, DEV_SIZE_MB);
+       acq_dev_size = size ? size : DEV_SIZE_MB;

        dev = getenv("LTP_DEV");


This shouldn't break anything while it allows better controll for the
test over the device size.

Hi!
Pushed with minor changes, thanks.

Apart from removing the needs_tmpdir I've also changed the TPASS message
to something more meaningful and move the TINFO message to setup so that
it's not printed on each iteration.

Full diff:

diff --git a/testcases/kernel/fs/squashfs/squashfs01.c b/testcases/kernel/fs/squashfs/squashfs01.c
index f02c91f83..502de419d 100644
--- a/testcases/kernel/fs/squashfs/squashfs01.c
+++ b/testcases/kernel/fs/squashfs/squashfs01.c
@@ -51,6 +51,8 @@ static void setup(void)
 {
 	int i;
 
+	tst_res(TINFO, "Test squashfs sanity check regressions");
+
 	SAFE_MKDIR(DATA_DIR, 0777);
 
 	for (i = 0; i < 2048; ++i) {
@@ -85,8 +87,6 @@ static void setup(void)
 
 static void run(void)
 {
-	tst_res(TINFO, "Test squashfs sanity check regressions");
-
 	if (mount(tst_device->dev, MOUNT_DIR, "squashfs", 0, NULL) != 0)
 		tst_brk(TFAIL | TERRNO, "Mount failed");
 	mounted = 1;
@@ -94,7 +94,7 @@ static void run(void)
 	SAFE_UMOUNT("mnt");
 	mounted = 0;
 
-	tst_res(TPASS, "Test passed");
+	tst_res(TPASS, "Regression not detected");
 }
 
 static struct tst_test test = {
@@ -117,5 +117,4 @@ static struct tst_test test = {
 		{"linux-git", "8b44ca2b634"},
 		{}
 	},
-	.needs_tmpdir = 1,
 };