[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
default but the struct flock uses 32bit off_t for 32bit binaries (unless
_FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
fcntl64(). Hence kernel will read some garbage that is a few bytes after
the 32bit flock structure in this case which will likely end up with the
syscall returning EINVAL.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Yuriy Kolerov <Yuriy.Kolerov@synopsys.com>
---
 man2/fcntl.2 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/man2/fcntl.2 b/man2/fcntl.2
index f0c1acf..4606709 100644
--- a/man2/fcntl.2
+++ b/man2/fcntl.2
@@ -533,7 +533,7 @@ As with traditional advisory locks, the third argument to
 .BR fcntl (),
 .IR lock ,
 is a pointer to an
-.IR flock
+.IR flock64
 structure.
 By contrast with traditional record locks, the
 .I l_pid
@@ -543,7 +543,7 @@ when using the commands described below.
 The commands for working with open file description locks are analogous
 to those used with traditional locks:
 .TP
-.BR F_OFD_SETLK " (\fIstruct flock *\fP)"
+.BR F_OFD_SETLK " (\fIstruct flock64 *\fP)"
 Acquire an open file description lock (when
 .I l_type
 is
@@ -564,7 +564,7 @@ this call returns \-1 and sets
 to
 .BR EAGAIN .
 .TP
-.BR F_OFD_SETLKW " (\fIstruct flock *\fP)"
+.BR F_OFD_SETLKW " (\fIstruct flock64 *\fP)"
 As for
 .BR F_OFD_SETLK ,
 but if a conflicting lock is held on the file, then wait for that lock to be
@@ -578,7 +578,7 @@ set to
 see
 .BR signal (7)).
 .TP
-.BR F_OFD_GETLK " (\fIstruct flock *\fP)"
+.BR F_OFD_GETLK " (\fIstruct flock64 *\fP)"
 On input to this call,
 .I lock
 describes an open file description lock we would like to place on the file.
-- 
2.7.3


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
> EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
> default but the struct flock uses 32bit off_t for 32bit binaries (unless
> _FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
> fcntl64(). Hence kernel will read some garbage that is a few bytes after
> the 32bit flock structure in this case which will likely end up with the
> syscall returning EINVAL.

Here is also a commit that fixes the corresponding LTP testcase:

https://github.com/linux-test-project/ltp/commit/ae09800dfed8630f67796501bef3a88bb4fd3daa

Before this the fcntl34 test was failing on 32bit platform or with
CFLAGS=-m32 LDFLAGS=-m32 passed to configure.



Before:

testcases/kernel/syscalls/fcntl $./fcntl34
tst_test.c:756: INFO: Timeout per run is 0h 05m 00s
fcntl34.c:104: INFO: write to a file inside threads with OFD locks
fcntl34.c:48: INFO: spawning '12' threads
fcntl34.c:79: BROK: fcntl() failed: EINVAL

Summary:
passed   0
failed   0
skipped  0
warnings 0


After:

testcases/kernel/syscalls/fcntl $./fcntl34
tst_test.c:756: INFO: Timeout per run is 0h 05m 00s
fcntl34.c:104: INFO: write to a file inside threads with OFD locks
fcntl34.c:48: INFO: spawning '12' threads
fcntl34.c:57: INFO: waiting for '12' threads
fcntl34.c:113: INFO: verifying file's data
fcntl34.c:141: PASS: OFD locks synchronized access between threads

Summary:
passed   1
failed   0
skipped  0
warnings 0


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Michael Kerrisk]

[Jeff, can you comment?]

Hi Cyril,

On 08/16/2016 11:55 PM, Cyril Hrubis wrote:
> If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
> EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
> default but the struct flock uses 32bit off_t for 32bit binaries (unless
> _FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
> fcntl64(). Hence kernel will read some garbage that is a few bytes after
> the 32bit flock structure in this case which will likely end up with the
> syscall returning EINVAL.

Okay -- I confirm the problem you report. I'm just not sure that the
patch below is the best fix. So, to summarize:

* On 64-bit, flock{} and flock64{} are the same structure.
* On 32-bit, flock{} and flock64{} are different.
* On 32-bit, F_OFD operations require flock64{}, but the traditional
  F_* lock operations do not.
* To use flock64{} with F_OFD operations, we can either explicitly use
  flock64{} or we can compile with -D_FILE_OFFSET_BITS=64

One solution would be your patch below, but it feels wrong: on 64-bit
flock{} suffices, and is consistent with the traditional F_* operations.
An alternative would be a note in the man page that says something along
the lines that on 32-bit, one must compile with -D_FILE_OFFSET_BITS=64
when using the F_OFD operations.

Your thoughts?

Cheers,

Michael

> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Yuriy Kolerov <Yuriy.Kolerov@synopsys.com>
> ---
>  man2/fcntl.2 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/man2/fcntl.2 b/man2/fcntl.2
> index f0c1acf..4606709 100644
> --- a/man2/fcntl.2
> +++ b/man2/fcntl.2
> @@ -533,7 +533,7 @@ As with traditional advisory locks, the third argument to
>  .BR fcntl (),
>  .IR lock ,
>  is a pointer to an
> -.IR flock
> +.IR flock64
>  structure.
>  By contrast with traditional record locks, the
>  .I l_pid
> @@ -543,7 +543,7 @@ when using the commands described below.
>  The commands for working with open file description locks are analogous
>  to those used with traditional locks:
>  .TP
> -.BR F_OFD_SETLK " (\fIstruct flock *\fP)"
> +.BR F_OFD_SETLK " (\fIstruct flock64 *\fP)"
>  Acquire an open file description lock (when
>  .I l_type
>  is
> @@ -564,7 +564,7 @@ this call returns \-1 and sets
>  to
>  .BR EAGAIN .
>  .TP
> -.BR F_OFD_SETLKW " (\fIstruct flock *\fP)"
> +.BR F_OFD_SETLKW " (\fIstruct flock64 *\fP)"
>  As for
>  .BR F_OFD_SETLK ,
>  but if a conflicting lock is held on the file, then wait for that lock to be
> @@ -578,7 +578,7 @@ set to
>  see
>  .BR signal (7)).
>  .TP
> -.BR F_OFD_GETLK " (\fIstruct flock *\fP)"
> +.BR F_OFD_GETLK " (\fIstruct flock64 *\fP)"
>  On input to this call,
>  .I lock
>  describes an open file description lock we would like to place on the file.
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Jeff Layton]

On Wed, 2016-08-17 at 08:04 +1200, Michael Kerrisk (man-pages) wrote:
> [Jeff, can you comment?]
> 
> Hi Cyril,
> 
> On 08/16/2016 11:55 PM, Cyril Hrubis wrote:
> > 
> > If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
> > EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
> > default but the struct flock uses 32bit off_t for 32bit binaries (unless
> > _FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
> > fcntl64(). Hence kernel will read some garbage that is a few bytes after
> > the 32bit flock structure in this case which will likely end up with the
> > syscall returning EINVAL.
> 
> Okay -- I confirm the problem you report. I'm just not sure that the
> patch below is the best fix. So, to summarize:
> 
> * On 64-bit, flock{} and flock64{} are the same structure.
> * On 32-bit, flock{} and flock64{} are different.
> * On 32-bit, F_OFD operations require flock64{}, but the traditional
>   F_* lock operations do not.
> * To use flock64{} with F_OFD operations, we can either explicitly use
>   flock64{} or we can compile with -D_FILE_OFFSET_BITS=64
> 
> One solution would be your patch below, but it feels wrong: on 64-bit
> flock{} suffices, and is consistent with the traditional F_* operations.
> An alternative would be a note in the man page that says something along
> the lines that on 32-bit, one must compile with -D_FILE_OFFSET_BITS=64
> when using the F_OFD operations.
> 
> Your thoughts?
> 
> Cheers,
> 
> Michael
> 

This sounds like a regular old bug, rather than a documentation issue. 

The way the kernel works is that if you call fcntl(), then you need to
pass in a struct flock. If you call fcntl64() then you need to pass in
a struct flock64. Of course this is only on 32-bit arches. On 64-bit,
it's there is no flock64 or fcntl64.

Typically, glibc papers over all of this by deciding which syscall it's
going to use based on -D_FILE_OFFSET_BITS. IIRC, it basically redefines
the fields in struct flock to be like the one in struct flock64, so you
shouldn't need to do anything special here.

It sounds here like you got a mismatch, somehow and were calling
fcntl64() with the smaller struct flock? Or was it vice versa?

What would be ideal would be a small reproducer program, and
instructions on how to build it. With that we should be able to nail
down why this is happening.

Also, what arch are you using here?

> > > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > > > CC: Yuriy Kolerov <Yuriy.Kolerov@synopsys.com>
> > ---
> >  man2/fcntl.2 | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/man2/fcntl.2 b/man2/fcntl.2
> > index f0c1acf..4606709 100644
> > --- a/man2/fcntl.2
> > +++ b/man2/fcntl.2
> > @@ -533,7 +533,7 @@ As with traditional advisory locks, the third argument to
> >  .BR fcntl (),
> >  .IR lock ,
> >  is a pointer to an
> > -.IR flock
> > +.IR flock64
> >  structure.
> >  By contrast with traditional record locks, the
> >  .I l_pid
> > @@ -543,7 +543,7 @@ when using the commands described below.
> >  The commands for working with open file description locks are analogous
> >  to those used with traditional locks:
> >  .TP
> > -.BR F_OFD_SETLK " (\fIstruct flock *\fP)"
> > +.BR F_OFD_SETLK " (\fIstruct flock64 *\fP)"
> >  Acquire an open file description lock (when
> >  .I l_type
> >  is
> > @@ -564,7 +564,7 @@ this call returns \-1 and sets
> >  to
> >  .BR EAGAIN .
> >  .TP
> > -.BR F_OFD_SETLKW " (\fIstruct flock *\fP)"
> > +.BR F_OFD_SETLKW " (\fIstruct flock64 *\fP)"
> >  As for
> >  .BR F_OFD_SETLK ,
> >  but if a conflicting lock is held on the file, then wait for that lock to be
> > @@ -578,7 +578,7 @@ set to
> >  see
> >  .BR signal (7)).
> >  .TP
> > -.BR F_OFD_GETLK " (\fIstruct flock *\fP)"
> > +.BR F_OFD_GETLK " (\fIstruct flock64 *\fP)"
> >  On input to this call,
> >  .I lock
> >  describes an open file description lock we would like to place on the file.
> > 
> 
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Michael Kerrisk]

Hi Jeff,

Thanks for jumping in.

On 17 August 2016 at 11:41, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Wed, 2016-08-17 at 08:04 +1200, Michael Kerrisk (man-pages) wrote:
>> [Jeff, can you comment?]
>>
>> Hi Cyril,
>>
>> On 08/16/2016 11:55 PM, Cyril Hrubis wrote:
>> >
>> > If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
>> > EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
>> > default but the struct flock uses 32bit off_t for 32bit binaries (unless
>> > _FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
>> > fcntl64(). Hence kernel will read some garbage that is a few bytes after
>> > the 32bit flock structure in this case which will likely end up with the
>> > syscall returning EINVAL.
>>
>> Okay -- I confirm the problem you report. I'm just not sure that the
>> patch below is the best fix. So, to summarize:
>>
>> * On 64-bit, flock{} and flock64{} are the same structure.
>> * On 32-bit, flock{} and flock64{} are different.
>> * On 32-bit, F_OFD operations require flock64{}, but the traditional
>>   F_* lock operations do not.
>> * To use flock64{} with F_OFD operations, we can either explicitly use
>>   flock64{} or we can compile with -D_FILE_OFFSET_BITS=64
>>
>> One solution would be your patch below, but it feels wrong: on 64-bit
>> flock{} suffices, and is consistent with the traditional F_* operations.
>> An alternative would be a note in the man page that says something along
>> the lines that on 32-bit, one must compile with -D_FILE_OFFSET_BITS=64
>> when using the F_OFD operations.
>>
>> Your thoughts?
>>
>> Cheers,
>>
>> Michael
>>
>
> This sounds like a regular old bug, rather than a documentation issue.
>
> The way the kernel works is that if you call fcntl(), then you need to
> pass in a struct flock. If you call fcntl64() then you need to pass in
> a struct flock64. Of course this is only on 32-bit arches. On 64-bit,
> it's there is no flock64 or fcntl64.
>
> Typically, glibc papers over all of this by deciding which syscall it's
> going to use based on -D_FILE_OFFSET_BITS. IIRC, it basically redefines
> the fields in struct flock to be like the one in struct flock64, so you
> shouldn't need to do anything special here.
>
> It sounds here like you got a mismatch, somehow and were calling
> fcntl64() with the smaller struct flock? Or was it vice versa?
>
> What would be ideal would be a small reproducer program, and
> instructions on how to build it. With that we should be able to nail
> down why this is happening.
>
> Also, what arch are you using here?

x86 is enough.

8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#ifndef F_OFD_GETLK     /* In case we are on a system with glibc version
                           earlier than 2.20 */
#define F_OFD_GETLK     36
#define F_OFD_SETLK     37
#define F_OFD_SETLKW    38
#endif


#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                        } while (0)

#define usageErr(msg, progName) \
                        do { fprintf(stderr, "Usage: "); \
                             fprintf(stderr, msg, progName); \
                             exit(EXIT_FAILURE); } while (0)
int
main(int argc, char *argv[])
{
    int fd;
    struct flock fl;

    if (argc < 2)
        usageErr("%s file\n", argv[0]);

    fd = open(argv[1], O_CREAT | O_RDWR, 0600);
    if (fd == -1)
        errExit("open");

    memset(&fl, 99, sizeof(fl));        /* Ensure any uninitialized
                                           bytes contain junk */

    fl.l_start = 0;
    fl.l_len = 1;
    fl.l_type = F_WRLCK;
    fl.l_whence = SEEK_CUR;
    fl.l_pid = 0;
    if (fcntl(fd, F_OFD_SETLK, &fl) == -1)
        errExit("fcntl");
}

8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---

$ cc -m32 prog.c
$ ./a.out /tmp/x
fcntl: Invalid argument

Cheers,

Michael

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> The way the kernel works is that if you call fcntl(), then you need to
> pass in a struct flock. If you call fcntl64() then you need to pass in
> a struct flock64. Of course this is only on 32-bit arches. On 64-bit,
> it's there is no flock64 or fcntl64.

This does not seem to be the case.

It looks like kernel expect F_{SET,GET}LK to be 32bit for fcntl64() as
well. It just calls do_fcntl() that casts the pointer to struct flock
that seems to be defined with __kernel_off_t in the uapi headers which
is alias to long.

And the same in the compat implementation, there the F_{SET,GET}LK works
with struct compat_flock that has 32bit off_t as well.

Then we have the F_{SET,GET}LK64 that expect 64bit flock and the
F_OFD_XXX behaves exactly same. As a matter of the fact they are handled
mostly in the same branches of the switch() statements which lead me to
belive that they were intended to be used with flock64 explicitly.

Glibc does not seem to do much work here. The only thing it does is to
switch between the F_{SET,GET}LK and F_{SET,GET}LK64 based on
_FILE_OFFSET_BITS to match the off_t type in the flock structure.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Jeff Layton]

On Wed, 2016-08-17 at 10:10 +0200, Cyril Hrubis wrote:
> Hi!
> > 
> > The way the kernel works is that if you call fcntl(), then you need to
> > pass in a struct flock. If you call fcntl64() then you need to pass in
> > a struct flock64. Of course this is only on 32-bit arches. On 64-bit,
> > it's there is no flock64 or fcntl64.
> 
> This does not seem to be the case.
> 
> It looks like kernel expect F_{SET,GET}LK to be 32bit for fcntl64() as
> well. It just calls do_fcntl() that casts the pointer to struct flock
> that seems to be defined with __kernel_off_t in the uapi headers which
> is alias to long.
> 
> And the same in the compat implementation, there the F_{SET,GET}LK works
> with struct compat_flock that has 32bit off_t as well.
> 
> Then we have the F_{SET,GET}LK64 that expect 64bit flock and the
> F_OFD_XXX behaves exactly same. As a matter of the fact they are handled
> mostly in the same branches of the switch() statements which lead me to
> belive that they were intended to be used with flock64 explicitly.
> 
> Glibc does not seem to do much work here. The only thing it does is to
> switch between the F_{SET,GET}LK and F_{SET,GET}LK64 based on
> _FILE_OFFSET_BITS to match the off_t type in the flock structure.
> 

Thanks, I think I understand now. I think there are a couple of
potential fixes...

The simplest thing is to do what you're suggesting and simply document
that F_OFD_* locks require large file offsets. If we do that though,
then I think we also ought to do something to ensure that the build
breaks if you try to use F_OFD_* commands without large offsets.

The simplest way would be to put the F_OFD_* constant definitions under
"#ifdef __USE_FILE_OFFSET64", but I'm open to suggestions that would
make the compiler error out with a more helpful error message.

The other option would be to fix glibc and the kernel to handle legacy
struct flock with F_OFD_ cmd values. That would mean adding F_OFD_*64
command values and fixing glibc to swap them in appropriately. That's
doable, but I'm not sure it's really worth it. We'd also have to think
about how to handle the old kernel/new glibc case (and vice versa), and
that probably won't be trivial.

I'm leaning toward option #1 here. I can cook up a patch for glibc if
you guys agree.

Thoughts?
-- 
Jeff Layton <jlayton@poochiereds.net>

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> Thanks, I think I understand now. I think there are a couple of
> potential fixes...
> 
> The simplest thing is to do what you're suggesting and simply document
> that F_OFD_* locks require large file offsets. If we do that though,
> then I think we also ought to do something to ensure that the build
> breaks if you try to use F_OFD_* commands without large offsets.
> 
> The simplest way would be to put the F_OFD_* constant definitions under
> "#ifdef??__USE_FILE_OFFSET64", but I'm open to suggestions that would
> make the compiler error out with a more helpful error message.

Hmm, I do not think that this is a good idea. The usuall way how to
handle missing constants are fallback definitions such as:

#ifndef F_OFD_FOO
# define F_OFD_FOO xyz
#endif

This wouldn't do much.

Also these should be only disable on 32bit if __USE_FILE_OFFSET64 is not
defined. But you likely meant that here.

> The other option would be to fix glibc and the kernel to handle legacy
> struct flock with F_OFD_ cmd values. That would mean adding F_OFD_*64
> command values and fixing glibc to swap them in appropriately. That's
> doable, but I'm not sure it's really worth it. We'd also have to think
> about how to handle the old kernel/new glibc case (and vice versa), and
> that probably won't be trivial.

I do not think that this is worth the work either.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Jeff Layton]

On Wed, 2016-08-17 at 13:53 +0200, Cyril Hrubis wrote:
> Hi!
> > 
> > Thanks, I think I understand now. I think there are a couple of
> > potential fixes...
> > 
> > The simplest thing is to do what you're suggesting and simply
> > document
> > that F_OFD_* locks require large file offsets. If we do that
> > though,
> > then I think we also ought to do something to ensure that the build
> > breaks if you try to use F_OFD_* commands without large offsets.
> > 
> > The simplest way would be to put the F_OFD_* constant definitions
> > under
> > "#ifdef??__USE_FILE_OFFSET64", but I'm open to suggestions that
> > would
> > make the compiler error out with a more helpful error message.
> 
> Hmm, I do not think that this is a good idea. The usuall way how to
> handle missing constants are fallback definitions such as:
> 
> #ifndef F_OFD_FOO
> # define F_OFD_FOO xyz
> #endif
> 
> This wouldn't do much.
> 
> Also these should be only disable on 32bit if __USE_FILE_OFFSET64 is
> not
> defined. But you likely meant that here.
> 

That's the usual way, but in this case we wouldn't have a fallback
constant. You'd just get an error about F_OFD_* being undefined at
build time, which I think is what we'd want here. It's better to fail
to compile than to build a binary that passes a bogus struct into the
kernel.

> > The other option would be to fix glibc and the kernel to handle
> > legacy
> > struct flock with F_OFD_ cmd values. That would mean adding
> > F_OFD_*64
> > command values and fixing glibc to swap them in appropriately.
> > That's
> > doable, but I'm not sure it's really worth it. We'd also have to
> > think
> > about how to handle the old kernel/new glibc case (and vice versa),
> > and
> > that probably won't be trivial.
> 
> I do not think that this is worth the work either.
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> > Hmm, I do not think that this is a good idea. The usuall way how to
> > handle missing constants are fallback definitions such as:
> > 
> > #ifndef F_OFD_FOO
> > # define F_OFD_FOO xyz
> > #endif
> > 
> > This wouldn't do much.
> > 
> > Also these should be only disable on 32bit if __USE_FILE_OFFSET64 is
> > not
> > defined. But you likely meant that here.
> > 
> 
> That's the usual way, but in this case we wouldn't have a fallback
> constant. You'd just get an error about F_OFD_* being undefined at
> build time, which I think is what we'd want here. It's better to fail
> to compile than to build a binary that passes a bogus struct into the
> kernel.

You probably misunderstand what I was trying to say. If you look at the
sources out there (for instance at https://codesearch.debian.net/) most
of it has fallback definitions for F_OFD_* constants included in its own
header files since these flags are relatively new. Not defining these
would not accomplish anything.

One option would be to define them to something invalid such as INT_MAX
so that it's rejected by kernel on runtime. But I do not think this is
very good idea either.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Jeff Layton]

On Wed, 2016-08-17 at 15:19 +0200, Cyril Hrubis wrote:
> Hi!
> > 
> > > 
> > > Hmm, I do not think that this is a good idea. The usuall way how to
> > > handle missing constants are fallback definitions such as:
> > > 
> > > #ifndef F_OFD_FOO
> > > # define F_OFD_FOO xyz
> > > #endif
> > > 
> > > This wouldn't do much.
> > > 
> > > Also these should be only disable on 32bit if __USE_FILE_OFFSET64 is
> > > not
> > > defined. But you likely meant that here.
> > > 
> > 
> > That's the usual way, but in this case we wouldn't have a fallback
> > constant. You'd just get an error about F_OFD_* being undefined at
> > build time, which I think is what we'd want here. It's better to fail
> > to compile than to build a binary that passes a bogus struct into the
> > kernel.
> 
> You probably misunderstand what I was trying to say. If you look at the
> > sources out there (for instance at https://codesearch.debian.net/) most
> of it has fallback definitions for F_OFD_* constants included in its own
> header files since these flags are relatively new. Not defining these
> would not accomplish anything.
> 
> One option would be to define them to something invalid such as INT_MAX
> so that it's rejected by kernel on runtime. But I do not think this is
> very good idea either.
> 

Yeah, not much we can do about people that define them on their own. If
you do that, then you're basically saying "I know what I'm doing".

Still, I think it's worthwhile to do this in glibc since we _can_
prevent this problem for folks who aren't doing that.

-- 
Jeff Layton <jlayton@poochiereds.net>

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> > You probably misunderstand what I was trying to say. If you look at the
> > > sources out there (for instance at https://codesearch.debian.net/) most
> > of it has fallback definitions for F_OFD_* constants included in its own
> > header files since these flags are relatively new. Not defining these
> > would not accomplish anything.
> > 
> > One option would be to define them to something invalid such as INT_MAX
> > so that it's rejected by kernel on runtime. But I do not think this is
> > very good idea either.
> > 
> 
> Yeah, not much we can do about people that define them on their own. If
> you do that, then you're basically saying "I know what I'm doing".
> 
> Still, I think it's worthwhile to do this in glibc since we _can_
> prevent this problem for folks who aren't doing that.

Ok.

Then this should be also paired with patch for the manual page that
explains that these locks are only available with the
_FILE_OFFSET_BITS=64.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Michael Kerrisk]

Hi Jeff,

On 08/17/2016 11:44 PM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 10:10 +0200, Cyril Hrubis wrote:
>> Hi!
>>>
>>> The way the kernel works is that if you call fcntl(), then you need to
>>> pass in a struct flock. If you call fcntl64() then you need to pass in
>>> a struct flock64. Of course this is only on 32-bit arches. On 64-bit,
>>> it's there is no flock64 or fcntl64.
>>
>> This does not seem to be the case.
>>
>> It looks like kernel expect F_{SET,GET}LK to be 32bit for fcntl64() as
>> well. It just calls do_fcntl() that casts the pointer to struct flock
>> that seems to be defined with __kernel_off_t in the uapi headers which
>> is alias to long.
>>
>> And the same in the compat implementation, there the F_{SET,GET}LK works
>> with struct compat_flock that has 32bit off_t as well.
>>
>> Then we have the F_{SET,GET}LK64 that expect 64bit flock and the
>> F_OFD_XXX behaves exactly same. As a matter of the fact they are handled
>> mostly in the same branches of the switch() statements which lead me to
>> belive that they were intended to be used with flock64 explicitly.
>>
>> Glibc does not seem to do much work here. The only thing it does is to
>> switch between the F_{SET,GET}LK and F_{SET,GET}LK64 based on
>> _FILE_OFFSET_BITS to match the off_t type in the flock structure.
>>
> 
> Thanks, I think I understand now. I think there are a couple of
> potential fixes...
> 
> The simplest thing is to do what you're suggesting and simply document
> that F_OFD_* locks require large file offsets. If we do that though,
> then I think we also ought to do something to ensure that the build
> breaks if you try to use F_OFD_* commands without large offsets.
> 
> The simplest way would be to put the F_OFD_* constant definitions under
> "#ifdef __USE_FILE_OFFSET64", but I'm open to suggestions that would
> make the compiler error out with a more helpful error message.
> 
> The other option would be to fix glibc and the kernel to handle legacy
> struct flock with F_OFD_ cmd values. That would mean adding F_OFD_*64
> command values and fixing glibc to swap them in appropriately. That's
> doable, but I'm not sure it's really worth it. We'd also have to think
> about how to handle the old kernel/new glibc case (and vice versa), and
> that probably won't be trivial.
> 
> I'm leaning toward option #1 here. I can cook up a patch for glibc if
> you guys agree.
> 
> Thoughts?

Looking at the long picture, it seems to me that option 2 would be
preferable, albeit more work.

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[LTP] [PATCH] fcntl.2: F_OFD_XXX needs flock64

[Cyril Hrubis]

Hi!
> > If we pass struct flock to the F_OFD_XXX fcntl() it will fail with
> > EINVAL with a 32bit binary. That is because glibc uses fcntl64() by
> > default but the struct flock uses 32bit off_t for 32bit binaries (unless
> > _FILE_OFFSET_BITS=64) and kernel always expect flock64 for F_OFD_XXX in
> > fcntl64(). Hence kernel will read some garbage that is a few bytes after
> > the 32bit flock structure in this case which will likely end up with the
> > syscall returning EINVAL.
> 
> Okay -- I confirm the problem you report. I'm just not sure that the
> patch below is the best fix. So, to summarize:

Either we do that or we have to translate the flock{} to flock64{} at
the runtime if F_OFD_XXX was the fcntl() cmd. However the problem is
that we have no idea if _FILE_OFFSET_BITS was set or not once we reach
fcntl.c in glibc. So the whole translation would have been put into the
fcntl.h header into some ugly macro or we would have to do some trickery
like passing down the sizeof(struct flock) as additional fcntl
parameter.

> One solution would be your patch below, but it feels wrong: on 64-bit
> flock{} suffices, and is consistent with the traditional F_* operations.
> An alternative would be a note in the man page that says something along
> the lines that on 32-bit, one must compile with -D_FILE_OFFSET_BITS=64
> when using the F_OFD operations.

That would be solution as well.

-- 
Cyril Hrubis
chrubis@suse.cz