[v4] Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251)

Message ID 1529530993-20897-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [v4] Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251)
Related show

Commit Message

Adhemerval Zanella June 20, 2018, 9:43 p.m.
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

Comments

Joseph Myers June 20, 2018, 9:49 p.m. | #1
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
Adhemerval Zanella June 21, 2018, 11:03 a.m. | #2
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?
Florian Weimer June 22, 2018, 10:13 a.m. | #3
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
Adhemerval Zanella June 22, 2018, 12:20 p.m. | #4
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
Joseph Myers June 22, 2018, 3:02 p.m. | #5
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
Florian Weimer June 26, 2018, 2:45 p.m. | #6
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
Adhemerval Zanella June 26, 2018, 4:13 p.m. | #7
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.
Joseph Myers June 26, 2018, 9:43 p.m. | #8
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
Adhemerval Zanella June 26, 2018, 11:07 p.m. | #9
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.
Joseph Myers June 26, 2018, 11:44 p.m. | #10
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
Szabolcs Nagy June 30, 2018, 12:26 p.m. | #11
* 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...
Adhemerval Zanella July 2, 2018, 12:44 p.m. | #12
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.
Carlos O'Donell July 5, 2018, 7:56 p.m. | #13
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.
Adhemerval Zanella July 5, 2018, 7:58 p.m. | #14
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
Carlos O'Donell July 5, 2018, 8:13 p.m. | #15
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.
Stefan Liebler July 6, 2018, 1 p.m. | #16
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
Adhemerval Zanella July 6, 2018, 2:09 p.m. | #17
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...
Adhemerval Zanella July 6, 2018, 2:45 p.m. | #18
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).
Stefan Liebler July 9, 2018, 1:44 p.m. | #19
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.
Adhemerval Zanella July 9, 2018, 2:20 p.m. | #20
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.
Stefan Liebler July 9, 2018, 2:56 p.m. | #21
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
Adhemerval Zanella July 9, 2018, 6:37 p.m. | #22
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.
Stefan Liebler July 10, 2018, 6:31 a.m. | #23
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?
Adhemerval Zanella July 10, 2018, 12:28 p.m. | #24
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
Carlos O'Donell July 10, 2018, 12:31 p.m. | #25
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.

Patch

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