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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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