[PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Michal Nazarewicz]

Since Linux 4.13, kernel limits the maximum command line arguments
length to 6 MiB.¹  Normally the limit is still quarter of the maximum
stack size but if that limit exceeds 6 MiB it’s clamped down.

glibc’s __sysconf implementation for Linux platform is not aware of
this limitation and for stack sizes of over 24 MiB it returns higher
ARG_MAX than Linux will actually accept.  This can be verified by
executing the following application on Linux 4.13 or newer:

    #include <stdio.h>
    #include <string.h>
    #include <sys/resource.h>
    #include <sys/time.h>
    #include <unistd.h>

    int main(void) {
            const struct rlimit rlim = { 40 * 1024 * 1024,
                                         40 * 1024 * 1024 };
            if (setrlimit(RLIMIT_STACK, &rlim) < 0) {
                    perror("setrlimit: RLIMIT_STACK");
                    return 1;
            }

            printf("ARG_MAX     : %8ld\n", sysconf(_SC_ARG_MAX));
            printf("63 * 100 KiB: %8ld\n", 63L * 100 * 1024);
            printf("6 MiB       : %8ld\n", 6L * 1024 * 1024);

            char str[100 * 1024], *argv[64], *envp[1];
            memset(&str, 'A', sizeof str);
            str[sizeof str - 1] = '\0';
            for (size_t i = 0; i < sizeof argv / sizeof *argv - 1; ++i) {
                    argv[i] = str;
            }
            argv[sizeof argv / sizeof *argv - 1] = envp[0] = 0;

            execve("/bin/true", argv, envp);
            perror("execve");
            return 1;
    }

On affected systems the program will report ARG_MAX as 10 MiB but
despite that executing /bin/true with a bit over 6 MiB of command line
arguments will fail with E2BIG error.  Expected result is that ARG_MAX
is reported as 6 MiB.

Update the __sysconf function to clamp ARG_MAX value to 6 MiB if it
would otherwise exceed it.  This resolves bug #25305 which was market
WONTFIX as suggested solution was to cap ARG_MAX at 128 KiB.

As an aside and point of comparison, bionic (a libc implementation for
Android systems) decided to resolve this issue by always returning 128
KiB ignoring any potential xargs regressions.²

On older kernels this results in returning overly conservative value
but that’s a safer option than being aggressive and returning invalid
value on recent systems.  It’s also worth noting that at this point
all supported Linux releases have the 6 MiB barrier so only someone
running an unsupported kernel version would get incorrectly truncated
result.

¹ See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=da029c11e6b12f321f36dac8771e833b65cec962
² See https://android.googlesource.com/platform/bionic/+/baed51ee3a13dae4b87b11870bdf7f10bdc9efc1
---
 sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
index 366fcef01e..bd711795c7 100644
--- a/sysdeps/unix/sysv/linux/sysconf.c
+++ b/sysdeps/unix/sysv/linux/sysconf.c
@@ -55,7 +55,10 @@ __sysconf (int name)
         struct rlimit rlimit;
         /* Use getrlimit to get the stack limit.  */
         if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
-	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
+	  {
+	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
+	    return MIN (limit, 6 << 10 << 10);
+	  }
 
         return legacy_ARG_MAX;
       }
