diff mbox series

unshare03: using soft limit of NOFILE

Message ID 20250314044257.1673303-1-lufei@uniontech.com
State Changes Requested
Headers show
Series unshare03: using soft limit of NOFILE | expand

Commit Message

lufei March 14, 2025, 4:42 a.m. UTC
I think it's safer to set NOFILE increasing from soft limit than from
hard limit.

Hard limit may lead to dup2 ENOMEM error which bring the result to
TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
limit in /proc/sys/fs/nr_open come out to be 1073741816)

Signed-off-by: lufei <lufei@uniontech.com>
---
 testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Petr Vorel March 27, 2025, 10:33 a.m. UTC | #1
Hi lufei, Al,

@Al, you're the author of the original test unshare_test.c [1] in kselftest.
This is a patch to LTP test unshare03.c, which is based on your test.

> I think it's safer to set NOFILE increasing from soft limit than from
> hard limit.

> Hard limit may lead to dup2 ENOMEM error which bring the result to
> TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
> limit in /proc/sys/fs/nr_open come out to be 1073741816)

IMHO lowering number to ~ half (in my case) by using rlimit.rlim_max instead of
/proc/sys/fs/nr_open should not affect the functionality of the test, right?
Or am I missing something obvious?

@lufei I guess kselftest tools/testing/selftests/core/unshare_test.c would fail
for you as well, right?

Kind regards,
Petr

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=611fbeb44a777e5ab54ab3127ec85f72147911d8

> Signed-off-by: lufei <lufei@uniontech.com>
> ---
>  testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

> diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> index 7c5e71c4e..bb568264c 100644
> --- a/testcases/kernel/syscalls/unshare/unshare03.c
> +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> @@ -24,7 +24,7 @@

>  static void run(void)
>  {
> -	int nr_open;
> +	int rlim_max;
>  	int nr_limit;
>  	struct rlimit rlimit;
>  	struct tst_clone_args args = {
> @@ -32,14 +32,12 @@ static void run(void)
>  		.exit_signal = SIGCHLD,
>  	};

> -	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> -	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
> +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> +	rlim_max = rlimit.rlim_max;

> -	nr_limit = nr_open + NR_OPEN_LIMIT;
> +	nr_limit = rlim_max + NR_OPEN_LIMIT;
>  	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);

> -	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> -
>  	rlimit.rlim_cur = nr_limit;
>  	rlimit.rlim_max = nr_limit;

> @@ -47,10 +45,10 @@ static void run(void)
>  	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
>  		nr_limit);

> -	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
> +	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);

>  	if (!SAFE_CLONE(&args)) {
> -		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
>  		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
>  		exit(0);
>  	}
Petr Vorel March 28, 2025, 8:50 a.m. UTC | #2
Hi lufei, Al,

> Hi Petr,

> Yes, kselftest tools/testing/selftests/core/unshare_test.c failed as
> well, dup2 failed:

> ```
> unshare_test.c:60:unshare_EMFILE:Expected dup2(2, nr_open + 64) (-1) >= 0 (0)
> ```

Thanks for info.  Maybe also sending a patch to kselftest?

Kind regards,
Petr

PS: lufei, please keep Cc to keep other informed.

> Thanks for reply.
> Best regards.

> On Thu, Mar 27, 2025 at 11:33:36AM +0100, Petr Vorel wrote:
> > Hi lufei, Al,

> > @Al, you're the author of the original test unshare_test.c [1] in kselftest.
> > This is a patch to LTP test unshare03.c, which is based on your test.

> > > I think it's safer to set NOFILE increasing from soft limit than from
> > > hard limit.

> > > Hard limit may lead to dup2 ENOMEM error which bring the result to
> > > TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
> > > limit in /proc/sys/fs/nr_open come out to be 1073741816)

> > IMHO lowering number to ~ half (in my case) by using rlimit.rlim_max instead of
> > /proc/sys/fs/nr_open should not affect the functionality of the test, right?
> > Or am I missing something obvious?

> > @lufei I guess kselftest tools/testing/selftests/core/unshare_test.c would fail
> > for you as well, right?

> > Kind regards,
> > Petr

> > [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=611fbeb44a777e5ab54ab3127ec85f72147911d8

> > > Signed-off-by: lufei <lufei@uniontech.com>
> > > ---
> > >  testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)

> > > diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> > > index 7c5e71c4e..bb568264c 100644
> > > --- a/testcases/kernel/syscalls/unshare/unshare03.c
> > > +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> > > @@ -24,7 +24,7 @@

> > >  static void run(void)
> > >  {
> > > -	int nr_open;
> > > +	int rlim_max;
> > >  	int nr_limit;
> > >  	struct rlimit rlimit;
> > >  	struct tst_clone_args args = {
> > > @@ -32,14 +32,12 @@ static void run(void)
> > >  		.exit_signal = SIGCHLD,
> > >  	};

> > > -	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> > > -	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
> > > +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > > +	rlim_max = rlimit.rlim_max;

> > > -	nr_limit = nr_open + NR_OPEN_LIMIT;
> > > +	nr_limit = rlim_max + NR_OPEN_LIMIT;
> > >  	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);

