[PATCH v4] Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251)
Stefan Liebler
stli@linux.ibm.com
Mon Jul 9 13:44:00 GMT 2018
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.
More information about the Libc-alpha
mailing list