-- 
2.30.2

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 07/04/2021 12:10, Michal Nazarewicz wrote:
> Since Linux 4.13, kernel limits the maximum command line arguments
> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
> stack size but if that limit exceeds 6 MiB it’s clamped down.
> 
> glibc’s __sysconf implementation for Linux platform is not aware of
> this limitation and for stack sizes of over 24 MiB it returns higher
> ARG_MAX than Linux will actually accept.  This can be verified by
> executing the following application on Linux 4.13 or newer:
> 
>     #include <stdio.h>
>     #include <string.h>
>     #include <sys/resource.h>
>     #include <sys/time.h>
>     #include <unistd.h>
> 
>     int main(void) {
>             const struct rlimit rlim = { 40 * 1024 * 1024,
>                                          40 * 1024 * 1024 };
>             if (setrlimit(RLIMIT_STACK, &rlim) < 0) {
>                     perror("setrlimit: RLIMIT_STACK");
>                     return 1;
>             }
> 
>             printf("ARG_MAX     : %8ld\n", sysconf(_SC_ARG_MAX));
>             printf("63 * 100 KiB: %8ld\n", 63L * 100 * 1024);
>             printf("6 MiB       : %8ld\n", 6L * 1024 * 1024);
> 
>             char str[100 * 1024], *argv[64], *envp[1];
>             memset(&str, 'A', sizeof str);
>             str[sizeof str - 1] = '\0';
>             for (size_t i = 0; i < sizeof argv / sizeof *argv - 1; ++i) {
>                     argv[i] = str;
>             }
>             argv[sizeof argv / sizeof *argv - 1] = envp[0] = 0;
> 
>             execve("/bin/true", argv, envp);
>             perror("execve");
>             return 1;
>     }
> 
> On affected systems the program will report ARG_MAX as 10 MiB but
> despite that executing /bin/true with a bit over 6 MiB of command line
> arguments will fail with E2BIG error.  Expected result is that ARG_MAX
> is reported as 6 MiB.
> 
> Update the __sysconf function to clamp ARG_MAX value to 6 MiB if it
> would otherwise exceed it.  This resolves bug #25305 which was market
> WONTFIX as suggested solution was to cap ARG_MAX at 128 KiB.
> 
> As an aside and point of comparison, bionic (a libc implementation for
> Android systems) decided to resolve this issue by always returning 128
> KiB ignoring any potential xargs regressions.²
> 
> On older kernels this results in returning overly conservative value
> but that’s a safer option than being aggressive and returning invalid
> value on recent systems.  It’s also worth noting that at this point
> all supported Linux releases have the 6 MiB barrier so only someone
> running an unsupported kernel version would get incorrectly truncated
> result.
> 
> ¹ See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=da029c11e6b12f321f36dac8771e833b65cec962
> ² See https://android.googlesource.com/platform/bionic/+/baed51ee3a13dae4b87b11870bdf7f10bdc9efc1

It is unfortunate that Linux did not provide a dynamic way to obtain
this value, specially since it has changed over versions.  So we will
need to continue using heuristics. 

And I am not sure by what Carlos has stated in last comment from 
BZ#25305 since the only information we have is the maximum stack from
getrlimit:

| You need to implement dynamic detection of the usable value between 
| the two extreme values depending on your application requirements
| and kernel behaviour. There simply isn't a portable way to optimize 
| for the largest value.

I don't see how we can't really get the maximum upper limit with current
kernel interfaces unless we parse the kernel version and start to mimic
the kernel heuristics (which has its own issues).

And I am not sure if this characterizes as a performance regression
for process like xargs: it seems to use a conservative value for
internal arg_max (131Kb) and limits to about 2MB (1/4 of current 8MB
stack limit):

$ ./xargs/xargs --show-limits
Your environment variables take up 4599 bytes
POSIX upper limit on argument length (this system): 2090505
POSIX smallest allowable upper limit on argument length (all systems): 4096
Maximum length of command we could actually use: 2085906
Size of command buffer we are actually using: 131072
Maximum parallelism (--max-procs must be no greater): 2147483647

It would be a problem only uses start to play with -s option, but
I don't see a better way to handle.  Same for programs that 
tries to use large _SC_MAX_ARG, with current behavior they will
need to handle possible E2BIG from execve on newer kernels.

Carlos and Florian, what possible issues do you see by limiting
_SC_MAX_ARG to what kernel now expects?  IMHO this should align
with what Linux supports now.

> ---
>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
> index 366fcef01e..bd711795c7 100644
> --- a/sysdeps/unix/sysv/linux/sysconf.c
> +++ b/sysdeps/unix/sysv/linux/sysconf.c
> @@ -55,7 +55,10 @@ __sysconf (int name)
>          struct rlimit rlimit;
>          /* Use getrlimit to get the stack limit.  */
>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +	  {
> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +	    return MIN (limit, 6 << 10 << 10);
> +	  }
>  
>          return legacy_ARG_MAX;
>        }
> 

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Florian Weimer]

* Adhemerval Zanella:

> It is unfortunate that Linux did not provide a dynamic way to obtain
> this value, specially since it has changed over versions.  So we will
> need to continue using heuristics. 

We can do a binary search using a test binary, perhaps the dynamic
linker itself.  We can probe a few known values first to speed things
up, I guess.

It's hideous, but given that the kernel doesn't support us here, what
else can we do?

Thanks,
Florian

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 07/04/2021 15:41, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> It is unfortunate that Linux did not provide a dynamic way to obtain
>> this value, specially since it has changed over versions.  So we will
>> need to continue using heuristics. 
> 
> We can do a binary search using a test binary, perhaps the dynamic
> linker itself.  We can probe a few known values first to speed things
> up, I guess.
> 
> It's hideous, but given that the kernel doesn't support us here, what
> else can we do?

