[LTP] [PATCH v2 1/2] syscalls/fcntl30: clean up && add more range test

[LTP] [PATCH v2 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

When I write pipe12 test case, I have a question
about F_SETPIPE_SZ can set the min and max value.
So cleanup this case and add min/max/gt_max value test.

--------
v1->v2:
1. add memfree check
2. limit max shift, so pipe will not beyond 2^31 value
on big page size machine(such as 64kb pg size on arm machine)
--------

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 include/lapi/capability.h                 |   4 +
 testcases/kernel/syscalls/fcntl/fcntl30.c | 216 +++++++++++++---------
 2 files changed, 133 insertions(+), 87 deletions(-)

diff --git a/include/lapi/capability.h b/include/lapi/capability.h
index 8833f0605..7ade78985 100644
--- a/include/lapi/capability.h
+++ b/include/lapi/capability.h
@@ -28,6 +28,10 @@
 # define CAP_SYS_ADMIN        21
 #endif
 
+#ifndef CAP_SYS_RESOURCE
+# define CAP_SYS_RESOURCE     24
+#endif
+
 #ifndef CAP_AUDIT_READ
 # define CAP_AUDIT_READ       37
 #endif
diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
index 4a409b868..0cb42babe 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl30.c
+++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
@@ -1,111 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Copyright (c) 2014 Fujitsu Ltd.
+ * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
  * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
+ * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
  *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-/*
  * Description:
- * Verify that,
- *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
+ * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
  */
 
-
-#include <stdio.h>
-#include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
-#include <string.h>
-#include <signal.h>
 #include <sys/types.h>
-#include <pwd.h>
-
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 #include "lapi/fcntl.h"
-
-char *TCID = "fcntl30";
-int TST_TOTAL = 1;
-
-static void setup(void);
-static void cleanup(void);
-
-int main(int ac, char **av)
+#include "lapi/abisize.h"
+#include "lapi/capability.h"
+
+static int fds[2];
+static unsigned int orig_value, struct_shift, max_shift, pg_shift;
+
+static struct tcase {
+	unsigned int multi;
+	unsigned int exp_multi;
+	int hole;
+	int pass_flag;
+	char *message;
+} tcases[] = {
+	{1, 1, 1, 1, "set a value of blew page size"},
+	{2, 2, 0, 1, "set a normal value"},
+	{0, 0, 0, 1, "set a max value"},
+	{0, 0, -1, 0, "set a value beyond max"},
+};
+
+static void verify_fcntl(unsigned int n)
 {
-	int lc;
-	int pipe_fds[2], test_fd;
-	int orig_pipe_size, new_pipe_size;
-
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		SAFE_PIPE(cleanup, pipe_fds);
-		test_fd = pipe_fds[1];
-
-		TEST(fcntl(test_fd, F_GETPIPE_SZ));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl get pipe size failed");
-		}
-
-		orig_pipe_size = TEST_RETURN;
-		new_pipe_size = orig_pipe_size * 2;
-		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl test F_SETPIPE_SZ failed");
-		}
+	struct tcase *tc = &tcases[n];
+	unsigned int pipe_sz, exp_pipe_sz, shift;
+	long memfree;
+
+	SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &memfree);
+
+	shift = max_shift - struct_shift + 2 * pg_shift;
+	/*
+	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
+	 * so truncate it.
+	 */
+	if (shift > 31) {
+		tst_res(TINFO, "pipe size truncated into 2G");
+		shift = 31;
+	}
+	if (tc->multi)
+		pipe_sz = (tc->multi << pg_shift) - tc->hole;
+	else
+		pipe_sz = (1 << shift) - tc->hole;
+	if (tc->exp_multi)
+		exp_pipe_sz = tc->exp_multi << pg_shift;
+	else
+		exp_pipe_sz = 1 << shift;
+
+	tst_res(TINFO, "%s, size is %d", tc->message, pipe_sz);
+
+	if (pipe_sz > memfree * 1024) {
+		tst_res(TCONF, "No enough free memory");
+		return;
+	}
 
-		TEST(fcntl(test_fd, F_GETPIPE_SZ));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl test F_GETPIPE_SZ failed");
-		}
-		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
-			 orig_pipe_size, new_pipe_size);
-		if (TEST_RETURN >= new_pipe_size) {
-			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
-				 "and F_SETPIPE_SZ success");
-		} else {
-			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
-				 "and F_SETPIPE_SZ fail");
+	TEST(fcntl(fds[1], F_SETPIPE_SZ, pipe_sz));
+	if (tc->pass_flag && TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
+		return;
+	}
+	if (!tc->pass_flag) {
+		if (TST_RET == -1) {
+			if ((TST_ERR == ENOMEM && shift < 31 && tc->hole) ||
+				(TST_ERR == EINVAL && shift == 31 && tc->hole))
+				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
+			else
+				tst_res(TFAIL | TTERRNO,
+					"F_SETPIPE_SZ failed with unexpected error");
+			return;
 		}
-		SAFE_CLOSE(cleanup, pipe_fds[0]);
-		SAFE_CLOSE(cleanup, pipe_fds[1]);
+		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
 	}
 
-	cleanup();
-	tst_exit();
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
+		return;
+	}
+	if ((unsigned int)TST_RET == exp_pipe_sz)
+		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
+			pipe_sz, (unsigned int)TST_RET);
+	else
+		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
+			pipe_sz, (unsigned int)TST_RET);
 }
 
 static void setup(void)
 {
-	if ((tst_kvercmp(2, 6, 35)) < 0) {
-		tst_brkm(TCONF, NULL, "This test can only run on kernels"
-			 "that are 2.6.35 and higher");
-	}
-
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
+	unsigned int pg_size;
+
+	SAFE_PIPE(fds);
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_ERR == EINVAL)
+		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
+	orig_value = TST_RET;
+	/*
+	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
+	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
+	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
+	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
+	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
+	 * the MAX_ORDER is 11.
+	 * For example, if page size is 4k, on 64bit system. the max pipe size
+	 * as below:
+	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
+	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
+	 *  max pipe size: 2^16* 2^12 = 2^28
+	 * it should be 256M. On 32bit system, this value is 512M.
+	 */
+#ifdef TST_ABI64
+	struct_shift = 6;
+#else
+	struct_shift = 5;
+#endif
+	max_shift = 10;
+
+	pg_size = getpagesize();
+	tst_res(TINFO, "page size is %d bytes", pg_size);
+	while (pg_size >>= 1)
+		pg_shift++;
 }
 
 static void cleanup(void)
 {
+	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);
+	if (fds[0] > 0)
+		SAFE_CLOSE(fds[0]);
+	if (fds[1] > 0)
+		SAFE_CLOSE(fds[1]);
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = verify_fcntl,
+	.caps = (struct tst_cap []) {
+		TST_CAP(TST_CAP_REQ, CAP_SYS_RESOURCE),
+		{}
+	},
+};
-- 
2.18.0

[LTP] [PATCH v2 2/2] syscalls/fcntl37: add error test for fcntl with F_SETPIPE_SZ

[Yang Xu]

-------------
v1->v2?
code style and comment fix
-----------

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                           |   2 +
 testcases/kernel/syscalls/fcntl/.gitignore |   2 +
 testcases/kernel/syscalls/fcntl/fcntl37.c  | 101 +++++++++++++++++++++
 3 files changed, 105 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c

diff --git a/runtest/syscalls b/runtest/syscalls
index f58fefe17..ca53e5745 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -301,6 +301,8 @@ fcntl35 fcntl35
 fcntl35_64 fcntl35_64
 fcntl36 fcntl36
 fcntl36_64 fcntl36_64
+fcntl37 fcntl37
+fcntl37_64 fcntl37_64
 
 fdatasync01 fdatasync01
 fdatasync02 fdatasync02
diff --git a/testcases/kernel/syscalls/fcntl/.gitignore b/testcases/kernel/syscalls/fcntl/.gitignore
index 8d5738f97..be8d9739e 100644
--- a/testcases/kernel/syscalls/fcntl/.gitignore
+++ b/testcases/kernel/syscalls/fcntl/.gitignore
@@ -70,3 +70,5 @@
 /fcntl35_64
 /fcntl36
 /fcntl36_64
