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 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;
> [....]
>
> 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.
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).