> > > -	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > > -
> > >  	rlimit.rlim_cur = nr_limit;
> > >  	rlimit.rlim_max = nr_limit;

> > > @@ -47,10 +45,10 @@ static void run(void)
> > >  	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
> > >  		nr_limit);

> > > -	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
> > > +	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);

> > >  	if (!SAFE_CLONE(&args)) {
> > > -		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> > > +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
> > >  		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
> > >  		exit(0);
> > >  	}
Cyril Hrubis April 1, 2025, 10:11 a.m. UTC | #3
Hi!
> I think it's safer to set NOFILE increasing from soft limit than from
> hard limit.
> 
> Hard limit may lead to dup2 ENOMEM error which bring the result to
> TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
> limit in /proc/sys/fs/nr_open come out to be 1073741816)
> 
> Signed-off-by: lufei <lufei@uniontech.com>
> ---
>  testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> index 7c5e71c4e..bb568264c 100644
> --- a/testcases/kernel/syscalls/unshare/unshare03.c
> +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> @@ -24,7 +24,7 @@
>  
>  static void run(void)
>  {
> -	int nr_open;
> +	int rlim_max;
>  	int nr_limit;
>  	struct rlimit rlimit;
>  	struct tst_clone_args args = {
> @@ -32,14 +32,12 @@ static void run(void)
>  		.exit_signal = SIGCHLD,
>  	};
>  
> -	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> -	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
> +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> +	rlim_max = rlimit.rlim_max;
>  
> -	nr_limit = nr_open + NR_OPEN_LIMIT;
> +	nr_limit = rlim_max + NR_OPEN_LIMIT;
>  	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);
>  
> -	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> -
>  	rlimit.rlim_cur = nr_limit;
>  	rlimit.rlim_max = nr_limit;
>  
> @@ -47,10 +45,10 @@ static void run(void)
>  	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
>  		nr_limit);
>  
> -	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
> +	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);
>  
>  	if (!SAFE_CLONE(&args)) {
> -		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
>  		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
>  		exit(0);
>  	}

Why do we bother with reading the /rpoc/sys/fs/nr_open file? All that we
need to to do is to dup() a file descriptor and tnen set the nr_open
limit to fd - 2. And if we do so we can drop the rlimit that increases
the limit so that it's greater than nr_open as well.
Petr Vorel April 1, 2025, 12:54 p.m. UTC | #4
> Hi!
> > I think it's safer to set NOFILE increasing from soft limit than from
> > hard limit.

> > Hard limit may lead to dup2 ENOMEM error which bring the result to
> > TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
> > limit in /proc/sys/fs/nr_open come out to be 1073741816)

> > Signed-off-by: lufei <lufei@uniontech.com>
> > ---
> >  testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)

> > diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> > index 7c5e71c4e..bb568264c 100644
> > --- a/testcases/kernel/syscalls/unshare/unshare03.c
> > +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> > @@ -24,7 +24,7 @@

> >  static void run(void)
> >  {
> > -	int nr_open;
> > +	int rlim_max;
> >  	int nr_limit;
> >  	struct rlimit rlimit;
> >  	struct tst_clone_args args = {
> > @@ -32,14 +32,12 @@ static void run(void)
> >  		.exit_signal = SIGCHLD,
> >  	};

> > -	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> > -	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
> > +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > +	rlim_max = rlimit.rlim_max;

> > -	nr_limit = nr_open + NR_OPEN_LIMIT;
> > +	nr_limit = rlim_max + NR_OPEN_LIMIT;
> >  	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);

> > -	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > -
> >  	rlimit.rlim_cur = nr_limit;
> >  	rlimit.rlim_max = nr_limit;

> > @@ -47,10 +45,10 @@ static void run(void)
> >  	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
> >  		nr_limit);

> > -	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
> > +	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);

> >  	if (!SAFE_CLONE(&args)) {
> > -		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> > +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
> >  		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
> >  		exit(0);
> >  	}

> Why do we bother with reading the /rpoc/sys/fs/nr_open file? All that we
> need to to do is to dup() a file descriptor and tnen set the nr_open
> limit to fd - 2. And if we do so we can drop the rlimit that increases
> the limit so that it's greater than nr_open as well.

IMHO file descriptor will be 3, fd - 2 == 1. And trying to set 1 to
/rpoc/sys/fs/nr_open leads to EINVAL. You probably mean something different.

The test is based on unshare_test.c [1] from Al Viro (611fbeb44a777 [2]). Both
tests IMHO use nr_open + 1024 nr_open + 1024 and then call dup2() on nr_open + 64
to avoid later EINVAL when writing /rpoc/sys/fs/nr_open.

Kind regards,
Petr

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/core/unshare_test.c
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=611fbeb44a777e5ab54ab3127ec85f72147911d8
Cyril Hrubis April 1, 2025, 1:55 p.m. UTC | #5
Hi!
> IMHO file descriptor will be 3, fd - 2 == 1. And trying to set 1 to
> /rpoc/sys/fs/nr_open leads to EINVAL. You probably mean something different.

