| Message ID | 20171230184441.25392-2-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
| Series | Alpha: Fix getrlimit/setrlimit with RLIM_INFINITY | expand |
On 30/12/2017 16:44, Aurelien Jarno wrote: > RLIM64_INFINITY was supposed to be a glibc convention rather than > anything seen by the kernel, but it ended being passed to the kernel > through the prlimit64 syscall. > > * On the kernel side, the value is defined for the prlimit64 syscall for > all architectures in include/uapi/linux/resource.h: > > #define RLIM64_INFINITY (~0ULL) > > * On the kernel side, the value is defined for getrlimit and setrlimit > in arch/alpha/include/uapi/asm/resource.h > > #define RLIM_INFINITY 0x7ffffffffffffffful > > * On the GNU libc side, the value is defined in > sysdeps/unix/sysv/linux/alpha/bits/resource.h: > > # define RLIM64_INFINITY 0x7fffffffffffffffLL > > This was not an issue until the getrlimit and setrlimit glibc functions > have been changed in commit 045c13d185 ("Consolidate Linux setrlimit and > getrlimit implementation") to use the prlimit64 syscall instead of the > getrlimit and setrlimit ones. > > This patch fixes that by adding a wrapper to fix the value passed to or > received from the kernel, before or after calling the prlimit64 syscall. Sigh, at least seem only alpha still defines RLIMIT64_INFINITY as 0x7ffffffffffffffful. I did see some failures related to 'allocatestack.c:480' you pointed out (basically all the rt/* ones from 2.26 release info), however the issue only triggers when the tests are ran from make with either make check or make <folder>/tests. If I ran the tests directly using 'testrun.sh' the same one succeeds. Anyway, do you think you can came up with a regression tests for this issues? Otherwise change LGTM [1] https://sourceware.org/glibc/wiki/Release/2.26 Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Changelog: > [BZ #22648] > * sysdeps/unix/sysv/linux/alpha/getrlimit64.c: New file. > * sysdeps/unix/sysv/linux/alpha/setrlimit64.c: Ditto. > --- > ChangeLog | 6 +++ > sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 64 +++++++++++++++++++++++++++++ > sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 61 +++++++++++++++++++++++++++ > 3 files changed, 131 insertions(+) > create mode 100644 sysdeps/unix/sysv/linux/alpha/getrlimit64.c > create mode 100644 sysdeps/unix/sysv/linux/alpha/setrlimit64.c > > diff --git a/ChangeLog b/ChangeLog > index 26e5f96f5d..433b10145d 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,9 @@ > +2017-12-30 Aurelien Jarno <aurelien@aurel32.net> > + > + [BZ #22648] > + * sysdeps/unix/sysv/linux/alpha/getrlimit64.c: New file. > + * sysdeps/unix/sysv/linux/alpha/setrlimit64.c: Ditto. > + > 2017-12-29 Dmitry V. Levin <ldv@altlinux.org> > > [BZ #22433] > diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c > new file mode 100644 > index 0000000000..ad398a136f > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c > @@ -0,0 +1,64 @@ > +/* Copyright (C) 2017 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <sys/types.h> > + > +/* Add this redirection so the strong_alias linking getrlimit64 to > + {__}getrlimit does not throw a type error. */ > +#undef getrlimit > +#undef __getrlimit > +#define getrlimit getrlimit_redirect > +#define __getrlimit __getrlimit_redirect > +#include <sys/resource.h> > +#undef getrlimit > +#undef __getrlimit > + > +/* RLIM64_INFINITY was supposed to be a glibc convention rather than > + anything seen by the kernel, but it ended being passed to the kernel > + through the prlimit64 syscall. Given that a lot of binaries with > + the wrong constant value are in the wild, provide a wrapper function > + fixing the value after the syscall. */ > +#define KERNEL_RLIM64_INFINITY 0xffffffffffffffffULL > + > +int > +__getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits) > +{ > + struct rlimit64 krlimits; > + > + if (INLINE_SYSCALL_CALL (prlimit64, 0, resource, NULL, &krlimits) < 0) > + return -1; > + > + if (krlimits.rlim_cur == KERNEL_RLIM64_INFINITY) > + rlimits->rlim_cur = RLIM64_INFINITY; > + else > + rlimits->rlim_cur = krlimits.rlim_cur; > + if (krlimits.rlim_max == KERNEL_RLIM64_INFINITY) > + rlimits->rlim_max = RLIM64_INFINITY; > + else > + rlimits->rlim_max = krlimits.rlim_max; > + > + return 0; > +} > +libc_hidden_def (__getrlimit64) > +strong_alias (__getrlimit64, __GI_getrlimit) > +strong_alias (__getrlimit64, __GI___getrlimit) > +strong_alias (__getrlimit64, __getrlimit) > +weak_alias (__getrlimit64, getrlimit) > + > +weak_alias (__getrlimit64, getrlimit64) > +libc_hidden_weak (getrlimit64) Ok. > diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c > new file mode 100644 > index 0000000000..a5f7907afd > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c > @@ -0,0 +1,61 @@ > +/* Copyright (C) 2013-2017 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <sys/types.h> > + > +/* Add this redirection so the strong_alias linking setrlimit64 to > + {__}setrlimit does not throw a type error. */ > +#undef setrlimit > +#undef __setrlimit > +#define setrlimit setrlimit_redirect > +#define __setrlimit __setrlimit_redirect > +#include <sys/resource.h> > +#undef setrlimit > +#undef __setrlimit > + > +/* RLIM64_INFINITY was supposed to be a glibc convention rather than > + anything seen by the kernel, but it ended being passed to the kernel > + through the prlimit64 syscall. Given that a lot of binaries with > + the wrong constant value are in the wild, provide a wrapper function > + fixing the value before the syscall. */ > +#define KERNEL_RLIM64_INFINITY 0xffffffffffffffffULL > + > +int > +__setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits) > +{ > + struct rlimit64 krlimits; > + > + if (rlimits->rlim_cur == RLIM64_INFINITY) > + krlimits.rlim_cur = KERNEL_RLIM64_INFINITY; > + else > + krlimits.rlim_cur = rlimits->rlim_cur; > + if (rlimits->rlim_max == RLIM64_INFINITY) > + krlimits.rlim_max = KERNEL_RLIM64_INFINITY; > + else > + krlimits.rlim_max = rlimits->rlim_max; > + > + return INLINE_SYSCALL_CALL (prlimit64, 0, resource, &krlimits, NULL); > +} > + > +weak_alias (__setrlimit64, setrlimit64) > + > +strong_alias (__setrlimit64, __setrlimit) > +weak_alias (__setrlimit64, setrlimit) > +#ifdef SHARED > +__hidden_ver1 (__setrlimit64, __GI___setrlimit, __setrlimit64); > +#endif > Ok.
On Sun, 31 Dec 2017, Adhemerval Zanella wrote: > however the issue only triggers when the tests are ran from make with > either make check or make <folder>/tests. If I ran the tests directly > using 'testrun.sh' the same one succeeds. Maybe this is the issue with some versions of GNU make unlimiting the stack and applying that unlimited stack to subprocesses? Any test that relies on properties of the stack limit needs to explicitly limit or unlimit the stack itself.
On 2017-12-31 17:33, Adhemerval Zanella wrote: > On 30/12/2017 16:44, Aurelien Jarno wrote: > > RLIM64_INFINITY was supposed to be a glibc convention rather than > > anything seen by the kernel, but it ended being passed to the kernel > > through the prlimit64 syscall. > > > > * On the kernel side, the value is defined for the prlimit64 syscall for > > all architectures in include/uapi/linux/resource.h: > > > > #define RLIM64_INFINITY (~0ULL) > > > > * On the kernel side, the value is defined for getrlimit and setrlimit > > in arch/alpha/include/uapi/asm/resource.h > > > > #define RLIM_INFINITY 0x7ffffffffffffffful > > > > * On the GNU libc side, the value is defined in > > sysdeps/unix/sysv/linux/alpha/bits/resource.h: > > > > # define RLIM64_INFINITY 0x7fffffffffffffffLL > > > > This was not an issue until the getrlimit and setrlimit glibc functions > > have been changed in commit 045c13d185 ("Consolidate Linux setrlimit and > > getrlimit implementation") to use the prlimit64 syscall instead of the > > getrlimit and setrlimit ones. > > > > This patch fixes that by adding a wrapper to fix the value passed to or > > received from the kernel, before or after calling the prlimit64 syscall. > > Sigh, at least seem only alpha still defines RLIMIT64_INFINITY as > 0x7ffffffffffffffful. I did see some failures related to 'allocatestack.c:480' > you pointed out (basically all the rt/* ones from 2.26 release info), > however the issue only triggers when the tests are ran from make with > either make check or make <folder>/tests. If I ran the tests directly > using 'testrun.sh' the same one succeeds. > > Anyway, do you think you can came up with a regression tests for this > issues? Otherwise change LGTM As long as getrlimit and setrlimit behave the same way, it's difficult to do so. I guess it's possible to call prlimit directly and compare the values with getrlimit and setrlimit. Such a test will fail for architectures which have a different representation of RLIM_INFINITY in the kernel and the libc, so it is somehow dependent on patch 2 (assuming that alpha is the last concerned architecture, which seems to be the case). Aurelien
On 12/30/2017 10:44 AM, Aurelien Jarno wrote: > RLIM64_INFINITY was supposed to be a glibc convention rather than > anything seen by the kernel, but it ended being passed to the kernel > through the prlimit64 syscall. > > * On the kernel side, the value is defined for the prlimit64 syscall for > all architectures in include/uapi/linux/resource.h: > > #define RLIM64_INFINITY (~0ULL) > > * On the kernel side, the value is defined for getrlimit and setrlimit > in arch/alpha/include/uapi/asm/resource.h > > #define RLIM_INFINITY 0x7ffffffffffffffful However, do_prlimit still uses RLIM_INFINITY, so I'm a bit confused about how this change is supposed to make any difference. Even if it did, surely the best solution is not to map __{old}_getrlimit64 to the prlimit64 syscall, but to the original getrlimit syscall so that userland need do no mapping at all of the return value structure. r~
On 2018-01-01 15:00, Richard Henderson wrote: > On 12/30/2017 10:44 AM, Aurelien Jarno wrote: > > RLIM64_INFINITY was supposed to be a glibc convention rather than > > anything seen by the kernel, but it ended being passed to the kernel > > through the prlimit64 syscall. > > > > * On the kernel side, the value is defined for the prlimit64 syscall for > > all architectures in include/uapi/linux/resource.h: > > > > #define RLIM64_INFINITY (~0ULL) > > > > * On the kernel side, the value is defined for getrlimit and setrlimit > > in arch/alpha/include/uapi/asm/resource.h > > > > #define RLIM_INFINITY 0x7ffffffffffffffful > > However, do_prlimit still uses RLIM_INFINITY, so I'm a bit confused about how > this change is supposed to make any difference. The prlimit64 syscall does the conversion between the two variants of RLIM_INFINITY, through the rlim_to_rlim64 and rlim64_to_rlim functions. Therefore depending if the getrlimit/setrlimit or the prlimit64 syscalls are used, the RLIM_INFINITY value is different. > Even if it did, surely the best solution is not to map __{old}_getrlimit64 to > the prlimit64 syscall, but to the original getrlimit syscall so that userland > need do no mapping at all of the return value structure. This has two issues: - The RLIM_INFINITY value defined on the glibc side is correct for the getrlimit/setrlimit functions, but not for the prlimit one which is also provided by the GNU libc. The goal of the second patch is actually to change this value to fix that issue. - Using different syscalls depending on the architecture causes issues when seccomp is being used, and requires additional whitelisting for them. It's something that is usually not well done for non-mainstream architectures like alpha. This has caused issue for example in systemd (not sure about alpha in particular though). Aurelien
On 01/02/2018 01:09 AM, Aurelien Jarno wrote: > - Using different syscalls depending on the architecture causes issues > when seccomp is being used, and requires additional whitelisting for > them. It's something that is usually not well done for non-mainstream > architectures like alpha. This has caused issue for example in systemd > (not sure about alpha in particular though). Huh. I hadn't considered that issue. The patch series looks fine, then. r~
diff --git a/ChangeLog b/ChangeLog index 26e5f96f5d..433b10145d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2017-12-30 Aurelien Jarno <aurelien@aurel32.net> + + [BZ #22648] + * sysdeps/unix/sysv/linux/alpha/getrlimit64.c: New file. + * sysdeps/unix/sysv/linux/alpha/setrlimit64.c: Ditto. + 2017-12-29 Dmitry V. Levin <ldv@altlinux.org> [BZ #22433] diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c new file mode 100644 index 0000000000..ad398a136f --- /dev/null +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c @@ -0,0 +1,64 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <sys/types.h> + +/* Add this redirection so the strong_alias linking getrlimit64 to + {__}getrlimit does not throw a type error. */ +#undef getrlimit +#undef __getrlimit +#define getrlimit getrlimit_redirect +#define __getrlimit __getrlimit_redirect +#include <sys/resource.h> +#undef getrlimit +#undef __getrlimit + +/* RLIM64_INFINITY was supposed to be a glibc convention rather than + anything seen by the kernel, but it ended being passed to the kernel + through the prlimit64 syscall. Given that a lot of binaries with + the wrong constant value are in the wild, provide a wrapper function + fixing the value after the syscall. */ +#define KERNEL_RLIM64_INFINITY 0xffffffffffffffffULL + +int +__getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits) +{ + struct rlimit64 krlimits; + + if (INLINE_SYSCALL_CALL (prlimit64, 0, resource, NULL, &krlimits) < 0) + return -1; + + if (krlimits.rlim_cur == KERNEL_RLIM64_INFINITY) + rlimits->rlim_cur = RLIM64_INFINITY; + else + rlimits->rlim_cur = krlimits.rlim_cur; + if (krlimits.rlim_max == KERNEL_RLIM64_INFINITY) + rlimits->rlim_max = RLIM64_INFINITY; + else + rlimits->rlim_max = krlimits.rlim_max; + + return 0; +} +libc_hidden_def (__getrlimit64) +strong_alias (__getrlimit64, __GI_getrlimit) +strong_alias (__getrlimit64, __GI___getrlimit) +strong_alias (__getrlimit64, __getrlimit) +weak_alias (__getrlimit64, getrlimit) + +weak_alias (__getrlimit64, getrlimit64) +libc_hidden_weak (getrlimit64) diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c new file mode 100644 index 0000000000..a5f7907afd --- /dev/null +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c @@ -0,0 +1,61 @@ +/* Copyright (C) 2013-2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <sys/types.h> + +/* Add this redirection so the strong_alias linking setrlimit64 to + {__}setrlimit does not throw a type error. */ +#undef setrlimit +#undef __setrlimit +#define setrlimit setrlimit_redirect +#define __setrlimit __setrlimit_redirect +#include <sys/resource.h> +#undef setrlimit +#undef __setrlimit + +/* RLIM64_INFINITY was supposed to be a glibc convention rather than + anything seen by the kernel, but it ended being passed to the kernel + through the prlimit64 syscall. Given that a lot of binaries with + the wrong constant value are in the wild, provide a wrapper function + fixing the value before the syscall. */ +#define KERNEL_RLIM64_INFINITY 0xffffffffffffffffULL + +int +__setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits) +{ + struct rlimit64 krlimits; + + if (rlimits->rlim_cur == RLIM64_INFINITY) + krlimits.rlim_cur = KERNEL_RLIM64_INFINITY; + else + krlimits.rlim_cur = rlimits->rlim_cur; + if (rlimits->rlim_max == RLIM64_INFINITY) + krlimits.rlim_max = KERNEL_RLIM64_INFINITY; + else + krlimits.rlim_max = rlimits->rlim_max; + + return INLINE_SYSCALL_CALL (prlimit64, 0, resource, &krlimits, NULL); +} + +weak_alias (__setrlimit64, setrlimit64) + +strong_alias (__setrlimit64, __setrlimit) +weak_alias (__setrlimit64, setrlimit) +#ifdef SHARED +__hidden_ver1 (__setrlimit64, __GI___setrlimit, __setrlimit64); +#endif
RLIM64_INFINITY was supposed to be a glibc convention rather than anything seen by the kernel, but it ended being passed to the kernel through the prlimit64 syscall. * On the kernel side, the value is defined for the prlimit64 syscall for all architectures in include/uapi/linux/resource.h: #define RLIM64_INFINITY (~0ULL) * On the kernel side, the value is defined for getrlimit and setrlimit in arch/alpha/include/uapi/asm/resource.h #define RLIM_INFINITY 0x7ffffffffffffffful * On the GNU libc side, the value is defined in sysdeps/unix/sysv/linux/alpha/bits/resource.h: # define RLIM64_INFINITY 0x7fffffffffffffffLL This was not an issue until the getrlimit and setrlimit glibc functions have been changed in commit 045c13d185 ("Consolidate Linux setrlimit and getrlimit implementation") to use the prlimit64 syscall instead of the getrlimit and setrlimit ones. This patch fixes that by adding a wrapper to fix the value passed to or received from the kernel, before or after calling the prlimit64 syscall. Changelog: [BZ #22648] * sysdeps/unix/sysv/linux/alpha/getrlimit64.c: New file. * sysdeps/unix/sysv/linux/alpha/setrlimit64.c: Ditto. --- ChangeLog | 6 +++ sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 64 +++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 61 +++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100644 sysdeps/unix/sysv/linux/alpha/getrlimit64.c create mode 100644 sysdeps/unix/sysv/linux/alpha/setrlimit64.c