This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

    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

Also F_OFD_SETLKW command is handled a cancellation point, as for

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

    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)
               err = fixup_compat_flock(&flock);
               if (err)
                       return err;
               err = put_compat_flock64(&flock, compat_ptr(arg));

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));

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:

         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)
                 old_fs = get_fs();
                 conv_cmd = convert_fcntl_cmd(cmd);
                 ret = sys_fcntl(fd, conv_cmd, (unsigned long)&f);
                 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));

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:
case F_GETLK:
... if (f.l_start > COMPAT_OFF_T_MAX)
 ret = -EOVERFLOW;
... 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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]