[v4] syscalls/statx09: Add new test

This test is basically the same as statx04 but here we check for the
STATX_ATTR_VERITY flag which is currently only implemented on ext4.

Signed-off-by: Dai Shili <daisl.fnst@fujitsu.com>
---
 configure.ac                               |   1 +
 include/lapi/fs.h                          |   4 +
 include/lapi/fsverity.h                    |  39 +++++++
 include/lapi/stat.h                        |   4 +
 m4/ltp-fsverity.m4                         |  10 ++
 runtest/syscalls                           |   1 +
 testcases/kernel/syscalls/statx/.gitignore |   1 +
 testcases/kernel/syscalls/statx/statx09.c  | 160 +++++++++++++++++++++++++++++
 8 files changed, 220 insertions(+)
 create mode 100644 include/lapi/fsverity.h
 create mode 100644 m4/ltp-fsverity.m4
 create mode 100644 testcases/kernel/syscalls/statx/statx09.c

Hi Dai

Now, looks good to me
Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com>

ps: I won't be online next week because of enjoying the Chinese New Year
holiday

Best Regards
Yang Xu
> This test is basically the same as statx04 but here we check for the
> STATX_ATTR_VERITY flag which is currently only implemented on ext4.
> 
> Signed-off-by: Dai Shili<daisl.fnst@fujitsu.com>
> ---
>   configure.ac                               |   1 +
>   include/lapi/fs.h                          |   4 +
>   include/lapi/fsverity.h                    |  39 +++++++
>   include/lapi/stat.h                        |   4 +
>   m4/ltp-fsverity.m4                         |  10 ++
>   runtest/syscalls                           |   1 +
>   testcases/kernel/syscalls/statx/.gitignore |   1 +
>   testcases/kernel/syscalls/statx/statx09.c  | 160 +++++++++++++++++++++++++++++
>   8 files changed, 220 insertions(+)
>   create mode 100644 include/lapi/fsverity.h
>   create mode 100644 m4/ltp-fsverity.m4
>   create mode 100644 testcases/kernel/syscalls/statx/statx09.c
> 
> diff --git a/configure.ac b/configure.ac
> index 3c56d19..aeb486f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -367,6 +367,7 @@ LTP_CHECK_SELINUX
>   LTP_CHECK_SYNC_ADD_AND_FETCH
>   LTP_CHECK_SYSCALL_EVENTFD
>   LTP_CHECK_SYSCALL_FCNTL
> +LTP_CHECK_FSVERITY
> 
>   if test "x$with_numa" = xyes; then
>   	LTP_CHECK_SYSCALL_NUMA
> diff --git a/include/lapi/fs.h b/include/lapi/fs.h
> index aafeab4..27b3a18 100644
> --- a/include/lapi/fs.h
> +++ b/include/lapi/fs.h
> @@ -41,6 +41,10 @@
>   #define FS_NODUMP_FL	   0x00000040 /* do not dump file */
>   #endif
> 
> +#ifndef FS_VERITY_FL
> +#define FS_VERITY_FL	   0x00100000 /* Verity protected inode */
> +#endif
> +
>   /*
>    * Helper function to get MAX_LFS_FILESIZE.
>    * Missing PAGE_SHIFT on some libc prevents defining MAX_LFS_FILESIZE.
> diff --git a/include/lapi/fsverity.h b/include/lapi/fsverity.h
> new file mode 100644
> index 0000000..3a33ca8
> --- /dev/null
> +++ b/include/lapi/fsverity.h
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili<daisl.fnst@cn.fujitsu.com>
> + */
> +#ifndef LAPI_FSVERITY_H__
> +#define LAPI_FSVERITY_H__
> +
> +#include "config.h"
> +#include<stdint.h>
> +#include<sys/ioctl.h>
> +
> +#ifdef HAVE_LINUX_FSVERITY_H
> +#include<linux/fsverity.h>
> +#endif
> +
> +#ifndef FS_VERITY_HASH_ALG_SHA256
> +# define FS_VERITY_HASH_ALG_SHA256       1
> +#endif
> +
> +#ifndef HAVE_STRUCT_FSVERITY_ENABLE_ARG
> +struct fsverity_enable_arg {
> +	uint32_t version;
> +	uint32_t hash_algorithm;
> +	uint32_t block_size;
> +	uint32_t salt_size;
> +	uint64_t salt_ptr;
> +	uint32_t sig_size;
> +	uint32_t __reserved1;
> +	uint64_t sig_ptr;
> +	uint64_t __reserved2[11];
> +};
> +#endif
> +
> +#ifndef FS_IOC_ENABLE_VERITY
> +# define FS_IOC_ENABLE_VERITY    _IOW('f', 133, struct fsverity_enable_arg)
> +#endif
> +
> +#endif
> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index d596058..ce1f2b6 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -223,6 +223,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>   # define STATX_ATTR_AUTOMOUNT	0x00001000
>   #endif
> 
> +#ifndef STATX_ATTR_VERITY
> +# define STATX_ATTR_VERITY	0x00100000
> +#endif
> +
>   #ifndef AT_SYMLINK_NOFOLLOW
>   # define AT_SYMLINK_NOFOLLOW	0x100
>   #endif
> diff --git a/m4/ltp-fsverity.m4 b/m4/ltp-fsverity.m4
> new file mode 100644
> index 0000000..7104886
> --- /dev/null
> +++ b/m4/ltp-fsverity.m4
> @@ -0,0 +1,10 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (c) 2022 Fujitsu Ltd.
> +dnl Author: Dai Shili<daisl.fnst@fujitsu.com>
> +
> +AC_DEFUN([LTP_CHECK_FSVERITY],[
> +	AC_CHECK_HEADERS([linux/fsverity.h], [have_fsverity=yes], [AC_MSG_WARN(missing linux/fsverity.h header)])
> +	if test "x$have_fsverity" = "xyes"; then
> +		AC_CHECK_TYPES(struct fsverity_enable_arg,,,[#include<linux/fsverity.h>])
> +	fi
> +])
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 3b2deb6..7ba0331 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1744,6 +1744,7 @@ statx05 statx05
>   statx06 statx06
>   statx07 statx07
>   statx08 statx08
> +statx09 statx09
> 
>   membarrier01 membarrier01
> 
> diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
> index 4db060d..1cea43c 100644
> --- a/testcases/kernel/syscalls/statx/.gitignore
> +++ b/testcases/kernel/syscalls/statx/.gitignore
> @@ -6,3 +6,4 @@
>   /statx06
>   /statx07
>   /statx08
> +/statx09
> diff --git a/testcases/kernel/syscalls/statx/statx09.c b/testcases/kernel/syscalls/statx/statx09.c
> new file mode 100644
> index 0000000..ba8246b
> --- /dev/null
> +++ b/testcases/kernel/syscalls/statx/statx09.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili<daisl.fnst@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This code tests if the attributes field of statx received expected value.
> + * File set with following flags by using SAFE_IOCTL:
> + *
> + * - STATX_ATTR_VERITY: statx() system call sets STATX_ATTR_VERITY if the file
> + * has fs-verity enabled. This can perform better than FS_IOC_GETFLAGS and
> + * FS_IOC_MEASURE_VERITY because it doesn't require opening the file,
> + * and opening verity files can be expensive.
> + *
> + * Minimum Linux version required is v5.5.
> + */
> +
> +#define _GNU_SOURCE
> +#include<sys/mount.h>
> +#include<stdlib.h>
> +#include "tst_test.h"
> +#include "lapi/fs.h"
> +#include "lapi/fsverity.h"
> +#include "lapi/stat.h"
> +#include<inttypes.h>
> +
> +#define MNTPOINT "mnt_point"
> +#define TESTFILE_FLAGGED MNTPOINT"/test_file1"
> +#define TESTFILE_UNFLAGGED MNTPOINT"/test_file2"
> +
> +static int mount_flag;
> +
> +static const uint32_t hash_algorithms[] = {
> +	FS_VERITY_HASH_ALG_SHA256,
> +};
> +
> +static void test_flagged(void)
> +{
> +	struct statx buf;
> +
> +	TST_EXP_PASS(statx(AT_FDCWD, TESTFILE_FLAGGED, 0, 0,&buf),
> +		"statx(AT_FDCWD, %s, 0, 0,&buf)", TESTFILE_FLAGGED);
> +
> +	if (buf.stx_attributes&  STATX_ATTR_VERITY)
> +		tst_res(TPASS, "STATX_ATTR_VERITY flag is set: (%"PRIu64") ",
> +			(uint64_t)buf.stx_attributes);
> +	else
> +		tst_res(TFAIL, "STATX_ATTR_VERITY flag is not set");
> +}
> +
> +static void test_unflagged(void)
> +{
> +	struct statx buf;
> +
> +	TST_EXP_PASS(statx(AT_FDCWD, TESTFILE_UNFLAGGED, 0, 0,&buf),
> +		"statx(AT_FDCWD, %s, 0, 0,&buf)", TESTFILE_UNFLAGGED);
> +
> +	if ((buf.stx_attributes&  STATX_ATTR_VERITY) == 0)
> +		tst_res(TPASS, "STATX_ATTR_VERITY flag is not set");
> +	else
> +		tst_res(TFAIL, "STATX_ATTR_VERITY flag is set");
> +}
> +
> +static struct test_cases {
> +	void (*tfunc)(void);
> +} tcases[] = {
> +	{&test_flagged},
> +	{&test_unflagged},
> +};
> +
> +static void run(unsigned int i)
> +{
> +	tcases[i].tfunc();
> +}
> +
> +static void flag_setup(void)
> +{
> +	int fd, attr, ret;
> +	struct fsverity_enable_arg enable;
> +
> +	fd = SAFE_OPEN(TESTFILE_FLAGGED, O_RDONLY, 0664);
> +
> +	ret = ioctl(fd, FS_IOC_GETFLAGS,&attr);
> +	if (ret<  0) {
> +		if (errno == ENOTTY)
> +			tst_brk(TCONF | TERRNO, "FS_IOC_GETFLAGS not supported");
> +
> +		tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_GETFLAGS, ...)", fd);
> +	}
> +
> +	memset(&enable, 0, sizeof(enable));
> +	enable.version = 1;
> +	enable.hash_algorithm = hash_algorithms[0];
> +	enable.block_size = 4096;
> +	enable.salt_size = 0;
> +	enable.salt_ptr = (intptr_t)NULL;
> +	enable.sig_size = 0;
> +	enable.sig_ptr = (intptr_t)NULL;
> +
> +	ret = ioctl(fd, FS_IOC_ENABLE_VERITY,&enable);
> +	if (ret<  0) {
> +		if (errno == EOPNOTSUPP) {
> +			tst_brk(TCONF,
> +				"fs-verity is not supported on the file system or by the kernel");
> +		}
> +		tst_brk(TBROK | TERRNO, "ioctl(%i, FS_IOC_ENABLE_VERITY) failed", fd);
> +	}
> +
> +	ret = ioctl(fd, FS_IOC_GETFLAGS,&attr);
> +	if ((ret == 0)&&  !(attr&  FS_VERITY_FL))
> +		tst_res(TFAIL, "%i: fs-verity enabled but FS_VERITY_FL bit not set", fd);
> +
> +	SAFE_CLOSE(fd);
> +}
> +
> +static void setup(void)
> +{
> +	TEST(mount(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, NULL));
> +	if (TST_RET) {
> +		if (TST_ERR == EINVAL)
> +			tst_brk(TCONF, "fs-verity not supported on loopdev");
> +
> +		tst_brk(TBROK | TERRNO, "mount() failed with %ld", TST_RET);
> +	}
> +	mount_flag = 1;
> +
> +	SAFE_FILE_PRINTF(TESTFILE_FLAGGED, "a");
> +	SAFE_FILE_PRINTF(TESTFILE_UNFLAGGED, "a");
> +
> +	flag_setup();
> +}
> +
> +static void cleanup(void)
> +{
> +	if (mount_flag)
> +		tst_umount(MNTPOINT);
> +}
> +
> +static struct tst_test test = {
> +	.test = run,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.format_device = 1,
> +	.dev_fs_type = "ext4",
> +	.dev_fs_opts = (const char *const []){"-O verity", NULL},
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_FS_VERITY",
> +		NULL
> +	},
> +	.needs_cmds = (const char *[]) {
> +		"mkfs.ext4>= 1.45.2",
> +		NULL
> +	}
> +};