Ah, we cannot set nr_open to anything that is smaller than BITS_PER_LONG:

...
unsigned int sysctl_nr_open __read_mostly = 1024*1024;
unsigned int sysctl_nr_open_min = BITS_PER_LONG;
/* our min() is unusable in constant expressions ;-/ */
#define __const_min(x, y) ((x) < (y) ? (x) : (y))
unsigned int sysctl_nr_open_max =
        __const_min(INT_MAX, ~(size_t)0/sizeof(void *)) & -BITS_PER_LONG;
...

Then we need a filedescriptor that is >= 64 and set the nr_open to 64.
lufei April 9, 2025, 6:41 a.m. UTC | #6
Hi, Cyril

Thanks very much for the smart suggestion. I'll submit a v2 patch.

On Tue, Apr 01, 2025 at 12:11:21PM +0200, Cyril Hrubis wrote:
> Hi!
> > I think it's safer to set NOFILE increasing from soft limit than from
> > hard limit.
> > 
> > Hard limit may lead to dup2 ENOMEM error which bring the result to
> > TBROK on little memory machine. (e.g. 2GB memory in my situation, hard
> > limit in /proc/sys/fs/nr_open come out to be 1073741816)
> > 
> > Signed-off-by: lufei <lufei@uniontech.com>
> > ---
> >  testcases/kernel/syscalls/unshare/unshare03.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
> > index 7c5e71c4e..bb568264c 100644
> > --- a/testcases/kernel/syscalls/unshare/unshare03.c
> > +++ b/testcases/kernel/syscalls/unshare/unshare03.c
> > @@ -24,7 +24,7 @@
> >  
> >  static void run(void)
> >  {
> > -	int nr_open;
> > +	int rlim_max;
> >  	int nr_limit;
> >  	struct rlimit rlimit;
> >  	struct tst_clone_args args = {
> > @@ -32,14 +32,12 @@ static void run(void)
> >  		.exit_signal = SIGCHLD,
> >  	};
> >  
> > -	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
> > -	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
> > +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > +	rlim_max = rlimit.rlim_max;
> >  
> > -	nr_limit = nr_open + NR_OPEN_LIMIT;
> > +	nr_limit = rlim_max + NR_OPEN_LIMIT;
> >  	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);
> >  
> > -	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
> > -
> >  	rlimit.rlim_cur = nr_limit;
> >  	rlimit.rlim_max = nr_limit;
> >  
> > @@ -47,10 +45,10 @@ static void run(void)
> >  	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
> >  		nr_limit);
> >  
> > -	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
> > +	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);
> >  
> >  	if (!SAFE_CLONE(&args)) {
> > -		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
> > +		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
> >  		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
> >  		exit(0);
> >  	}
> 
> Why do we bother with reading the /rpoc/sys/fs/nr_open file? All that we
> need to to do is to dup() a file descriptor and tnen set the nr_open
> limit to fd - 2. And if we do so we can drop the rlimit that increases
> the limit so that it's greater than nr_open as well.
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
>
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/unshare/unshare03.c b/testcases/kernel/syscalls/unshare/unshare03.c
index 7c5e71c4e..bb568264c 100644
--- a/testcases/kernel/syscalls/unshare/unshare03.c
+++ b/testcases/kernel/syscalls/unshare/unshare03.c
@@ -24,7 +24,7 @@ 
 
 static void run(void)
 {
-	int nr_open;
+	int rlim_max;
 	int nr_limit;
 	struct rlimit rlimit;
 	struct tst_clone_args args = {
@@ -32,14 +32,12 @@  static void run(void)
 		.exit_signal = SIGCHLD,
 	};
 
-	SAFE_FILE_SCANF(FS_NR_OPEN, "%d", &nr_open);
-	tst_res(TDEBUG, "Maximum number of file descriptors: %d", nr_open);
+	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
+	rlim_max = rlimit.rlim_max;
 
-	nr_limit = nr_open + NR_OPEN_LIMIT;
+	nr_limit = rlim_max + NR_OPEN_LIMIT;
 	SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_limit);
 
-	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlimit);
-
 	rlimit.rlim_cur = nr_limit;
 	rlimit.rlim_max = nr_limit;
 
@@ -47,10 +45,10 @@  static void run(void)
 	tst_res(TDEBUG, "Set new maximum number of file descriptors to : %d",
 		nr_limit);
 
-	SAFE_DUP2(2, nr_open + NR_OPEN_DUP);
+	SAFE_DUP2(2, rlim_max + NR_OPEN_DUP);
 
 	if (!SAFE_CLONE(&args)) {
-		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", nr_open);
+		SAFE_FILE_PRINTF(FS_NR_OPEN, "%d", rlim_max);
 		TST_EXP_FAIL(unshare(CLONE_FILES), EMFILE);
 		exit(0);
 	}