[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Joerg Vehlow]

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 v1:
 - Implement whole test in c
 - Fixed whitespaces...

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

diff --git a/runtest/fs b/runtest/fs
index 17b1415eb..2091b00f8 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
+
+squashfs_regression squashfs_regression
diff --git a/testcases/kernel/fs/squashfs/.gitignore b/testcases/kernel/fs/squashfs/.gitignore
new file mode 100644
index 000000000..45c908fff
--- /dev/null
+++ b/testcases/kernel/fs/squashfs/.gitignore
@@ -0,0 +1 @@
+squashfs_regression
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/squashfs_regression.c b/testcases/kernel/fs/squashfs/squashfs_regression.c
new file mode 100644
index 000000000..23f681367
--- /dev/null
+++ b/testcases/kernel/fs/squashfs/squashfs_regression.c
@@ -0,0 +1,99 @@
+// 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 void cleanup(void)
+{
+	umount("mnt");
+}
+
+static void run(void)
+{
+	int i;
+
+	tst_res(TINFO, "Test squashfs sanity check regressions");
+
+	SAFE_MKDIR("data", 0777);
+
+	for (i = 0; i < 2048; ++i) {
+		int fd;
+		char name[20];
+
+		sprintf(name, "data/%d", 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 comporession.
+	 * This allows reasoning about block sizes
+	 */
+	TST_EXP_PASS(tst_system(
+		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
+	), "Create squashfs");
+
+	SAFE_MKDIR("mnt", 0777);
+	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
+
+	SAFE_UMOUNT("mnt");
+
+	tst_res(TPASS, "Test passed");
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.cleanup = cleanup,
+	.needs_root = 1,
+	.needs_drivers = (const char *const []) {
+		"squashfs",
+		"loop",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "c1b2028315c"},
+		{"linux-git", "8b44ca2b634"},
+		{}
+	},
+	.needs_tmpdir = 1,
+};
-- 
2.25.1

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Richard Palethorpe]

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>
> ---
>
> 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 ++
>  .../kernel/fs/squashfs/squashfs_regression.c  | 99 +++++++++++++++++++
>  4 files changed, 111 insertions(+)
>  create mode 100644 testcases/kernel/fs/squashfs/.gitignore
>  create mode 100644 testcases/kernel/fs/squashfs/Makefile
>  create mode 100644 testcases/kernel/fs/squashfs/squashfs_regression.c
>
> diff --git a/runtest/fs b/runtest/fs
> index 17b1415eb..2091b00f8 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
> +
> +squashfs_regression squashfs_regression
> diff --git a/testcases/kernel/fs/squashfs/.gitignore b/testcases/kernel/fs/squashfs/.gitignore
> new file mode 100644
> index 000000000..45c908fff
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/.gitignore
> @@ -0,0 +1 @@
> +squashfs_regression
> 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/squashfs_regression.c b/testcases/kernel/fs/squashfs/squashfs_regression.c
> new file mode 100644
> index 000000000..23f681367
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/squashfs_regression.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Joerg Vehlow <joerg.vehlow@aox-tech.de>
> + */
> +
> +/*\
> + * [DESCRIPTION]

I think it is [Description] now.

> + *
> + * 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 void cleanup(void)
> +{
> +	umount("mnt");
> +}
> +
> +static void run(void)
> +{
> +	int i;
> +
> +	tst_res(TINFO, "Test squashfs sanity check regressions");
> +
> +	SAFE_MKDIR("data", 0777);
> +
> +	for (i = 0; i < 2048; ++i) {
> +		int fd;
> +		char name[20];
> +
> +		sprintf(name, "data/%d", 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 comporession.
> +	 * This allows reasoning about block sizes
> +	 */
> +	TST_EXP_PASS(tst_system(
> +		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"

I guess the existing API functions to create an image will not work with
squashfs?

At any rate, mksquashfs should be added to .needs_cmds.

> +	), "Create squashfs");
> +
> +	SAFE_MKDIR("mnt", 0777);
> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw
> mnt"));

Also why not use safe_mount? I think we have some infra to find a spare
loop device (.needs_device).

> +
> +	SAFE_UMOUNT("mnt");
> +
> +	tst_res(TPASS, "Test passed");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_drivers = (const char *const []) {
> +		"squashfs",
> +		"loop",
> +		NULL
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "c1b2028315c"},
> +		{"linux-git", "8b44ca2b634"},
> +		{}
> +	},
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.25.1


