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

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

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...