+/fcntl37
+/fcntl37_64
diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
new file mode 100644
index 000000000..757d670a0
--- /dev/null
+++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
+ *
+ * Test basic error handling for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ
+ * argument.
+ * 1)fcntl fails with EINVAL when cmd is F_SETPIPE_SZ and the arg is
+ * beyond 1<<31.
+ * 2)fcntl fails with EBUSY when cmd is F_SETPIPE_SZ and the arg is smaller
+ * than the amount of the current used buffer space.
+ * 3)fcntl fails with EPERM when cmd is F_SETPIPE_SZ and the arg is over
+ * /proc/sys/fs/pipe-max-size limit under unprivileged users.
+ */
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <limits.h>
+#include <stdlib.h>
+#include "tst_test.h"
+#include "lapi/fcntl.h"
+#include "lapi/capability.h"
+
+static int fds[2];
+static unsigned int orig_value, invalid_value, half_value, sys_value;
+static char *wrbuf;
+
+static struct tcase {
+	unsigned int *setvalue;
+	int exp_err;
+	char *message;
+} tcases[] = {
+	{&invalid_value, EINVAL,
+	"cmd is F_SETPIPE_SZ and the arg is beyond 1<<31"},
+
+	{&half_value, EBUSY,
+	"cmd is F_SETPIPE_SZ and the arg is smaller than the amount of the current used buffer space"},
+
+	{&sys_value, EPERM,
+	"cmd is F_SETPIPE_SZ and the arg is over /proc/sys/fs/pipe-max-size limit under unprivileged users"},
+};
+
+static void verify_fcntl(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	tst_res(TINFO, "%s", tc->message);
+
+	TEST(fcntl(fds[1], F_SETPIPE_SZ, *(tc->setvalue)));
+	if (TST_RET != -1)
+		tst_res(TFAIL, "F_SETPIPE_SZ succeed");
+	if (TST_ERR == tc->exp_err)
+		tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed as expected");
+	else
+		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed expected %s bu got",
+				tst_strerrno(tc->exp_err));
+}
+
+static void setup(void)
+{
+	SAFE_PIPE(fds);
+
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_ERR == EINVAL)
+		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
+
+	orig_value = TST_RET;
+
+	wrbuf = SAFE_MALLOC(orig_value);
+	memset(wrbuf, 'x', orig_value);
+	SAFE_WRITE(1, fds[1], wrbuf, orig_value);
+
+	SAFE_FILE_SCANF("/proc/sys/fs/pipe-max-size", "%d", &sys_value);
+	sys_value++;
+
+	half_value = orig_value / 2;
+	invalid_value = (1U << 31) + 1;
+}
+
+static void cleanup(void)
+{
+	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);
+	if (fds[0] > 0)
+		SAFE_CLOSE(fds[0]);
+	if (fds[1] > 0)
+		SAFE_CLOSE(fds[1]);
+	if (wrbuf)
+		free(wrbuf);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = verify_fcntl,
+	.caps = (struct tst_cap []) {
+		TST_CAP(TST_CAP_DROP, CAP_SYS_RESOURCE),
+		{}
+	},
+};
-- 
2.18.0

[LTP] [PATCH v2 1/2] syscalls/fcntl30: clean up && add more range test

[Cyril Hrubis]

Hi!
> --------
> v1->v2:
> 1. add memfree check

Do we really need this? Looking at the kernel code the fcntl() just
reallocates the array that is holding the slots, so we only allocate new
array of struct pipe_buffer which contains a pointer for the actuall
page that is allocated when we _WRITE_ to the pipe.

> 2. limit max shift, so pipe will not beyond 2^31 value
> on big page size machine(such as 64kb pg size on arm machine)
> --------
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  include/lapi/capability.h                 |   4 +
>  testcases/kernel/syscalls/fcntl/fcntl30.c | 216 +++++++++++++---------
>  2 files changed, 133 insertions(+), 87 deletions(-)
> 
> diff --git a/include/lapi/capability.h b/include/lapi/capability.h
> index 8833f0605..7ade78985 100644
> --- a/include/lapi/capability.h
> +++ b/include/lapi/capability.h
> @@ -28,6 +28,10 @@
>  # define CAP_SYS_ADMIN        21
>  #endif
>  
> +#ifndef CAP_SYS_RESOURCE
> +# define CAP_SYS_RESOURCE     24
> +#endif
> +
>  #ifndef CAP_AUDIT_READ
>  # define CAP_AUDIT_READ       37
>  #endif
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
> index 4a409b868..0cb42babe 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
> @@ -1,111 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * Copyright (c) 2014 Fujitsu Ltd.
> + * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
>   * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> + * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
>   *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of version 2 of the GNU General Public License as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, write the Free Software Foundation, Inc.,
> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> - */
> -
> -/*
>   * Description:
> - * Verify that,
> - *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
> + * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>   */
>  
> -
> -#include <stdio.h>
> -#include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> -#include <string.h>
> -#include <signal.h>
>  #include <sys/types.h>
> -#include <pwd.h>
> -
> -#include "test.h"
> -#include "safe_macros.h"
> +#include "tst_test.h"
>  #include "lapi/fcntl.h"
> -
> -char *TCID = "fcntl30";
> -int TST_TOTAL = 1;
> -
> -static void setup(void);
> -static void cleanup(void);
> -
> -int main(int ac, char **av)
> +#include "lapi/abisize.h"
> +#include "lapi/capability.h"
> +
> +static int fds[2];
> +static unsigned int orig_value, struct_shift, max_shift, pg_shift;
> +
> +static struct tcase {
> +	unsigned int multi;
> +	unsigned int exp_multi;
> +	int hole;
> +	int pass_flag;
> +	char *message;
> +} tcases[] = {
> +	{1, 1, 1, 1, "set a value of blew page size"},
                                      ^
				      below?
> +	{2, 2, 0, 1, "set a normal value"},
> +	{0, 0, 0, 1, "set a max value"},
> +	{0, 0, -1, 0, "set a value beyond max"},
> +};
> +
> +static void verify_fcntl(unsigned int n)
>  {
> -	int lc;
> -	int pipe_fds[2], test_fd;
> -	int orig_pipe_size, new_pipe_size;
> -
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		SAFE_PIPE(cleanup, pipe_fds);
> -		test_fd = pipe_fds[1];
> -
> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl get pipe size failed");
> -		}
> -
> -		orig_pipe_size = TEST_RETURN;
> -		new_pipe_size = orig_pipe_size * 2;
> -		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl test F_SETPIPE_SZ failed");
> -		}
> +	struct tcase *tc = &tcases[n];
> +	unsigned int pipe_sz, exp_pipe_sz, shift;
> +	long memfree;
> +
> +	SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &memfree);
> +
> +	shift = max_shift - struct_shift + 2 * pg_shift;
> +	/*
> +	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
> +	 * so truncate it.
> +	 */
> +	if (shift > 31) {
> +		tst_res(TINFO, "pipe size truncated into 2G");
> +		shift = 31;
> +	}
> +	if (tc->multi)
> +		pipe_sz = (tc->multi << pg_shift) - tc->hole;
> +	else
> +		pipe_sz = (1 << shift) - tc->hole;
> +	if (tc->exp_multi)
> +		exp_pipe_sz = tc->exp_multi << pg_shift;
> +	else
> +		exp_pipe_sz = 1 << shift;

I do wonder why can't we simply compute these sizes in the test setup
instead.

> +	tst_res(TINFO, "%s, size is %d", tc->message, pipe_sz);
> +
> +	if (pipe_sz > memfree * 1024) {
> +		tst_res(TCONF, "No enough free memory");
> +		return;
> +	}
>  
> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl test F_GETPIPE_SZ failed");
> -		}
> -		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
> -			 orig_pipe_size, new_pipe_size);
> -		if (TEST_RETURN >= new_pipe_size) {
> -			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
> -				 "and F_SETPIPE_SZ success");
> -		} else {
> -			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
> -				 "and F_SETPIPE_SZ fail");
> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, pipe_sz));
> +	if (tc->pass_flag && TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
> +		return;
> +	}
> +	if (!tc->pass_flag) {

Simple else would be more readable here.

> +		if (TST_RET == -1) {
> +			if ((TST_ERR == ENOMEM && shift < 31 && tc->hole) ||
> +				(TST_ERR == EINVAL && shift == 31 && tc->hole))
> +				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
> +			else
> +				tst_res(TFAIL | TTERRNO,
> +					"F_SETPIPE_SZ failed with unexpected error");
> +			return;
>  		}
> -		SAFE_CLOSE(cleanup, pipe_fds[0]);
> -		SAFE_CLOSE(cleanup, pipe_fds[1]);
> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
> +		return;
> +	}
> +	if ((unsigned int)TST_RET == exp_pipe_sz)
> +		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
> +			pipe_sz, (unsigned int)TST_RET);
> +	else
> +		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
> +			pipe_sz, (unsigned int)TST_RET);
>  }
>  
>  static void setup(void)
>  {
> -	if ((tst_kvercmp(2, 6, 35)) < 0) {
> -		tst_brkm(TCONF, NULL, "This test can only run on kernels"
> -			 "that are 2.6.35 and higher");
> -	}
> -
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> +	unsigned int pg_size;
> +
> +	SAFE_PIPE(fds);
> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
> +	if (TST_ERR == EINVAL)
> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
> +	orig_value = TST_RET;
> +	/*
> +	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
> +	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
> +	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
> +	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
> +	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
> +	 * the MAX_ORDER is 11.
> +	 * For example, if page size is 4k, on 64bit system. the max pipe size
> +	 * as below:
> +	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
> +	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
> +	 *  max pipe size: 2^16* 2^12 = 2^28
> +	 * it should be 256M. On 32bit system, this value is 512M.
> +	 */
> +#ifdef TST_ABI64
> +	struct_shift = 6;
> +#else
> +	struct_shift = 5;
> +#endif
> +	max_shift = 10;
> +
> +	pg_size = getpagesize();
> +	tst_res(TINFO, "page size is %d bytes", pg_size);
> +	while (pg_size >>= 1)
> +		pg_shift++;
>  }
>  
>  static void cleanup(void)
>  {
> +	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);

Do we really restore the value? We are closing the the pipe here
anyways.

> +	if (fds[0] > 0)
> +		SAFE_CLOSE(fds[0]);
> +	if (fds[1] > 0)
> +		SAFE_CLOSE(fds[1]);
>  }
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = verify_fcntl,
> +	.caps = (struct tst_cap []) {
> +		TST_CAP(TST_CAP_REQ, CAP_SYS_RESOURCE),
> +		{}
> +	},
> +};

Also btw, looking at the code there are couple of other things to test:

* unpriviledged user can shrink buffer and grow it if the size is below /proc/sys/fs/pipe-max-size

* write data to page and shrink the buffer, then read it back and check
  the content, also check that pipe cannot be shrunk below the size the
  currently used slots

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] syscalls/fcntl37: add error test for fcntl with F_SETPIPE_SZ

[Cyril Hrubis]