-- 
Thank you,
Richard.

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Joerg Vehlow]

Hi Richard,

On 7/14/2021 8:53 AM, Richard Palethorpe wrote:
> Hello Joerg,
>
> Joerg Vehlow <lkml@jv-coder.de> writes:
>
>> + */
>> +
>> +/*\
>> + * [DESCRIPTION]
> I think it is [Description] now.
Both seem to work and there is no documentation for this.
But looks the lower case variant is used more often. Will update this 
for v3.
>
>> + *
>> +
>> +	/* Create squashfs without any comporession.
>> +	 * This allows reasoning about block sizes
>> +	 */
>> +	TST_EXP_PASS(tst_system(
>> +		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
> I guess the existing API functions to create an image will not work with
> squashfs?
Honestly I don't know what they are... If it is .format_device stuff, 
then no, this cannot be used
for squashfs, because the data has to be prepared, before the filesystem 
can be created.
>
> At any rate, mksquashfs should be added to .needs_cmds.
Right, forgot about that when converting from shell
>
>> +	), "Create squashfs");
>> +
>> +	SAFE_MKDIR("mnt", 0777);
>> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw
>> mnt"));
> Also why not use safe_mount? I think we have some infra to find a spare
> loop device (.needs_device).
safe_mount would result in TBROK instead of TFAIL. Since mounting is the 
actual test,
a failed mount must be TFAIL.
But TST_EXP_PASS is also not ideal, because it should use tst_brk and 
not tst_res.
Otherwise SAFE_UMOUNT fails with TBROK..
Is there some other api function or do I have to implement the return 
value check myself?

Here is the diff for using needs_device:
diff --git a/testcases/kernel/fs/squashfs/squashfs_regression.c 
b/testcases/kernel/fs/squashfs/squashfs_regression.c
index 23f681367..affba8069 100644
--- a/testcases/kernel/fs/squashfs/squashfs_regression.c
+++ b/testcases/kernel/fs/squashfs/squashfs_regression.c
@@ -44,6 +44,7 @@ static void cleanup(void)
 ?static void run(void)
 ?{
 ???? int i;
+??? char cmd[1024];

 ???? tst_res(TINFO, "Test squashfs sanity check regressions");

@@ -66,15 +67,17 @@ static void run(void)
 ???? ??? close(fd);
 ???? }

-??? /* Create squashfs without any comporession.
-??? ?* This allows reasoning about block sizes
+??? /* Create squashfs without any compression.
+??? ?* This allows reasoning about block sizes.
+??? ?* Redirect stdout, to get rid of undefined uid messages
 ???? ?*/
-??? TST_EXP_PASS(tst_system(
-??? ??? "mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
-??? ), "Create squashfs");
+??? sprintf(cmd,
+??? ??????? "mksquashfs data %s -noI -noD -noX -noF -noappend >/dev/null",
+??? ??? ??? tst_device->dev);
+??? TST_EXP_PASS(tst_system(cmd), "Create squashfs");

 ???? SAFE_MKDIR("mnt", 0777);
-??? TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
+??? TST_EXP_PASS(mount(tst_device->dev, "mnt", "squashfs", 0, NULL));

 ???? SAFE_UMOUNT("mnt");

