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

[Adhemerval Zanella]

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