Hi!
> +static int fds[2];
> +static unsigned int orig_value, invalid_value, half_value, sys_value;
> +static char *wrbuf;
> +
> +static struct tcase {
> +	unsigned int *setvalue;
> +	int exp_err;
> +	char *message;
> +} tcases[] = {
> +	{&invalid_value, EINVAL,
> +	"cmd is F_SETPIPE_SZ and the arg is beyond 1<<31"},
> +
> +	{&half_value, EBUSY,
> +	"cmd is F_SETPIPE_SZ and the arg is smaller than the amount of the current used buffer space"},

Ah the EBUSY is handled here.


Also these descriptions are way too long, ideally these should be
shorter and to the point. Something as:

"F_SETPIPE_SZ and size < data stored in pipe"

> +	{&sys_value, EPERM,
> +	"cmd is F_SETPIPE_SZ and the arg is over /proc/sys/fs/pipe-max-size limit under unprivileged users"},

Here something as:

"F_SETPIPE_SZ and size is over limit for unpriviledged user"

> +};
> +
> +static void verify_fcntl(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +
> +	tst_res(TINFO, "%s", tc->message);
> +
> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, *(tc->setvalue)));
> +	if (TST_RET != -1)
> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed");

Maybe we should print the TST_RET here as well, it may return completely
random number that != -1.

> +	if (TST_ERR == tc->exp_err)
> +		tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed as expected");
> +	else
> +		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed expected %s bu got",
                                                                          ^
									  but?

I guess that we can completely drop the "but" here and just keep it
"... expected %s got"

> +				tst_strerrno(tc->exp_err));
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_PIPE(fds);
> +
> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
> +	if (TST_ERR == EINVAL)
> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
> +
> +	orig_value = TST_RET;
> +
> +	wrbuf = SAFE_MALLOC(orig_value);
> +	memset(wrbuf, 'x', orig_value);
> +	SAFE_WRITE(1, fds[1], wrbuf, orig_value);
> +
> +	SAFE_FILE_SCANF("/proc/sys/fs/pipe-max-size", "%d", &sys_value);
> +	sys_value++;
> +
> +	half_value = orig_value / 2;
> +	invalid_value = (1U << 31) + 1;
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);

Again there is no point in restoring the value if we close the pipe
right after.

> +	if (fds[0] > 0)
> +		SAFE_CLOSE(fds[0]);
> +	if (fds[1] > 0)
> +		SAFE_CLOSE(fds[1]);
> +	if (wrbuf)
> +		free(wrbuf);

Why don't we free the buffer right in the test setup? There is no point
in keeping it allocated.

> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = verify_fcntl,
> +	.caps = (struct tst_cap []) {
> +		TST_CAP(TST_CAP_DROP, CAP_SYS_RESOURCE),
> +		{}
> +	},
> +};

Other than the minor things the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

Hi Cyril
> Hi!
>> --------
>> v1->v2:
>> 1. add memfree check
> 
> Do we really need this? Looking at the kernel code the fcntl() just
> reallocates the array that is holding the slots, so we only allocate new
> array of struct pipe_buffer which contains a pointer for the actuall
> page that is allocated when we _WRITE_ to the pipe.
> 
Yes, you are right. But this case indeed fail when on low memory machine 
(4kb page size, 256/512M memory).
>> 2. limit max shift, so pipe will not beyond 2^31 value
>> on big page size machine(such as 64kb pg size on arm machine)
>> --------
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
>> ---
>>   include/lapi/capability.h                 |   4 +
>>   testcases/kernel/syscalls/fcntl/fcntl30.c | 216 +++++++++++++---------
>>   2 files changed, 133 insertions(+), 87 deletions(-)
>>
>> diff --git a/include/lapi/capability.h b/include/lapi/capability.h
>> index 8833f0605..7ade78985 100644
>> --- a/include/lapi/capability.h
>> +++ b/include/lapi/capability.h
>> @@ -28,6 +28,10 @@
>>   # define CAP_SYS_ADMIN        21
>>   #endif
>>   
>> +#ifndef CAP_SYS_RESOURCE
>> +# define CAP_SYS_RESOURCE     24
>> +#endif
>> +
>>   #ifndef CAP_AUDIT_READ
>>   # define CAP_AUDIT_READ       37
>>   #endif
>> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> index 4a409b868..0cb42babe 100644
>> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
>> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> @@ -1,111 +1,153 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>   /*
>> - * Copyright (c) 2014 Fujitsu Ltd.
>> + * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
>>    * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> + * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
>>    *
>> - * This program is free software; you can redistribute it and/or modify it
>> - * under the terms of version 2 of the GNU General Public License as
>> - * published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it would be useful, but
>> - * WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>> - *
>> - * You should have received a copy of the GNU General Public License along
>> - * with this program; if not, write the Free Software Foundation, Inc.,
>> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> - */
>> -
>> -/*
>>    * Description:
>> - * Verify that,
>> - *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>> + * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>>    */
>>   
>> -
>> -#include <stdio.h>
>> -#include <errno.h>
>>   #include <unistd.h>
>>   #include <fcntl.h>
>> -#include <string.h>
>> -#include <signal.h>
>>   #include <sys/types.h>
>> -#include <pwd.h>
>> -
>> -#include "test.h"
>> -#include "safe_macros.h"
>> +#include "tst_test.h"
>>   #include "lapi/fcntl.h"
>> -
>> -char *TCID = "fcntl30";
>> -int TST_TOTAL = 1;
>> -
>> -static void setup(void);
>> -static void cleanup(void);
>> -
>> -int main(int ac, char **av)
>> +#include "lapi/abisize.h"
>> +#include "lapi/capability.h"
>> +
>> +static int fds[2];
>> +static unsigned int orig_value, struct_shift, max_shift, pg_shift;
>> +
>> +static struct tcase {
>> +	unsigned int multi;
>> +	unsigned int exp_multi;
>> +	int hole;
>> +	int pass_flag;
>> +	char *message;
>> +} tcases[] = {
>> +	{1, 1, 1, 1, "set a value of blew page size"},
>                                        ^
> 				      below?
ok.
>> +	{2, 2, 0, 1, "set a normal value"},
>> +	{0, 0, 0, 1, "set a max value"},
>> +	{0, 0, -1, 0, "set a value beyond max"},
>> +};
>> +
>> +static void verify_fcntl(unsigned int n)
>>   {
>> -	int lc;
>> -	int pipe_fds[2], test_fd;
>> -	int orig_pipe_size, new_pipe_size;
>> -
>> -
>> -	tst_parse_opts(ac, av, NULL, NULL);
>> -
>> -	setup();
>> -
>> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> -		tst_count = 0;
>> -
>> -		SAFE_PIPE(cleanup, pipe_fds);
>> -		test_fd = pipe_fds[1];
>> -
>> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl get pipe size failed");
>> -		}
>> -
>> -		orig_pipe_size = TEST_RETURN;
>> -		new_pipe_size = orig_pipe_size * 2;
>> -		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl test F_SETPIPE_SZ failed");
>> -		}
>> +	struct tcase *tc = &tcases[n];
>> +	unsigned int pipe_sz, exp_pipe_sz, shift;
>> +	long memfree;
>> +
>> +	SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &memfree);
>> +
>> +	shift = max_shift - struct_shift + 2 * pg_shift;
>> +	/*
>> +	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
>> +	 * so truncate it.
>> +	 */
>> +	if (shift > 31) {
>> +		tst_res(TINFO, "pipe size truncated into 2G");
>> +		shift = 31;
>> +	}
>> +	if (tc->multi)
>> +		pipe_sz = (tc->multi << pg_shift) - tc->hole;
>> +	else
>> +		pipe_sz = (1 << shift) - tc->hole;
>> +	if (tc->exp_multi)
>> +		exp_pipe_sz = tc->exp_multi << pg_shift;
>> +	else
>> +		exp_pipe_sz = 1 << shift;
> 
> I do wonder why can't we simply compute these sizes in the test setup
> instead.
> 
ok. I will move this into setup.
>> +	tst_res(TINFO, "%s, size is %d", tc->message, pipe_sz);
>> +
>> +	if (pipe_sz > memfree * 1024) {
>> +		tst_res(TCONF, "No enough free memory");
>> +		return;
>> +	}
>>   
>> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl test F_GETPIPE_SZ failed");
>> -		}
>> -		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
>> -			 orig_pipe_size, new_pipe_size);
>> -		if (TEST_RETURN >= new_pipe_size) {
>> -			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
>> -				 "and F_SETPIPE_SZ success");
>> -		} else {
>> -			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
>> -				 "and F_SETPIPE_SZ fail");
>> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, pipe_sz));
>> +	if (tc->pass_flag && TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
>> +		return;
>> +	}
>> +	if (!tc->pass_flag) {
> 
> Simple else would be more readable here.
> 
OK.
>> +		if (TST_RET == -1) {
>> +			if ((TST_ERR == ENOMEM && shift < 31 && tc->hole) ||
>> +				(TST_ERR == EINVAL && shift == 31 && tc->hole))
>> +				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
>> +			else
>> +				tst_res(TFAIL | TTERRNO,
>> +					"F_SETPIPE_SZ failed with unexpected error");
>> +			return;
>>   		}
>> -		SAFE_CLOSE(cleanup, pipe_fds[0]);
>> -		SAFE_CLOSE(cleanup, pipe_fds[1]);
>> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
>>   	}
>>   
>> -	cleanup();
>> -	tst_exit();
>> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
>> +		return;
>> +	}
>> +	if ((unsigned int)TST_RET == exp_pipe_sz)
>> +		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
>> +			pipe_sz, (unsigned int)TST_RET);
>> +	else
>> +		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
>> +			pipe_sz, (unsigned int)TST_RET);
>>   }
>>   
>>   static void setup(void)
>>   {
>> -	if ((tst_kvercmp(2, 6, 35)) < 0) {
>> -		tst_brkm(TCONF, NULL, "This test can only run on kernels"
>> -			 "that are 2.6.35 and higher");
>> -	}
>> -
>> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>> -
>> -	TEST_PAUSE;
>> +	unsigned int pg_size;
>> +
>> +	SAFE_PIPE(fds);
>> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
>> +	if (TST_ERR == EINVAL)
>> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
>> +	orig_value = TST_RET;
>> +	/*
>> +	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
>> +	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
>> +	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
>> +	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
>> +	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
>> +	 * the MAX_ORDER is 11.
>> +	 * For example, if page size is 4k, on 64bit system. the max pipe size
>> +	 * as below:
>> +	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
>> +	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
>> +	 *  max pipe size: 2^16* 2^12 = 2^28
>> +	 * it should be 256M. On 32bit system, this value is 512M.
>> +	 */
>> +#ifdef TST_ABI64
>> +	struct_shift = 6;
>> +#else
>> +	struct_shift = 5;
>> +#endif
>> +	max_shift = 10;
>> +
>> +	pg_size = getpagesize();
>> +	tst_res(TINFO, "page size is %d bytes", pg_size);
>> +	while (pg_size >>= 1)
>> +		pg_shift++;
>>   }
>>   
>>   static void cleanup(void)
>>   {
>> +	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);
> 
> Do we really restore the value? We are closing the the pipe here
> anyways.
> 
It is useless, I will remove this restore.
>> +	if (fds[0] > 0)
>> +		SAFE_CLOSE(fds[0]);
>> +	if (fds[1] > 0)
>> +		SAFE_CLOSE(fds[1]);
>>   }
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.test = verify_fcntl,
>> +	.caps = (struct tst_cap []) {
>> +		TST_CAP(TST_CAP_REQ, CAP_SYS_RESOURCE),
>> +		{}
>> +	},
>> +};
> 
> Also btw, looking at the code there are couple of other things to test:
> 
> * unpriviledged user can shrink buffer and grow it if the size is below /proc/sys/fs/pipe-max-size
Ok. I will add it.


