This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] Fix Linux fcntl OFD locks for non-LFS architectures (BZ#20251)
On 09/07/2018 10:44, Stefan Liebler wrote:
> 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,...
Not sure, I just try to see if there a kernel change by inspecting the
kernel patches.
> ...
> 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.
This is the expected behaviour, the new fcntl symbol for non default LFS
ABI returns EOVERFLOW.
> 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.
>
Can you verify that for s390-32 tst-ofdlocks-compat.c is a really using the
old version (i.e passing the arguments to kernel unmodified)? Otherwise
I am not sure if this is a glibc bug.