@@ -85,9 +88,14 @@ static struct tst_test test = {
 ???? .test_all = run,
 ???? .cleanup = cleanup,
 ???? .needs_root = 1,
+??? .needs_device = 1,
+??? .dev_min_size = 1,
+??? .needs_cmds = (const char *const []) {
+??? ??? "mksquashfs",
+??? ??? NULL
+??? },
 ???? .needs_drivers = (const char *const []) {
 ???? ??? "squashfs",
-??? ??? "loop",
 ???? ??? NULL
 ???? },
 ???? .tags = (const struct tst_tag[]) {


Joerg

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Cyril Hrubis]

Hi!
> 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 v1:
>  - Implement whole test in c
>  - Fixed whitespaces...
> 
>  runtest/fs                                    |  2 +
>  testcases/kernel/fs/squashfs/.gitignore       |  1 +
>  testcases/kernel/fs/squashfs/Makefile         |  9 ++
>  .../kernel/fs/squashfs/squashfs_regression.c  | 99 +++++++++++++++++++
>  4 files changed, 111 insertions(+)
>  create mode 100644 testcases/kernel/fs/squashfs/.gitignore
>  create mode 100644 testcases/kernel/fs/squashfs/Makefile
>  create mode 100644 testcases/kernel/fs/squashfs/squashfs_regression.c
> 
> diff --git a/runtest/fs b/runtest/fs
> index 17b1415eb..2091b00f8 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
> +
> +squashfs_regression squashfs_regression

I wonder if we should add a number suffix or just rename it to
squashfs01

> diff --git a/testcases/kernel/fs/squashfs/.gitignore b/testcases/kernel/fs/squashfs/.gitignore
> new file mode 100644
> index 000000000..45c908fff
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/.gitignore
> @@ -0,0 +1 @@
> +squashfs_regression
> 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/squashfs_regression.c b/testcases/kernel/fs/squashfs/squashfs_regression.c
> new file mode 100644
> index 000000000..23f681367
> --- /dev/null
> +++ b/testcases/kernel/fs/squashfs/squashfs_regression.c
> @@ -0,0 +1,99 @@
> +// 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 void cleanup(void)
> +{
> +	umount("mnt");

We do have tst_umount() in the test library that retries the umount if
it failed because the mount point was bussy. This is because certain
damons scan all newly mounted filesystems and prevent us from umounting
shortly after mount.

Also we usually keep track if device was mounted in a global flag that
is set after succesful mount and unset after successful umount and the
cleanup does:

	if (device_mounted)
		tst_umount("mnt");

> +}
> +
> +static void run(void)
> +{
> +	int i;
> +
> +	tst_res(TINFO, "Test squashfs sanity check regressions");
> +
> +	SAFE_MKDIR("data", 0777);
> +
> +	for (i = 0; i < 2048; ++i) {
> +		int fd;
> +		char name[20];
> +
> +		sprintf(name, "data/%d", 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 comporession.
> +	 * This allows reasoning about block sizes
> +	 */
> +	TST_EXP_PASS(tst_system(
> +		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
> +	), "Create squashfs");

We do have tst_cmd() that can do all this easily in C including the
redirection, it will look like:

	const char *argv[] = {"mksquashfs", "data", "image.raw", "-noI", "-noD", "-noX", "-noF"};

	tst_cmd(argv, "/dev/null", "/dev/null", 0);

And this will redirect stdout and stderr to "/dev/null" and also do all
the error checks and exit the test if mksquashfs has failed.

> +	SAFE_MKDIR("mnt", 0777);

This preparatory part should be in the test setup otherwise the test
will fail with '-i 2'.

> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));

Can we please use the mount() syscall here instead? That should be as
easy as mount("image.raw", "mnt", "squashfs", 0, "-oloop")

> +
> +	SAFE_UMOUNT("mnt");

Here as well, please use tst_umount();

> +	tst_res(TPASS, "Test passed");

This is redundant, isn't it? Or is the umount part that fails?

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_drivers = (const char *const []) {
> +		"squashfs",
> +		"loop",
> +		NULL
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "c1b2028315c"},
> +		{"linux-git", "8b44ca2b634"},
> +		{}
> +	},
> +	.needs_tmpdir = 1,
> +};
> -- 
> 2.25.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Cyril Hrubis]

Hi!
> >> + * [DESCRIPTION]
> > I think it is [Description] now.
> Both seem to work and there is no documentation for this.
> But looks the lower case variant is used more often. Will update this 
> for v3.

I started with uppercase but then everybody else decided that only first
letter in uppercase is the right thing. Ideally there should be only
[Description] in the tree at this point.