> * write data to page and shrink the buffer, then read it back and check
>    the content, also check that pipe cannot be shrunk below the size the
>    currently used slots
My other patch tests this .
> 

[LTP] [PATCH v2 2/2] syscalls/fcntl37: add error test for fcntl with F_SETPIPE_SZ

[Yang Xu]

Hi Cyril
> Hi!
>> +static int fds[2];
>> +static unsigned int orig_value, invalid_value, half_value, sys_value;
>> +static char *wrbuf;
>> +
>> +static struct tcase {
>> +	unsigned int *setvalue;
>> +	int exp_err;
>> +	char *message;
>> +} tcases[] = {
>> +	{&invalid_value, EINVAL,
>> +	"cmd is F_SETPIPE_SZ and the arg is beyond 1<<31"},
>> +
>> +	{&half_value, EBUSY,
>> +	"cmd is F_SETPIPE_SZ and the arg is smaller than the amount of the current used buffer space"},
> 
> Ah the EBUSY is handled here.
> 
> 
> Also these descriptions are way too long, ideally these should be
> shorter and to the point. Something as:
> 
> "F_SETPIPE_SZ and size < data stored in pipe"
> 
OK. I will short them.
>> +	{&sys_value, EPERM,
>> +	"cmd is F_SETPIPE_SZ and the arg is over /proc/sys/fs/pipe-max-size limit under unprivileged users"},
> 
> Here something as:
> 
> "F_SETPIPE_SZ and size is over limit for unpriviledged user"
> 
>> +};
>> +
>> +static void verify_fcntl(unsigned int n)
>> +{
>> +	struct tcase *tc = &tcases[n];
>> +
>> +	tst_res(TINFO, "%s", tc->message);
>> +
>> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, *(tc->setvalue)));
>> +	if (TST_RET != -1)
>> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed");
> 
> Maybe we should print the TST_RET here as well, it may return completely
> random number that != -1.
OK.
> 
>> +	if (TST_ERR == tc->exp_err)
>> +		tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed as expected");
>> +	else
>> +		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed expected %s bu got",
>                                                                            ^
> 									  but?
> 
> I guess that we can completely drop the "but" here and just keep it
> "... expected %s got"
> 
OK.
>> +				tst_strerrno(tc->exp_err));
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	SAFE_PIPE(fds);
>> +
>> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
>> +	if (TST_ERR == EINVAL)
>> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
>> +
>> +	orig_value = TST_RET;
>> +
>> +	wrbuf = SAFE_MALLOC(orig_value);
>> +	memset(wrbuf, 'x', orig_value);
>> +	SAFE_WRITE(1, fds[1], wrbuf, orig_value);
>> +
>> +	SAFE_FILE_SCANF("/proc/sys/fs/pipe-max-size", "%d", &sys_value);
>> +	sys_value++;
>> +
>> +	half_value = orig_value / 2;
>> +	invalid_value = (1U << 31) + 1;
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	SAFE_FCNTL(fds[1], F_SETPIPE_SZ, orig_value);
> 
> Again there is no point in restoring the value if we close the pipe
> right after.
> 
OK.
>> +	if (fds[0] > 0)
>> +		SAFE_CLOSE(fds[0]);
>> +	if (fds[1] > 0)
>> +		SAFE_CLOSE(fds[1]);
>> +	if (wrbuf)
>> +		free(wrbuf);
> 
> Why don't we free the buffer right in the test setup? There is no point
> in keeping it allocated.
> 
OK. I will free the buffer in setup.
>> +}
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.test = verify_fcntl,
>> +	.caps = (struct tst_cap []) {
>> +		TST_CAP(TST_CAP_DROP, CAP_SYS_RESOURCE),
>> +		{}
>> +	},
>> +};
> 
> Other than the minor things the rest looks good.
> 

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

When I write pipe12 test case, I have a question
about F_SETPIPE_SZ can set the min and max value.
So cleanup this case and add min/max/gt_max value test.

--------
v2->v3:
1. remove memfree check
2. move size compution into setup and add size check under unprivileged user
3. fix other things pointed by Cyril
--------

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 include/lapi/capability.h                 |   4 +
 testcases/kernel/syscalls/fcntl/fcntl30.c | 223 ++++++++++++++--------
 2 files changed, 147 insertions(+), 80 deletions(-)

diff --git a/include/lapi/capability.h b/include/lapi/capability.h
index 8833f0605..7ade78985 100644
--- a/include/lapi/capability.h
+++ b/include/lapi/capability.h
@@ -28,6 +28,10 @@
 # define CAP_SYS_ADMIN        21
 #endif
 
+#ifndef CAP_SYS_RESOURCE
+# define CAP_SYS_RESOURCE     24
+#endif
+
 #ifndef CAP_AUDIT_READ
 # define CAP_AUDIT_READ       37
 #endif
diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
index 4a409b868..860d42e8d 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl30.c
+++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
@@ -1,111 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Copyright (c) 2014 Fujitsu Ltd.
+ * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
  * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
+ * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
  *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-/*
  * Description:
- * Verify that,
- *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
+ * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
  */
 
-
-#include <stdio.h>
-#include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
-#include <string.h>
-#include <signal.h>
 #include <sys/types.h>
 #include <pwd.h>
-
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 #include "lapi/fcntl.h"
-
-char *TCID = "fcntl30";
-int TST_TOTAL = 1;
-
-static void setup(void);
-static void cleanup(void);
-
-int main(int ac, char **av)
+#include "lapi/abisize.h"
+#include "lapi/capability.h"
+
+static int fds[2];
+static unsigned int shift;
+static struct passwd *pw;
+
+static struct tcase {
+	unsigned int setsize;
+	unsigned int expsize;
+	unsigned int root_user;
+	int pass_flag;
+	char *message;
+} tcases[] = {
+	{0, 0, 0, 1, "set a value of below page size"},
+	{0, 0, 0, 1, "set a normal value"},
+	{0, 0, 1, 1, "set a value of below page size"},
+	{0, 0, 1, 1, "set a normal value"},
+	{0, 0, 1, 1, "set a max value"},
+	{0, 0, 1, 0, "set a value beyond max"},
+};
+
+static void verify_fcntl(unsigned int n)
 {
-	int lc;
-	int pipe_fds[2], test_fd;
-	int orig_pipe_size, new_pipe_size;
-
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
+	struct tcase *tc = &tcases[n];
 
-		SAFE_PIPE(cleanup, pipe_fds);
-		test_fd = pipe_fds[1];
+	tst_res(TINFO, "%s, size is %u", tc->message, tc->setsize);
 
-		TEST(fcntl(test_fd, F_GETPIPE_SZ));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl get pipe size failed");
+	TEST(fcntl(fds[1], F_SETPIPE_SZ, tc->setsize));
+	if (tc->pass_flag) {
+		if (TST_RET == -1) {
+			tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
+			return;
 		}
-
-		orig_pipe_size = TEST_RETURN;
-		new_pipe_size = orig_pipe_size * 2;
-		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl test F_SETPIPE_SZ failed");
+		tst_res(TPASS, "F_SETPIPE_SZ succeed as expected");
+	} else {
+		if (TST_RET == -1) {
+			if ((TST_ERR == ENOMEM && shift < 31) ||
+				(TST_ERR == EINVAL && shift == 31))
+				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
+			else
+				tst_res(TFAIL | TTERRNO,
+					"F_SETPIPE_SZ failed with unexpected error");
+			return;
 		}
+		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
+	}
 
-		TEST(fcntl(test_fd, F_GETPIPE_SZ));
-		if (TEST_RETURN < 0) {
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "fcntl test F_GETPIPE_SZ failed");
-		}
-		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
-			 orig_pipe_size, new_pipe_size);
-		if (TEST_RETURN >= new_pipe_size) {
-			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
-				 "and F_SETPIPE_SZ success");
-		} else {
-			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
-				 "and F_SETPIPE_SZ fail");
-		}
-		SAFE_CLOSE(cleanup, pipe_fds[0]);
-		SAFE_CLOSE(cleanup, pipe_fds[1]);
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
+		return;
 	}
