[LTP] [PATCH 1/2] syscalls/faccessat201: Add new testcase

[Yang Xu (Fujitsu)]

Hi Cyril
> Hi!
>> diff --git a/include/lapi/faccessat2.h b/include/lapi/faccessat2.h
>> new file mode 100644
>> index 000000000..e8b27d6b1
>> --- /dev/null
>> +++ b/include/lapi/faccessat2.h
>> @@ -0,0 +1,20 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#ifndef LAPI_FACCESSAT2_H__
>> +#define LAPI_FACCESSAT2_H__
>> +
>> +#include "tst_test.h"
>> +#include "lapi/syscalls.h"
>> +
>> +#ifndef HAVE_FACCESSAT2
>> +int faccessat2(int dirfd, const char *pathname, int mode, int flags)
>> +{
>> +	return tst_syscall(__NR_faccessat2, dirfd, pathname, mode, flags);
>> +}
>> +#endif
>> +
>> +#endif /* LAPI_FACCESSAT2_H__ */
> This one needs the configure check, and I would call the header just
> faccessat.h.
i will update it.
>> diff --git a/testcases/kernel/syscalls/faccessat2/faccessat201.c b/testcases/kernel/syscalls/faccessat2/faccessat201.c
>> new file mode 100644
>> index 000000000..658d1dd81
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/faccessat2/faccessat201.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * Check the basic functionality of faccessat2().
>> + *
>> + * Minimum Linux version required is v5.8.
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <pwd.h>
>> +
>> +#include "tst_test.h"
>> +#include "lapi/syscalls.h"
>> +#include "lapi/faccessat2.h"
>> +
>> +#define TESTUSER	"nobody"
>> +#define TESTDIR		"faccessat2dir"
>> +#define TESTFILE	"faccessat2file"
>> +#define	RELPATH		"faccessat2dir/faccessat2file"
>> +#define TESTSYMLINK	"faccessat2symlink"
> There is a mix of tabs and spaces in this part that should be cleaned
> up.
My mistake, I will correct it.
>> +static int dir_fd = -1, bad_fd = -1;
>> +static int atcwd_fd = AT_FDCWD;
>> +static char *filepaths[4];
>> +static char abs_path[128];
>> +static struct passwd *ltpuser;
>> +
>> +static struct tcase {
>> +	int *fd;
>> +	char **filename;
>> +	int flags;
>> +	int exp_errno;
>> +} tcases[] = {
>> +	{&dir_fd, &filepaths[0], 0, 0},
>> +	{&bad_fd, &filepaths[1], 0, 0},
>> +	{&atcwd_fd, &filepaths[2], 0, 0},
>> +	{&dir_fd, &filepaths[0], AT_EACCESS, 0},
>> +	{&bad_fd, &filepaths[1], AT_EACCESS, 0},
>> +	{&atcwd_fd, &filepaths[2], AT_EACCESS, 0},
>> +	{&dir_fd, &filepaths[0], AT_EACCESS, EACCES},
>> +	{&bad_fd, &filepaths[1], AT_EACCESS, EACCES},
>> +	{&atcwd_fd, &filepaths[2], AT_EACCESS, EACCES},
>> +	{&atcwd_fd, &filepaths[3], AT_SYMLINK_NOFOLLOW, 0},
>> +};
>> +
>> +static void verify_faccessat2(unsigned int i)
>> +{
>> +	struct tcase *tc = &tcases[i];
>> +
>> +	if (tc->exp_errno == EACCES) {
>> +		if (SAFE_FORK() == 0) {
>> +			SAFE_SETUID(ltpuser->pw_uid);
>> +			TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, R_OK, tc->flags),
>> +				    tc->exp_errno, "faccessat2(%d, %s, R_OK, %d) as %s",
>> +				    *tc->fd, *tc->filename, tc->flags, TESTUSER);
>> +		}
>> +
>> +		tst_reap_children();
> I would just put the EACCESS tests to a separate testcase, which would
> make the test more straightforward and easier to read.
ok.
>> +	} else {
>> +		TST_EXP_PASS(faccessat2(*tc->fd, *tc->filename, R_OK, tc->flags),
>> +			     "faccessat2(%d, %s, R_OK, %d) as root",
>> +			     *tc->fd, *tc->filename, tc->flags);
>> +	}
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	char *tmpdir_path = tst_get_tmpdir();
>> +
>> +	sprintf(abs_path, "%s/%s", tmpdir_path, RELPATH);
>> +	free(tmpdir_path);
>> +
>> +	SAFE_MKDIR(TESTDIR, 0666);
>> +	dir_fd = SAFE_OPEN(TESTDIR, O_DIRECTORY);
>> +	SAFE_TOUCH(abs_path, 0444, NULL);
>> +	SAFE_SYMLINK(abs_path, TESTSYMLINK);
>> +
>> +	filepaths[0] = TESTFILE;
>> +	filepaths[1] = abs_path;
>> +	filepaths[2] = RELPATH;
>> +	filepaths[3] = TESTSYMLINK;
>> +
>> +	ltpuser = SAFE_GETPWNAM(TESTUSER);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (dir_fd > 0)
>> +		SAFE_CLOSE(dir_fd);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test = verify_faccessat2,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.bufs = (struct tst_buffers []) {
>> +		{&filepaths[0], .size = sizeof(*filepaths[0])},
>> +		{&filepaths[1], .size = sizeof(*filepaths[1])},
>> +		{&filepaths[2], .size = sizeof(*filepaths[2])},
>> +		{&filepaths[3], .size = sizeof(*filepaths[3])},
>> +		{&ltpuser, .size = sizeof(ltpuser)},
> Why do we allocate anything here when we replace the pointers in the
> test setup anyways?
>
> If anything it would make sense to allocate the buffers in the test
> setup instead of using static strings there. Also filepath[x] way less
> readable than actually giving the variables proper names such as
> abs_path.
>
> I guess that we can as well add a helper to the buffer library such as:
>
> diff --git a/include/tst_buffers.h b/include/tst_buffers.h
> index d19ac8cf0..a12d70a62 100644
> --- a/include/tst_buffers.h
> +++ b/include/tst_buffers.h
> @@ -46,6 +46,11 @@ char *tst_strdup(const char *str);
>    */
>   void *tst_alloc(size_t size);
>   
> +/*
> + * Strdup into a guarded buffer.
> + */
> +char *tst_strdup(const char *str);
> +
>   /*
>    * Allocates iovec structure including the buffers.
>    *
> diff --git a/lib/tst_buffers.c b/lib/tst_buffers.c
> index b8b597a12..4488f458e 100644
> --- a/lib/tst_buffers.c
> +++ b/lib/tst_buffers.c
> @@ -76,6 +76,13 @@ void *tst_alloc(size_t size)
>          return ret + map->buf_shift;
>   }
>   
> +char *tst_strup(const char *str)
> +{
> +       char *ret = tst_alloc(strlen(str) + 1);
> +
> +       return strcpy(ret, str);
> +}
> +
>   static int count_iovec(int *sizes)
>   {
>          int ret = 0;
>
>
> And slightly more complicated would be addition of tst_aprintf() that
> would printf into buffer allocated by tst_alloc(). I will send a patch
> that adds this tomorrow.
>
> Once that is done we can use these in the test setup such as:
>
> static char *abs_path;
> static char *testfile;
>
> static struct tcase {
>       int *fd;
>       char **filename;
>       int flags;
>       int exp_errno;
> } tcases[] = {
>       {..., &abs_path, ...},
>       {..., &testfile, ...},
> }
>
> static void setup(void)
> {
> 	...
>
> 	testfile = tst_strdup(TESTFILE);
> 	abs_path = tst_aprintf(abs_path, "%s/%s", tmpdir_path, TESTFILE);
>
> 	...
> }
>
> Which allocates the exact sized guarded buffers so that the canaries are
> exaclty after end of the data, not at the end of the rather large
> buffer.
>
> We can't pre-allocate buffers in the tst_test structure for data whose
> size is not known in advance, which is the case for the tmpdir_path.
I have overlooked these issues and will make modifications based on your 
suggestions.