| Message ID | 1529530993-20897-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 06ab719d30b01da401150068054d3b8ea93dd12f |
| Headers | show |
| Series | [v4] Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251) | expand |
On Wed, 20 Jun 2018, Adhemerval Zanella wrote: > +#include <shlib-compat.h> > +#if TEST_COMPAT (libc, GLIBC_2_0, GLIBC_2_28) > +compat_symbol_reference (libc, fcntl, fcntl, GLIBC_2_0); > +#endif I'd expect the entire test to be under such a TEST_COMPAT conditional (returning 77 for UNSUPPORTED otherwise) - for a new port without older symbol versions, no such compat version with compat semantics is available for testing. -- Joseph S. Myers joseph@codesourcery.com
On 20/06/2018 18:49, Joseph Myers wrote: > On Wed, 20 Jun 2018, Adhemerval Zanella wrote: > >> +#include <shlib-compat.h> >> +#if TEST_COMPAT (libc, GLIBC_2_0, GLIBC_2_28) >> +compat_symbol_reference (libc, fcntl, fcntl, GLIBC_2_0); >> +#endif > > I'd expect the entire test to be under such a TEST_COMPAT conditional > (returning 77 for UNSUPPORTED otherwise) - for a new port without older > symbol versions, no such compat version with compat semantics is available > for testing. > Right, I forgot about news ports. I have adjusted it locally. Is this patch ok to apply with this change?
On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: > diff --git a/sysdeps/unix/sysv/linux/fcntl.c b/sysdeps/unix/sysv/linux/fcntl.c > index e3992dc..0e37ed0 100644 > --- a/sysdeps/unix/sysv/linux/fcntl.c > +++ b/sysdeps/unix/sysv/linux/fcntl.c > @@ -20,15 +20,12 @@ > #include <stdarg.h> > #include <errno.h> > #include <sysdep-cancel.h> > -#include <not-cancel.h> > > -#ifndef __NR_fcntl64 > -# define __NR_fcntl64 __NR_fcntl > -#endif > +#ifndef __OFF_T_MATCHES_OFF64_T > > -#ifndef FCNTL_ADJUST_CMD > -# define FCNTL_ADJUST_CMD(__cmd) __cmd > -#endif > +# ifndef FCNTL_ADJUST_CMD > +# define FCNTL_ADJUST_CMD(__cmd) __cmd > +# endif > > int > __libc_fcntl (int fd, int cmd, ...) > @@ -42,13 +39,83 @@ __libc_fcntl (int fd, int cmd, ...) > > cmd = FCNTL_ADJUST_CMD (cmd); > > - if (cmd == F_SETLKW || cmd == F_SETLKW64) > - return SYSCALL_CANCEL (fcntl64, fd, cmd, (void *) arg); > - > - return __fcntl_nocancel_adjusted (fd, cmd, arg); > + switch (cmd) > + { > + case F_SETLKW: > + case F_SETLKW64: > + return SYSCALL_CANCEL (fcntl64, fd, cmd, arg); > + case F_OFD_SETLKW: > + { > + struct flock *flk = (struct flock *) arg; > + struct flock64 flk64 = > + { > + .l_type = flk->l_type, > + .l_whence = flk->l_whence, > + .l_start = flk->l_start, > + .l_len = flk->l_len, > + .l_pid = flk->l_pid > + }; > + return SYSCALL_CANCEL (fcntl64, fd, cmd, &flk64); > + } > + case F_OFD_GETLK: > + case F_OFD_SETLK: > + { > + struct flock *flk = (struct flock *) arg; > + struct flock64 flk64 = > + { > + .l_type = flk->l_type, > + .l_whence = flk->l_whence, > + .l_start = flk->l_start, > + .l_len = flk->l_len, > + .l_pid = flk->l_pid > + }; > + int ret = INLINE_SYSCALL_CALL (fcntl64, fd, cmd, &flk64); > + if (ret == -1) > + return -1; > + if ((off_t) flk64.l_start != flk64.l_start > + || (off_t) flk64.l_len != flk64.l_len) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + flk->l_type = flk64.l_type; > + flk->l_whence = flk64.l_whence; > + flk->l_start = flk64.l_start; > + flk->l_len = flk64.l_len; > + flk->l_pid = flk64.l_pid; > + return ret; > + } > + /* case F_OFD_GETLK: > + case F_OFD_GETLK64: > + case F_SETLK64: > + case F_GETOWN: */ > + default: > + return __fcntl64_nocancel_adjusted (fd, cmd, arg); > + } > } The comment before the default case looks wrong to me. F_OFD_GETLK is duplicated. Maybe add comments for the cases where mapping is not needed, explaining why. > libc_hidden_def (__libc_fcntl) > > weak_alias (__libc_fcntl, __fcntl) > libc_hidden_weak (__fcntl) > + > +# include <shlib-compat.h> > +# if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_28) > +int > +__old_libc_fcntl64 (int fd, int cmd, ...) > +{ > + va_list ap; > + void *arg; > + > + va_start (ap, cmd); > + arg = va_arg (ap, void *); > + va_end (ap); > + > + return __libc_fcntl64 (fd, cmd, arg); > +} > +compat_symbol (libc, __old_libc_fcntl64, fcntl, GLIBC_2_0); This should have a comment why you call it __old_libc_fcntl64, when there never was a fcntl64 before. > +versioned_symbol (libc, __libc_fcntl, fcntl, GLIBC_2_28); Doesn't this create a strong symbol, leading to static link namespace issues? > +# else > weak_alias (__libc_fcntl, fcntl) Here' it's a weak symbol. Thanks, Florian
On 22/06/2018 07:13, Florian Weimer wrote: > On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >> diff --git a/sysdeps/unix/sysv/linux/fcntl.c b/sysdeps/unix/sysv/linux/fcntl.c >> index e3992dc..0e37ed0 100644 >> --- a/sysdeps/unix/sysv/linux/fcntl.c >> +++ b/sysdeps/unix/sysv/linux/fcntl.c >> @@ -20,15 +20,12 @@ >> #include <stdarg.h> >> #include <errno.h> >> #include <sysdep-cancel.h> >> -#include <not-cancel.h> >> -#ifndef __NR_fcntl64 >> -# define __NR_fcntl64 __NR_fcntl >> -#endif >> +#ifndef __OFF_T_MATCHES_OFF64_T >> -#ifndef FCNTL_ADJUST_CMD >> -# define FCNTL_ADJUST_CMD(__cmd) __cmd >> -#endif >> +# ifndef FCNTL_ADJUST_CMD >> +# define FCNTL_ADJUST_CMD(__cmd) __cmd >> +# endif >> int >> __libc_fcntl (int fd, int cmd, ...) >> @@ -42,13 +39,83 @@ __libc_fcntl (int fd, int cmd, ...) >> cmd = FCNTL_ADJUST_CMD (cmd); >> - if (cmd == F_SETLKW || cmd == F_SETLKW64) >> - return SYSCALL_CANCEL (fcntl64, fd, cmd, (void *) arg); >> - >> - return __fcntl_nocancel_adjusted (fd, cmd, arg); >> + switch (cmd) >> + { >> + case F_SETLKW: >> + case F_SETLKW64: >> + return SYSCALL_CANCEL (fcntl64, fd, cmd, arg); >> + case F_OFD_SETLKW: >> + { >> + struct flock *flk = (struct flock *) arg; >> + struct flock64 flk64 = >> + { >> + .l_type = flk->l_type, >> + .l_whence = flk->l_whence, >> + .l_start = flk->l_start, >> + .l_len = flk->l_len, >> + .l_pid = flk->l_pid >> + }; >> + return SYSCALL_CANCEL (fcntl64, fd, cmd, &flk64); >> + } >> + case F_OFD_GETLK: >> + case F_OFD_SETLK: >> + { >> + struct flock *flk = (struct flock *) arg; >> + struct flock64 flk64 = >> + { >> + .l_type = flk->l_type, >> + .l_whence = flk->l_whence, >> + .l_start = flk->l_start, >> + .l_len = flk->l_len, >> + .l_pid = flk->l_pid >> + }; >> + int ret = INLINE_SYSCALL_CALL (fcntl64, fd, cmd, &flk64); >> + if (ret == -1) >> + return -1; >> + if ((off_t) flk64.l_start != flk64.l_start >> + || (off_t) flk64.l_len != flk64.l_len) >> + { >> + __set_errno (EOVERFLOW); >> + return -1; >> + } >> + flk->l_type = flk64.l_type; >> + flk->l_whence = flk64.l_whence; >> + flk->l_start = flk64.l_start; >> + flk->l_len = flk64.l_len; >> + flk->l_pid = flk64.l_pid; >> + return ret; >> + } >> + /* case F_OFD_GETLK: >> + case F_OFD_GETLK64: >> + case F_SETLK64: >> + case F_GETOWN: */ >> + default: >> + return __fcntl64_nocancel_adjusted (fd, cmd, arg); >> + } >> } > > The comment before the default case looks wrong to me. F_OFD_GETLK is duplicated. Maybe add comments for the cases where mapping is not needed, explaining why. I changed to: /* Since only F_SETLKW{64}/F_OLD_SETLK are cancellation entrypoints and only OFD locks requires LFS handling, all others flags are handled unmodified by calling __NR_fcntl64. */ > >> libc_hidden_def (__libc_fcntl) >> weak_alias (__libc_fcntl, __fcntl) >> libc_hidden_weak (__fcntl) >> + >> +# include <shlib-compat.h> >> +# if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_28) >> +int >> +__old_libc_fcntl64 (int fd, int cmd, ...) >> +{ >> + va_list ap; >> + void *arg; >> + >> + va_start (ap, cmd); >> + arg = va_arg (ap, void *); >> + va_end (ap); >> + >> + return __libc_fcntl64 (fd, cmd, arg); >> +} > +compat_symbol (libc, __old_libc_fcntl64, fcntl, GLIBC_2_0); > > This should have a comment why you call it __old_libc_fcntl64, when there never was a fcntl64 before. I added. /* Previous versions called __NR_fcntl64 for fcntl (which do not handle OFD locks in LFS mode). */ > >> +versioned_symbol (libc, __libc_fcntl, fcntl, GLIBC_2_28); > > Doesn't this create a strong symbol, leading to static link namespace issues? The SHLIB_COMPAT takes care to avoid this in static objects. > >> +# else >> weak_alias (__libc_fcntl, fcntl) > > Here' it's a weak symbol. > > Thanks, > Florian
On Fri, 22 Jun 2018, Florian Weimer wrote: > > +versioned_symbol (libc, __libc_fcntl, fcntl, GLIBC_2_28); > > Doesn't this create a strong symbol, leading to static link namespace issues? For static linking, versioned_symbol is mapped to weak_alias. -- Joseph S. Myers joseph@codesourcery.com
On 06/22/2018 02:20 PM, Adhemerval Zanella wrote: >>> + /* case F_OFD_GETLK: >>> + case F_OFD_GETLK64: >>> + case F_SETLK64: >>> + case F_GETOWN: */ >>> + default: >>> + return __fcntl64_nocancel_adjusted (fd, cmd, arg); >>> + } >>> } >> >> The comment before the default case looks wrong to me. F_OFD_GETLK is duplicated. Maybe add comments for the cases where mapping is not needed, explaining why. > > I changed to: > > /* Since only F_SETLKW{64}/F_OLD_SETLK are cancellation entrypoints and > only OFD locks requires LFS handling, all others flags are handled > unmodified by calling __NR_fcntl64. */ Thanks. I think it should read “only OFD locks require LFS handling”. >>> +# include <shlib-compat.h> >>> +# if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_28) >>> +int >>> +__old_libc_fcntl64 (int fd, int cmd, ...) >>> +{ >>> + va_list ap; >>> + void *arg; >>> + >>> + va_start (ap, cmd); >>> + arg = va_arg (ap, void *); >>> + va_end (ap); >>> + >>> + return __libc_fcntl64 (fd, cmd, arg); >>> +} > +compat_symbol (libc, __old_libc_fcntl64, fcntl, GLIBC_2_0); >> >> This should have a comment why you call it __old_libc_fcntl64, when there never was a fcntl64 before. > > I added. > > /* Previous versions called __NR_fcntl64 for fcntl (which do not handle > OFD locks in LFS mode). */ “which did not handle”? Florian
On 26/06/2018 11:45, Florian Weimer wrote: > On 06/22/2018 02:20 PM, Adhemerval Zanella wrote: >>>> + /* case F_OFD_GETLK: >>>> + case F_OFD_GETLK64: >>>> + case F_SETLK64: >>>> + case F_GETOWN: */ >>>> + default: >>>> + return __fcntl64_nocancel_adjusted (fd, cmd, arg); >>>> + } >>>> } >>> >>> The comment before the default case looks wrong to me. F_OFD_GETLK is duplicated. Maybe add comments for the cases where mapping is not needed, explaining why. >> >> I changed to: >> >> /* Since only F_SETLKW{64}/F_OLD_SETLK are cancellation entrypoints and >> only OFD locks requires LFS handling, all others flags are handled >> unmodified by calling __NR_fcntl64. */ > > Thanks. I think it should read “only OFD locks require LFS handling”. Fixed. > >>>> +# include <shlib-compat.h> >>>> +# if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_28) >>>> +int >>>> +__old_libc_fcntl64 (int fd, int cmd, ...) >>>> +{ >>>> + va_list ap; >>>> + void *arg; >>>> + >>>> + va_start (ap, cmd); >>>> + arg = va_arg (ap, void *); >>>> + va_end (ap); >>>> + >>>> + return __libc_fcntl64 (fd, cmd, arg); >>>> +} > +compat_symbol (libc, __old_libc_fcntl64, fcntl, GLIBC_2_0); >>> >>> This should have a comment why you call it __old_libc_fcntl64, when there never was a fcntl64 before. >> >> I added. >> >> /* Previous versions called __NR_fcntl64 for fcntl (which do not handle >> OFD locks in LFS mode). */ > > “which did not handle”? > Indeed, fixed. I will push it shortly.
On Tue, 26 Jun 2018, Adhemerval Zanella wrote:
> I will push it shortly.
The pushed commit includes a dubious change to
sysdeps/mach/hurd/bits/errno.h.
Maybe parts of that change are correct in that they are in fact the
results of regenerating that file, but if regenerating that file involves
inserting a line saying
"/home/azanella/Projects/glibc/build/i686-gnu/errnos.d", there is clearly
a bug in the generation process; generated checked-in files should never
depend on individual build paths like that.
--
Joseph S. Myers
joseph@codesourcery.com
On 26/06/2018 18:43, Joseph Myers wrote: > On Tue, 26 Jun 2018, Adhemerval Zanella wrote: > >> I will push it shortly. > > The pushed commit includes a dubious change to > sysdeps/mach/hurd/bits/errno.h. > > Maybe parts of that change are correct in that they are in fact the > results of regenerating that file, but if regenerating that file involves > inserting a line saying > "/home/azanella/Projects/glibc/build/i686-gnu/errnos.d", there is clearly > a bug in the generation process; generated checked-in files should never > depend on individual build paths like that. > This is clearly a mistake from my part, I will revert this change. The i686-gnu build seems to change this source file for some reason.
On Tue, 26 Jun 2018, Adhemerval Zanella wrote: > On 26/06/2018 18:43, Joseph Myers wrote: > > On Tue, 26 Jun 2018, Adhemerval Zanella wrote: > > > >> I will push it shortly. > > > > The pushed commit includes a dubious change to > > sysdeps/mach/hurd/bits/errno.h. > > > > Maybe parts of that change are correct in that they are in fact the > > results of regenerating that file, but if regenerating that file involves > > inserting a line saying > > "/home/azanella/Projects/glibc/build/i686-gnu/errnos.d", there is clearly > > a bug in the generation process; generated checked-in files should never > > depend on individual build paths like that. > > > > This is clearly a mistake from my part, I will revert this change. The > i686-gnu build seems to change this source file for some reason. The files in the source tree that may have makefile dependencies on other files in the source tree and so get regenerated during the build include at least configure and configure fragments, some preconfigure fragments, gperf-generated *-kw.h, INSTALL, sysdeps/gnu/errlist.c, sysdeps/mach/hurd/bits/errno.h, posix/testcases.h, posix/ptestcases.h, locale/C-translit.h, sysdeps/sparc/sparc32/{sdiv,udiv,rem,urem}.S. (There may be others I've missed. Files such as po/libc.pot, with makefile dependencies but which don't get referenced during a normal build, aren't relevant for this purpose.) Eventually it would be good for build-many-glibcs.py to touch all such files in fix_glibc_timestamps to ensure that no regeneration in the source tree takes place accidentally during a build (and given verification that this makes a read-only source tree with any timestamp ordering work for all supported configurations, build-many-glibcs.py could then stop copying the glibc source tree at all). Of course that doesn't help so much when not using build-many-glibcs.py; maybe we should have an equivalent of GCC's contrib/gcc_update --touch that people could run during checkout (and then build-many-glibcs.py would just use that). -- Joseph S. Myers joseph@codesourcery.com
On 28/06/2018 09:39, Szabolcs Nagy wrote: > On 20/06/18 22:43, Adhemerval Zanella wrote: >> diff --git a/manual/llio.texi b/manual/llio.texi >> index 82f03be..e840c55 100644 >> --- a/manual/llio.texi >> +++ b/manual/llio.texi >> @@ -3281,12 +3281,13 @@ Set process or process group ID to receive @code{SIGIO} signals. >> @xref{Interrupt Input}. >> @end vtable >> -This function is a cancellation point in multi-threaded programs. This >> -is a problem if the thread allocates some resources (like memory, file >> -descriptors, semaphores or whatever) at the time @code{fcntl} is >> -called. If the thread gets canceled these resources stay allocated >> -until the program ends. To avoid this calls to @code{fcntl} should be >> -protected using cancellation handlers. >> +This function is a cancellation point in multi-threaded programs for the >> +commands @code{F_SETLKW} (and the LFS analogous @code{F_SETLKW64}) and >> +@code {F_OFD_SETLKW}. This is a problem if the thread allocates some > > this broke the build for me on systems with old makeinfo. > committed the patch below as obvious. > > With old makeinfo '@code {' fails because of the extra space. > > 2018-06-28 Szabolcs Nagy <szabolcs.nagy@arm.com> > > * manual/llio.texi: Remove spurious space. Thanks for catching it. > > diff --git a/manual/llio.texi b/manual/llio.texi > index e840c55f86..2733b9cb73 100644 > --- a/manual/llio.texi > +++ b/manual/llio.texi > @@ -3283,7 +3283,7 @@ Set process or process group ID to receive @code{SIGIO} signals. > > This function is a cancellation point in multi-threaded programs for the > commands @code{F_SETLKW} (and the LFS analogous @code{F_SETLKW64}) and > -@code {F_OFD_SETLKW}. This is a problem if the thread allocates some > +@code{F_OFD_SETLKW}. This is a problem if the thread allocates some > resources (like memory, file descriptors, semaphores or whatever) at the time > @code{fcntl} is called. If the thread gets canceled these resources stay > allocated until the program ends. To avoid this calls to @code{fcntl} should
* Adhemerval Zanella <adhemerval.zanella@linaro.org> [2018-06-20 18:43:13 -0300]: > This patch fixes the OFD ("file private") locks for architectures that > support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The > issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and > F_{SET,GET}L{W}K64 expects a flock64 argument and when using old > F_OFD_* flags with a non LFS flock argument the kernel might interpret > the underlying data wrongly. Kernel idea originally was to avoid using > such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS > semantic as default it is possible to provide the functionality and > avoid the bogus struct kernel passing by adjusting the struct manually > for the required flags. > > The idea follows other LFS interfaces that provide two symbols: > > 1. A new LFS fcntl64 is added on default ABI with the usual macros to > select it for FILE_OFFSET_BITS=64. > > 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for > F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided > struct. > > 3. Keep a compat symbol with old broken semantic for architectures > that do not define __OFF_T_MATCHES_OFF64_T. > > So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will > aliased to fcntl and no adjustment would be required. So to actually > use F_OFD_* with LFS support the source must be built with LFS support > (_FILE_OFFSET_BITS=64). > > Also F_OFD_SETLKW command is handled a cancellation point, as for > F_SETLKW{64}. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > build-many-glibcs fails for me on i686 in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see FAIL: elf/check-abi-libc FAIL: elf/check-execstack FAIL: hurd/check-installed-headers-c FAIL: hurd/check-installed-headers-cxx FAIL: mach/check-installed-headers-c FAIL: mach/check-installed-headers-cxx the libc abi failure is --- ../sysdeps/mach/hurd/i386/libc.abilist 2018-06-29 18:53:06.105524353 +0100 +++ /B/build/glibcs/i686-gnu/glibc/libc.symlist 2018-06-30 13:02:53.044456983 +0100 @@ -2036 +2035,0 @@ GLIBC_2.27 wcstof64x_l F -GLIBC_2.28 fcntl F but i don't know if hurd is supposed to work...
On 30/06/2018 09:26, Szabolcs Nagy wrote: > * Adhemerval Zanella <adhemerval.zanella@linaro.org> [2018-06-20 18:43:13 -0300]: >> This patch fixes the OFD ("file private") locks for architectures that >> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >> F_OFD_* flags with a non LFS flock argument the kernel might interpret >> the underlying data wrongly. Kernel idea originally was to avoid using >> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >> semantic as default it is possible to provide the functionality and >> avoid the bogus struct kernel passing by adjusting the struct manually >> for the required flags. >> >> The idea follows other LFS interfaces that provide two symbols: >> >> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >> select it for FILE_OFFSET_BITS=64. >> >> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >> struct. >> >> 3. Keep a compat symbol with old broken semantic for architectures >> that do not define __OFF_T_MATCHES_OFF64_T. >> >> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >> aliased to fcntl and no adjustment would be required. So to actually >> use F_OFD_* with LFS support the source must be built with LFS support >> (_FILE_OFFSET_BITS=64). >> >> Also F_OFD_SETLKW command is handled a cancellation point, as for >> F_SETLKW{64}. >> >> Checked on x86_64-linux-gnu and i686-linux-gnu. >> > > build-many-glibcs fails for me on i686 > in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see > > FAIL: elf/check-abi-libc > FAIL: elf/check-execstack > FAIL: hurd/check-installed-headers-c > FAIL: hurd/check-installed-headers-cxx > FAIL: mach/check-installed-headers-c > FAIL: mach/check-installed-headers-cxx > > the libc abi failure is > > --- ../sysdeps/mach/hurd/i386/libc.abilist 2018-06-29 18:53:06.105524353 +0100 > +++ /B/build/glibcs/i686-gnu/glibc/libc.symlist 2018-06-30 13:02:53.044456983 +0100 > @@ -2036 +2035,0 @@ GLIBC_2.27 wcstof64x_l F > -GLIBC_2.28 fcntl F > > but i don't know if hurd is supposed to work... > I will look into it.
On 07/02/2018 08:44 AM, Adhemerval Zanella wrote: > > > On 30/06/2018 09:26, Szabolcs Nagy wrote: >> * Adhemerval Zanella <adhemerval.zanella@linaro.org> [2018-06-20 18:43:13 -0300]: >>> This patch fixes the OFD ("file private") locks for architectures that >>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>> the underlying data wrongly. Kernel idea originally was to avoid using >>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>> semantic as default it is possible to provide the functionality and >>> avoid the bogus struct kernel passing by adjusting the struct manually >>> for the required flags. >>> >>> The idea follows other LFS interfaces that provide two symbols: >>> >>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>> select it for FILE_OFFSET_BITS=64. >>> >>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>> struct. >>> >>> 3. Keep a compat symbol with old broken semantic for architectures >>> that do not define __OFF_T_MATCHES_OFF64_T. >>> >>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>> aliased to fcntl and no adjustment would be required. So to actually >>> use F_OFD_* with LFS support the source must be built with LFS support >>> (_FILE_OFFSET_BITS=64). >>> >>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>> F_SETLKW{64}. >>> >>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>> >> >> build-many-glibcs fails for me on i686 >> in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see >> >> FAIL: elf/check-abi-libc >> FAIL: elf/check-execstack >> FAIL: hurd/check-installed-headers-c >> FAIL: hurd/check-installed-headers-cxx >> FAIL: mach/check-installed-headers-c >> FAIL: mach/check-installed-headers-cxx >> >> the libc abi failure is >> >> --- ../sysdeps/mach/hurd/i386/libc.abilist 2018-06-29 18:53:06.105524353 +0100 >> +++ /B/build/glibcs/i686-gnu/glibc/libc.symlist 2018-06-30 13:02:53.044456983 +0100 >> @@ -2036 +2035,0 @@ GLIBC_2.27 wcstof64x_l F >> -GLIBC_2.28 fcntl F >> >> but i don't know if hurd is supposed to work... >> > > I will look into it. > If you don't have time to look into this please put this on the release blocker list for 2.28 so we can look into this. Having an ABI failure like this for release would be bad. Thanks! Cheers, Carlos. -- Cheers, Carlos.
On 05/07/2018 16:56, Carlos O'Donell wrote: > On 07/02/2018 08:44 AM, Adhemerval Zanella wrote: >> >> >> On 30/06/2018 09:26, Szabolcs Nagy wrote: >>> * Adhemerval Zanella <adhemerval.zanella@linaro.org> [2018-06-20 18:43:13 -0300]: >>>> This patch fixes the OFD ("file private") locks for architectures that >>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>> semantic as default it is possible to provide the functionality and >>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>> for the required flags. >>>> >>>> The idea follows other LFS interfaces that provide two symbols: >>>> >>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>> select it for FILE_OFFSET_BITS=64. >>>> >>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>> struct. >>>> >>>> 3. Keep a compat symbol with old broken semantic for architectures >>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>> >>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>> aliased to fcntl and no adjustment would be required. So to actually >>>> use F_OFD_* with LFS support the source must be built with LFS support >>>> (_FILE_OFFSET_BITS=64). >>>> >>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>> F_SETLKW{64}. >>>> >>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>> >>> >>> build-many-glibcs fails for me on i686 >>> in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see >>> >>> FAIL: elf/check-abi-libc >>> FAIL: elf/check-execstack >>> FAIL: hurd/check-installed-headers-c >>> FAIL: hurd/check-installed-headers-cxx >>> FAIL: mach/check-installed-headers-c >>> FAIL: mach/check-installed-headers-cxx >>> >>> the libc abi failure is >>> >>> --- ../sysdeps/mach/hurd/i386/libc.abilist 2018-06-29 18:53:06.105524353 +0100 >>> +++ /B/build/glibcs/i686-gnu/glibc/libc.symlist 2018-06-30 13:02:53.044456983 +0100 >>> @@ -2036 +2035,0 @@ GLIBC_2.27 wcstof64x_l F >>> -GLIBC_2.28 fcntl F >>> >>> but i don't know if hurd is supposed to work... >>> >> >> I will look into it. >> > > If you don't have time to look into this please put this on the > release blocker list for 2.28 so we can look into this. > > Having an ABI failure like this for release would be bad. > > Thanks! > > Cheers, > Carlos. > Sorry, I forgot to sent a [committed] message. I already fixed it upstream [1]. [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=7279af007c420a9d5f88a6909d11e7cb712c16a4
On 07/05/2018 03:58 PM, Adhemerval Zanella wrote: > > > On 05/07/2018 16:56, Carlos O'Donell wrote: >> On 07/02/2018 08:44 AM, Adhemerval Zanella wrote: >>> >>> >>> On 30/06/2018 09:26, Szabolcs Nagy wrote: >>>> * Adhemerval Zanella <adhemerval.zanella@linaro.org> [2018-06-20 18:43:13 -0300]: >>>>> This patch fixes the OFD ("file private") locks for architectures that >>>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>>> semantic as default it is possible to provide the functionality and >>>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>>> for the required flags. >>>>> >>>>> The idea follows other LFS interfaces that provide two symbols: >>>>> >>>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>>> select it for FILE_OFFSET_BITS=64. >>>>> >>>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>>> struct. >>>>> >>>>> 3. Keep a compat symbol with old broken semantic for architectures >>>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>>> >>>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>>> aliased to fcntl and no adjustment would be required. So to actually >>>>> use F_OFD_* with LFS support the source must be built with LFS support >>>>> (_FILE_OFFSET_BITS=64). >>>>> >>>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>>> F_SETLKW{64}. >>>>> >>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>> >>>> >>>> build-many-glibcs fails for me on i686 >>>> in logs/glibcs/i686-gnu/010-glibcs-i686-gnu-check-log.txt i see >>>> >>>> FAIL: elf/check-abi-libc >>>> FAIL: elf/check-execstack >>>> FAIL: hurd/check-installed-headers-c >>>> FAIL: hurd/check-installed-headers-cxx >>>> FAIL: mach/check-installed-headers-c >>>> FAIL: mach/check-installed-headers-cxx >>>> >>>> the libc abi failure is >>>> >>>> --- ../sysdeps/mach/hurd/i386/libc.abilist 2018-06-29 18:53:06.105524353 +0100 >>>> +++ /B/build/glibcs/i686-gnu/glibc/libc.symlist 2018-06-30 13:02:53.044456983 +0100 >>>> @@ -2036 +2035,0 @@ GLIBC_2.27 wcstof64x_l F >>>> -GLIBC_2.28 fcntl F >>>> >>>> but i don't know if hurd is supposed to work... >>>> >>> >>> I will look into it. >>> >> >> If you don't have time to look into this please put this on the >> release blocker list for 2.28 so we can look into this. >> >> Having an ABI failure like this for release would be bad. >> >> Thanks! >> >> Cheers, >> Carlos. >> > > Sorry, I forgot to sent a [committed] message. I already fixed it > upstream [1]. > > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=7279af007c420a9d5f88a6909d11e7cb712c16a4 > Thanks. Just trying to cross check what's fixed or not fixed for 2.28. Cheers, Carlos. -- Cheers, Carlos.
On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: > Changes from previous version: > > - Add a testcase for compat fcntl using OFD locks. > > --- > > This patch fixes the OFD ("file private") locks for architectures that > support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The > issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and > F_{SET,GET}L{W}K64 expects a flock64 argument and when using old > F_OFD_* flags with a non LFS flock argument the kernel might interpret > the underlying data wrongly. Kernel idea originally was to avoid using > such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS > semantic as default it is possible to provide the functionality and > avoid the bogus struct kernel passing by adjusting the struct manually > for the required flags. > > The idea follows other LFS interfaces that provide two symbols: > > 1. A new LFS fcntl64 is added on default ABI with the usual macros to > select it for FILE_OFFSET_BITS=64. > > 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for > F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided > struct. > > 3. Keep a compat symbol with old broken semantic for architectures > that do not define __OFF_T_MATCHES_OFF64_T. > > So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will > aliased to fcntl and no adjustment would be required. So to actually > use F_OFD_* with LFS support the source must be built with LFS support > (_FILE_OFFSET_BITS=64). > > Also F_OFD_SETLKW command is handled a cancellation point, as for > F_SETLKW{64}. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > ... Hi Adhemerval, I'm running the new test misc/tst-ofdlocks-compat on s390-32. If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: (gdb) p/x lck $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW In this case, lck is not updated: p/x lck $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. Are the different behaviours related to a change in kernel-code? Bye Stefan
On 06/07/2018 10:00, Stefan Liebler wrote: > On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >> Changes from previous version: >> >> - Add a testcase for compat fcntl using OFD locks. >> >> --- >> >> This patch fixes the OFD ("file private") locks for architectures that >> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >> F_OFD_* flags with a non LFS flock argument the kernel might interpret >> the underlying data wrongly. Kernel idea originally was to avoid using >> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >> semantic as default it is possible to provide the functionality and >> avoid the bogus struct kernel passing by adjusting the struct manually >> for the required flags. >> >> The idea follows other LFS interfaces that provide two symbols: >> >> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >> select it for FILE_OFFSET_BITS=64. >> >> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >> struct. >> >> 3. Keep a compat symbol with old broken semantic for architectures >> that do not define __OFF_T_MATCHES_OFF64_T. >> >> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >> aliased to fcntl and no adjustment would be required. So to actually >> use F_OFD_* with LFS support the source must be built with LFS support >> (_FILE_OFFSET_BITS=64). >> >> Also F_OFD_SETLKW command is handled a cancellation point, as for >> F_SETLKW{64}. >> >> Checked on x86_64-linux-gnu and i686-linux-gnu. >> > ... > > Hi Adhemerval, > > I'm running the new test misc/tst-ofdlocks-compat on s390-32. > If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: > (gdb) p/x lck > $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} > > If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW > In this case, lck is not updated: > p/x lck > $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} > > In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. > > Are the different behaviours related to a change in kernel-code? I think it is due the patch 'fcntl: don't cap l_start and l_end values for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel did: static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, compat_ulong_t arg) [...] case F_GETLK64: case F_OFD_GETLK: err = get_compat_flock64(&flock, compat_ptr(arg)); if (err) break; err = fixup_compat_flock(&flock); if (err) return err; err = put_compat_flock64(&flock, compat_ptr(arg)); break; [....] It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch changed to: [...] case F_GETLK64: case F_OFD_GETLK: err = get_compat_flock64(&flock, compat_ptr(arg)); if (!err) err= put_compat_flock64(&flock, compat_ptr(arg)); break; [....] So if compat fcntl will just return if get_compat_flock64 succeed. Also afaiu only compat syscall is affected by it I think, the generic OFD locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW in the aforementioned case. I am not sure which would be the best way to get around this kernel issue, any suggestion to harden the testcase? It also suggest to me that the possible usercase I assume in my testcase (OFD locks with struct flock64) never really worked on previous releases...
On 06/07/2018 11:09, Adhemerval Zanella wrote: > > > On 06/07/2018 10:00, Stefan Liebler wrote: >> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>> Changes from previous version: >>> >>> - Add a testcase for compat fcntl using OFD locks. >>> >>> --- >>> >>> This patch fixes the OFD ("file private") locks for architectures that >>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>> the underlying data wrongly. Kernel idea originally was to avoid using >>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>> semantic as default it is possible to provide the functionality and >>> avoid the bogus struct kernel passing by adjusting the struct manually >>> for the required flags. >>> >>> The idea follows other LFS interfaces that provide two symbols: >>> >>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>> select it for FILE_OFFSET_BITS=64. >>> >>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>> struct. >>> >>> 3. Keep a compat symbol with old broken semantic for architectures >>> that do not define __OFF_T_MATCHES_OFF64_T. >>> >>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>> aliased to fcntl and no adjustment would be required. So to actually >>> use F_OFD_* with LFS support the source must be built with LFS support >>> (_FILE_OFFSET_BITS=64). >>> >>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>> F_SETLKW{64}. >>> >>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>> >> ... >> >> Hi Adhemerval, >> >> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >> (gdb) p/x lck >> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >> >> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >> In this case, lck is not updated: >> p/x lck >> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >> >> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >> >> Are the different behaviours related to a change in kernel-code? > > I think it is due the patch 'fcntl: don't cap l_start and l_end values > for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb > added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel > did: > > static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, > compat_ulong_t arg) > [...] > case F_GETLK64: > case F_OFD_GETLK: > err = get_compat_flock64(&flock, compat_ptr(arg)); > if (err) > break; > err = fixup_compat_flock(&flock); > if (err) > return err; > err = put_compat_flock64(&flock, compat_ptr(arg)); > break; > [....] > > It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not > fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch > changed to: > > [...] > case F_GETLK64: > case F_OFD_GETLK: > err = get_compat_flock64(&flock, compat_ptr(arg)); > if (!err) > err= put_compat_flock64(&flock, compat_ptr(arg)); > break; > [....] > > So if compat fcntl will just return if get_compat_flock64 succeed. Also > afaiu only compat syscall is affected by it I think, the generic OFD > locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW > in the aforementioned case. > > I am not sure which would be the best way to get around this kernel > issue, any suggestion to harden the testcase? It also suggest to me that > the possible usercase I assume in my testcase (OFD locks with struct > flock64) never really worked on previous releases... > Also, on x86 4.4 where actually tested the kernel OFD locks does: fs/compat.c: [...] case F_GETLK64: case F_SETLK64: case F_SETLKW64: case F_OFD_GETLK: case F_OFD_SETLK: case F_OFD_SETLKW: ret = get_compat_flock64(&f, compat_ptr(arg)); if (ret != 0) break; old_fs = get_fs(); set_fs(KERNEL_DS); conv_cmd = convert_fcntl_cmd(cmd); ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); set_fs(old_fs); if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { /* need to return lock information - see above for commentary */ if (f.l_start > COMPAT_LOFF_T_MAX) ret = -EOVERFLOW; if (f.l_len > COMPAT_LOFF_T_MAX) f.l_len = COMPAT_LOFF_T_MAX; if (ret == 0) ret = put_compat_flock64(&f, compat_ptr(arg)); } break; [...] Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as 0x7fffffffffffffffL for all architectures and l_start/l_len are defined as __kernel_long_t (which for a 64 bits kernel is 'long'). So the tests are not actually checking for overflow. Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX with a correct value (0x7fffffff on x86). It seems to be fixed by 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12).
On 07/06/2018 04:45 PM, Adhemerval Zanella wrote: > > > On 06/07/2018 11:09, Adhemerval Zanella wrote: >> >> >> On 06/07/2018 10:00, Stefan Liebler wrote: >>> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>>> Changes from previous version: >>>> >>>> - Add a testcase for compat fcntl using OFD locks. >>>> >>>> --- >>>> >>>> This patch fixes the OFD ("file private") locks for architectures that >>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>> semantic as default it is possible to provide the functionality and >>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>> for the required flags. >>>> >>>> The idea follows other LFS interfaces that provide two symbols: >>>> >>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>> select it for FILE_OFFSET_BITS=64. >>>> >>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>> struct. >>>> >>>> 3. Keep a compat symbol with old broken semantic for architectures >>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>> >>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>> aliased to fcntl and no adjustment would be required. So to actually >>>> use F_OFD_* with LFS support the source must be built with LFS support >>>> (_FILE_OFFSET_BITS=64). >>>> >>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>> F_SETLKW{64}. >>>> >>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>> >>> ... >>> >>> Hi Adhemerval, >>> >>> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >>> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >>> (gdb) p/x lck >>> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >>> >>> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >>> In this case, lck is not updated: >>> p/x lck >>> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >>> >>> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >>> >>> Are the different behaviours related to a change in kernel-code? >> >> I think it is due the patch 'fcntl: don't cap l_start and l_end values >> for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb >> added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel >> did: >> >> static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, >> compat_ulong_t arg) >> [...] >> case F_GETLK64: >> case F_OFD_GETLK: >> err = get_compat_flock64(&flock, compat_ptr(arg)); >> if (err) >> break; >> err = fixup_compat_flock(&flock); >> if (err) >> return err; >> err = put_compat_flock64(&flock, compat_ptr(arg)); >> break; >> [....] >> >> It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not >> fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch >> changed to: >> >> [...] >> case F_GETLK64: >> case F_OFD_GETLK: >> err = get_compat_flock64(&flock, compat_ptr(arg)); >> if (!err) >> err= put_compat_flock64(&flock, compat_ptr(arg)); >> break; >> [....] >> Yes, this sounds reasonable. It was introduced with commit 'fs/locks: don't mess with the address limit in compat_fcntl64' (94073ad77fff221b5e66b8b9863a546ba212d6a3) >> So if compat fcntl will just return if get_compat_flock64 succeed. Also >> afaiu only compat syscall is affected by it I think, the generic OFD >> locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW >> in the aforementioned case. >> >> I am not sure which would be the best way to get around this kernel >> issue, any suggestion to harden the testcase? It also suggest to me that >> the possible usercase I assume in my testcase (OFD locks with struct >> flock64) never really worked on previous releases... >> > > Also, on x86 4.4 where actually tested the kernel OFD locks does: > > fs/compat.c: > [...] > case F_GETLK64: > case F_SETLK64: > case F_SETLKW64: > case F_OFD_GETLK: > case F_OFD_SETLK: > case F_OFD_SETLKW: > ret = get_compat_flock64(&f, compat_ptr(arg)); > if (ret != 0) > break; > old_fs = get_fs(); > set_fs(KERNEL_DS); > conv_cmd = convert_fcntl_cmd(cmd); > ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); > set_fs(old_fs); > if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { > /* need to return lock information - see above for commentary */ > if (f.l_start > COMPAT_LOFF_T_MAX) > ret = -EOVERFLOW; > if (f.l_len > COMPAT_LOFF_T_MAX) > f.l_len = COMPAT_LOFF_T_MAX; > if (ret == 0) > ret = put_compat_flock64(&f, compat_ptr(arg)); > } > break; > [...] > > Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as > 0x7fffffffffffffffL for all architectures and l_start/l_len are > defined as __kernel_long_t (which for a 64 bits kernel is 'long'). > So the tests are not actually checking for overflow. Yes. They do not test for overflowing 32bit long. But as fcntl64 with command F_OFD_GETLK assumes 64bit long, it is fine? > > Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX > with a correct value (0x7fffffff on x86). It seems to be fixed by > 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12). > Sure? This commit introduces the following implementation and I think those checks are the same as before: COMPAT_SYSCALL_DEFINE3(fcntl64,... ... case F_GETLK: ... if (f.l_start > COMPAT_OFF_T_MAX) ret = -EOVERFLOW; ... case F_OFD_GETLK: ... if (f.l_start > COMPAT_LOFF_T_MAX) ret = -EOVERFLOW; In recent kernels, only the command F_GETLK is using fixup_compat_flock() which checks against COMPAT_OFF_T_MAX. The errno=EOVERFLOW for the second fcntl call in tst-ofdlocks.c is generated by the new glibc check, but not from the kernel. I've got errno=EOVERFLOW with my kernel 4.14 due to the call of fixup_compat_flock. I've also retested it on a system with kernel 4.10. There the kernel does not return with errno=EOVERFLOW.
On 09/07/2018 10:44, Stefan Liebler wrote: > On 07/06/2018 04:45 PM, Adhemerval Zanella wrote: >> >> >> On 06/07/2018 11:09, Adhemerval Zanella wrote: >>> >>> >>> On 06/07/2018 10:00, Stefan Liebler wrote: >>>> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>>>> Changes from previous version: >>>>> >>>>> - Add a testcase for compat fcntl using OFD locks. >>>>> >>>>> --- >>>>> >>>>> This patch fixes the OFD ("file private") locks for architectures that >>>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>>> semantic as default it is possible to provide the functionality and >>>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>>> for the required flags. >>>>> >>>>> The idea follows other LFS interfaces that provide two symbols: >>>>> >>>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>>> select it for FILE_OFFSET_BITS=64. >>>>> >>>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>>> struct. >>>>> >>>>> 3. Keep a compat symbol with old broken semantic for architectures >>>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>>> >>>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>>> aliased to fcntl and no adjustment would be required. So to actually >>>>> use F_OFD_* with LFS support the source must be built with LFS support >>>>> (_FILE_OFFSET_BITS=64). >>>>> >>>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>>> F_SETLKW{64}. >>>>> >>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>> >>>> ... >>>> >>>> Hi Adhemerval, >>>> >>>> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >>>> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >>>> (gdb) p/x lck >>>> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >>>> >>>> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >>>> In this case, lck is not updated: >>>> p/x lck >>>> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >>>> >>>> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >>>> >>>> Are the different behaviours related to a change in kernel-code? >>> >>> I think it is due the patch 'fcntl: don't cap l_start and l_end values >>> for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb >>> added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel >>> did: >>> >>> static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, >>> compat_ulong_t arg) >>> [...] >>> case F_GETLK64: >>> case F_OFD_GETLK: >>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>> if (err) >>> break; >>> err = fixup_compat_flock(&flock); >>> if (err) >>> return err; >>> err = put_compat_flock64(&flock, compat_ptr(arg)); >>> break; >>> [....] >>> >>> It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not >>> fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch >>> changed to: >>> >>> [...] >>> case F_GETLK64: >>> case F_OFD_GETLK: >>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>> if (!err) >>> err= put_compat_flock64(&flock, compat_ptr(arg)); >>> break; >>> [....] >>> > Yes, this sounds reasonable. > It was introduced with commit 'fs/locks: don't mess with the address limit in compat_fcntl64' (94073ad77fff221b5e66b8b9863a546ba212d6a3) > >>> So if compat fcntl will just return if get_compat_flock64 succeed. Also >>> afaiu only compat syscall is affected by it I think, the generic OFD >>> locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW >>> in the aforementioned case. >>> >>> I am not sure which would be the best way to get around this kernel >>> issue, any suggestion to harden the testcase? It also suggest to me that >>> the possible usercase I assume in my testcase (OFD locks with struct >>> flock64) never really worked on previous releases... >>> >> >> Also, on x86 4.4 where actually tested the kernel OFD locks does: >> >> fs/compat.c: >> [...] >> case F_GETLK64: >> case F_SETLK64: >> case F_SETLKW64: >> case F_OFD_GETLK: >> case F_OFD_SETLK: >> case F_OFD_SETLKW: >> ret = get_compat_flock64(&f, compat_ptr(arg)); >> if (ret != 0) >> break; >> old_fs = get_fs(); >> set_fs(KERNEL_DS); >> conv_cmd = convert_fcntl_cmd(cmd); >> ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); >> set_fs(old_fs); >> if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { >> /* need to return lock information - see above for commentary */ >> if (f.l_start > COMPAT_LOFF_T_MAX) >> ret = -EOVERFLOW; >> if (f.l_len > COMPAT_LOFF_T_MAX) >> f.l_len = COMPAT_LOFF_T_MAX; >> if (ret == 0) >> ret = put_compat_flock64(&f, compat_ptr(arg)); >> } >> break; >> [...] >> >> Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as >> 0x7fffffffffffffffL for all architectures and l_start/l_len are >> defined as __kernel_long_t (which for a 64 bits kernel is 'long'). >> So the tests are not actually checking for overflow. > Yes. They do not test for overflowing 32bit long. But as fcntl64 with command F_OFD_GETLK assumes 64bit long, it is fine? >> >> Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX >> with a correct value (0x7fffffff on x86). It seems to be fixed by >> 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12). >> > Sure? This commit introduces the following implementation and I think those checks are the same as before: > COMPAT_SYSCALL_DEFINE3(fcntl64,... Not sure, I just try to see if there a kernel change by inspecting the kernel patches. > ... > case F_GETLK: > ... if (f.l_start > COMPAT_OFF_T_MAX) > ret = -EOVERFLOW; > ... > case F_OFD_GETLK: > ... if (f.l_start > COMPAT_LOFF_T_MAX) > ret = -EOVERFLOW; > > In recent kernels, only the command F_GETLK is using fixup_compat_flock() which checks against COMPAT_OFF_T_MAX. > > The errno=EOVERFLOW for the second fcntl call in tst-ofdlocks.c is generated by the new glibc check, but not from the kernel. This is the expected behaviour, the new fcntl symbol for non default LFS ABI returns EOVERFLOW. > I've got errno=EOVERFLOW with my kernel 4.14 due to the call of fixup_compat_flock. > I've also retested it on a system with kernel 4.10. There the kernel does not return with errno=EOVERFLOW. > Can you verify that for s390-32 tst-ofdlocks-compat.c is a really using the old version (i.e passing the arguments to kernel unmodified)? Otherwise I am not sure if this is a glibc bug.
On 07/09/2018 04:20 PM, Adhemerval Zanella wrote: > > > On 09/07/2018 10:44, Stefan Liebler wrote: >> On 07/06/2018 04:45 PM, Adhemerval Zanella wrote: >>> >>> >>> On 06/07/2018 11:09, Adhemerval Zanella wrote: >>>> >>>> >>>> On 06/07/2018 10:00, Stefan Liebler wrote: >>>>> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>>>>> Changes from previous version: >>>>>> >>>>>> - Add a testcase for compat fcntl using OFD locks. >>>>>> >>>>>> --- >>>>>> >>>>>> This patch fixes the OFD ("file private") locks for architectures that >>>>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>>>> semantic as default it is possible to provide the functionality and >>>>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>>>> for the required flags. >>>>>> >>>>>> The idea follows other LFS interfaces that provide two symbols: >>>>>> >>>>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>>>> select it for FILE_OFFSET_BITS=64. >>>>>> >>>>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>>>> struct. >>>>>> >>>>>> 3. Keep a compat symbol with old broken semantic for architectures >>>>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>>>> >>>>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>>>> aliased to fcntl and no adjustment would be required. So to actually >>>>>> use F_OFD_* with LFS support the source must be built with LFS support >>>>>> (_FILE_OFFSET_BITS=64). >>>>>> >>>>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>>>> F_SETLKW{64}. >>>>>> >>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>>> >>>>> ... >>>>> >>>>> Hi Adhemerval, >>>>> >>>>> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >>>>> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >>>>> (gdb) p/x lck >>>>> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >>>>> >>>>> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >>>>> In this case, lck is not updated: >>>>> p/x lck >>>>> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >>>>> >>>>> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >>>>> >>>>> Are the different behaviours related to a change in kernel-code? >>>> >>>> I think it is due the patch 'fcntl: don't cap l_start and l_end values >>>> for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb >>>> added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel >>>> did: >>>> >>>> static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, >>>> compat_ulong_t arg) >>>> [...] >>>> case F_GETLK64: >>>> case F_OFD_GETLK: >>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>> if (err) >>>> break; >>>> err = fixup_compat_flock(&flock); >>>> if (err) >>>> return err; >>>> err = put_compat_flock64(&flock, compat_ptr(arg)); >>>> break; >>>> [....] >>>> >>>> It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not >>>> fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch >>>> changed to: >>>> >>>> [...] >>>> case F_GETLK64: >>>> case F_OFD_GETLK: >>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>> if (!err) >>>> err= put_compat_flock64(&flock, compat_ptr(arg)); >>>> break; >>>> [....] >>>> >> Yes, this sounds reasonable. >> It was introduced with commit 'fs/locks: don't mess with the address limit in compat_fcntl64' (94073ad77fff221b5e66b8b9863a546ba212d6a3) >> >>>> So if compat fcntl will just return if get_compat_flock64 succeed. Also >>>> afaiu only compat syscall is affected by it I think, the generic OFD >>>> locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW >>>> in the aforementioned case. >>>> >>>> I am not sure which would be the best way to get around this kernel >>>> issue, any suggestion to harden the testcase? It also suggest to me that >>>> the possible usercase I assume in my testcase (OFD locks with struct >>>> flock64) never really worked on previous releases... >>>> >>> >>> Also, on x86 4.4 where actually tested the kernel OFD locks does: >>> >>> fs/compat.c: >>> [...] >>> case F_GETLK64: >>> case F_SETLK64: >>> case F_SETLKW64: >>> case F_OFD_GETLK: >>> case F_OFD_SETLK: >>> case F_OFD_SETLKW: >>> ret = get_compat_flock64(&f, compat_ptr(arg)); >>> if (ret != 0) >>> break; >>> old_fs = get_fs(); >>> set_fs(KERNEL_DS); >>> conv_cmd = convert_fcntl_cmd(cmd); >>> ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); >>> set_fs(old_fs); >>> if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { >>> /* need to return lock information - see above for commentary */ >>> if (f.l_start > COMPAT_LOFF_T_MAX) >>> ret = -EOVERFLOW; >>> if (f.l_len > COMPAT_LOFF_T_MAX) >>> f.l_len = COMPAT_LOFF_T_MAX; >>> if (ret == 0) >>> ret = put_compat_flock64(&f, compat_ptr(arg)); >>> } >>> break; >>> [...] >>> >>> Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as >>> 0x7fffffffffffffffL for all architectures and l_start/l_len are >>> defined as __kernel_long_t (which for a 64 bits kernel is 'long'). >>> So the tests are not actually checking for overflow. >> Yes. They do not test for overflowing 32bit long. But as fcntl64 with command F_OFD_GETLK assumes 64bit long, it is fine? >>> >>> Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX >>> with a correct value (0x7fffffff on x86). It seems to be fixed by >>> 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12). >>> >> Sure? This commit introduces the following implementation and I think those checks are the same as before: >> COMPAT_SYSCALL_DEFINE3(fcntl64,... > > Not sure, I just try to see if there a kernel change by inspecting the > kernel patches. > >> ... >> case F_GETLK: >> ... if (f.l_start > COMPAT_OFF_T_MAX) >> ret = -EOVERFLOW; >> ... >> case F_OFD_GETLK: >> ... if (f.l_start > COMPAT_LOFF_T_MAX) >> ret = -EOVERFLOW; >> >> In recent kernels, only the command F_GETLK is using fixup_compat_flock() which checks against COMPAT_OFF_T_MAX. >> >> The errno=EOVERFLOW for the second fcntl call in tst-ofdlocks.c is generated by the new glibc check, but not from the kernel. > > This is the expected behaviour, the new fcntl symbol for non default LFS > ABI returns EOVERFLOW. > >> I've got errno=EOVERFLOW with my kernel 4.14 due to the call of fixup_compat_flock. >> I've also retested it on a system with kernel 4.10. There the kernel does not return with errno=EOVERFLOW. >> > > Can you verify that for s390-32 tst-ofdlocks-compat.c is a really using the > old version (i.e passing the arguments to kernel unmodified)? Otherwise > I am not sure if this is a glibc bug. > Yes it is. I'm running the s390-32 tst-ofdlocks-compat testcase in 64bit gdb and we are here: 72 TEST_VERIFY (fcntl (fd, F_OFD_GETLK, &lck) == 0); (gdb) p sizeof (lck) $2 = 32 (gdb) p/x lck $3 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} (gdb) x/4gx &lck 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 Stepping to syscall: ... (gdb) where #0 __fcntl64_nocancel_adjusted (fd=fd@entry=4, cmd=cmd@entry=36, arg=arg@entry=0x7fffeae8) at ../sysdeps/unix/sysv/linux/fcntl_nocancel.c:64 #1 0x7df1e076 in __GI___libc_fcntl64 (fd=4, cmd=36) at ../sysdeps/unix/sysv/linux/fcntl64.c:51 #2 0x7df1e006 in __old_libc_fcntl64 (fd=<optimized out>, cmd=<optimized out>) at ../sysdeps/unix/sysv/linux/fcntl.c:114 #3 0x00401a98 in do_test () at ../sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c:72 #4 0x00401f42 in run_test_function (config=0x7fffebe8, config=0x7fffebe8, argv=0x7fffed48, argc=<optimized out>) at support_test_main.c:156 #5 support_test_main (argc=<optimized out>, argv=0x7fffed48, config=config@entry=0x7fffebe8) at support_test_main.c:322 #6 0x004017b0 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:168 We are just before the syscall: >│0x7df25fe0 <__fcntl64_nocancel_adjusted+32> svc 221 (gdb) x/4gx arg 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 (gdb) si (gdb) ir 2 r2 0xffffffffffffffb5 18446744073709551541 (gdb) x/4gx arg 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff 0x7fffeaf8: 0x0000000000001000 0x0000000000000000
On 09/07/2018 11:56, Stefan Liebler wrote: > On 07/09/2018 04:20 PM, Adhemerval Zanella wrote: >> >> >> On 09/07/2018 10:44, Stefan Liebler wrote: >>> On 07/06/2018 04:45 PM, Adhemerval Zanella wrote: >>>> >>>> >>>> On 06/07/2018 11:09, Adhemerval Zanella wrote: >>>>> >>>>> >>>>> On 06/07/2018 10:00, Stefan Liebler wrote: >>>>>> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>>>>>> Changes from previous version: >>>>>>> >>>>>>> - Add a testcase for compat fcntl using OFD locks. >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> This patch fixes the OFD ("file private") locks for architectures that >>>>>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>>>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>>>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>>>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>>>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>>>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>>>>> semantic as default it is possible to provide the functionality and >>>>>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>>>>> for the required flags. >>>>>>> >>>>>>> The idea follows other LFS interfaces that provide two symbols: >>>>>>> >>>>>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>>>>> select it for FILE_OFFSET_BITS=64. >>>>>>> >>>>>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>>>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>>>>> struct. >>>>>>> >>>>>>> 3. Keep a compat symbol with old broken semantic for architectures >>>>>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>>>>> >>>>>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>>>>> aliased to fcntl and no adjustment would be required. So to actually >>>>>>> use F_OFD_* with LFS support the source must be built with LFS support >>>>>>> (_FILE_OFFSET_BITS=64). >>>>>>> >>>>>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>>>>> F_SETLKW{64}. >>>>>>> >>>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>>>> >>>>>> ... >>>>>> >>>>>> Hi Adhemerval, >>>>>> >>>>>> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >>>>>> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >>>>>> (gdb) p/x lck >>>>>> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >>>>>> >>>>>> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >>>>>> In this case, lck is not updated: >>>>>> p/x lck >>>>>> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >>>>>> >>>>>> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >>>>>> >>>>>> Are the different behaviours related to a change in kernel-code? >>>>> >>>>> I think it is due the patch 'fcntl: don't cap l_start and l_end values >>>>> for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb >>>>> added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel >>>>> did: >>>>> >>>>> static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, >>>>> compat_ulong_t arg) >>>>> [...] >>>>> case F_GETLK64: >>>>> case F_OFD_GETLK: >>>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>>> if (err) >>>>> break; >>>>> err = fixup_compat_flock(&flock); >>>>> if (err) >>>>> return err; >>>>> err = put_compat_flock64(&flock, compat_ptr(arg)); >>>>> break; >>>>> [....] >>>>> >>>>> It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not >>>>> fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch >>>>> changed to: >>>>> >>>>> [...] >>>>> case F_GETLK64: >>>>> case F_OFD_GETLK: >>>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>>> if (!err) >>>>> err= put_compat_flock64(&flock, compat_ptr(arg)); >>>>> break; >>>>> [....] >>>>> >>> Yes, this sounds reasonable. >>> It was introduced with commit 'fs/locks: don't mess with the address limit in compat_fcntl64' (94073ad77fff221b5e66b8b9863a546ba212d6a3) >>> >>>>> So if compat fcntl will just return if get_compat_flock64 succeed. Also >>>>> afaiu only compat syscall is affected by it I think, the generic OFD >>>>> locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW >>>>> in the aforementioned case. >>>>> >>>>> I am not sure which would be the best way to get around this kernel >>>>> issue, any suggestion to harden the testcase? It also suggest to me that >>>>> the possible usercase I assume in my testcase (OFD locks with struct >>>>> flock64) never really worked on previous releases... >>>>> >>>> >>>> Also, on x86 4.4 where actually tested the kernel OFD locks does: >>>> >>>> fs/compat.c: >>>> [...] >>>> case F_GETLK64: >>>> case F_SETLK64: >>>> case F_SETLKW64: >>>> case F_OFD_GETLK: >>>> case F_OFD_SETLK: >>>> case F_OFD_SETLKW: >>>> ret = get_compat_flock64(&f, compat_ptr(arg)); >>>> if (ret != 0) >>>> break; >>>> old_fs = get_fs(); >>>> set_fs(KERNEL_DS); >>>> conv_cmd = convert_fcntl_cmd(cmd); >>>> ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); >>>> set_fs(old_fs); >>>> if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { >>>> /* need to return lock information - see above for commentary */ >>>> if (f.l_start > COMPAT_LOFF_T_MAX) >>>> ret = -EOVERFLOW; >>>> if (f.l_len > COMPAT_LOFF_T_MAX) >>>> f.l_len = COMPAT_LOFF_T_MAX; >>>> if (ret == 0) >>>> ret = put_compat_flock64(&f, compat_ptr(arg)); >>>> } >>>> break; >>>> [...] >>>> >>>> Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as >>>> 0x7fffffffffffffffL for all architectures and l_start/l_len are >>>> defined as __kernel_long_t (which for a 64 bits kernel is 'long'). >>>> So the tests are not actually checking for overflow. >>> Yes. They do not test for overflowing 32bit long. But as fcntl64 with command F_OFD_GETLK assumes 64bit long, it is fine? >>>> >>>> Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX >>>> with a correct value (0x7fffffff on x86). It seems to be fixed by >>>> 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12). >>>> >>> Sure? This commit introduces the following implementation and I think those checks are the same as before: >>> COMPAT_SYSCALL_DEFINE3(fcntl64,... >> >> Not sure, I just try to see if there a kernel change by inspecting the >> kernel patches. >> >>> ... >>> case F_GETLK: >>> ... if (f.l_start > COMPAT_OFF_T_MAX) >>> ret = -EOVERFLOW; >>> ... >>> case F_OFD_GETLK: >>> ... if (f.l_start > COMPAT_LOFF_T_MAX) >>> ret = -EOVERFLOW; >>> >>> In recent kernels, only the command F_GETLK is using fixup_compat_flock() which checks against COMPAT_OFF_T_MAX. >>> >>> The errno=EOVERFLOW for the second fcntl call in tst-ofdlocks.c is generated by the new glibc check, but not from the kernel. >> >> This is the expected behaviour, the new fcntl symbol for non default LFS >> ABI returns EOVERFLOW. >> >>> I've got errno=EOVERFLOW with my kernel 4.14 due to the call of fixup_compat_flock. >>> I've also retested it on a system with kernel 4.10. There the kernel does not return with errno=EOVERFLOW. >>> >> >> Can you verify that for s390-32 tst-ofdlocks-compat.c is a really using the >> old version (i.e passing the arguments to kernel unmodified)? Otherwise >> I am not sure if this is a glibc bug. >> > Yes it is. I'm running the s390-32 tst-ofdlocks-compat testcase in 64bit gdb and we are here: > 72 TEST_VERIFY (fcntl (fd, F_OFD_GETLK, &lck) == 0); > > (gdb) p sizeof (lck) > $2 = 32 > > (gdb) p/x lck > $3 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, > l_pid = 0x0} > > (gdb) x/4gx &lck > 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff > 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 > > Stepping to syscall: ... > > (gdb) where > #0 __fcntl64_nocancel_adjusted (fd=fd@entry=4, cmd=cmd@entry=36, arg=arg@entry=0x7fffeae8) > at ../sysdeps/unix/sysv/linux/fcntl_nocancel.c:64 > #1 0x7df1e076 in __GI___libc_fcntl64 (fd=4, cmd=36) at ../sysdeps/unix/sysv/linux/fcntl64.c:51 > #2 0x7df1e006 in __old_libc_fcntl64 (fd=<optimized out>, cmd=<optimized out>) at ../sysdeps/unix/sysv/linux/fcntl.c:114 > #3 0x00401a98 in do_test () at ../sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c:72 > #4 0x00401f42 in run_test_function (config=0x7fffebe8, config=0x7fffebe8, argv=0x7fffed48, argc=<optimized out>) > at support_test_main.c:156 > #5 support_test_main (argc=<optimized out>, argv=0x7fffed48, config=config@entry=0x7fffebe8) at support_test_main.c:322 > #6 0x004017b0 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:168 > > We are just before the syscall: > >│0x7df25fe0 <__fcntl64_nocancel_adjusted+32> svc 221 > > (gdb) x/4gx arg > 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff > 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 > > (gdb) si > (gdb) ir 2 > r2 0xffffffffffffffb5 18446744073709551541 > > (gdb) x/4gx arg > 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff > 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 > Right, so we are sure the test is actually doing what is intending to do. Now back to kernel, afaiu commit 4d2dc2cc76 does reference it fixes 94073ad77fff2: --- fcntl: don't cap l_start and l_end values for F_GETLK64 in compat syscall Currently, we're capping the values too low in the F_GETLK64 case. The fields in that structure are 64-bit values, so we shouldn't need to do any sort of fixup there. Make sure we check that assumption at build time in the future however by ensuring that the sizes we're copying will fit. With this, we no longer need COMPAT_LOFF_T_MAX either, so remove it. Fixes: 94073ad77fff2 (fs/locks: don't mess with the address limit in compat_fcntl64) --- And 94073ad77fff2 does add a fixup_compat_flock check for F_OFD_GETLK (not only for F_GETLK64): --- + case F_GETLK64: + case F_OFD_GETLK: + err = get_compat_flock64(&flock, compat_ptr(arg)); + if (err) + break; + err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); + if (err) + break; + err = fixup_compat_flock(&flock); + if (err) + return err; + err = put_compat_flock64(&flock, compat_ptr(arg)); + break; --- In any way, it is still a kernel issue because prior 94073ad77fff2 F_OFD_GETLK were handle as: --- case F_GETLK64: case F_SETLK64: case F_SETLKW64: case F_OFD_GETLK: case F_OFD_SETLK: case F_OFD_SETLKW: ret = get_compat_flock64(&f, compat_ptr(arg)); if (ret != 0) break; old_fs = get_fs(); set_fs(KERNEL_DS); conv_cmd = convert_fcntl_cmd(cmd); ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); set_fs(old_fs); if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { /* need to return lock information - see above for commentary */ if (f.l_start > COMPAT_LOFF_T_MAX) ret = -EOVERFLOW; if (f.l_len > COMPAT_LOFF_T_MAX) f.l_len = COMPAT_LOFF_T_MAX; if (ret == 0) ret = put_compat_flock64(&f, compat_ptr(arg)); } break; --- And COMPAT_LOFF_T_MAX was defined as 0x7fffffffffffffffL for all architectures. So it seems that kernel between 4.13 through 4.15 have this issue with for compat kernels and I do think it is a kernel issue because fcntl64 is the expected way to use OFD locks. GLIBC returns EOVERFLOW because from application standpoint, it should use LFS variant instead.
On 07/09/2018 08:37 PM, Adhemerval Zanella wrote: > > > On 09/07/2018 11:56, Stefan Liebler wrote: >> On 07/09/2018 04:20 PM, Adhemerval Zanella wrote: >>> >>> >>> On 09/07/2018 10:44, Stefan Liebler wrote: >>>> On 07/06/2018 04:45 PM, Adhemerval Zanella wrote: >>>>> >>>>> >>>>> On 06/07/2018 11:09, Adhemerval Zanella wrote: >>>>>> >>>>>> >>>>>> On 06/07/2018 10:00, Stefan Liebler wrote: >>>>>>> On 06/20/2018 11:43 PM, Adhemerval Zanella wrote: >>>>>>>> Changes from previous version: >>>>>>>> >>>>>>>> - Add a testcase for compat fcntl using OFD locks. >>>>>>>> >>>>>>>> --- >>>>>>>> >>>>>>>> This patch fixes the OFD ("file private") locks for architectures that >>>>>>>> support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The >>>>>>>> issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and >>>>>>>> F_{SET,GET}L{W}K64 expects a flock64 argument and when using old >>>>>>>> F_OFD_* flags with a non LFS flock argument the kernel might interpret >>>>>>>> the underlying data wrongly. Kernel idea originally was to avoid using >>>>>>>> such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS >>>>>>>> semantic as default it is possible to provide the functionality and >>>>>>>> avoid the bogus struct kernel passing by adjusting the struct manually >>>>>>>> for the required flags. >>>>>>>> >>>>>>>> The idea follows other LFS interfaces that provide two symbols: >>>>>>>> >>>>>>>> 1. A new LFS fcntl64 is added on default ABI with the usual macros to >>>>>>>> select it for FILE_OFFSET_BITS=64. >>>>>>>> >>>>>>>> 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for >>>>>>>> F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided >>>>>>>> struct. >>>>>>>> >>>>>>>> 3. Keep a compat symbol with old broken semantic for architectures >>>>>>>> that do not define __OFF_T_MATCHES_OFF64_T. >>>>>>>> >>>>>>>> So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will >>>>>>>> aliased to fcntl and no adjustment would be required. So to actually >>>>>>>> use F_OFD_* with LFS support the source must be built with LFS support >>>>>>>> (_FILE_OFFSET_BITS=64). >>>>>>>> >>>>>>>> Also F_OFD_SETLKW command is handled a cancellation point, as for >>>>>>>> F_SETLKW{64}. >>>>>>>> >>>>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu. >>>>>>>> >>>>>>> ... >>>>>>> >>>>>>> Hi Adhemerval, >>>>>>> >>>>>>> I'm running the new test misc/tst-ofdlocks-compat on s390-32. >>>>>>> If I run it on linux 4.17, the test succeeds and after the second fcntl call which returns zero, lck contains the region from the first fcntl call: >>>>>>> (gdb) p/x lck >>>>>>> $2 = {l_type = 0x1, l_whence = 0x0, l_start = 0x800003ff, l_len = 0x400, l_pid = 0xffffffff} >>>>>>> >>>>>>> If I run it on linux 4.14, the test fails. There the second fcntl returns -1 and errno = EOVERFLOW >>>>>>> In this case, lck is not updated: >>>>>>> p/x lck >>>>>>> $4 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, l_pid = 0x0} >>>>>>> >>>>>>> In both cases struct flock64 is just passed to syscall fcntl64 via __old_libc_fcntl64. >>>>>>> >>>>>>> Are the different behaviours related to a change in kernel-code? >>>>>> >>>>>> I think it is due the patch 'fcntl: don't cap l_start and l_end values >>>>>> for F_GETLK64 in compat syscall' (4d2dc2cc766c3b51929658cacbc6e34fc8e242fb >>>>>> added on v4.15). Previously for COMPAT_SYSCALL_DEFINE3(fcntl64,...) kernel >>>>>> did: >>>>>> >>>>>> static long do_compat_fcntl64(unsigned int fd, unsigned int cmd, >>>>>> compat_ulong_t arg) >>>>>> [...] >>>>>> case F_GETLK64: >>>>>> case F_OFD_GETLK: >>>>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>>>> if (err) >>>>>> break; >>>>>> err = fixup_compat_flock(&flock); >>>>>> if (err) >>>>>> return err; >>>>>> err = put_compat_flock64(&flock, compat_ptr(arg)); >>>>>> break; >>>>>> [....] >>>>>> >>>>>> It means that even if the fcntl(..., F_OFD_GETLK, struct flock64) did not >>>>>> fail, kernel will return EOVERFLOW due 'fixup_compat_flock'. The patch >>>>>> changed to: >>>>>> >>>>>> [...] >>>>>> case F_GETLK64: >>>>>> case F_OFD_GETLK: >>>>>> err = get_compat_flock64(&flock, compat_ptr(arg)); >>>>>> if (!err) >>>>>> err= put_compat_flock64(&flock, compat_ptr(arg)); >>>>>> break; >>>>>> [....] >>>>>> >>>> Yes, this sounds reasonable. >>>> It was introduced with commit 'fs/locks: don't mess with the address limit in compat_fcntl64' (94073ad77fff221b5e66b8b9863a546ba212d6a3) >>>> >>>>>> So if compat fcntl will just return if get_compat_flock64 succeed. Also >>>>>> afaiu only compat syscall is affected by it I think, the generic OFD >>>>>> locks for 32 bits go through fcntl_getlk64 and it does return EOVERFLOW >>>>>> in the aforementioned case. >>>>>> >>>>>> I am not sure which would be the best way to get around this kernel >>>>>> issue, any suggestion to harden the testcase? It also suggest to me that >>>>>> the possible usercase I assume in my testcase (OFD locks with struct >>>>>> flock64) never really worked on previous releases... >>>>>> >>>>> >>>>> Also, on x86 4.4 where actually tested the kernel OFD locks does: >>>>> >>>>> fs/compat.c: >>>>> [...] >>>>> case F_GETLK64: >>>>> case F_SETLK64: >>>>> case F_SETLKW64: >>>>> case F_OFD_GETLK: >>>>> case F_OFD_SETLK: >>>>> case F_OFD_SETLKW: >>>>> ret = get_compat_flock64(&f, compat_ptr(arg)); >>>>> if (ret != 0) >>>>> break; >>>>> old_fs = get_fs(); >>>>> set_fs(KERNEL_DS); >>>>> conv_cmd = convert_fcntl_cmd(cmd); >>>>> ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); >>>>> set_fs(old_fs); >>>>> if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { >>>>> /* need to return lock information - see above for commentary */ >>>>> if (f.l_start > COMPAT_LOFF_T_MAX) >>>>> ret = -EOVERFLOW; >>>>> if (f.l_len > COMPAT_LOFF_T_MAX) >>>>> f.l_len = COMPAT_LOFF_T_MAX; >>>>> if (ret == 0) >>>>> ret = put_compat_flock64(&f, compat_ptr(arg)); >>>>> } >>>>> break; >>>>> [...] >>>>> >>>>> Which is similar to 4.14, however COMPAT_LOFF_T_MAX is defined as >>>>> 0x7fffffffffffffffL for all architectures and l_start/l_len are >>>>> defined as __kernel_long_t (which for a 64 bits kernel is 'long'). >>>>> So the tests are not actually checking for overflow. >>>> Yes. They do not test for overflowing 32bit long. But as fcntl64 with command F_OFD_GETLK assumes 64bit long, it is fine? >>>>> >>>>> Recent kernels do the right overflow check by using COMPAT_OFF_T_MAX >>>>> with a correct value (0x7fffffff on x86). It seems to be fixed by >>>>> 80f0cce6aadebf6caf74d1f8ceb4b008ca72a9e9 (v4.12). >>>>> >>>> Sure? This commit introduces the following implementation and I think those checks are the same as before: >>>> COMPAT_SYSCALL_DEFINE3(fcntl64,... >>> >>> Not sure, I just try to see if there a kernel change by inspecting the >>> kernel patches. >>> >>>> ... >>>> case F_GETLK: >>>> ... if (f.l_start > COMPAT_OFF_T_MAX) >>>> ret = -EOVERFLOW; >>>> ... >>>> case F_OFD_GETLK: >>>> ... if (f.l_start > COMPAT_LOFF_T_MAX) >>>> ret = -EOVERFLOW; >>>> >>>> In recent kernels, only the command F_GETLK is using fixup_compat_flock() which checks against COMPAT_OFF_T_MAX. >>>> >>>> The errno=EOVERFLOW for the second fcntl call in tst-ofdlocks.c is generated by the new glibc check, but not from the kernel. >>> >>> This is the expected behaviour, the new fcntl symbol for non default LFS >>> ABI returns EOVERFLOW. >>> >>>> I've got errno=EOVERFLOW with my kernel 4.14 due to the call of fixup_compat_flock. >>>> I've also retested it on a system with kernel 4.10. There the kernel does not return with errno=EOVERFLOW. >>>> >>> >>> Can you verify that for s390-32 tst-ofdlocks-compat.c is a really using the >>> old version (i.e passing the arguments to kernel unmodified)? Otherwise >>> I am not sure if this is a glibc bug. >>> >> Yes it is. I'm running the s390-32 tst-ofdlocks-compat testcase in 64bit gdb and we are here: >> 72 TEST_VERIFY (fcntl (fd, F_OFD_GETLK, &lck) == 0); >> >> (gdb) p sizeof (lck) >> $2 = 32 >> >> (gdb) p/x lck >> $3 = {l_type = 0x1, l_whence = 0x0, l_start = 0x7ffffbff, l_len = 0x1000, >> l_pid = 0x0} >> >> (gdb) x/4gx &lck >> 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff >> 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 >> >> Stepping to syscall: ... >> >> (gdb) where >> #0 __fcntl64_nocancel_adjusted (fd=fd@entry=4, cmd=cmd@entry=36, arg=arg@entry=0x7fffeae8) >> at ../sysdeps/unix/sysv/linux/fcntl_nocancel.c:64 >> #1 0x7df1e076 in __GI___libc_fcntl64 (fd=4, cmd=36) at ../sysdeps/unix/sysv/linux/fcntl64.c:51 >> #2 0x7df1e006 in __old_libc_fcntl64 (fd=<optimized out>, cmd=<optimized out>) at ../sysdeps/unix/sysv/linux/fcntl.c:114 >> #3 0x00401a98 in do_test () at ../sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c:72 >> #4 0x00401f42 in run_test_function (config=0x7fffebe8, config=0x7fffebe8, argv=0x7fffed48, argc=<optimized out>) >> at support_test_main.c:156 >> #5 support_test_main (argc=<optimized out>, argv=0x7fffed48, config=config@entry=0x7fffebe8) at support_test_main.c:322 >> #6 0x004017b0 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:168 >> >> We are just before the syscall: >> >│0x7df25fe0 <__fcntl64_nocancel_adjusted+32> svc 221 >> >> (gdb) x/4gx arg >> 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff >> 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 >> >> (gdb) si >> (gdb) ir 2 >> r2 0xffffffffffffffb5 18446744073709551541 >> >> (gdb) x/4gx arg >> 0x7fffeae8: 0x0001000000000000 0x000000007ffffbff >> 0x7fffeaf8: 0x0000000000001000 0x0000000000000000 >> > > Right, so we are sure the test is actually doing what is intending to do. > Now back to kernel, afaiu commit 4d2dc2cc76 does reference it fixes 94073ad77fff2: > > --- > fcntl: don't cap l_start and l_end values for F_GETLK64 in compat syscall > > Currently, we're capping the values too low in the F_GETLK64 case. The > fields in that structure are 64-bit values, so we shouldn't need to do > any sort of fixup there. > > Make sure we check that assumption at build time in the future however > by ensuring that the sizes we're copying will fit. > > With this, we no longer need COMPAT_LOFF_T_MAX either, so remove it. > > Fixes: 94073ad77fff2 (fs/locks: don't mess with the address limit in compat_fcntl64) > --- > > And 94073ad77fff2 does add a fixup_compat_flock check for F_OFD_GETLK (not > only for F_GETLK64): > > --- > + case F_GETLK64: > + case F_OFD_GETLK: > + err = get_compat_flock64(&flock, compat_ptr(arg)); > + if (err) > + break; > + err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > + if (err) > + break; > + err = fixup_compat_flock(&flock); > + if (err) > + return err; > + err = put_compat_flock64(&flock, compat_ptr(arg)); > + break; > --- > > In any way, it is still a kernel issue because prior 94073ad77fff2 F_OFD_GETLK > were handle as: > > --- > case F_GETLK64: > case F_SETLK64: > case F_SETLKW64: > case F_OFD_GETLK: > case F_OFD_SETLK: > case F_OFD_SETLKW: > ret = get_compat_flock64(&f, compat_ptr(arg)); > if (ret != 0) > break; > old_fs = get_fs(); > set_fs(KERNEL_DS); > conv_cmd = convert_fcntl_cmd(cmd); > ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f); > set_fs(old_fs); > if ((conv_cmd == F_GETLK || conv_cmd == F_OFD_GETLK) && ret == 0) { > /* need to return lock information - see above for commentary */ > if (f.l_start > COMPAT_LOFF_T_MAX) > ret = -EOVERFLOW; > if (f.l_len > COMPAT_LOFF_T_MAX) > f.l_len = COMPAT_LOFF_T_MAX; > if (ret == 0) > ret = put_compat_flock64(&f, compat_ptr(arg)); > } > break; > --- > > And COMPAT_LOFF_T_MAX was defined as 0x7fffffffffffffffL for all architectures. > > So it seems that kernel between 4.13 through 4.15 have this issue with for > compat kernels and I do think it is a kernel issue because fcntl64 is the > expected way to use OFD locks. GLIBC returns EOVERFLOW because from > application standpoint, it should use LFS variant instead. > Yes, I agree with you. Shall we document this kernel issue in the release-wiki and/or the testcase itself?
On 10/07/2018 03:31, Stefan Liebler wrote: > On 07/09/2018 08:37 PM, Adhemerval Zanella wrote: >> >> So it seems that kernel between 4.13 through 4.15 have this issue with for >> compat kernels and I do think it is a kernel issue because fcntl64 is the >> expected way to use OFD locks. GLIBC returns EOVERFLOW because from >> application standpoint, it should use LFS variant instead. >> > Yes, I agree with you. > Shall we document this kernel issue in the release-wiki and/or the testcase itself? > I have added a note on 2.28 release wiki [1] about this potential failure and if Carlos approves I would like to push the patch below. [1] https://sourceware.org/glibc/wiki/Release/2.28 --- From aca2b996f3ba8fdf76e5e71e3868b701c5aa5c2b Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Tue, 10 Jul 2018 09:24:40 -0300 Subject: [PATCH] Comment tst-ofdlocks-compat expected failure in some Linux releases As pointed out in a libc-alpha thread [1], the misc/tst-ofdlocks-compat may fail in some specific Linux releases. This patch adds a comment along with a link to discussion in the test source code. No changes are expected. * sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c: Add a comment about a kernel issue which lead to test failure in some cases. [1] https://sourceware.org/ml/libc-alpha/2018-07/msg00243.html --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/ChangeLog b/ChangeLog index 7a052c3..9c57396 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2018-07-10 Adhemerval Zanella <adhemerval.zanella@linaro.org> + + * sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c: Add a comment about + a kernel issue which lead to test failure in some cases. + 2018-07-06 Florian Weimer <fweimer@redhat.com> [BZ #18991] diff --git a/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c index d1d00eb..03c4abf 100644 --- a/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c +++ b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c @@ -40,6 +40,14 @@ do_prepare (int argc, char **argv) #define PREPARE do_prepare +/* Linux between 4.13 and 4.15 return EOVERFLOW for LFS OFD locks usage + in compat mode (non-LFS ABI running on a LFS default kernel, such as + i386 on a x86_64 kernel or s390-32 on a s390-64 kernel) [1]. This is + a kernel issue because __NR_fcntl64 is the expected way to use OFD locks + (used on GLIBC for both fcntl and fcntl64). + + [1] https://sourceware.org/ml/libc-alpha/2018-07/msg00243.html */ + static int do_test (void) { -- 2.7.4
On 07/10/2018 08:28 AM, Adhemerval Zanella wrote: > > > On 10/07/2018 03:31, Stefan Liebler wrote: >> On 07/09/2018 08:37 PM, Adhemerval Zanella wrote: >>> >>> So it seems that kernel between 4.13 through 4.15 have this issue with for >>> compat kernels and I do think it is a kernel issue because fcntl64 is the >>> expected way to use OFD locks. GLIBC returns EOVERFLOW because from >>> application standpoint, it should use LFS variant instead. >>> >> Yes, I agree with you. >> Shall we document this kernel issue in the release-wiki and/or the testcase itself? >> > > I have added a note on 2.28 release wiki [1] about this potential failure > and if Carlos approves I would like to push the patch below. Note looks good. Please commit. The compat for 32->64 has always had problems over the years and they often take several releases to get ironed out. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > [1] https://sourceware.org/glibc/wiki/Release/2.28 > > --- > > From aca2b996f3ba8fdf76e5e71e3868b701c5aa5c2b Mon Sep 17 00:00:00 2001 > From: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Date: Tue, 10 Jul 2018 09:24:40 -0300 > Subject: [PATCH] Comment tst-ofdlocks-compat expected failure in some Linux > releases > > As pointed out in a libc-alpha thread [1], the misc/tst-ofdlocks-compat > may fail in some specific Linux releases. This patch adds a comment > along with a link to discussion in the test source code. > > No changes are expected. > > * sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c: Add a comment about > a kernel issue which lead to test failure in some cases. > > [1] https://sourceware.org/ml/libc-alpha/2018-07/msg00243.html > --- > ChangeLog | 5 +++++ > sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c | 8 ++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/ChangeLog b/ChangeLog > index 7a052c3..9c57396 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,8 @@ > +2018-07-10 Adhemerval Zanella <adhemerval.zanella@linaro.org> > + > + * sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c: Add a comment about > + a kernel issue which lead to test failure in some cases. > + > 2018-07-06 Florian Weimer <fweimer@redhat.com> > > [BZ #18991] > diff --git a/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c > index d1d00eb..03c4abf 100644 > --- a/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c > +++ b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c > @@ -40,6 +40,14 @@ do_prepare (int argc, char **argv) > > #define PREPARE do_prepare > > +/* Linux between 4.13 and 4.15 return EOVERFLOW for LFS OFD locks usage > + in compat mode (non-LFS ABI running on a LFS default kernel, such as > + i386 on a x86_64 kernel or s390-32 on a s390-64 kernel) [1]. This is > + a kernel issue because __NR_fcntl64 is the expected way to use OFD locks > + (used on GLIBC for both fcntl and fcntl64). > + > + [1] https://sourceware.org/ml/libc-alpha/2018-07/msg00243.html */ > + > static int > do_test (void) > { > -- Cheers, Carlos.
diff --git a/NEWS b/NEWS index d51fa09..5996e53 100644 --- a/NEWS +++ b/NEWS @@ -110,6 +110,12 @@ Deprecated and removed features, and other changes affecting compatibility: restriction (rejecting '_' in host names, among other things) has been removed, for increased compatibility with non-IDN name resolution. +* The fcntl function now have a Long File Support variant named fcntl64. It + is added to fix some Linux Open File Description (OFD) locks usage on non + LFS mode. As for others *64 functions, fcntl64 semantics are analogous with + fcntl and LFS support is handled transparently. Also for Linux, the OFD + locks act as a cancellation entrypoint. + Changes to build and runtime requirements: [Add changes to build and runtime requirements here] diff --git a/csu/check_fds.c b/csu/check_fds.c index 605ee4f..ad1763b 100644 --- a/csu/check_fds.c +++ b/csu/check_fds.c @@ -39,7 +39,7 @@ static void check_one_fd (int fd, int mode) { - if (__builtin_expect (__fcntl_nocancel (fd, F_GETFD), 0) == -1 + if (__builtin_expect (__fcntl64_nocancel (fd, F_GETFD), 0) == -1 && errno == EBADF) { const char *name; diff --git a/include/fcntl.h b/include/fcntl.h index 966f797..be43504 100644 --- a/include/fcntl.h +++ b/include/fcntl.h @@ -10,11 +10,16 @@ extern int __libc_open (const char *file, int oflag, ...); libc_hidden_proto (__libc_open) extern int __libc_fcntl (int fd, int cmd, ...); libc_hidden_proto (__libc_fcntl) -extern int __fcntl_nocancel_adjusted (int fd, int cmd, void *arg) attribute_hidden; +extern int __fcntl64_nocancel_adjusted (int fd, int cmd, void *arg) + attribute_hidden; +extern int __libc_fcntl64 (int fd, int cmd, ...); +libc_hidden_proto (__libc_fcntl64) extern int __open (const char *__file, int __oflag, ...); libc_hidden_proto (__open) extern int __fcntl (int __fd, int __cmd, ...); libc_hidden_proto (__fcntl) +extern int __fcntl64 (int __fd, int __cmd, ...) attribute_hidden; +libc_hidden_proto (__fcntl64) extern int __openat (int __fd, const char *__file, int __oflag, ...) __nonnull ((2)); libc_hidden_proto (__openat) diff --git a/io/Makefile b/io/Makefile index 2117cb6..4a0d8fe 100644 --- a/io/Makefile +++ b/io/Makefile @@ -40,7 +40,7 @@ routines := \ mkdir mkdirat \ open open_2 open64 open64_2 openat openat_2 openat64 openat64_2 \ read write lseek lseek64 access euidaccess faccessat \ - fcntl flock lockf lockf64 \ + fcntl fcntl64 flock lockf lockf64 \ close dup dup2 dup3 pipe pipe2 \ creat creat64 \ chdir fchdir \ @@ -89,6 +89,7 @@ CFLAGS-open64.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-creat.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-creat64.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-fcntl.c += -fexceptions -fasynchronous-unwind-tables +CFLAGS-fcntl64.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-poll.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-ppoll.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-lockf.c += -fexceptions diff --git a/io/Versions b/io/Versions index 4448b71..98037fb 100644 --- a/io/Versions +++ b/io/Versions @@ -128,8 +128,11 @@ libc { GLIBC_2.27 { copy_file_range; } + GLIBC_2.28 { + fcntl64; + } GLIBC_PRIVATE { - __libc_fcntl; + __libc_fcntl64; __fcntl_nocancel; __open64_nocancel; __write_nocancel; diff --git a/io/fcntl.h b/io/fcntl.h index 69a4394..3afc620 100644 --- a/io/fcntl.h +++ b/io/fcntl.h @@ -167,7 +167,18 @@ typedef __pid_t pid_t; This function is a cancellation point and therefore not marked with __THROW. */ +#ifndef __USE_FILE_OFFSET64 extern int fcntl (int __fd, int __cmd, ...); +#else +# ifdef __REDIRECT +extern int __REDIRECT (fcntl, (int __fd, int __cmd, ...), fcntl64); +# else +# define fcntl fcntl64 +# endif +#endif +#ifdef __USE_LARGEFILE64 +extern int fcntl64 (int __fd, int __cmd, ...); +#endif /* Open FILE and return a new file descriptor for it, or -1 on error. OFLAG determines the type of access used. If O_CREAT or O_TMPFILE is set diff --git a/io/fcntl64.c b/io/fcntl64.c new file mode 100644 index 0000000..f4e6809 --- /dev/null +++ b/io/fcntl64.c @@ -0,0 +1,38 @@ +/* Manipulate file descriptor. Stub LFS version. + Copyright (C) 2018 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 <fcntl.h> + +/* Perform file control operations on FD. */ +int +__fcntl64 (int fd, int cmd, ...) +{ + if (fd < 0) + { + __set_errno (EBADF); + return -1; + } + + __set_errno (ENOSYS); + return -1; +} +libc_hidden_def (__fcntl64) +stub_warning (fcntl64) + +weak_alias (__fcntl64, fcntl64) diff --git a/login/utmp_file.c b/login/utmp_file.c index 72dd213..040a505 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -82,7 +82,7 @@ static void timeout_handler (int signum) {}; memset (&fl, '\0', sizeof (struct flock)); \ fl.l_type = (type); \ fl.l_whence = SEEK_SET; \ - if (__fcntl_nocancel ((fd), F_SETLKW, &fl) < 0) + if (__fcntl64_nocancel ((fd), F_SETLKW, &fl) < 0) #define LOCKING_FAILED() \ goto unalarm_return @@ -90,7 +90,7 @@ static void timeout_handler (int signum) {}; #define UNLOCK_FILE(fd) \ /* Unlock the file. */ \ fl.l_type = F_UNLCK; \ - __fcntl_nocancel ((fd), F_SETLKW, &fl); \ + __fcntl64_nocancel ((fd), F_SETLKW, &fl); \ \ unalarm_return: \ /* Reset the signal handler and alarm. We must reset the alarm \ diff --git a/manual/llio.texi b/manual/llio.texi index 82f03be..e840c55 100644 --- a/manual/llio.texi +++ b/manual/llio.texi @@ -3281,12 +3281,13 @@ Set process or process group ID to receive @code{SIGIO} signals. @xref{Interrupt Input}. @end vtable -This function is a cancellation point in multi-threaded programs. This -is a problem if the thread allocates some resources (like memory, file -descriptors, semaphores or whatever) at the time @code{fcntl} is -called. If the thread gets canceled these resources stay allocated -until the program ends. To avoid this calls to @code{fcntl} should be -protected using cancellation handlers. +This function is a cancellation point in multi-threaded programs for the +commands @code{F_SETLKW} (and the LFS analogous @code{F_SETLKW64}) and +@code {F_OFD_SETLKW}. This is a problem if the thread allocates some +resources (like memory, file descriptors, semaphores or whatever) at the time +@code{fcntl} is called. If the thread gets canceled these resources stay +allocated until the program ends. To avoid this calls to @code{fcntl} should +be protected using cancellation handlers. @c ref pthread_cleanup_push / pthread_cleanup_pop @end deftypefun diff --git a/nptl/Makefile b/nptl/Makefile index 6cfc8c6..0f9c44a 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -191,6 +191,7 @@ CFLAGS-sem_timedwait.c += -fexceptions -fasynchronous-unwind-tables # These are the function wrappers we have to duplicate here. CFLAGS-fcntl.c += -fexceptions -fasynchronous-unwind-tables +CFLAGS-fcntl64.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-lockf.c += -fexceptions CFLAGS-pread.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-pread64.c += -fexceptions -fasynchronous-unwind-tables diff --git a/sysdeps/mach/hurd/fcntl.c b/sysdeps/mach/hurd/fcntl.c index 0b23164..598317d 100644 --- a/sysdeps/mach/hurd/fcntl.c +++ b/sysdeps/mach/hurd/fcntl.c @@ -210,3 +210,8 @@ libc_hidden_def (__libc_fcntl) weak_alias (__libc_fcntl, __fcntl) libc_hidden_weak (__fcntl) weak_alias (__libc_fcntl, fcntl) + +strong_alias (__libc_fcntl, __libc_fcntl64) +libc_hidden_def (__libc_fcntl64) +weak_alias (__libc_fcntl64, __fcntl64) +libc_hidden_weak (__fcntl64) diff --git a/sysdeps/mach/hurd/i386/libc.abilist b/sysdeps/mach/hurd/i386/libc.abilist index 2cb5070..3d46de7 100644 --- a/sysdeps/mach/hurd/i386/libc.abilist +++ b/sysdeps/mach/hurd/i386/libc.abilist @@ -2033,6 +2033,8 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/posix/fdopendir.c b/sysdeps/posix/fdopendir.c index dafb5d2..b72eecc 100644 --- a/sysdeps/posix/fdopendir.c +++ b/sysdeps/posix/fdopendir.c @@ -38,7 +38,7 @@ __fdopendir (int fd) } /* Make sure the descriptor allows for reading. */ - int flags = __fcntl_nocancel (fd, F_GETFL); + int flags = __fcntl64_nocancel (fd, F_GETFL); if (__glibc_unlikely (flags == -1)) return NULL; if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c index 0875385..bb6bd7c 100644 --- a/sysdeps/posix/opendir.c +++ b/sysdeps/posix/opendir.c @@ -99,7 +99,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp) /* We have to set the close-on-exit flag if the user provided the file descriptor. */ if (!close_fd - && __glibc_unlikely (__fcntl_nocancel (fd, F_SETFD, FD_CLOEXEC) < 0)) + && __glibc_unlikely (__fcntl64_nocancel (fd, F_SETFD, FD_CLOEXEC) < 0)) goto lose; const size_t default_allocation = (4 * BUFSIZ < sizeof (struct dirent64) diff --git a/sysdeps/unix/pt-fcntl.c b/sysdeps/unix/pt-fcntl.c index 8113d52..3d64054 100644 --- a/sysdeps/unix/pt-fcntl.c +++ b/sysdeps/unix/pt-fcntl.c @@ -37,7 +37,7 @@ fcntl_compat (int fd, int cmd, ...) va_start (ap, cmd); arg = va_arg (ap, void *); va_end (ap); - return __libc_fcntl (fd, cmd, arg); + return __libc_fcntl64 (fd, cmd, arg); } weak_alias (fcntl_compat, fcntl_alias) diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 215fd5d..f71cc39 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -45,7 +45,9 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \ test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \ - tst-rlimit-infinity + tst-rlimit-infinity tst-ofdlocks +tests-internal += tst-ofdlocks-compat + # Generate the list of SYS_* macros for the system calls (__NR_* # macros). The file syscall-names.list contains all possible system diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist index 80cdb98..884d0df 100644 --- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist +++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist @@ -2131,3 +2131,4 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist index c761f61..28d54b9 100644 --- a/sysdeps/unix/sysv/linux/alpha/libc.abilist +++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist @@ -2026,6 +2026,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/arm/libc.abilist b/sysdeps/unix/sysv/linux/arm/libc.abilist index 6aa58c3..dfde3bd 100644 --- a/sysdeps/unix/sysv/linux/arm/libc.abilist +++ b/sysdeps/unix/sysv/linux/arm/libc.abilist @@ -115,6 +115,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.4 _Exit F GLIBC_2.4 _IO_2_1_stderr_ D 0xa0 GLIBC_2.4 _IO_2_1_stdin_ D 0xa0 diff --git a/sysdeps/unix/sysv/linux/fcntl.c b/sysdeps/unix/sysv/linux/fcntl.c index e3992dc..0e37ed0 100644 --- a/sysdeps/unix/sysv/linux/fcntl.c +++ b/sysdeps/unix/sysv/linux/fcntl.c @@ -20,15 +20,12 @@ #include <stdarg.h> #include <errno.h> #include <sysdep-cancel.h> -#include <not-cancel.h> -#ifndef __NR_fcntl64 -# define __NR_fcntl64 __NR_fcntl -#endif +#ifndef __OFF_T_MATCHES_OFF64_T -#ifndef FCNTL_ADJUST_CMD -# define FCNTL_ADJUST_CMD(__cmd) __cmd -#endif +# ifndef FCNTL_ADJUST_CMD +# define FCNTL_ADJUST_CMD(__cmd) __cmd +# endif int __libc_fcntl (int fd, int cmd, ...) @@ -42,13 +39,83 @@ __libc_fcntl (int fd, int cmd, ...) cmd = FCNTL_ADJUST_CMD (cmd); - if (cmd == F_SETLKW || cmd == F_SETLKW64) - return SYSCALL_CANCEL (fcntl64, fd, cmd, (void *) arg); - - return __fcntl_nocancel_adjusted (fd, cmd, arg); + switch (cmd) + { + case F_SETLKW: + case F_SETLKW64: + return SYSCALL_CANCEL (fcntl64, fd, cmd, arg); + case F_OFD_SETLKW: + { + struct flock *flk = (struct flock *) arg; + struct flock64 flk64 = + { + .l_type = flk->l_type, + .l_whence = flk->l_whence, + .l_start = flk->l_start, + .l_len = flk->l_len, + .l_pid = flk->l_pid + }; + return SYSCALL_CANCEL (fcntl64, fd, cmd, &flk64); + } + case F_OFD_GETLK: + case F_OFD_SETLK: + { + struct flock *flk = (struct flock *) arg; + struct flock64 flk64 = + { + .l_type = flk->l_type, + .l_whence = flk->l_whence, + .l_start = flk->l_start, + .l_len = flk->l_len, + .l_pid = flk->l_pid + }; + int ret = INLINE_SYSCALL_CALL (fcntl64, fd, cmd, &flk64); + if (ret == -1) + return -1; + if ((off_t) flk64.l_start != flk64.l_start + || (off_t) flk64.l_len != flk64.l_len) + { + __set_errno (EOVERFLOW); + return -1; + } + flk->l_type = flk64.l_type; + flk->l_whence = flk64.l_whence; + flk->l_start = flk64.l_start; + flk->l_len = flk64.l_len; + flk->l_pid = flk64.l_pid; + return ret; + } + /* case F_OFD_GETLK: + case F_OFD_GETLK64: + case F_SETLK64: + case F_GETOWN: */ + default: + return __fcntl64_nocancel_adjusted (fd, cmd, arg); + } } libc_hidden_def (__libc_fcntl) weak_alias (__libc_fcntl, __fcntl) libc_hidden_weak (__fcntl) + +# include <shlib-compat.h> +# if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_28) +int +__old_libc_fcntl64 (int fd, int cmd, ...) +{ + va_list ap; + void *arg; + + va_start (ap, cmd); + arg = va_arg (ap, void *); + va_end (ap); + + return __libc_fcntl64 (fd, cmd, arg); +} +compat_symbol (libc, __old_libc_fcntl64, fcntl, GLIBC_2_0); +versioned_symbol (libc, __libc_fcntl, fcntl, GLIBC_2_28); +# else weak_alias (__libc_fcntl, fcntl) +# endif + +#endif /* __OFF_T_MATCHES_OFF64_T */ diff --git a/sysdeps/unix/sysv/linux/fcntl64.c b/sysdeps/unix/sysv/linux/fcntl64.c new file mode 100644 index 0000000..f21667a --- /dev/null +++ b/sysdeps/unix/sysv/linux/fcntl64.c @@ -0,0 +1,63 @@ +/* Manipulate file descriptor. Linux LFS version. + Copyright (C) 2018 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/>. */ + +#define fcntl __no_decl_fcntl +#define __fcntl __no_decl___fcntl +#include <fcntl.h> +#undef fcntl +#undef __fcntl +#include <stdarg.h> +#include <errno.h> +#include <sysdep-cancel.h> + +#ifndef __NR_fcntl64 +# define __NR_fcntl64 __NR_fcntl +#endif + +#ifndef FCNTL_ADJUST_CMD +# define FCNTL_ADJUST_CMD(__cmd) __cmd +#endif + +int +__libc_fcntl64 (int fd, int cmd, ...) +{ + va_list ap; + void *arg; + + va_start (ap, cmd); + arg = va_arg (ap, void *); + va_end (ap); + + cmd = FCNTL_ADJUST_CMD (cmd); + + if (cmd == F_SETLKW || cmd == F_SETLKW64 || cmd == F_OFD_SETLKW) + return SYSCALL_CANCEL (fcntl64, fd, cmd, arg); + + return __fcntl64_nocancel_adjusted (fd, cmd, arg); +} +libc_hidden_def (__libc_fcntl64) +weak_alias (__libc_fcntl64, __fcntl64) +libc_hidden_weak (__fcntl64) +weak_alias (__libc_fcntl64, fcntl64) + +#ifdef __OFF_T_MATCHES_OFF64_T +weak_alias (__libc_fcntl64, __libc_fcntl) +weak_alias (__libc_fcntl64, __fcntl) +weak_alias (__libc_fcntl64, __GI___fcntl) +weak_alias (__libc_fcntl64, fcntl) +#endif diff --git a/sysdeps/unix/sysv/linux/fcntl_nocancel.c b/sysdeps/unix/sysv/linux/fcntl_nocancel.c index f50e382..dd336b5 100644 --- a/sysdeps/unix/sysv/linux/fcntl_nocancel.c +++ b/sysdeps/unix/sysv/linux/fcntl_nocancel.c @@ -31,7 +31,7 @@ #endif int -__fcntl_nocancel (int fd, int cmd, ...) +__fcntl64_nocancel (int fd, int cmd, ...) { va_list ap; void *arg; @@ -42,12 +42,12 @@ __fcntl_nocancel (int fd, int cmd, ...) cmd = FCNTL_ADJUST_CMD (cmd); - return __fcntl_nocancel_adjusted (fd, cmd, arg); + return __fcntl64_nocancel_adjusted (fd, cmd, arg); } -hidden_def (__fcntl_nocancel) +hidden_def (__fcntl64_nocancel) int -__fcntl_nocancel_adjusted (int fd, int cmd, void *arg) +__fcntl64_nocancel_adjusted (int fd, int cmd, void *arg) { if (cmd == F_GETOWN) { diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist index d10695b..06b00f7 100644 --- a/sysdeps/unix/sysv/linux/hppa/libc.abilist +++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist @@ -1872,6 +1872,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist index 23092ab..1c1cc00 100644 --- a/sysdeps/unix/sysv/linux/i386/libc.abilist +++ b/sysdeps/unix/sysv/linux/i386/libc.abilist @@ -2037,6 +2037,8 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist index 7bf259e..f6e17a0 100644 --- a/sysdeps/unix/sysv/linux/ia64/libc.abilist +++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist @@ -1907,6 +1907,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist index 4673bcd..ee054a6 100644 --- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist @@ -116,6 +116,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.4 _Exit F GLIBC_2.4 _IO_2_1_stderr_ D 0x98 GLIBC_2.4 _IO_2_1_stdin_ D 0x98 diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist index 1f8ac40..227a058 100644 --- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist @@ -1981,6 +1981,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/microblaze/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/libc.abilist index 09277f5..18781b3 100644 --- a/sysdeps/unix/sysv/linux/microblaze/libc.abilist +++ b/sysdeps/unix/sysv/linux/microblaze/libc.abilist @@ -2122,3 +2122,5 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist index f562e20..2d86989 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist @@ -1959,6 +1959,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist index ceb7388..b8b113e 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist @@ -1957,6 +1957,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist index 5765f48..6a3cd13 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist @@ -1965,6 +1965,8 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist index a84bb45..596ec05 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist @@ -1961,6 +1961,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist index e432959..8da18ee 100644 --- a/sysdeps/unix/sysv/linux/nios2/libc.abilist +++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist @@ -2163,3 +2163,5 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h index 0b5c955..09de92d 100644 --- a/sysdeps/unix/sysv/linux/not-cancel.h +++ b/sysdeps/unix/sysv/linux/not-cancel.h @@ -76,7 +76,7 @@ __typeof (pause) __pause_nocancel; __typeof (__nanosleep) __nanosleep_nocancel; /* Uncancelable fcntl. */ -__typeof (__fcntl) __fcntl_nocancel; +__typeof (__fcntl) __fcntl64_nocancel; #if IS_IN (libc) || IS_IN (rtld) hidden_proto (__open_nocancel) @@ -89,7 +89,7 @@ hidden_proto (__close_nocancel) hidden_proto (__waitpid_nocancel) hidden_proto (__pause_nocancel) hidden_proto (__nanosleep_nocancel) -hidden_proto (__fcntl_nocancel) +hidden_proto (__fcntl64_nocancel) #endif #endif /* NOT_CANCEL_H */ diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist index a5f2b23..555751e 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist @@ -1985,6 +1985,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist index e4cbe36..80324e4 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist @@ -1989,6 +1989,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist index 9869feb..97b1d35 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist @@ -2221,3 +2221,4 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist index e526dc4..15be314 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist @@ -116,6 +116,7 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 _Exit F GLIBC_2.3 _IO_2_1_stderr_ D 0xe0 GLIBC_2.3 _IO_2_1_stdin_ D 0xe0 diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist index e6319ee..436b992 100644 --- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist @@ -2093,3 +2093,4 @@ GLIBC_2.27 xdrstdio_create F GLIBC_2.27 xencrypt F GLIBC_2.27 xprt_register F GLIBC_2.27 xprt_unregister F +GLIBC_2.28 fcntl64 F diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist index 41cdda0..f66715f 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist @@ -1994,6 +1994,8 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist index 8a756cf..bd62428 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist @@ -1900,6 +1900,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/sh/libc.abilist b/sysdeps/unix/sysv/linux/sh/libc.abilist index 999bddd..f2f070f 100644 --- a/sysdeps/unix/sysv/linux/sh/libc.abilist +++ b/sysdeps/unix/sysv/linux/sh/libc.abilist @@ -1876,6 +1876,8 @@ GLIBC_2.27 wcstof32x F GLIBC_2.27 wcstof32x_l F GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist index 7c4296f..265087f 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist @@ -1988,6 +1988,8 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist index dafe9d7..16a6981 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist @@ -1930,6 +1930,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c new file mode 100644 index 0000000..a614580 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c @@ -0,0 +1,78 @@ +/* Check non representable OFD locks regions in non-LFS mode for compat + mode (BZ #20251) + Copyright (C) 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + as published by the Free Software Foundation; either version 2 + of the License, or (at your option) any later version. + + This program 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 General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, see <http://www.gnu.org/licenses/>. +*/ + +#include <unistd.h> +#include <fcntl.h> +#include <stdint.h> +#include <errno.h> + +#include <support/temp_file.h> +#include <support/check.h> + +#include <shlib-compat.h> +#if TEST_COMPAT (libc, GLIBC_2_0, GLIBC_2_28) +compat_symbol_reference (libc, fcntl, fcntl, GLIBC_2_0); +#endif + +static char *temp_filename; +static int temp_fd; + +static void +do_prepare (int argc, char **argv) +{ + temp_fd = create_temp_file ("tst-ofdlocks-compat.", &temp_filename); + TEST_VERIFY_EXIT (temp_fd != -1); +} + +#define PREPARE do_prepare + +static int +do_test (void) +{ + /* The compat fcntl version for architectures which support non-LFS + operations does not wrap the flock OFD argument, so the struct is passed + unmodified to kernel. It means no EOVERFLOW is returned, so operations + with LFS should not incur in failure. */ + + struct flock64 lck64 = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = (off64_t)INT32_MAX + 1024, + .l_len = 1024, + }; + TEST_VERIFY_EXIT (fcntl (temp_fd, F_OFD_SETLKW, &lck64) == 0); + + /* Open file description locks placed through the same open file description + (either by same file descriptor or a duplicated one created by fork, + dup, fcntl F_DUPFD, etc.) overwrites then old lock. To force a + conflicting lock combination, it creates a new file descriptor. */ + int fd = open64 (temp_filename, O_RDWR, 0666); + TEST_VERIFY_EXIT (fd != -1); + + struct flock64 lck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = INT32_MAX - 1024, + .l_len = 4 * 1024, + }; + TEST_VERIFY (fcntl (fd, F_OFD_GETLK, &lck) == 0); + + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-ofdlocks.c b/sysdeps/unix/sysv/linux/tst-ofdlocks.c new file mode 100644 index 0000000..bd345e9 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-ofdlocks.c @@ -0,0 +1,76 @@ +/* Check non representable OFD locks regions in non-LFS mode (BZ #20251) + Copyright (C) 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + as published by the Free Software Foundation; either version 2 + of the License, or (at your option) any later version. + + This program 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 General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, see <http://www.gnu.org/licenses/>. +*/ + +#include <unistd.h> +#include <fcntl.h> +#include <stdint.h> +#include <errno.h> + +#include <support/temp_file.h> +#include <support/check.h> + +static char *temp_filename; +static int temp_fd; + +static void +do_prepare (int argc, char **argv) +{ + temp_fd = create_temp_file ("tst-ofdlocks.", &temp_filename); + TEST_VERIFY_EXIT (temp_fd != -1); +} + +#define PREPARE do_prepare + +static int +do_test (void) +{ + /* It first allocates a open file description lock range which can not + be represented in a 32 bit struct flock. */ + struct flock64 lck64 = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = (off64_t)INT32_MAX + 1024, + .l_len = 1024, + }; + TEST_VERIFY_EXIT (fcntl64 (temp_fd, F_OFD_SETLKW, &lck64) == 0); + + /* Open file description locks placed through the same open file description + (either by same file descriptor or a duplicated one created by fork, + dup, fcntl F_DUPFD, etc.) overwrites then old lock. To force a + conflicting lock combination, it creates a new file descriptor. */ + int fd = open64 (temp_filename, O_RDWR, 0666); + TEST_VERIFY_EXIT (fd != -1); + + /* It tries then to allocate another open file descriptior with a valid + non-LFS bits struct flock but which will result in a conflicted region + which can not be represented in a non-LFS struct flock. */ + struct flock lck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = INT32_MAX - 1024, + .l_len = 4 * 1024, + }; + int r = fcntl (fd, F_OFD_GETLK, &lck); + if (sizeof (off_t) != sizeof (off64_t)) + TEST_VERIFY (r == -1 && errno == EOVERFLOW); + else + TEST_VERIFY (r == 0); + + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist index f72d494..fa8c198 100644 --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist @@ -1888,6 +1888,7 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F GLIBC_2.3 __ctype_b_loc F GLIBC_2.3 __ctype_tolower_loc F GLIBC_2.3 __ctype_toupper_loc F diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist index 96c9fa0..2536971 100644 --- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist @@ -2139,3 +2139,4 @@ GLIBC_2.27 wcstof64 F GLIBC_2.27 wcstof64_l F GLIBC_2.27 wcstof64x F GLIBC_2.27 wcstof64x_l F +GLIBC_2.28 fcntl64 F
Changes from previous version: - Add a testcase for compat fcntl using OFD locks. --- This patch fixes the OFD ("file private") locks for architectures that support non-LFS flock definition (__USE_FILE_OFFSET64 not defined). The issue in this case is both F_OFD_{GETLK,SETLK,SETLKW} and F_{SET,GET}L{W}K64 expects a flock64 argument and when using old F_OFD_* flags with a non LFS flock argument the kernel might interpret the underlying data wrongly. Kernel idea originally was to avoid using such flags in non-LFS syscall, but since GLIBC uses fcntl with LFS semantic as default it is possible to provide the functionality and avoid the bogus struct kernel passing by adjusting the struct manually for the required flags. The idea follows other LFS interfaces that provide two symbols: 1. A new LFS fcntl64 is added on default ABI with the usual macros to select it for FILE_OFFSET_BITS=64. 2. The Linux non-LFS fcntl use a stack allocated struct flock64 for F_OFD_{GETLK,SETLK,SETLKW} copy the results on the user provided struct. 3. Keep a compat symbol with old broken semantic for architectures that do not define __OFF_T_MATCHES_OFF64_T. So for architectures which defines __USE_FILE_OFFSET64, fcntl64 will aliased to fcntl and no adjustment would be required. So to actually use F_OFD_* with LFS support the source must be built with LFS support (_FILE_OFFSET_BITS=64). Also F_OFD_SETLKW command is handled a cancellation point, as for F_SETLKW{64}. Checked on x86_64-linux-gnu and i686-linux-gnu. [BZ #20251] * NEWS: Mention fcntl64 addition. * csu/check_fds.c: Replace __fcntl_nocancel by __fcntl64_nocancel. * login/utmp_file.c: Likewise. * sysdeps/posix/fdopendir.c: Likewise. * sysdeps/posix/opendir.c: Likewise. * sysdeps/unix/pt-fcntl.c: Likewise. * include/fcntl.h (__libc_fcntl64, __fcntl64, __fcntl64_nocancel_adjusted): New prototype. (__fcntl_nocancel_adjusted): Remove prototype. * io/Makefile (routines): Add fcntl64. (CFLAGS-fcntl64.c): New rule. * io/Versions [GLIBC_2.28] (fcntl64): New symbol. [GLIBC_PRIVATE] (__libc_fcntl): Rename to __libc_fcntl64. * io/fcntl.h (fcntl64): Add prototype and redirect if __USE_FILE_OFFSET64 is defined. * io/fcntl64.c: New file. * manual/llio.text: Add a note for which commands fcntl acts a cancellation point. * nptl/Makefile (CFLAGS-fcntl64.c): New rule. * sysdeps/mach/hurd/fcntl.c: Alias fcntl to fcntl64 symbols. * sysdeps/mach/hurd/i386/libc.abilist [GLIBC_2.28] (fcntl, fcntl64): New symbols. * sysdeps/unix/sysv/linux/fcntl.c (__libc_fcntl): Fix F_GETLK64, F_OFD_GETLK, F_SETLK64, F_SETLKW64, F_OFD_SETLK, and F_OFD_SETLKW for non-LFS case. * sysdeps/unix/sysv/linux/fcntl64.c: New file. * sysdeps/unix/sysv/linux/fcntl_nocancel.c (__fcntl_nocancel): Rename to __fcntl64_nocancel. (__fcntl_nocancel_adjusted): Rename to __fcntl64_nocancel_adjusted. * sysdeps/unix/sysv/linux/not-cancel.h (__fcntl_nocancel): Rename to __fcntl64_nocancel. * sysdeps/unix/sysv/linux/tst-ofdlocks.c: New file. * sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c: Likewise. * sysdeps/unix/sysv/linux/Makefile (tests): Add tst-ofdlocks. (tests-internal): Add tst-ofdlocks-compat. * sysdeps/unix/sysv/linux/aarch64/libc.abilist [GLIBC_2.28] (fcntl64): New symbol. * sysdeps/unix/sysv/linux/alpha/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/ia64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist: Likewise. * sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/x86_64/64/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/arm/libc.abilist [GLIBC_2.28] (fcntl, fcntl64): Likewise. * sysdeps/unix/sysv/linux/hppa/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/i386/libc.abilis: Likewise. * sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/microblaze/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/nios2/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/sh/libc.abilist: Likewise. * sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist: Likewise. --- ChangeLog | 67 ++++++++++++++++ NEWS | 6 ++ csu/check_fds.c | 2 +- include/fcntl.h | 7 +- io/Makefile | 3 +- io/Versions | 5 +- io/fcntl.h | 11 +++ io/fcntl64.c | 38 +++++++++ login/utmp_file.c | 4 +- manual/llio.texi | 13 ++-- nptl/Makefile | 1 + sysdeps/mach/hurd/fcntl.c | 5 ++ sysdeps/mach/hurd/i386/libc.abilist | 2 + sysdeps/posix/fdopendir.c | 2 +- sysdeps/posix/opendir.c | 2 +- sysdeps/unix/pt-fcntl.c | 2 +- sysdeps/unix/sysv/linux/Makefile | 4 +- sysdeps/unix/sysv/linux/aarch64/libc.abilist | 1 + sysdeps/unix/sysv/linux/alpha/libc.abilist | 1 + sysdeps/unix/sysv/linux/arm/libc.abilist | 2 + sysdeps/unix/sysv/linux/fcntl.c | 89 +++++++++++++++++++--- sysdeps/unix/sysv/linux/fcntl64.c | 63 +++++++++++++++ sysdeps/unix/sysv/linux/fcntl_nocancel.c | 8 +- sysdeps/unix/sysv/linux/hppa/libc.abilist | 2 + sysdeps/unix/sysv/linux/i386/libc.abilist | 2 + sysdeps/unix/sysv/linux/ia64/libc.abilist | 1 + sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist | 2 + sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist | 2 + sysdeps/unix/sysv/linux/microblaze/libc.abilist | 2 + .../unix/sysv/linux/mips/mips32/fpu/libc.abilist | 2 + .../unix/sysv/linux/mips/mips32/nofpu/libc.abilist | 2 + .../unix/sysv/linux/mips/mips64/n32/libc.abilist | 2 + .../unix/sysv/linux/mips/mips64/n64/libc.abilist | 1 + sysdeps/unix/sysv/linux/nios2/libc.abilist | 2 + sysdeps/unix/sysv/linux/not-cancel.h | 4 +- .../sysv/linux/powerpc/powerpc32/fpu/libc.abilist | 2 + .../linux/powerpc/powerpc32/nofpu/libc.abilist | 2 + .../sysv/linux/powerpc/powerpc64/libc-le.abilist | 1 + .../unix/sysv/linux/powerpc/powerpc64/libc.abilist | 1 + sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist | 1 + sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist | 2 + sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist | 1 + sysdeps/unix/sysv/linux/sh/libc.abilist | 2 + sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist | 2 + sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist | 1 + sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c | 78 +++++++++++++++++++ sysdeps/unix/sysv/linux/tst-ofdlocks.c | 76 ++++++++++++++++++ sysdeps/unix/sysv/linux/x86_64/64/libc.abilist | 1 + sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist | 1 + 49 files changed, 500 insertions(+), 33 deletions(-) create mode 100644 io/fcntl64.c create mode 100644 sysdeps/unix/sysv/linux/fcntl64.c create mode 100644 sysdeps/unix/sysv/linux/tst-ofdlocks-compat.c create mode 100644 sysdeps/unix/sysv/linux/tst-ofdlocks.c -- 2.7.4