+	if ((unsigned int)TST_RET == tc->expsize)
+		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
+			tc->setsize, (unsigned int)TST_RET);
+	else
+		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
+			tc->setsize, (unsigned int)TST_RET);
+}
 
-	cleanup();
-	tst_exit();
+static void do_test(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	if (!SAFE_FORK()) {
+		if (!tc->root_user) {
+			SAFE_SETUID(pw->pw_uid);
+			tst_res(TINFO, "under an unprivileged user");
+		} else
+			tst_res(TINFO, "under a privileged user");
+		verify_fcntl(n);
+	}
+	tst_reap_children();
 }
 
 static void setup(void)
 {
-	if ((tst_kvercmp(2, 6, 35)) < 0) {
-		tst_brkm(TCONF, NULL, "This test can only run on kernels"
-			 "that are 2.6.35 and higher");
+	unsigned int pg_size, struct_shift, max_shift, pg_shift = 0;
+
+	SAFE_PIPE(fds);
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_ERR == EINVAL)
+		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
+
+	/*
+	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
+	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
+	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
+	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
+	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
+	 * the MAX_ORDER is 11.
+	 * For example, if page size is 4k, on 64bit system. the max pipe size
+	 * as below:
+	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
+	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
+	 *  max pipe size: 2^16* 2^12 = 2^28
+	 * it should be 256M. On 32bit system, this value is 512M.
+	 */
+#ifdef TST_ABI64
+	struct_shift = 6;
+#else
+	struct_shift = 5;
+#endif
+	max_shift = 10;
+
+	pg_size = getpagesize();
+	tst_res(TINFO, "page size is %d bytes", pg_size);
+	while (pg_size >>= 1)
+		pg_shift++;
+
+	shift = max_shift - struct_shift + 2 * pg_shift;
+
+	/*
+	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
+	 * so truncate it.
+	 */
+	if (shift > 31) {
+		tst_res(TINFO, "pipe size truncated into 2G");
+		shift = 31;
 	}
 
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
+	tcases[0].setsize = (1 << pg_shift) - 1;
+	tcases[0].expsize = 1 << pg_shift;
 
-	TEST_PAUSE;
+	tcases[1].setsize = 2 << pg_shift;
+	tcases[1].expsize = 2 << pg_shift;
+
+	tcases[2].setsize = (1 << pg_shift) - 1;
+	tcases[2].expsize = 1 << pg_shift;
+
+	tcases[3].setsize = 2 << pg_shift;
+	tcases[3].expsize = 2 << pg_shift;
+
+	tcases[4].setsize = 1 << shift;
+	tcases[4].expsize = 1 << shift;
+
+	tcases[5].setsize = (1 << shift) + 1;
+
+	pw = SAFE_GETPWNAM("nobody");
 }
 
 static void cleanup(void)
 {
+	if (fds[0] > 0)
+		SAFE_CLOSE(fds[0]);
+	if (fds[1] > 0)
+		SAFE_CLOSE(fds[1]);
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = do_test,
+	.forks_child = 1,
+	.needs_root = 1,
+};
-- 
2.18.0

[LTP] [PATCH v3 2/2] syscalls/fcntl37: add error test for fcntl with F_SETPIPE_SZ

[Yang Xu]

-------------
v2->v3?
fix things pointed by Cyril
-----------

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                           |  2 +
 testcases/kernel/syscalls/fcntl/.gitignore |  2 +
 testcases/kernel/syscalls/fcntl/fcntl37.c  | 98 ++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 0743cf4e3..577a4a527 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -301,6 +301,8 @@ fcntl35 fcntl35
 fcntl35_64 fcntl35_64
 fcntl36 fcntl36
 fcntl36_64 fcntl36_64
+fcntl37 fcntl37
+fcntl37_64 fcntl37_64
 
 fdatasync01 fdatasync01
 fdatasync02 fdatasync02
diff --git a/testcases/kernel/syscalls/fcntl/.gitignore b/testcases/kernel/syscalls/fcntl/.gitignore
index 8d5738f97..be8d9739e 100644
--- a/testcases/kernel/syscalls/fcntl/.gitignore
+++ b/testcases/kernel/syscalls/fcntl/.gitignore
@@ -70,3 +70,5 @@
 /fcntl35_64
 /fcntl36
 /fcntl36_64
+/fcntl37
+/fcntl37_64
diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
new file mode 100644
index 000000000..c52af22dd
--- /dev/null
+++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
+ *
+ * Test basic error handling for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ
+ * argument.
+ * 1)fcntl fails with EINVAL when cmd is F_SETPIPE_SZ and the arg is
+ * beyond 1<<31.
+ * 2)fcntl fails with EBUSY when cmd is F_SETPIPE_SZ and the arg is smaller
+ * than the amount of the current used buffer space.
+ * 3)fcntl fails with EPERM when cmd is F_SETPIPE_SZ and the arg is over
+ * /proc/sys/fs/pipe-max-size limit under unprivileged users.
+ */
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <limits.h>
+#include <stdlib.h>
+#include "tst_test.h"
+#include "lapi/fcntl.h"
+#include "lapi/capability.h"
+
+static int fds[2];
+static unsigned int invalid_value, half_value, sys_value;
+
+static struct tcase {
+	unsigned int *setvalue;
+	int exp_err;
+	char *message;
+} tcases[] = {
+	{&invalid_value, EINVAL, "F_SETPIPE_SZ and size is beyond 1<<31"},
+	{&half_value, EBUSY, "F_SETPIPE_SZ and size < data stored in pipe"},
+	{&sys_value, EPERM, "F_SETPIPE_SZ and size is over limit for unpriviledged user"},
+};
+
+static void verify_fcntl(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+
+	tst_res(TINFO, "%s", tc->message);
+
+	TEST(fcntl(fds[1], F_SETPIPE_SZ, *(tc->setvalue)));
+	if (TST_RET != -1) {
+		tst_res(TFAIL, "F_SETPIPE_SZ succeed and return %ld", TST_RET);
+		return;
+	}
+	if (TST_ERR == tc->exp_err)
+		tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed as expected");
+	else
+		tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed expected %s got",
+				tst_strerrno(tc->exp_err));
+}
+
+static void setup(void)
+{
+	char *wrbuf;
+	unsigned int orig_value;
+
+	SAFE_PIPE(fds);
+
+	TEST(fcntl(fds[1], F_GETPIPE_SZ));
+	if (TST_ERR == EINVAL)
+		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
+
+	orig_value = TST_RET;
+
+	wrbuf = SAFE_MALLOC(orig_value);
+	memset(wrbuf, 'x', orig_value);
+	SAFE_WRITE(1, fds[1], wrbuf, orig_value);
+	free(wrbuf);
+
+	SAFE_FILE_SCANF("/proc/sys/fs/pipe-max-size", "%d", &sys_value);
+	sys_value++;
+
+	half_value = orig_value / 2;
+	invalid_value = (1U << 31) + 1;
+}
+
+static void cleanup(void)
+{
+	if (fds[0] > 0)
+		SAFE_CLOSE(fds[0]);
+	if (fds[1] > 0)
+		SAFE_CLOSE(fds[1]);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = verify_fcntl,
+	.caps = (struct tst_cap []) {
+		TST_CAP(TST_CAP_DROP, CAP_SYS_RESOURCE),
+		{}
+	},
+};
-- 
2.18.0

[LTP] [PATCH v2 1/2] syscalls/fcntl30: clean up && add more range test

[Cyril Hrubis]

Hi!
> > Do we really need this? Looking at the kernel code the fcntl() just
> > reallocates the array that is holding the slots, so we only allocate new
> > array of struct pipe_buffer which contains a pointer for the actuall
> > page that is allocated when we _WRITE_ to the pipe.
> > 
> Yes, you are right. But this case indeed fail when on low memory machine 
> (4kb page size, 256/512M memory).

That's strange, I had a look at the code today again and as far as I can
tell we only check for user ulimit there.

What was the errno when the ioctl() has failed? Was it EPERM or ENOMEM?
The ENOMEM may have happened if the system overcommit was disabled and
the system was out of memory.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Cyril Hrubis]