Hi!
Pushed with minor adjustenment to the documentation comment, thanks.

Hi all,

> +++ b/testcases/kernel/syscalls/statx/statx09.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Dai Shili <daisl.fnst@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This code tests if the attributes field of statx received expected value.
> + * File set with following flags by using SAFE_IOCTL:
> + *
> + * - STATX_ATTR_VERITY: statx() system call sets STATX_ATTR_VERITY if the file
> + * has fs-verity enabled. This can perform better than FS_IOC_GETFLAGS and
> + * FS_IOC_MEASURE_VERITY because it doesn't require opening the file,
> + * and opening verity files can be expensive.
> + *
> + * Minimum Linux version required is v5.5.
I know this has been merged, this probably not worth of fixing it,
but the "required" is misleading when we detect via EOPNOTSUPP and EINVAL
and thus not use .min_kver. If it was due possible backport,
thus it should have been e.g. "functionality has been merged in kernel v5.5".

> + */
> +
...
> +static struct test_cases {
> +	void (*tfunc)(void);
> +} tcases[] = {
> +	{&test_flagged},
> +	{&test_unflagged},
> +};
> +
> +static void run(unsigned int i)
> +{
> +	tcases[i].tfunc();
> +}
OT: we may lack something in the API, when function like this need to be
defined.