> >> + *
> >> +
> >> +	/* Create squashfs without any comporession.
> >> +	 * This allows reasoning about block sizes
> >> +	 */
> >> +	TST_EXP_PASS(tst_system(
> >> +		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
> > I guess the existing API functions to create an image will not work with
> > squashfs?
> Honestly I don't know what they are... If it is .format_device stuff, 
> then no, this cannot be used
> for squashfs, because the data has to be prepared, before the filesystem 
> can be created.

Indeed, there is not much to be done here.

>  ???????? SAFE_MKDIR("mnt", 0777);
> -?????? TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
> +?????? TST_EXP_PASS(mount(tst_device->dev, "mnt", "squashfs", 0, NULL));

I do not think that it matters that much who allocates the device, if
it's the test library or the kernel code. I guess that in both cases it
just picks up the first free device anyways.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Joerg Vehlow]

Hi Cyril,

On 7/14/2021 10:35 AM, Cyril Hrubis wrote:
> Hi!
>> 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 v1:
>>   - Implement whole test in c
>>   - Fixed whitespaces...
>>
>>   runtest/fs                                    |  2 +
>>   testcases/kernel/fs/squashfs/.gitignore       |  1 +
>>   testcases/kernel/fs/squashfs/Makefile         |  9 ++
>>   .../kernel/fs/squashfs/squashfs_regression.c  | 99 +++++++++++++++++++
>>   4 files changed, 111 insertions(+)
>>   create mode 100644 testcases/kernel/fs/squashfs/.gitignore
>>   create mode 100644 testcases/kernel/fs/squashfs/Makefile
>>   create mode 100644 testcases/kernel/fs/squashfs/squashfs_regression.c
>>
>> diff --git a/runtest/fs b/runtest/fs
>> index 17b1415eb..2091b00f8 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
>> +
>> +squashfs_regression squashfs_regression
> I wonder if we should add a number suffix or just rename it to
> squashfs01
Is there any guideline? I used regression suffix, because it 
specifically is a regression test and there are several regression 
tests, that use it.
I dropped a number prefix, because there are several tests without a 
number...
I don't really care what the name is here. If you don't have a strong 
opinion on the regression suffix, I will use squashfs_regression01.

>> diff --git a/testcases/kernel/fs/squashfs/.gitignore b/testcases/kernel/fs/squashfs/.gitignore
>> new file mode 100644
>> index 000000000..45c908fff
>> --- /dev/null
>> +++ b/testcases/kernel/fs/squashfs/.gitignore
>> @@ -0,0 +1 @@
>> +squashfs_regression
>> 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/squashfs_regression.c b/testcases/kernel/fs/squashfs/squashfs_regression.c
>> new file mode 100644
>> index 000000000..23f681367
>> --- /dev/null
>> +++ b/testcases/kernel/fs/squashfs/squashfs_regression.c
>> @@ -0,0 +1,99 @@
>> +// 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 void cleanup(void)
>> +{
>> +	umount("mnt");
> We do have tst_umount() in the test library that retries the umount if
> it failed because the mount point was bussy. This is because certain
> damons scan all newly mounted filesystems and prevent us from umounting
> shortly after mount.
>
> Also we usually keep track if device was mounted in a global flag that
> is set after succesful mount and unset after successful umount and the
> cleanup does:
>
> 	if (device_mounted)
> 		tst_umount("mnt");
Ok, but this could leave the mount, if the test is aborted between 
mounting and setting of the flag, but that should be a rare case.

>
>> +}
>> +
>> +static void run(void)
>> +{
>> +	int i;
>> +
>> +	tst_res(TINFO, "Test squashfs sanity check regressions");
>> +
>> +	SAFE_MKDIR("data", 0777);
>> +
>> +	for (i = 0; i < 2048; ++i) {
>> +		int fd;
>> +		char name[20];
>> +
>> +		sprintf(name, "data/%d", 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 comporession.
>> +	 * This allows reasoning about block sizes
>> +	 */
>> +	TST_EXP_PASS(tst_system(
>> +		"mksquashfs data image.raw -noI -noD -noX -noF >/dev/null 2>&1"
>> +	), "Create squashfs");
> We do have tst_cmd() that can do all this easily in C including the
> redirection, it will look like:
>
> 	const char *argv[] = {"mksquashfs", "data", "image.raw", "-noI", "-noD", "-noX", "-noF"};
>
> 	tst_cmd(argv, "/dev/null", "/dev/null", 0);
>
> And this will redirect stdout and stderr to "/dev/null" and also do all
> the error checks and exit the test if mksquashfs has failed.
Did not know about that, also it requires a NULL at the end.

>
>> +	SAFE_MKDIR("mnt", 0777);
> This preparatory part should be in the test setup otherwise the test
> will fail with '-i 2'.
Right, I'll move the whole setup part to setup.
>
>> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
> Can we please use the mount() syscall here instead? That should be as
> easy as mount("image.raw", "mnt", "squashfs", 0, "-oloop")
Nope, -oloop does not work, because this is interpreted by the mount 
utility, not by the syscall.
I guess I'll use the need_device stuff, to get rid of mount utility call 
then.

>> +
>> +	SAFE_UMOUNT("mnt");
> Here as well, please use tst_umount();
Ok

>
>> +	tst_res(TPASS, "Test passed");
> This is redundant, isn't it? Or is the umount part that fails?
Since I cannot use TST_EXP_PASS further up, I will keep this and check 
the return of mount manually like this:

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;

 ??? tst_umount("mnt");
 ??? mounted = 0;

 ??? tst_res(TPASS, "Test passed");
}

Would that be ok for you or is there another variant of TST_EXP*, that 
uses tst_brk?

Joerg

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Cyril Hrubis]

Hi!
> Is there any guideline? I used regression suffix, because it 
> specifically is a regression test and there are several regression 
> tests, that use it.
> I dropped a number prefix, because there are several tests without a 
> number...
> I don't really care what the name is here. If you don't have a strong 
> opinion on the regression suffix, I will use squashfs_regression01.

Unfortunatelly apart from syscalls there is no clear rule how to name
tests and we are figuring out stuff as we go. Hoever most of the
regression tests do not have regression in name and generally tests are
named as "syscall/driver/filesystem/cve-nickname/etc" followed by a
number.

> > We do have tst_umount() in the test library that retries the umount if
> > it failed because the mount point was bussy. This is because certain
> > damons scan all newly mounted filesystems and prevent us from umounting
> > shortly after mount.
> >
> > Also we usually keep track if device was mounted in a global flag that
> > is set after succesful mount and unset after successful umount and the
> > cleanup does:
> >
> > 	if (device_mounted)
> > 		tst_umount("mnt");
> Ok, but this could leave the mount, if the test is aborted between 
> mounting and setting of the flag, but that should be a rare case.

As long as the flag is set/cleared right after the mount/umount it will
not happen.

Also looking at the code, we have to handle the return value from
tst_umount() in the run() function since it does not call tst_brk().

> > We do have tst_cmd() that can do all this easily in C including the
> > redirection, it will look like:
> >
> > 	const char *argv[] = {"mksquashfs", "data", "image.raw", "-noI", "-noD", "-noX", "-noF"};
> >
> > 	tst_cmd(argv, "/dev/null", "/dev/null", 0);
> >
> > And this will redirect stdout and stderr to "/dev/null" and also do all
> > the error checks and exit the test if mksquashfs has failed.
> Did not know about that, also it requires a NULL at the end.

We do have most of the library functions documented at:

https://github.com/linux-test-project/ltp/wiki/C-Test-API

And yes, the argv has to be NULL terminated, sorry for forgetting that
part.

> >> +	SAFE_MKDIR("mnt", 0777);
> > This preparatory part should be in the test setup otherwise the test
> > will fail with '-i 2'.
> Right, I'll move the whole setup part to setup.
> >
> >> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
> > Can we please use the mount() syscall here instead? That should be as
> > easy as mount("image.raw", "mnt", "squashfs", 0, "-oloop")
> Nope, -oloop does not work, because this is interpreted by the mount 
> utility, not by the syscall.
> I guess I'll use the need_device stuff, to get rid of mount utility call 
> then.

Ah my bad, so the mount command discovers the device in userspace then,
it makes much more sense to use the library to allocate free device for
the test.

But I guess that it would probably be better to use the raw
tst_find_free_loopdev() because the .needs_device flag prepares a
backing file for the device and attaches it as well.

> >> +
> >> +	SAFE_UMOUNT("mnt");
> > Here as well, please use tst_umount();
> Ok
> 
> >
> >> +	tst_res(TPASS, "Test passed");
> > This is redundant, isn't it? Or is the umount part that fails?
> Since I cannot use TST_EXP_PASS further up, I will keep this and check 
> the return of mount manually like this:
> 
> 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;
> 
>  ?????? tst_umount("mnt");
>  ?????? mounted = 0;
> 
>  ?????? tst_res(TPASS, "Test passed");
> }
> 
> Would that be ok for you or is there another variant of TST_EXP*, that 
> uses tst_brk?

I guess that we should check the return value from tst_umount() here as
well, so ti would be better as:

	if (tst_umount("mnt")) {
		tst_brk(TBROK, "Failed to unmount squashfs");
	} else {
		mounted = 0;
		tst_res(TPASS, "Squashfs unmounted");
	}

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] squashfs: Add regression test for sanity check bug