Hi!
> When I write pipe12 test case, I have a question
> about F_SETPIPE_SZ can set the min and max value.
> So cleanup this case and add min/max/gt_max value test.
> 
> --------
> v2->v3:
> 1. remove memfree check
> 2. move size compution into setup and add size check under unprivileged user
> 3. fix other things pointed by Cyril
> --------
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  include/lapi/capability.h                 |   4 +
>  testcases/kernel/syscalls/fcntl/fcntl30.c | 223 ++++++++++++++--------
>  2 files changed, 147 insertions(+), 80 deletions(-)
> 
> diff --git a/include/lapi/capability.h b/include/lapi/capability.h
> index 8833f0605..7ade78985 100644
> --- a/include/lapi/capability.h
> +++ b/include/lapi/capability.h
> @@ -28,6 +28,10 @@
>  # define CAP_SYS_ADMIN        21
>  #endif
>  
> +#ifndef CAP_SYS_RESOURCE
> +# define CAP_SYS_RESOURCE     24
> +#endif
> +
>  #ifndef CAP_AUDIT_READ
>  # define CAP_AUDIT_READ       37
>  #endif
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
> index 4a409b868..860d42e8d 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
> @@ -1,111 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * Copyright (c) 2014 Fujitsu Ltd.
> + * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
>   * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> + * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
>   *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of version 2 of the GNU General Public License as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, write the Free Software Foundation, Inc.,
> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> - */
> -
> -/*
>   * Description:
> - * Verify that,
> - *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
> + * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>   */
>  
> -
> -#include <stdio.h>
> -#include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> -#include <string.h>
> -#include <signal.h>
>  #include <sys/types.h>
>  #include <pwd.h>
> -
> -#include "test.h"
> -#include "safe_macros.h"
> +#include "tst_test.h"
>  #include "lapi/fcntl.h"
> -
> -char *TCID = "fcntl30";
> -int TST_TOTAL = 1;
> -
> -static void setup(void);
> -static void cleanup(void);
> -
> -int main(int ac, char **av)
> +#include "lapi/abisize.h"
> +#include "lapi/capability.h"
> +
> +static int fds[2];
> +static unsigned int shift;
> +static struct passwd *pw;
> +
> +static struct tcase {
> +	unsigned int setsize;
> +	unsigned int expsize;
> +	unsigned int root_user;
> +	int pass_flag;
> +	char *message;
> +} tcases[] = {
> +	{0, 0, 0, 1, "set a value of below page size"},
> +	{0, 0, 0, 1, "set a normal value"},
> +	{0, 0, 1, 1, "set a value of below page size"},
> +	{0, 0, 1, 1, "set a normal value"},
> +	{0, 0, 1, 1, "set a max value"},
> +	{0, 0, 1, 0, "set a value beyond max"},
> +};
> +
> +static void verify_fcntl(unsigned int n)
>  {
> -	int lc;
> -	int pipe_fds[2], test_fd;
> -	int orig_pipe_size, new_pipe_size;
> -
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> +	struct tcase *tc = &tcases[n];
>  
> -		SAFE_PIPE(cleanup, pipe_fds);
> -		test_fd = pipe_fds[1];
> +	tst_res(TINFO, "%s, size is %u", tc->message, tc->setsize);
>  
> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl get pipe size failed");
> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, tc->setsize));
> +	if (tc->pass_flag) {
> +		if (TST_RET == -1) {
> +			tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
> +			return;
>  		}
> -
> -		orig_pipe_size = TEST_RETURN;
> -		new_pipe_size = orig_pipe_size * 2;
> -		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl test F_SETPIPE_SZ failed");
> +		tst_res(TPASS, "F_SETPIPE_SZ succeed as expected");
> +	} else {
> +		if (TST_RET == -1) {
> +			if ((TST_ERR == ENOMEM && shift < 31) ||
> +				(TST_ERR == EINVAL && shift == 31))
> +				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
> +			else
> +				tst_res(TFAIL | TTERRNO,
> +					"F_SETPIPE_SZ failed with unexpected error");
> +			return;
>  		}
> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
> +	}
>  
> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
> -		if (TEST_RETURN < 0) {
> -			tst_brkm(TFAIL | TTERRNO, cleanup,
> -				 "fcntl test F_GETPIPE_SZ failed");
> -		}
> -		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
> -			 orig_pipe_size, new_pipe_size);
> -		if (TEST_RETURN >= new_pipe_size) {
> -			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
> -				 "and F_SETPIPE_SZ success");
> -		} else {
> -			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
> -				 "and F_SETPIPE_SZ fail");
> -		}
> -		SAFE_CLOSE(cleanup, pipe_fds[0]);
> -		SAFE_CLOSE(cleanup, pipe_fds[1]);
> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
> +		return;
>  	}
> +	if ((unsigned int)TST_RET == tc->expsize)
> +		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
> +			tc->setsize, (unsigned int)TST_RET);
> +	else
> +		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
> +			tc->setsize, (unsigned int)TST_RET);
> +}
>  
> -	cleanup();
> -	tst_exit();
> +static void do_test(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +
> +	if (!SAFE_FORK()) {
> +		if (!tc->root_user) {
> +			SAFE_SETUID(pw->pw_uid);
> +			tst_res(TINFO, "under an unprivileged user");
> +		} else
> +			tst_res(TINFO, "under a privileged user");
> +		verify_fcntl(n);
> +	}
> +	tst_reap_children();
>  }
>  
>  static void setup(void)
>  {
> -	if ((tst_kvercmp(2, 6, 35)) < 0) {
> -		tst_brkm(TCONF, NULL, "This test can only run on kernels"
> -			 "that are 2.6.35 and higher");
> +	unsigned int pg_size, struct_shift, max_shift, pg_shift = 0;
> +
> +	SAFE_PIPE(fds);
> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
> +	if (TST_ERR == EINVAL)
> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
> +
> +	/*
> +	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
> +	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
> +	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
> +	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
> +	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
> +	 * the MAX_ORDER is 11.
> +	 * For example, if page size is 4k, on 64bit system. the max pipe size
> +	 * as below:
> +	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
> +	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
> +	 *  max pipe size: 2^16* 2^12 = 2^28
> +	 * it should be 256M. On 32bit system, this value is 512M.
> +	 */
> +#ifdef TST_ABI64
> +	struct_shift = 6;
> +#else
> +	struct_shift = 5;
> +#endif
> +	max_shift = 10;
> +
> +	pg_size = getpagesize();
> +	tst_res(TINFO, "page size is %d bytes", pg_size);
> +	while (pg_size >>= 1)
> +		pg_shift++;
> +
> +	shift = max_shift - struct_shift + 2 * pg_shift;
> +
> +	/*
> +	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
> +	 * so truncate it.
> +	 */
> +	if (shift > 31) {
> +		tst_res(TINFO, "pipe size truncated into 2G");
> +		shift = 31;
>  	}
>  
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> +	tcases[0].setsize = (1 << pg_shift) - 1;
> +	tcases[0].expsize = 1 << pg_shift;
>  
> -	TEST_PAUSE;
> +	tcases[1].setsize = 2 << pg_shift;
> +	tcases[1].expsize = 2 << pg_shift;
> +
> +	tcases[2].setsize = (1 << pg_shift) - 1;
> +	tcases[2].expsize = 1 << pg_shift;
> +
> +	tcases[3].setsize = 2 << pg_shift;
> +	tcases[3].expsize = 2 << pg_shift;
> +
> +	tcases[4].setsize = 1 << shift;
> +	tcases[4].expsize = 1 << shift;

I was playing with the test and it seems that the kernel allocation may
fail even for much smaller sizes for various reasons. I guess that
memory framentation on long running systems may be the culprit here
because kmalloc() allocates physically continuous memory.

I guess that the safest bet here would be limiting the maximal size we
try to resize the pipe and succeed to something as 8MB which would be
something as 32 pages to allocate.

At the same time I would just define the size we expect to fail with
ENOMEM to 1<<30 and that would save us from this architecture specific
trickery that will probably fail on stranger architectures anyway.

> +	tcases[5].setsize = (1 << shift) + 1;
> +
> +	pw = SAFE_GETPWNAM("nobody");
>  }
>  
>  static void cleanup(void)
>  {
> +	if (fds[0] > 0)
> +		SAFE_CLOSE(fds[0]);
> +	if (fds[1] > 0)
> +		SAFE_CLOSE(fds[1]);
>  }
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = do_test,
> +	.forks_child = 1,
> +	.needs_root = 1,
> +};
> -- 
> 2.18.0
> 
> 
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

Hi