IMHO being conservative and use the lower bound of all supported kernels.
I really don't think trying to be smart here with dynamic probing
will add much, specially since this upper limit is what kernel will
support from now on.

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Florian Weimer]

* Adhemerval Zanella:

> IMHO being conservative and use the lower bound of all supported kernels.
> I really don't think trying to be smart here with dynamic probing
> will add much, specially since this upper limit is what kernel will
> support from now on.

Ah, if the conservative choice does not penalize newer kernels, then I
guess that's okay as well.

Then the argument goes like this: If you want us to make the limit
dynamic, add something to the auxiliary vector. 8-)

Thanks,
Florian

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 07/04/2021 16:04, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> IMHO being conservative and use the lower bound of all supported kernels.
>> I really don't think trying to be smart here with dynamic probing
>> will add much, specially since this upper limit is what kernel will
>> support from now on.
> 
> Ah, if the conservative choice does not penalize newer kernels, then I
> guess that's okay as well.

My understanding is newer kernel are more restrictive since they limit
maximum argument plus environment size to up to 6MB, different than older 
kernels that have a higher bar to up 1/4 of maximum stack size. 

So I take you don't oppose to the patch.

> 
> Then the argument goes like this: If you want us to make the limit
> dynamic, add something to the auxiliary vector. 8-)

I am not sure if kernel will be willing to make this dynamic, at least
it seems not be an issue.

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Florian Weimer]

* Adhemerval Zanella:

> On 07/04/2021 16:04, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> IMHO being conservative and use the lower bound of all supported kernels.
>>> I really don't think trying to be smart here with dynamic probing
>>> will add much, specially since this upper limit is what kernel will
>>> support from now on.
>> 
>> Ah, if the conservative choice does not penalize newer kernels, then I
>> guess that's okay as well.
>
> My understanding is newer kernel are more restrictive since they limit
> maximum argument plus environment size to up to 6MB, different than older 
> kernels that have a higher bar to up 1/4 of maximum stack size. 
>
> So I take you don't oppose to the patch.

No, I don't opposite it.  I haven't reviewed it, either.
(Sorry, this week has been a mess so far.)

Florian

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Michal Nazarewicz]

>> * Adhemerval Zanella:
>>> IMHO being conservative and use the lower bound of all supported kernels.
>>> I really don't think trying to be smart here with dynamic probing
>>> will add much, specially since this upper limit is what kernel will
>>> support from now on.

> On 07/04/2021 16:04, Florian Weimer wrote:
>> Ah, if the conservative choice does not penalize newer kernels, then I
>> guess that's okay as well.

Indeed; the 6 MiB bound penalises old unsupported kernels.  Barring
Linux changing things more, the calculation in this patch is what kernel
is using going forward.

On Wed, Apr 07 2021, Adhemerval Zanella wrote:
> My understanding is newer kernel are more restrictive since they limit
> maximum argument plus environment size to up to 6MB, different than older 
> kernels that have a higher bar to up 1/4 of maximum stack size.

Specifically, Linux’s limit is:

    clamp(128 KiB, max stack size / 4, 6 MiB)

so stack size still plays a role but only if it’s in 0.5–24 MiB range.
Outside of that range one of the static bounds is used.

> So I take you don't oppose to the patch.

>> Then the argument goes like this: If you want us to make the limit
>> dynamic, add something to the auxiliary vector. 8-)

> I am not sure if kernel will be willing to make this dynamic, at least
> it seems not be an issue.