Kind regards,
Petr

Hi!
> > +static void run(unsigned int i)
> > +{
> > +	tcases[i].tfunc();
> > +}
> OT: we may lack something in the API, when function like this need to be
> defined.

See:

https://lists.linux.it/pipermail/ltp/2017-October/005829.html
https://lists.linux.it/pipermail/ltp/2017-July/005132.html

Hi all,

> Hi!
> > > +static void run(unsigned int i)
> > > +{
> > > +	tcases[i].tfunc();
> > > +}
> > OT: we may lack something in the API, when function like this need to be
> > defined.

> See:

> https://lists.linux.it/pipermail/ltp/2017-October/005829.html
> https://lists.linux.it/pipermail/ltp/2017-July/005132.html
https://lore.kernel.org/ltp/860483630.25581747.1507017497043.JavaMail.zimbra@redhat.com/
https://lore.kernel.org/ltp/20170727081437.27995-1-chrubis@suse.cz/

Very nice that you remember your old work :) (we didn't have patchwork back then).

Now I remember it - you already implemented it in 5 years old RFC, Jan didn't
see a value and that's why it haven't been merged.

Yes, Jan is right that it complicates code a bit, but even if you replace this
code:

statx09.c
static struct test_cases {
    void (*tfunc)(void);
} tcases[] = {
    {&test_flagged},
    {&test_unflagged},
};

static void run(unsigned int i)
{
    tcases[i].tfunc();
}

with .test_all where you have the switch it still kind of boilerplate. Thus I
agree with cyrils argument:

https://lore.kernel.org/ltp/20171003125958.GB11692@rei/

	"aiming to avoid the need to have a switch () in each testcase that
	implements a similar tests but cannot be easily data driven (as we do
	for most of tests that loop over an array of structures describing the
	test data)"

Thus, not sure if we want to rething the implementation, but I'd be for adding
the support (sure doc and docparse adoption would need to be added but that's
obvious).

Kind regards,
Petr

Hi!

I also think that support for an array of functions is needed to cover 
all scenarios and cleanup the code a little bit. The real problem with 
tcases is that sometimes we are doing what we might do with multiple 
functions, but using an approach which is expecting struct and some sort 
of "filtering" in .test_all function.

And in some cases, where one particular testcase differs by a statement 
from an another, struct needs a flag to filter out the specific 
testcase. This would be easy to handle with two different functions.

Also the output message sometimes is stored into the struct, in order to 
show the correct TPASS/TFAIL message we need, according with the tcase. 
And this is probably an overengineering solution, since that would be 
handled well using multiple testcases functions, testing different 
scenarios and using different output messages.

Also simple tests, such as input arguments unit tests, would benefit 
from array of tests functions, since we can split tcases into multiple 
functions and make code more readable.

To sum up things, I think that having support for an array of test 
functions can cleanup code in many tests and make them easier to 
read/maintain. tcases can still do well sometimes, but adding the 
support for an array of functions can improve the LTP framework and so 
the way we are testing the kernel.

Andrea

On 2/7/22 12:55, Petr Vorel wrote:
> Hi all,
>
>> Hi!
>>>> +static void run(unsigned int i)
>>>> +{
>>>> +	tcases[i].tfunc();
>>>> +}
>>> OT: we may lack something in the API, when function like this need to be
>>> defined.
>> See:
>> https://lists.linux.it/pipermail/ltp/2017-October/005829.html
>> https://lists.linux.it/pipermail/ltp/2017-July/005132.html
> https://lore.kernel.org/ltp/860483630.25581747.1507017497043.JavaMail.zimbra@redhat.com/
> https://lore.kernel.org/ltp/20170727081437.27995-1-chrubis@suse.cz/
>
> Very nice that you remember your old work :) (we didn't have patchwork back then).
>
> Now I remember it - you already implemented it in 5 years old RFC, Jan didn't
> see a value and that's why it haven't been merged.
>
> Yes, Jan is right that it complicates code a bit, but even if you replace this
> code:
>
> statx09.c
> static struct test_cases {
>      void (*tfunc)(void);
> } tcases[] = {
>      {&test_flagged},
>      {&test_unflagged},
> };
>
> static void run(unsigned int i)
> {
>      tcases[i].tfunc();
> }
>
> with .test_all where you have the switch it still kind of boilerplate. Thus I
> agree with cyrils argument:
>
> https://lore.kernel.org/ltp/20171003125958.GB11692@rei/
>
> 	"aiming to avoid the need to have a switch () in each testcase that
> 	implements a similar tests but cannot be easily data driven (as we do
> 	for most of tests that loop over an array of structures describing the
> 	test data)"
>
> Thus, not sure if we want to rething the implementation, but I'd be for adding
> the support (sure doc and docparse adoption would need to be added but that's
> obvious).
>
> Kind regards,
> Petr
>