> Hi!
>> When I write pipe12 test case, I have a question
>> about F_SETPIPE_SZ can set the min and max value.
>> So cleanup this case and add min/max/gt_max value test.
>>
>> --------
>> v2->v3:
>> 1. remove memfree check
>> 2. move size compution into setup and add size check under unprivileged user
>> 3. fix other things pointed by Cyril
>> --------
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
>> ---
>>   include/lapi/capability.h                 |   4 +
>>   testcases/kernel/syscalls/fcntl/fcntl30.c | 223 ++++++++++++++--------
>>   2 files changed, 147 insertions(+), 80 deletions(-)
>>
>> diff --git a/include/lapi/capability.h b/include/lapi/capability.h
>> index 8833f0605..7ade78985 100644
>> --- a/include/lapi/capability.h
>> +++ b/include/lapi/capability.h
>> @@ -28,6 +28,10 @@
>>   # define CAP_SYS_ADMIN        21
>>   #endif
>>   
>> +#ifndef CAP_SYS_RESOURCE
>> +# define CAP_SYS_RESOURCE     24
>> +#endif
>> +
>>   #ifndef CAP_AUDIT_READ
>>   # define CAP_AUDIT_READ       37
>>   #endif
>> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> index 4a409b868..860d42e8d 100644
>> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
>> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> @@ -1,111 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>   /*
>> - * Copyright (c) 2014 Fujitsu Ltd.
>> + * Copyright (c) 2014-2020 FUJITSU LIMITED. All rights reserved.
>>    * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
>> + * Author: Yang Xu <xuyang2018.jy@cn.jujitsu.com>
>>    *
>> - * This program is free software; you can redistribute it and/or modify it
>> - * under the terms of version 2 of the GNU General Public License as
>> - * published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it would be useful, but
>> - * WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>> - *
>> - * You should have received a copy of the GNU General Public License along
>> - * with this program; if not, write the Free Software Foundation, Inc.,
>> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> - */
>> -
>> -/*
>>    * Description:
>> - * Verify that,
>> - *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>> + * Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ argument.
>>    */
>>   
>> -
>> -#include <stdio.h>
>> -#include <errno.h>
>>   #include <unistd.h>
>>   #include <fcntl.h>
>> -#include <string.h>
>> -#include <signal.h>
>>   #include <sys/types.h>
>>   #include <pwd.h>
>> -
>> -#include "test.h"
>> -#include "safe_macros.h"
>> +#include "tst_test.h"
>>   #include "lapi/fcntl.h"
>> -
>> -char *TCID = "fcntl30";
>> -int TST_TOTAL = 1;
>> -
>> -static void setup(void);
>> -static void cleanup(void);
>> -
>> -int main(int ac, char **av)
>> +#include "lapi/abisize.h"
>> +#include "lapi/capability.h"
>> +
>> +static int fds[2];
>> +static unsigned int shift;
>> +static struct passwd *pw;
>> +
>> +static struct tcase {
>> +	unsigned int setsize;
>> +	unsigned int expsize;
>> +	unsigned int root_user;
>> +	int pass_flag;
>> +	char *message;
>> +} tcases[] = {
>> +	{0, 0, 0, 1, "set a value of below page size"},
>> +	{0, 0, 0, 1, "set a normal value"},
>> +	{0, 0, 1, 1, "set a value of below page size"},
>> +	{0, 0, 1, 1, "set a normal value"},
>> +	{0, 0, 1, 1, "set a max value"},
>> +	{0, 0, 1, 0, "set a value beyond max"},
>> +};
>> +
>> +static void verify_fcntl(unsigned int n)
>>   {
>> -	int lc;
>> -	int pipe_fds[2], test_fd;
>> -	int orig_pipe_size, new_pipe_size;
>> -
>> -
>> -	tst_parse_opts(ac, av, NULL, NULL);
>> -
>> -	setup();
>> -
>> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> -		tst_count = 0;
>> +	struct tcase *tc = &tcases[n];
>>   
>> -		SAFE_PIPE(cleanup, pipe_fds);
>> -		test_fd = pipe_fds[1];
>> +	tst_res(TINFO, "%s, size is %u", tc->message, tc->setsize);
>>   
>> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl get pipe size failed");
>> +	TEST(fcntl(fds[1], F_SETPIPE_SZ, tc->setsize));
>> +	if (tc->pass_flag) {
>> +		if (TST_RET == -1) {
>> +			tst_res(TFAIL | TTERRNO, "F_SETPIPE_SZ failed");
>> +			return;
>>   		}
>> -
>> -		orig_pipe_size = TEST_RETURN;
>> -		new_pipe_size = orig_pipe_size * 2;
>> -		TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl test F_SETPIPE_SZ failed");
>> +		tst_res(TPASS, "F_SETPIPE_SZ succeed as expected");
>> +	} else {
>> +		if (TST_RET == -1) {
>> +			if ((TST_ERR == ENOMEM && shift < 31) ||
>> +				(TST_ERR == EINVAL && shift == 31))
>> +				tst_res(TPASS | TTERRNO, "F_SETPIPE_SZ failed");
>> +			else
>> +				tst_res(TFAIL | TTERRNO,
>> +					"F_SETPIPE_SZ failed with unexpected error");
>> +			return;
>>   		}
>> +		tst_res(TFAIL, "F_SETPIPE_SZ succeed unexpectedly");
>> +	}
>>   
>> -		TEST(fcntl(test_fd, F_GETPIPE_SZ));
>> -		if (TEST_RETURN < 0) {
>> -			tst_brkm(TFAIL | TTERRNO, cleanup,
>> -				 "fcntl test F_GETPIPE_SZ failed");
>> -		}
>> -		tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size: %d",
>> -			 orig_pipe_size, new_pipe_size);
>> -		if (TEST_RETURN >= new_pipe_size) {
>> -			tst_resm(TPASS, "fcntl test F_GETPIPE_SZ"
>> -				 "and F_SETPIPE_SZ success");
>> -		} else {
>> -			tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ"
>> -				 "and F_SETPIPE_SZ fail");
>> -		}
>> -		SAFE_CLOSE(cleanup, pipe_fds[0]);
>> -		SAFE_CLOSE(cleanup, pipe_fds[1]);
>> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "F_GETPIPE_SZ failed");
>> +		return;
>>   	}
>> +	if ((unsigned int)TST_RET == tc->expsize)
>> +		tst_res(TPASS, "F_SETPIPE_SZ %u bytes F_GETPIPE_SZ %u bytes",
>> +			tc->setsize, (unsigned int)TST_RET);
>> +	else
>> +		tst_res(TFAIL, "F_SETPIPE_SZ %u bytes but F_GETPIPE_SZ %u bytes",
>> +			tc->setsize, (unsigned int)TST_RET);
>> +}
>>   
>> -	cleanup();
>> -	tst_exit();
>> +static void do_test(unsigned int n)
>> +{
>> +	struct tcase *tc = &tcases[n];
>> +
>> +	if (!SAFE_FORK()) {
>> +		if (!tc->root_user) {
>> +			SAFE_SETUID(pw->pw_uid);
>> +			tst_res(TINFO, "under an unprivileged user");
>> +		} else
>> +			tst_res(TINFO, "under a privileged user");
>> +		verify_fcntl(n);
>> +	}
>> +	tst_reap_children();
>>   }
>>   
>>   static void setup(void)
>>   {
>> -	if ((tst_kvercmp(2, 6, 35)) < 0) {
>> -		tst_brkm(TCONF, NULL, "This test can only run on kernels"
>> -			 "that are 2.6.35 and higher");
>> +	unsigned int pg_size, struct_shift, max_shift, pg_shift = 0;
>> +
>> +	SAFE_PIPE(fds);
>> +	TEST(fcntl(fds[1], F_GETPIPE_SZ));
>> +	if (TST_ERR == EINVAL)
>> +		tst_brk(TCONF, "kernel doesn't support F_GET/SETPIPE_SZ");
>> +
>> +	/*
>> +	 * See kernel fs/pipe.c, the size of struct pipe buffer is 40 bytes
>> +	 * (round up 2^6) on 64bit system and 24 bytes(round up 2^5). kcalloc
>> +	 * mallocs a memory space range stores struct pipe buffer. kcalloc can
>> +	 * malloc max space depend on KMALLOC_SHIFT_MAX macro.
>> +	 *  #define KMALLOC_SHIFT_MAX  (MAX_ORDER + PAGE_SHIFT - 1)
>> +	 * the MAX_ORDER is 11.
>> +	 * For example, if page size is 4k, on 64bit system. the max pipe size
>> +	 * as below:
>> +	 *  kcalloc space(4M): 1 << (11+12-1)= 2^22
>> +	 *  space can store struct pipi buffer: 2^22/2^6= 2^16
>> +	 *  max pipe size: 2^16* 2^12 = 2^28
>> +	 * it should be 256M. On 32bit system, this value is 512M.
>> +	 */
>> +#ifdef TST_ABI64
>> +	struct_shift = 6;
>> +#else
>> +	struct_shift = 5;
>> +#endif
>> +	max_shift = 10;
>> +
>> +	pg_size = getpagesize();
>> +	tst_res(TINFO, "page size is %d bytes", pg_size);
>> +	while (pg_size >>= 1)
>> +		pg_shift++;
>> +
>> +	shift = max_shift - struct_shift + 2 * pg_shift;
>> +
>> +	/*
>> +	 * On 64k page size machine, this will beyond 2G and trigger EINVAL error,
>> +	 * so truncate it.
>> +	 */
>> +	if (shift > 31) {
>> +		tst_res(TINFO, "pipe size truncated into 2G");
>> +		shift = 31;
>>   	}
>>   
>> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>> +	tcases[0].setsize = (1 << pg_shift) - 1;
>> +	tcases[0].expsize = 1 << pg_shift;
>>   
>> -	TEST_PAUSE;
>> +	tcases[1].setsize = 2 << pg_shift;
>> +	tcases[1].expsize = 2 << pg_shift;
>> +
>> +	tcases[2].setsize = (1 << pg_shift) - 1;
>> +	tcases[2].expsize = 1 << pg_shift;
>> +
>> +	tcases[3].setsize = 2 << pg_shift;
>> +	tcases[3].expsize = 2 << pg_shift;
>> +
>> +	tcases[4].setsize = 1 << shift;
>> +	tcases[4].expsize = 1 << shift;
> 
> I was playing with the test and it seems that the kernel allocation may
> fail even for much smaller sizes for various reasons. I guess that
> memory framentation on long running systems may be the culprit here
> because kmalloc() allocates physically continuous memory.
> 
> I guess that the safest bet here would be limiting the maximal size we
> try to resize the pipe and succeed to something as 8MB which would be
> something as 32 pages to allocate.
> 
Agree.
> At the same time I would just define the size we expect to fail with
> ENOMEM to 1<<30 and that would save us from this architecture specific
> trickery that will probably fail on stranger architectures anyway.
On 64kb page size, it will over 1 <<30 for ENOMEM error .I think we can 
test MAX_SIZE+pg_size(< 1<<31) for ENOMEM error. If  beyond 1<<31, 
expect EINVAL error.

Best Regards
Yang Xu
> 
>> +	tcases[5].setsize = (1 << shift) + 1;
>> +
>> +	pw = SAFE_GETPWNAM("nobody");
>>   }
>>   
>>   static void cleanup(void)
>>   {
>> +	if (fds[0] > 0)
>> +		SAFE_CLOSE(fds[0]);
>> +	if (fds[1] > 0)
>> +		SAFE_CLOSE(fds[1]);
>>   }
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.test = do_test,
>> +	.forks_child = 1,
>> +	.needs_root = 1,
>> +};
>> -- 
>> 2.18.0
>>
>>
>>
> 

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Cyril Hrubis]

