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

[Florian Weimer <fweimer at redhat dot com>]

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