[Joerg Vehlow]

Hi,

On 7/14/2021 11:26 AM, Cyril Hrubis wrote:
> Hi!
>> Is there any guideline? I used regression suffix, because it
>> specifically is a regression test and there are several regression
>> tests, that use it.
>> I dropped a number prefix, because there are several tests without a
>> number...
>> I don't really care what the name is here. If you don't have a strong
>> opinion on the regression suffix, I will use squashfs_regression01.
> Unfortunatelly apart from syscalls there is no clear rule how to name
> tests and we are figuring out stuff as we go. Hoever most of the
> regression tests do not have regression in name and generally tests are
> named as "syscall/driver/filesystem/cve-nickname/etc" followed by a
> number.
Alright, then squashfs01 it is.
>
>>> We do have tst_umount() in the test library that retries the umount if
>>> it failed because the mount point was bussy. This is because certain
>>> damons scan all newly mounted filesystems and prevent us from umounting
>>> shortly after mount.
>>>
>>> Also we usually keep track if device was mounted in a global flag that
>>> is set after succesful mount and unset after successful umount and the
>>> cleanup does:
>>>
>>> 	if (device_mounted)
>>> 		tst_umount("mnt");
>> Ok, but this could leave the mount, if the test is aborted between
>> mounting and setting of the flag, but that should be a rare case.
> As long as the flag is set/cleared right after the mount/umount it will
> not happen.
>
> Also looking at the code, we have to handle the return value from
> tst_umount() in the run() function since it does not call tst_brk().
I guess SAFE_UMOUNT is the way to go here... It uses tst_umount 
internally and tst_brk in case of error.
>
>>> We do have tst_cmd() that can do all this easily in C including the
>>> redirection, it will look like:
>>>
>>> 	const char *argv[] = {"mksquashfs", "data", "image.raw", "-noI", "-noD", "-noX", "-noF"};
>>>
>>> 	tst_cmd(argv, "/dev/null", "/dev/null", 0);
>>>
>>> And this will redirect stdout and stderr to "/dev/null" and also do all
>>> the error checks and exit the test if mksquashfs has failed.
>> Did not know about that, also it requires a NULL at the end.
> We do have most of the library functions documented at:
>
> https://github.com/linux-test-project/ltp/wiki/C-Test-API
>
> And yes, the argv has to be NULL terminated, sorry for forgetting that
> part.
>
>>>> +	SAFE_MKDIR("mnt", 0777);
>>> This preparatory part should be in the test setup otherwise the test
>>> will fail with '-i 2'.
>> Right, I'll move the whole setup part to setup.
>>>> +	TST_EXP_PASS(tst_system("mount -tsquashfs -oloop image.raw mnt"));
>>> Can we please use the mount() syscall here instead? That should be as
>>> easy as mount("image.raw", "mnt", "squashfs", 0, "-oloop")
>> Nope, -oloop does not work, because this is interpreted by the mount
>> utility, not by the syscall.
>> I guess I'll use the need_device stuff, to get rid of mount utility call
>> then.
> Ah my bad, so the mount command discovers the device in userspace then,
> it makes much more sense to use the library to allocate free device for
> the test.
>
> But I guess that it would probably be better to use the raw
> tst_find_free_loopdev() because the .needs_device flag prepares a
> backing file for the device and attaches it as well.
I modified the test to use the backing file - or actually use the loop 
device as target for mksquashfs.
Otherwise it would add even more complexity (setting up and tearing down 
the loop device)
>
>>>> +
>>>> +	SAFE_UMOUNT("mnt");
>>> Here as well, please use tst_umount();
>> Ok
>>
>>>> +	tst_res(TPASS, "Test passed");
>>> This is redundant, isn't it? Or is the umount part that fails?
>> Since I cannot use TST_EXP_PASS further up, I will keep this and check
>> the return of mount manually like this:
>>
>> 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;
>>
>>   ?????? tst_umount("mnt");
>>   ?????? mounted = 0;
>>
>>   ?????? tst_res(TPASS, "Test passed");
>> }
>>
>> Would that be ok for you or is there another variant of TST_EXP*, that
>> uses tst_brk?
> I guess that we should check the return value from tst_umount() here as
> well, so ti would be better as:
>
> 	if (tst_umount("mnt")) {
> 		tst_brk(TBROK, "Failed to unmount squashfs");
> 	} else {
> 		mounted = 0;
> 		tst_res(TPASS, "Squashfs unmounted");
> 	}
SAFE_UMOUNT here as well. I don't care about the return value. The mount 
is what fails, not the umount.

Joerg