Hi!
> > I was playing with the test and it seems that the kernel allocation may
> > fail even for much smaller sizes for various reasons. I guess that
> > memory framentation on long running systems may be the culprit here
> > because kmalloc() allocates physically continuous memory.
> > 
> > I guess that the safest bet here would be limiting the maximal size we
> > try to resize the pipe and succeed to something as 8MB which would be
> > something as 32 pages to allocate.
> > 
> Agree.
> > At the same time I would just define the size we expect to fail with
> > ENOMEM to 1<<30 and that would save us from this architecture specific
> > trickery that will probably fail on stranger architectures anyway.
> On 64kb page size, it will over 1 <<30 for ENOMEM error .I think we can 
> test MAX_SIZE+pg_size(< 1<<31) for ENOMEM error. If  beyond 1<<31, 
> expect EINVAL error.

Hmm, maybe we can just double the size in a loop until we hit either
ENOMEM or EINVAL then and fail the test if we hit them too soon.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

Hi

> Hi!
>>> I was playing with the test and it seems that the kernel allocation may
>>> fail even for much smaller sizes for various reasons. I guess that
>>> memory framentation on long running systems may be the culprit here
>>> because kmalloc() allocates physically continuous memory.
>>>
>>> I guess that the safest bet here would be limiting the maximal size we
>>> try to resize the pipe and succeed to something as 8MB which would be
>>> something as 32 pages to allocate.
>>>
>> Agree.
>>> At the same time I would just define the size we expect to fail with
>>> ENOMEM to 1<<30 and that would save us from this architecture specific
>>> trickery that will probably fail on stranger architectures anyway.
>> On 64kb page size, it will over 1 <<30 for ENOMEM error .I think we can
>> test MAX_SIZE+pg_size(< 1<<31) for ENOMEM error. If  beyond 1<<31,
>> expect EINVAL error.
> 
> Hmm, maybe we can just double the size in a loop until we hit either
> ENOMEM or EINVAL then and fail the test if we hit them too soon.
I plan to remove this max test because of unknown kmalloc fail, test 
range as below

         {0, 0, 0, 1, "set a value of below page size"},
         {0, 0, 0, 1, "set a normal value"},             //under 
non-privileged user,maybe 128k (<1024k )
         {0, 0, 1, 1, "set a value of below page size"},
         {0, 0, 1, 1, "set a normal value"},    // test 8M as you suggested,
         {0, 0, 1, 0, "set a value beyond max"},  //expect EINVAL or ENOMEM
};

What do you think about it?

Best Regards
Yang Xu
> 

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

Hi!

> Hi
> 
>> Hi!
>>>> I was playing with the test and it seems that the kernel allocation may
>>>> fail even for much smaller sizes for various reasons. I guess that
>>>> memory framentation on long running systems may be the culprit here
>>>> because kmalloc() allocates physically continuous memory.
>>>>
>>>> I guess that the safest bet here would be limiting the maximal size we
>>>> try to resize the pipe and succeed to something as 8MB which would be
>>>> something as 32 pages to allocate.
>>>>
>>> Agree.
>>>> At the same time I would just define the size we expect to fail with
>>>> ENOMEM to 1<<30 and that would save us from this architecture specific
>>>> trickery that will probably fail on stranger architectures anyway.
>>> On 64kb page size, it will over 1 <<30 for ENOMEM error .I think we can
>>> test MAX_SIZE+pg_size(< 1<<31) for ENOMEM error. If? beyond 1<<31,
>>> expect EINVAL error.
>>
>> Hmm, maybe we can just double the size in a loop until we hit either
>> ENOMEM or EINVAL then and fail the test if we hit them too soon.
> I plan to remove this max test because of unknown kmalloc fail, test 
> range as below
> 
>  ??????? {0, 0, 0, 1, "set a value of below page size"},
>  ??????? {0, 0, 0, 1, "set a normal value"},???????????? //under 
> non-privileged user,maybe 128k (<1024k )
>  ??????? {0, 0, 1, 1, "set a value of below page size"},
>  ??????? {0, 0, 1, 1, "set a normal value"},??? // test 8M as you 
> suggested,
>  ??????? {0, 0, 1, 0, "set a value beyond max"},? //expect EINVAL or ENOMEM
> };
> 
> What do you think about it?
Ping.
diff as below:
diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c 
b/testcases/kernel/syscalls/fcntl/fcntl30.c
index 860d42e8d..28cdee165 100644
--- a/testcases/kernel/syscalls/fcntl/fcntl30.c
+++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
@@ -31,8 +31,7 @@ static struct tcase {
         {0, 0, 0, 1, "set a value of below page size"},
         {0, 0, 0, 1, "set a normal value"},
         {0, 0, 1, 1, "set a value of below page size"},
-       {0, 0, 1, 1, "set a normal value"},
-       {0, 0, 1, 1, "set a max value"},
+       {0, 0, 1, 1, "set a normal value(8M)"},
         {0, 0, 1, 0, "set a value beyond max"},
  };

@@ -145,13 +144,10 @@ static void setup(void)
         tcases[2].setsize = (1 << pg_shift) - 1;
         tcases[2].expsize = 1 << pg_shift;

-       tcases[3].setsize = 2 << pg_shift;
-       tcases[3].expsize = 2 << pg_shift;
+       tcases[3].setsize = 1 << 23;
+       tcases[3].expsize = 1 << 23;

-       tcases[4].setsize = 1 << shift;
-       tcases[4].expsize = 1 << shift;
-
-       tcases[5].setsize = (1 << shift) + 1;
+       tcases[4].setsize = (1 << shift) + 1;

         pw = SAFE_GETPWNAM("nobody");
  }

Best Regards
Yang Xu
> 
> Best Regards
> Yang Xu
>>
> 
> 
> 

[LTP] [PATCH v3 2/2] syscalls/fcntl37: add error test for fcntl with F_SETPIPE_SZ

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Cyril Hrubis]

Hi!
> Ping.
> diff as below:
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c 
> b/testcases/kernel/syscalls/fcntl/fcntl30.c
> index 860d42e8d..28cdee165 100644
> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
> @@ -31,8 +31,7 @@ static struct tcase {
>          {0, 0, 0, 1, "set a value of below page size"},
>          {0, 0, 0, 1, "set a normal value"},
>          {0, 0, 1, 1, "set a value of below page size"},
> -       {0, 0, 1, 1, "set a normal value"},
> -       {0, 0, 1, 1, "set a max value"},
> +       {0, 0, 1, 1, "set a normal value(8M)"},
>          {0, 0, 1, 0, "set a value beyond max"},
>   };
> 
> @@ -145,13 +144,10 @@ static void setup(void)
>          tcases[2].setsize = (1 << pg_shift) - 1;
>          tcases[2].expsize = 1 << pg_shift;
> 
> -       tcases[3].setsize = 2 << pg_shift;
> -       tcases[3].expsize = 2 << pg_shift;
> +       tcases[3].setsize = 1 << 23;
> +       tcases[3].expsize = 1 << 23;
> 
> -       tcases[4].setsize = 1 << shift;
> -       tcases[4].expsize = 1 << shift;
> -
> -       tcases[5].setsize = (1 << shift) + 1;
> +       tcases[4].setsize = (1 << shift) + 1;
> 
>          pw = SAFE_GETPWNAM("nobody");
>   }

Do we have to keep the shift in here?

Given that we are not aiming at a precise value now, we should be fine
as long as we request the buffer to be a few megabytes in lenght and we
can drop all the arch specific code from here, right?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/2] syscalls/fcntl30: clean up && add more range test

[Yang Xu]

Hi Cyril


> Hi!
>> Ping.
>> diff as below:
>> diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c
>> b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> index 860d42e8d..28cdee165 100644
>> --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
>> +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
>> @@ -31,8 +31,7 @@ static struct tcase {
>>           {0, 0, 0, 1, "set a value of below page size"},
>>           {0, 0, 0, 1, "set a normal value"},
>>           {0, 0, 1, 1, "set a value of below page size"},
>> -       {0, 0, 1, 1, "set a normal value"},
>> -       {0, 0, 1, 1, "set a max value"},
>> +       {0, 0, 1, 1, "set a normal value(8M)"},
>>           {0, 0, 1, 0, "set a value beyond max"},
>>    };
>>
>> @@ -145,13 +144,10 @@ static void setup(void)
>>           tcases[2].setsize = (1 << pg_shift) - 1;
>>           tcases[2].expsize = 1 << pg_shift;
>>
>> -       tcases[3].setsize = 2 << pg_shift;
>> -       tcases[3].expsize = 2 << pg_shift;
>> +       tcases[3].setsize = 1 << 23;
>> +       tcases[3].expsize = 1 << 23;
>>
>> -       tcases[4].setsize = 1 << shift;
>> -       tcases[4].expsize = 1 << shift;
>> -
>> -       tcases[5].setsize = (1 << shift) + 1;
>> +       tcases[4].setsize = (1 << shift) + 1;
>>
>>           pw = SAFE_GETPWNAM("nobody");
>>    }
> 
> Do we have to keep the shift in here?
> 
> Given that we are not aiming at a precise value now, we should be fine
> as long as we request the buffer to be a few megabytes in lenght and we
> can drop all the arch specific code from here, right?
Yes, if we don't want to test ENOMEM error, this arch specific code can 
be removed. Since only few people will set so large pipe size and 
trigger this error, I think we can remove this.

Best Regards
Yang Xu
>