I can’t read Linus’ head but I think you’re right; it seems the attitude
in LKML is to use 128 KiB and be done with it.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 07/04/2021 12:10, Michal Nazarewicz wrote:
> Since Linux 4.13, kernel limits the maximum command line arguments
> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
> stack size but if that limit exceeds 6 MiB it’s clamped down.
> 
> glibc’s __sysconf implementation for Linux platform is not aware of
> this limitation and for stack sizes of over 24 MiB it returns higher
> ARG_MAX than Linux will actually accept.  This can be verified by
> executing the following application on Linux 4.13 or newer:
> 
>     #include <stdio.h>
>     #include <string.h>
>     #include <sys/resource.h>
>     #include <sys/time.h>
>     #include <unistd.h>
> 
>     int main(void) {
>             const struct rlimit rlim = { 40 * 1024 * 1024,
>                                          40 * 1024 * 1024 };
>             if (setrlimit(RLIMIT_STACK, &rlim) < 0) {
>                     perror("setrlimit: RLIMIT_STACK");
>                     return 1;
>             }
> 
>             printf("ARG_MAX     : %8ld\n", sysconf(_SC_ARG_MAX));
>             printf("63 * 100 KiB: %8ld\n", 63L * 100 * 1024);
>             printf("6 MiB       : %8ld\n", 6L * 1024 * 1024);
> 
>             char str[100 * 1024], *argv[64], *envp[1];
>             memset(&str, 'A', sizeof str);
>             str[sizeof str - 1] = '\0';
>             for (size_t i = 0; i < sizeof argv / sizeof *argv - 1; ++i) {
>                     argv[i] = str;
>             }
>             argv[sizeof argv / sizeof *argv - 1] = envp[0] = 0;
> 
>             execve("/bin/true", argv, envp);
>             perror("execve");
>             return 1;
>     }
> 
> On affected systems the program will report ARG_MAX as 10 MiB but
> despite that executing /bin/true with a bit over 6 MiB of command line
> arguments will fail with E2BIG error.  Expected result is that ARG_MAX
> is reported as 6 MiB.
> 
> Update the __sysconf function to clamp ARG_MAX value to 6 MiB if it
> would otherwise exceed it.  This resolves bug #25305 which was market
> WONTFIX as suggested solution was to cap ARG_MAX at 128 KiB.
> 
> As an aside and point of comparison, bionic (a libc implementation for
> Android systems) decided to resolve this issue by always returning 128
> KiB ignoring any potential xargs regressions.²
> 
> On older kernels this results in returning overly conservative value
> but that’s a safer option than being aggressive and returning invalid
> value on recent systems.  It’s also worth noting that at this point
> all supported Linux releases have the 6 MiB barrier so only someone
> running an unsupported kernel version would get incorrectly truncated
> result.
> 
> ¹ See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=da029c11e6b12f321f36dac8771e833b65cec962
> ² See https://android.googlesource.com/platform/bionic/+/baed51ee3a13dae4b87b11870bdf7f10bdc9efc1

Patch look good to me, thanks.

Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
> index 366fcef01e..bd711795c7 100644
> --- a/sysdeps/unix/sysv/linux/sysconf.c
> +++ b/sysdeps/unix/sysv/linux/sysconf.c
> @@ -55,7 +55,10 @@ __sysconf (int name)
>          struct rlimit rlimit;
>          /* Use getrlimit to get the stack limit.  */
>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +	  {
> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +	    return MIN (limit, 6 << 10 << 10);

I think it a bit easier to read with the value expanded (6291456).

> +	  }
>  
>          return legacy_ARG_MAX;
>        }
> 

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Michal Nazarewicz]

> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>> Since Linux 4.13, kernel limits the maximum command line arguments
>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>> stack size but if that limit exceeds 6 MiB it’s clamped down.

On Mon, Apr 12 2021, Adhemerval Zanella wrote:
> Patch look good to me, thanks.
>
> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>
>> ---
>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>> index 366fcef01e..bd711795c7 100644
>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>          struct rlimit rlimit;
>>          /* Use getrlimit to get the stack limit.  */
>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>> +	  {
>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>> +	    return MIN (limit, 6 << 10 << 10);
>
> I think it a bit easier to read with the value expanded (6291456).

I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.

>> +	  }
>>  
>>          return legacy_ARG_MAX;
>>        }
>> 

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 12/04/2021 18:31, Michal Nazarewicz wrote:
>> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>>> Since Linux 4.13, kernel limits the maximum command line arguments
>>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>>> stack size but if that limit exceeds 6 MiB it’s clamped down.
> 
> On Mon, Apr 12 2021, Adhemerval Zanella wrote:
>> Patch look good to me, thanks.
>>
>> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>>> index 366fcef01e..bd711795c7 100644
>>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>>          struct rlimit rlimit;
>>>          /* Use getrlimit to get the stack limit.  */
>>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>> +	  {
>>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>> +	    return MIN (limit, 6 << 10 << 10);
>>
>> I think it a bit easier to read with the value expanded (6291456).
> 
> I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.

Alright, I will change it and commit for you.

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Andreas Schwab]

On Apr 12 2021, Michal Nazarewicz wrote:

>> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>>> Since Linux 4.13, kernel limits the maximum command line arguments
>>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>>> stack size but if that limit exceeds 6 MiB it’s clamped down.
>
> On Mon, Apr 12 2021, Adhemerval Zanella wrote:
>> Patch look good to me, thanks.
>>
>> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>>> index 366fcef01e..bd711795c7 100644
>>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>>          struct rlimit rlimit;
>>>          /* Use getrlimit to get the stack limit.  */
>>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>> +	  {
>>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>> +	    return MIN (limit, 6 << 10 << 10);
>>
>> I think it a bit easier to read with the value expanded (6291456).
>
> I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.

In addition, I'd give it a symbolic name with a comment.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 13/04/2021 09:13, Andreas Schwab wrote:
> On Apr 12 2021, Michal Nazarewicz wrote:
> 
>>> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>>>> Since Linux 4.13, kernel limits the maximum command line arguments
>>>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>>>> stack size but if that limit exceeds 6 MiB it’s clamped down.
>>
>> On Mon, Apr 12 2021, Adhemerval Zanella wrote:
>>> Patch look good to me, thanks.
>>>
>>> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>
>>>> ---
>>>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>>>> index 366fcef01e..bd711795c7 100644
>>>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>>>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>>>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>>>          struct rlimit rlimit;
>>>>          /* Use getrlimit to get the stack limit.  */
>>>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>>>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>> +	  {
>>>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>> +	    return MIN (limit, 6 << 10 << 10);
>>>
>>> I think it a bit easier to read with the value expanded (6291456).
>>
>> I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.
> 
> In addition, I'd give it a symbolic name with a comment.

What about:

diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
index 366fcef01e..aceedfa87c 100644
--- a/sysdeps/unix/sysv/linux/sysconf.c
+++ b/sysdeps/unix/sysv/linux/sysconf.c
@@ -33,6 +33,9 @@
    actual value varies based on the stack size.  */
 #define legacy_ARG_MAX 131072
 
+/* Newer kernels (4.13) limit the maximum command line arguments lengths to
+   6MiB.  */
+#define maximum_ARG_MAX 6291456
 
 static long int posix_sysconf (int name);
 
@@ -55,7 +58,10 @@ __sysconf (int name)
         struct rlimit rlimit;
         /* Use getrlimit to get the stack limit.  */
         if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
-         return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
+         {
+           const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
+           return MIN (limit, maximum_ARG_MAX);
+         }
 
         return legacy_ARG_MAX;
       }

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Andreas Schwab]

On Apr 13 2021, Adhemerval Zanella wrote:

> On 13/04/2021 09:13, Andreas Schwab wrote:
>> On Apr 12 2021, Michal Nazarewicz wrote:
>> 
>>>> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>>>>> Since Linux 4.13, kernel limits the maximum command line arguments
>>>>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>>>>> stack size but if that limit exceeds 6 MiB it’s clamped down.
>>>
>>> On Mon, Apr 12 2021, Adhemerval Zanella wrote:
>>>> Patch look good to me, thanks.
>>>>
>>>> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>>
>>>>> ---
>>>>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>>>>> index 366fcef01e..bd711795c7 100644
>>>>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>>>>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>>>>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>>>>          struct rlimit rlimit;
>>>>>          /* Use getrlimit to get the stack limit.  */
>>>>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>>>>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>>> +	  {
>>>>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>>> +	    return MIN (limit, 6 << 10 << 10);
>>>>
>>>> I think it a bit easier to read with the value expanded (6291456).
>>>
>>> I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.
>> 
>> In addition, I'd give it a symbolic name with a comment.
>
> What about:
>
> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
> index 366fcef01e..aceedfa87c 100644
> --- a/sysdeps/unix/sysv/linux/sysconf.c
> +++ b/sysdeps/unix/sysv/linux/sysconf.c
> @@ -33,6 +33,9 @@
>     actual value varies based on the stack size.  */
>  #define legacy_ARG_MAX 131072
>  
> +/* Newer kernels (4.13) limit the maximum command line arguments lengths to
> +   6MiB.  */
> +#define maximum_ARG_MAX 6291456

I'd still use 6 * 1024 * 1024 here, small numbers are much easier to
grok.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 13/04/2021 11:41, Andreas Schwab wrote:
> On Apr 13 2021, Adhemerval Zanella wrote:
> 
>> On 13/04/2021 09:13, Andreas Schwab wrote:
>>> On Apr 12 2021, Michal Nazarewicz wrote:
>>>
>>>>> On 07/04/2021 12:10, Michal Nazarewicz wrote:
>>>>>> Since Linux 4.13, kernel limits the maximum command line arguments
>>>>>> length to 6 MiB.¹  Normally the limit is still quarter of the maximum
>>>>>> stack size but if that limit exceeds 6 MiB it’s clamped down.
>>>>
>>>> On Mon, Apr 12 2021, Adhemerval Zanella wrote:
>>>>> Patch look good to me, thanks.
>>>>>
>>>>> Reviewed=by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>>>
>>>>>> ---
>>>>>>  sysdeps/unix/sysv/linux/sysconf.c | 5 ++++-
>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>>>>>> index 366fcef01e..bd711795c7 100644
>>>>>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>>>>>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>>>>>> @@ -55,7 +55,10 @@ __sysconf (int name)
>>>>>>          struct rlimit rlimit;
>>>>>>          /* Use getrlimit to get the stack limit.  */
>>>>>>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
>>>>>> -	  return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>>>> +	  {
>>>>>> +	    const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
>>>>>> +	    return MIN (limit, 6 << 10 << 10);
>>>>>
>>>>> I think it a bit easier to read with the value expanded (6291456).
>>>>
>>>> I’d rather go with ‘6 * 1024 * 1024’ if shifts aren’t readable.
>>>
>>> In addition, I'd give it a symbolic name with a comment.
>>
>> What about:
>>
>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>> index 366fcef01e..aceedfa87c 100644
>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>> @@ -33,6 +33,9 @@
>>     actual value varies based on the stack size.  */
>>  #define legacy_ARG_MAX 131072
>>  
>> +/* Newer kernels (4.13) limit the maximum command line arguments lengths to
>> +   6MiB.  */
>> +#define maximum_ARG_MAX 6291456
> 
> I'd still use 6 * 1024 * 1024 here, small numbers are much easier to
> grok.

Ack.

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Michal Nazarewicz]

> On 13/04/2021 09:13, Andreas Schwab wrote:
>> In addition, I'd give it a symbolic name with a comment.

On Tue, Apr 13 2021, Adhemerval Zanella wrote:
> What about:
>
> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
> index 366fcef01e..aceedfa87c 100644
> --- a/sysdeps/unix/sysv/linux/sysconf.c
> +++ b/sysdeps/unix/sysv/linux/sysconf.c
> @@ -33,6 +33,9 @@
>     actual value varies based on the stack size.  */
>  #define legacy_ARG_MAX 131072
>  
> +/* Newer kernels (4.13) limit the maximum command line arguments lengths to
> +   6MiB.  */
> +#define maximum_ARG_MAX 6291456

I’d still go with (6 * 1024 * 1024).  Otherwise looks good to me.

>  static long int posix_sysconf (int name);
>  
> @@ -55,7 +58,10 @@ __sysconf (int name)
>          struct rlimit rlimit;
>          /* Use getrlimit to get the stack limit.  */
>          if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
> -         return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +         {
> +           const long int limit = MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
> +           return MIN (limit, maximum_ARG_MAX);
> +         }
>  
>          return legacy_ARG_MAX;
>        }

On Tue, Apr 13 2021, Adhemerval Zanella wrote:
> Alright, I will change it and commit for you.

Thanks.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] linux: sysconf: limit _SC_MAX_ARG to 6 MiB [BZ #25305]

[Adhemerval Zanella]

On 13/04/2021 16:57, Michal Nazarewicz wrote:
>> On 13/04/2021 09:13, Andreas Schwab wrote:
>>> In addition, I'd give it a symbolic name with a comment.
> 
> On Tue, Apr 13 2021, Adhemerval Zanella wrote:
>> What about:
>>
>> diff --git a/sysdeps/unix/sysv/linux/sysconf.c b/sysdeps/unix/sysv/linux/sysconf.c
>> index 366fcef01e..aceedfa87c 100644
>> --- a/sysdeps/unix/sysv/linux/sysconf.c
>> +++ b/sysdeps/unix/sysv/linux/sysconf.c
>> @@ -33,6 +33,9 @@
>>     actual value varies based on the stack size.  */
>>  #define legacy_ARG_MAX 131072
>>  
>> +/* Newer kernels (4.13) limit the maximum command line arguments lengths to
>> +   6MiB.  */
>> +#define maximum_ARG_MAX 6291456
> 
> I’d still go with (6 * 1024 * 1024).  Otherwise looks good to me.

Ok, I fixed it upstream (I forgot to do it when I pushed it earlier).