[glibc] fcntl: put F_OFD_* constants under #ifdef __USE_FILE_OFFSET64
diff mbox

Message ID 1471445251-2450-1-git-send-email-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton Aug. 17, 2016, 2:47 p.m. UTC
The Linux kernel expects a flock64 structure whenever you use OFD locks
with fcntl64. Unfortunately, you can currently build a 32-bit program
that passes in a struct flock when it calls fcntl64.

Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
defined, so that the build fails in this situation rather than
producing a broken binary.

Reported-by: Cyril Hrubis <chrubis@suse.cz>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Yuriy Kolerov <Yuriy.Kolerov@synopsys.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 manual/examples/ofdlocks.c                 |  1 +
 manual/llio.texi                           |  8 +++++---
 sysdeps/unix/sysv/linux/bits/fcntl-linux.h | 15 +++++++++++----
 3 files changed, 17 insertions(+), 7 deletions(-)

Comments

Joseph Myers Aug. 17, 2016, 3:44 p.m. UTC | #1
On Wed, 17 Aug 2016, Jeff Layton wrote:

> +# if __WORDSIZE != 32 || defined __USE_FILE_OFFSET64

Are you sure __WORDSIZE is always defined here?  I don't see an include of 
<bits/wordsize.h> in this header.

Are you sure __WORDSIZE != 32 is the right condition on all architectures 
for the flock and flock64 structures being the same?  Wordsize is not a 
particularly well-defined concept all cases.  More specific tests tend to 
be preferred, e.g. __OFF_T_MATCHES_OFF64_T in bits/typesizes.h (so this 
would indicate having a new macro __FLOCK_MATCHES_FLOCK64 and arranging 
for it to be defined to 1 or 0 correctly in all cases - or at least a 
careful analysis of all architectures using this file to show that some 
other conditional is always correct).
Mike Frysinger Aug. 17, 2016, 4:13 p.m. UTC | #2
On 17 Aug 2016 10:47, Jeff Layton wrote:
> The Linux kernel expects a flock64 structure whenever you use OFD locks
> with fcntl64. Unfortunately, you can currently build a 32-bit program
> that passes in a struct flock when it calls fcntl64.
> 
> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> defined, so that the build fails in this situation rather than
> producing a broken binary.

what about ILP32 targets like x32 ?  sizeof(flock) == sizeof(flock64)
is the same there.

> --- a/manual/examples/ofdlocks.c
> +++ b/manual/examples/ofdlocks.c
> @@ -15,6 +15,7 @@
>     along with this program; if not, see <http://www.gnu.org/licenses/>.
>  */
>  
> +/* Note that this must be built with -D_FILE_OFFSET_BITS=64 on 32-bit arch */

GNU style says comments are complete sentences (so ends with a period),
and there's two spaces after the period.

> --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> @@ -127,11 +127,18 @@
>     This means that they are inherited across fork or clone with CLONE_FILES
>     like BSD (flock) locks, and they are only released automatically when the
>     last reference to the the file description against which they were acquired
> -   is put. */
> +   is put.
> +
> +   Note that the kernel does not support the legacy struct flock on 32-bit
> +   arches with OFD locks. On those arches you need define both _GNU_SOURCE
> +   and _FILE_OFFSET_BITS=64.
> +   */

comment style says the */ has to be on the previous line -- look at how
the code looked before your change.  also, two spaces after periods.
-mike
Florian Weimer Aug. 17, 2016, 5:34 p.m. UTC | #3
On 08/17/2016 04:47 PM, Jeff Layton wrote:
> The Linux kernel expects a flock64 structure whenever you use OFD locks
> with fcntl64. Unfortunately, you can currently build a 32-bit program
> that passes in a struct flock when it calls fcntl64.
>
> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> defined, so that the build fails in this situation rather than
> producing a broken binary.

Doesn't this affect legacy POSIX-style locks as well, under very similar 
circumstances?

Thanks,
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Aug. 17, 2016, 5:39 p.m. UTC | #4
On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
> On 08/17/2016 04:47 PM, Jeff Layton wrote:
> > 
> > The Linux kernel expects a flock64 structure whenever you use OFD locks
> > with fcntl64. Unfortunately, you can currently build a 32-bit program
> > that passes in a struct flock when it calls fcntl64.
> > 
> > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> > defined, so that the build fails in this situation rather than
> > producing a broken binary.
> 
> Doesn't this affect legacy POSIX-style locks as well, under very similar 
> circumstances?
> 
> 

No. The kernel will decide which type of struct it is based on whether
userland passes in F_SETLK or F_SETLK64. Since the older flock struct
is considered a legacy interface, I didn't plumb that in when I did
these patches originally. With my 20/20 hindsight, I probably should
have just done that, but it's a little late now...
Jeff Layton Aug. 17, 2016, 5:49 p.m. UTC | #5
On Wed, 2016-08-17 at 15:44 +0000, Joseph Myers wrote:
> On Wed, 17 Aug 2016, Jeff Layton wrote:
> 
> > 
> > +# if __WORDSIZE != 32 || defined __USE_FILE_OFFSET64
> 
> Are you sure __WORDSIZE is always defined here?  I don't see an include of 
> <bits/wordsize.h> in this header.
> 
> Are you sure __WORDSIZE != 32 is the right condition on all architectures 
> for the flock and flock64 structures being the same?  Wordsize is not a 
> particularly well-defined concept all cases.  More specific tests tend to 
> be preferred, e.g. __OFF_T_MATCHES_OFF64_T in bits/typesizes.h (so this 
> would indicate having a new macro __FLOCK_MATCHES_FLOCK64 and arranging 
> for it to be defined to 1 or 0 correctly in all cases - or at least a 
> careful analysis of all architectures using this file to show that some 
> other conditional is always correct).
> 

Ok, that makes sense -- thanks.

The only difference between struct flock and flock64 is the size of the
offset values. So, I think that __OFF_T_MATCHES_OFF64_T would suffice
here, actually. Is there any reason to do anything more elaborate than
this in place of what I proposed earlier?

    #if defined __OFF_T_MATCHES_OFF64_T || defined __USE_FILE_OFFSET64
Joseph Myers Aug. 17, 2016, 5:56 p.m. UTC | #6
On Wed, 17 Aug 2016, Jeff Layton wrote:

> The only difference between struct flock and flock64 is the size of the
> offset values. So, I think that __OFF_T_MATCHES_OFF64_T would suffice

Well, MIPS has e.g.:

#if _MIPS_SIM != _ABI64
    /* The 64-bit flock structure, used by the n64 ABI, and for 64-bit
       fcntls in o32 and n32, never has this field.  */
    long int l_sysid;
#endif

Now, this doesn't actually cause issues, because __OFF_T_MATCHES_OFF64_T 
isn't true for o32 or n32, and the layouts are indeed the same for n64.  
But you need to check every architecture to make sure there aren't any 
such issues that mean __OFF_T_MATCHES_OFF64_T is the wrong condition.
Florian Weimer Aug. 17, 2016, 6:02 p.m. UTC | #7
On 08/17/2016 07:39 PM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
>> On 08/17/2016 04:47 PM, Jeff Layton wrote:
>>>
>>> The Linux kernel expects a flock64 structure whenever you use OFD locks
>>> with fcntl64. Unfortunately, you can currently build a 32-bit program
>>> that passes in a struct flock when it calls fcntl64.
>>>
>>> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
>>> defined, so that the build fails in this situation rather than
>>> producing a broken binary.
>>
>> Doesn't this affect legacy POSIX-style locks as well, under very similar
>> circumstances?
>>
>>
>
> No. The kernel will decide which type of struct it is based on whether
> userland passes in F_SETLK or F_SETLK64.

Let me see if I can sort this out.  Is the situation like this?

         _FILE_OFFSET_…    …BITS == 32          …BITS == 64
         struct …       flock   flock64    flock   flock64
fcntl (F_SETLK)        ok      BAD        ok      BAD
fcntl (F_SETLK64)      BAD     ok         ok      ok
fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok

¹ is broken by your patch, right?

Looking at the definition of struct flock and struct flock64, the risk 
is that application silently succeed in locking the wrong thing when 
using struct flock64 with a 32-it interface.

Thanks,
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Aug. 17, 2016, 6:21 p.m. UTC | #8
On Wed, 2016-08-17 at 20:02 +0200, Florian Weimer wrote:
> On 08/17/2016 07:39 PM, Jeff Layton wrote:
> > 
> > On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
> > > 
> > > On 08/17/2016 04:47 PM, Jeff Layton wrote:
> > > > 
> > > > 
> > > > The Linux kernel expects a flock64 structure whenever you use
> > > > OFD locks
> > > > with fcntl64. Unfortunately, you can currently build a 32-bit
> > > > program
> > > > that passes in a struct flock when it calls fcntl64.
> > > > 
> > > > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is
> > > > also
> > > > defined, so that the build fails in this situation rather than
> > > > producing a broken binary.
> > > 
> > > Doesn't this affect legacy POSIX-style locks as well, under very
> > > similar
> > > circumstances?
> > > 
> > > 
> > 
> > No. The kernel will decide which type of struct it is based on
> > whether
> > userland passes in F_SETLK or F_SETLK64.
> 
> Let me see if I can sort this out.  Is the situation like this?
> 
>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>          struct …       flock   flock64    flock   flock64
> fcntl (F_SETLK)        ok      BAD        ok      BAD
> fcntl (F_SETLK64)      BAD     ok         ok      ok
> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
> 
> ¹ is broken by your patch, right?

Not sure I 100% understand your chart, but if I do then I think it's
more like:

         _FILE_OFFSET_…    …BITS == 32          …BITS == 64
         struct …       flock   flock64    flock   flock64
fcntl (F_SETLK)        ok      BAD        ok      ok
fcntl (F_SETLK64)      BAD     ok         ok      ok
fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok

struct flock and struct flock64 are generally equivalent when
_FILE_OFFSET_BITS==64.

I don't quite understand how ¹ would be broken by this patch. The idea
with the patch is to ensure that if you haven't defined
_FILE_OFFSET_BITS=64 on a 32 bit arch, that it's broken at compile time
instead of at runtime.

> 
> Looking at the definition of struct flock and struct flock64, the
> risk 
> is that application silently succeed in locking the wrong thing when 
> using struct flock64 with a 32-it interface.


Yes. The basic problem is that the kernel will expect a struct flock64,
but if you don't set _FILE_OFFSET_BITS=64 glibc will pass in a legacy
struct flock instead. The kernel can then read beyond the end of the
struct.

The bytes in l_start and l_len will be slurped into the kernel's
l_start field. The pid and whatever junk is beyond the struct will be
in the l_len and pid fields.

It's also possible the program will get back EFAULT as well if
copy_from_user fails.
Jeff Layton Aug. 17, 2016, 6:23 p.m. UTC | #9
On Wed, 2016-08-17 at 17:56 +0000, Joseph Myers wrote:
> On Wed, 17 Aug 2016, Jeff Layton wrote:
> 
> > 
> > The only difference between struct flock and flock64 is the size of the
> > offset values. So, I think that __OFF_T_MATCHES_OFF64_T would suffice
> 
> Well, MIPS has e.g.:
> 
> #if _MIPS_SIM != _ABI64
>     /* The 64-bit flock structure, used by the n64 ABI, and for 64-bit
>        fcntls in o32 and n32, never has this field.  */
>     long int l_sysid;
> #endif
> 
> Now, this doesn't actually cause issues, because __OFF_T_MATCHES_OFF64_T 
> isn't true for o32 or n32, and the layouts are indeed the same for n64.  
> But you need to check every architecture to make sure there aren't any 
> such issues that mean __OFF_T_MATCHES_OFF64_T is the wrong condition.
> 

Yeah, I saw that but all of that is down inside the pad that the kernel
doesn't really care about. The important bits are the parts up to and
including the pid field.

I did go through all of them before mentioning that and I still think
it is sufficient, but I certainly wouldn't mind having someone sanity
check me here!
Mike Frysinger Aug. 17, 2016, 6:43 p.m. UTC | #10
On 17 Aug 2016 10:47, Jeff Layton wrote:
> The Linux kernel expects a flock64 structure whenever you use OFD locks
> with fcntl64. Unfortunately, you can currently build a 32-bit program
> that passes in a struct flock when it calls fcntl64.
> 
> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> defined, so that the build fails in this situation rather than
> producing a broken binary.

this seems to be going against the glibc API/guarantees we've provided
before (or at least tried to promise), and what the fcntl(2) man page
says now.  namely, we haven't documented F_GETLK64 or struct flock64,
with the expectation that the user just calls fcntl() with a struct
flock.  in fact, the man page even goes so far as to discourage people
from using the *64 variants.

it should be possible using our existing LFS framework to make the OFD
cmds available even to 32-bit apps (where sizeof(off_t) == 32).  but
maybe the usage of F_GETLK64/struct flock64/etc... in the real world
has made it hard to put that genie back in the bottle ?  we'd have to
version the current fcntl symbol, create a new fcntl symbol that does
32->64 munging, and add a new fcntl64 symbol that we'd transparently
rewrite to when LFS is turned on.
-mike
Florian Weimer Aug. 17, 2016, 6:51 p.m. UTC | #11
On 08/17/2016 08:21 PM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 20:02 +0200, Florian Weimer wrote:
>> On 08/17/2016 07:39 PM, Jeff Layton wrote:
>>>
>>> On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
>>>>
>>>> On 08/17/2016 04:47 PM, Jeff Layton wrote:
>>>>>
>>>>>
>>>>> The Linux kernel expects a flock64 structure whenever you use
>>>>> OFD locks
>>>>> with fcntl64. Unfortunately, you can currently build a 32-bit
>>>>> program
>>>>> that passes in a struct flock when it calls fcntl64.
>>>>>
>>>>> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is
>>>>> also
>>>>> defined, so that the build fails in this situation rather than
>>>>> producing a broken binary.
>>>>
>>>> Doesn't this affect legacy POSIX-style locks as well, under very
>>>> similar
>>>> circumstances?
>>>>
>>>>
>>>
>>> No. The kernel will decide which type of struct it is based on
>>> whether
>>> userland passes in F_SETLK or F_SETLK64.
>>
>> Let me see if I can sort this out.  Is the situation like this?
>>
>>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>>          struct …       flock   flock64    flock   flock64
>> fcntl (F_SETLK)        ok      BAD        ok      BAD
>> fcntl (F_SETLK64)      BAD     ok         ok      ok
>> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
>>
>> ¹ is broken by your patch, right?
>
> Not sure I 100% understand your chart, but if I do then I think it's
> more like:
>
>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>          struct …       flock   flock64    flock   flock64
> fcntl (F_SETLK)        ok      BAD        ok      ok
> fcntl (F_SETLK64)      BAD     ok         ok      ok
> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
>
> struct flock and struct flock64 are generally equivalent when
> _FILE_OFFSET_BITS==64.

Why would the F_SETLK operation work with a struct flock64 in 
_FILE_OFFSET_BITS == 64 mode?  I think the kernel still expects a 32-bit 
struct.

glibc does not look at O_LARGEFILE and alters size expectations. 
Neither does the kernel.

> I don't quite understand how ¹ would be broken by this patch. The idea
> with the patch is to ensure that if you haven't defined
> _FILE_OFFSET_BITS=64 on a 32 bit arch, that it's broken at compile time
> instead of at runtime.

Compile time breakage is still breakage.  I want to avoid another 
strerror_r situation where it's very hard to get the job done due to the 
way the preprocessor conditionals work out.

>> Looking at the definition of struct flock and struct flock64, the
>> risk
>> is that application silently succeed in locking the wrong thing when
>> using struct flock64 with a 32-it interface.
>>
>
> Yes. The basic problem is that the kernel will expect a struct flock64,
> but if you don't set _FILE_OFFSET_BITS=64 glibc will pass in a legacy
> struct flock instead. The kernel can then read beyond the end of the
> struct.
>
> The bytes in l_start and l_len will be slurped into the kernel's
> l_start field. The pid and whatever junk is beyond the struct will be
> in the l_len and pid fields.
>
> It's also possible the program will get back EFAULT as well if
> copy_from_user fails.

I was mainly worried about the reverse case (calling 32-bit fcntl with 
struct flock64).  But this cannot happen because glibc always calls 
fcntl64 on 32-bit architectures.

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Aug. 17, 2016, 7:15 p.m. UTC | #12
On Wed, 2016-08-17 at 11:43 -0700, Mike Frysinger wrote:
> On 17 Aug 2016 10:47, Jeff Layton wrote:
> > 
> > The Linux kernel expects a flock64 structure whenever you use OFD locks
> > with fcntl64. Unfortunately, you can currently build a 32-bit program
> > that passes in a struct flock when it calls fcntl64.
> > 
> > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> > defined, so that the build fails in this situation rather than
> > producing a broken binary.
> 
> this seems to be going against the glibc API/guarantees we've provided
> before (or at least tried to promise), and what the fcntl(2) man page
> says now.  namely, we haven't documented F_GETLK64 or struct flock64,
> with the expectation that the user just calls fcntl() with a struct
> flock.  in fact, the man page even goes so far as to discourage people
> from using the *64 variants.
> 
> it should be possible using our existing LFS framework to make the OFD
> cmds available even to 32-bit apps (where sizeof(off_t) == 32).  but
> maybe the usage of F_GETLK64/struct flock64/etc... in the real world
> has made it hard to put that genie back in the bottle ?  we'd have to
> version the current fcntl symbol, create a new fcntl symbol that does
> 32->64 munging, and add a new fcntl64 symbol that we'd transparently
> rewrite to when LFS is turned on.
> -mike


There should be no need to use struct flock64 explicitly, and there is
already a proposed patch to fix the manpage accordingly.

What we _do_ want to ensure is that large file offsets are in use if
the application wants to use OFD locks (either by virtue of being on a
64 bit arch, or by defining _FILE_OFFSET_BITS=64).

In principle, we could try to fix it up so that the kernel can handle
OFD locks with legacy struct flock. That would mean adding
F_OFD_SETLK64 and friends in both the kernel and glibc, and we'd have
to ensure that legacy kernel+new glibc is handled sanely (and vice-
versa). That's a lot of effort (and more risk for breakage) to handle a
use case that I'm not sure even exists. This approach is much simpler,
and we'll just be breaking at build time a case that was already broken
at runtime.

In hindsight, I wish I had just introduced F_OFD_SETLK64 and friends to
make them work with legacy struct flock when I did these patches (mea
culpa!), but I don't really see the value in doing that at this point.
Jeff Layton Aug. 17, 2016, 7:20 p.m. UTC | #13
On Wed, 2016-08-17 at 20:51 +0200, Florian Weimer wrote:
> On 08/17/2016 08:21 PM, Jeff Layton wrote:
> > 
> > On Wed, 2016-08-17 at 20:02 +0200, Florian Weimer wrote:
> > > 
> > > On 08/17/2016 07:39 PM, Jeff Layton wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
> > > > > 
> > > > > 
> > > > > On 08/17/2016 04:47 PM, Jeff Layton wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > The Linux kernel expects a flock64 structure whenever you
> > > > > > use
> > > > > > OFD locks
> > > > > > with fcntl64. Unfortunately, you can currently build a 32-
> > > > > > bit
> > > > > > program
> > > > > > that passes in a struct flock when it calls fcntl64.
> > > > > > 
> > > > > > Only define the F_OFD_* constants when __USE_FILE_OFFSET64
> > > > > > is
> > > > > > also
> > > > > > defined, so that the build fails in this situation rather
> > > > > > than
> > > > > > producing a broken binary.
> > > > > 
> > > > > Doesn't this affect legacy POSIX-style locks as well, under
> > > > > very
> > > > > similar
> > > > > circumstances?
> > > > > 
> > > > > 
> > > > 
> > > > No. The kernel will decide which type of struct it is based on
> > > > whether
> > > > userland passes in F_SETLK or F_SETLK64.
> > > 
> > > Let me see if I can sort this out.  Is the situation like this?
> > > 
> > >          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
> > >          struct …       flock   flock64    flock   flock64
> > > fcntl (F_SETLK)        ok      BAD        ok      BAD
> > > fcntl (F_SETLK64)      BAD     ok         ok      ok
> > > fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
> > > 
> > > ¹ is broken by your patch, right?
> > 
> > Not sure I 100% understand your chart, but if I do then I think
> > it's
> > more like:
> > 
> >          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
> >          struct …       flock   flock64    flock   flock64
> > fcntl (F_SETLK)        ok      BAD        ok      ok
> > fcntl (F_SETLK64)      BAD     ok         ok      ok
> > fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
> > 
> > struct flock and struct flock64 are generally equivalent when
> > _FILE_OFFSET_BITS==64.
> 
> Why would the F_SETLK operation work with a struct flock64 in 
> _FILE_OFFSET_BITS == 64 mode?  I think the kernel still expects a 32-
> bit 
> struct.
> 

That's the part that was a little unclear to me in your diagram. Does
the struct refer to the struct definition in the program, or what's
_actually_ being passed into the kernel? I assumed the former since it
was co-located with the part about _FILE_OFFSET_BITS.

If it's what's being passed into the kernel then you're correct, and
the chart would look more like this, I think:

         _FILE_OFFSET_…    …BITS == 32          …BITS == 64
         struct …       flock   flock64    flock   flock64
fcntl64(F_SETLK)        ok      BAD        ok      BAD
fcntl64(F_SETLK64)      BAD     ok         BAD      ok
fcntl64(F_OFD_SETLK)    BAD     ok¹        BAD      ok

> glibc does not look at O_LARGEFILE and alters size expectations. 
> Neither does the kernel.
> 
> > 
> > I don't quite understand how ¹ would be broken by this patch. The
> > idea
> > with the patch is to ensure that if you haven't defined
> > _FILE_OFFSET_BITS=64 on a 32 bit arch, that it's broken at compile
> > time
> > instead of at runtime.
> 
> Compile time breakage is still breakage.  I want to avoid another 
> strerror_r situation where it's very hard to get the job done due to
> the 
> way the preprocessor conditionals work out.
> 

It is, but it's preferable to unexpected behavior at runtime. I think
it's entirely reasonable to require large file offsets in order to use
OFD locks, but I'm willing to be convinced otherwise if there are use
cases that you know of that this will break.

> > 
> > > 
> > > Looking at the definition of struct flock and struct flock64, the
> > > risk
> > > is that application silently succeed in locking the wrong thing
> > > when
> > > using struct flock64 with a 32-it interface.
> > > 
> > 
> > Yes. The basic problem is that the kernel will expect a struct
> > flock64,
> > but if you don't set _FILE_OFFSET_BITS=64 glibc will pass in a
> > legacy
> > struct flock instead. The kernel can then read beyond the end of
> > the
> > struct.
> > 
> > The bytes in l_start and l_len will be slurped into the kernel's
> > l_start field. The pid and whatever junk is beyond the struct will
> > be
> > in the l_len and pid fields.
> > 
> > It's also possible the program will get back EFAULT as well if
> > copy_from_user fails.
> 
> I was mainly worried about the reverse case (calling 32-bit fcntl
> with 
> struct flock64).  But this cannot happen because glibc always calls 
> fcntl64 on 32-bit architectures.
> 

Yes.
Michael Kerrisk (man-pages) Aug. 17, 2016, 7:59 p.m. UTC | #14
On 08/18/2016 07:15 AM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 11:43 -0700, Mike Frysinger wrote:
>> On 17 Aug 2016 10:47, Jeff Layton wrote:
>>>
>>> The Linux kernel expects a flock64 structure whenever you use OFD locks
>>> with fcntl64. Unfortunately, you can currently build a 32-bit program
>>> that passes in a struct flock when it calls fcntl64.
>>>
>>> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
>>> defined, so that the build fails in this situation rather than
>>> producing a broken binary.
>>
>> this seems to be going against the glibc API/guarantees we've provided
>> before (or at least tried to promise), and what the fcntl(2) man page
>> says now.  namely, we haven't documented F_GETLK64 or struct flock64,
>> with the expectation that the user just calls fcntl() with a struct
>> flock.  in fact, the man page even goes so far as to discourage people
>> from using the *64 variants.
>>
>> it should be possible using our existing LFS framework to make the OFD
>> cmds available even to 32-bit apps (where sizeof(off_t) == 32).  but
>> maybe the usage of F_GETLK64/struct flock64/etc... in the real world
>> has made it hard to put that genie back in the bottle ?  we'd have to
>> version the current fcntl symbol, create a new fcntl symbol that does
>> 32->64 munging, and add a new fcntl64 symbol that we'd transparently
>> rewrite to when LFS is turned on.
>> -mike
> 
> There should be no need to use struct flock64 explicitly, and there is
> already a proposed patch to fix the manpage accordingly.
> 
> What we _do_ want to ensure is that large file offsets are in use if
> the application wants to use OFD locks (either by virtue of being on a
> 64 bit arch, or by defining _FILE_OFFSET_BITS=64).
> 
> In principle, we could try to fix it up so that the kernel can handle
> OFD locks with legacy struct flock. That would mean adding
> F_OFD_SETLK64 and friends in both the kernel and glibc, and we'd have
> to ensure that legacy kernel+new glibc is handled sanely (and vice-
> versa). That's a lot of effort (and more risk for breakage) to handle a
> use case that I'm not sure even exists. This approach is much simpler,
> and we'll just be breaking at build time a case that was already broken
> at runtime.

Requiring _FILE_OFFSET_BITS=64 to use OFD locks on 32 bits seems rather
ugly. So, in the ideal world, the solution in the preceding paragraph
seems preferable. It's more work, but don't we have some precedents here
that can be used as patterns? However, I'm not sure I feel very strongly
about it all, since as you say the use case may not even exist.

> In hindsight, I wish I had just introduced F_OFD_SETLK64 and friends to
> make them work with legacy struct flock when I did these patches (mea
> culpa!), but I don't really see the value in doing that at this point.

Well, no one else spotted it at the time either :-).

Cheers,

Michael
Mike Frysinger Aug. 17, 2016, 8:03 p.m. UTC | #15
On 17 Aug 2016 15:15, Jeff Layton wrote:
> On Wed, 2016-08-17 at 11:43 -0700, Mike Frysinger wrote:
> > On 17 Aug 2016 10:47, Jeff Layton wrote:
> > > 
> > > The Linux kernel expects a flock64 structure whenever you use OFD locks
> > > with fcntl64. Unfortunately, you can currently build a 32-bit program
> > > that passes in a struct flock when it calls fcntl64.
> > > 
> > > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> > > defined, so that the build fails in this situation rather than
> > > producing a broken binary.
> > 
> > this seems to be going against the glibc API/guarantees we've provided
> > before (or at least tried to promise), and what the fcntl(2) man page
> > says now.  namely, we haven't documented F_GETLK64 or struct flock64,
> > with the expectation that the user just calls fcntl() with a struct
> > flock.  in fact, the man page even goes so far as to discourage people
> > from using the *64 variants.
> > 
> > it should be possible using our existing LFS framework to make the OFD
> > cmds available even to 32-bit apps (where sizeof(off_t) == 32).  but
> > maybe the usage of F_GETLK64/struct flock64/etc... in the real world
> > has made it hard to put that genie back in the bottle ?  we'd have to
> > version the current fcntl symbol, create a new fcntl symbol that does
> > 32->64 munging, and add a new fcntl64 symbol that we'd transparently
> > rewrite to when LFS is turned on.
> 
> There should be no need to use struct flock64 explicitly, and there is
> already a proposed patch to fix the manpage accordingly.
> 
> What we _do_ want to ensure is that large file offsets are in use if
> the application wants to use OFD locks (either by virtue of being on a
> 64 bit arch, or by defining _FILE_OFFSET_BITS=64).
> 
> In principle, we could try to fix it up so that the kernel can handle
> OFD locks with legacy struct flock. That would mean adding
> F_OFD_SETLK64 and friends in both the kernel and glibc, and we'd have
> to ensure that legacy kernel+new glibc is handled sanely (and vice-
> versa). That's a lot of effort (and more risk for breakage) to handle a
> use case that I'm not sure even exists. This approach is much simpler,
> and we'll just be breaking at build time a case that was already broken
> at runtime.
> 
> In hindsight, I wish I had just introduced F_OFD_SETLK64 and friends to
> make them work with legacy struct flock when I did these patches (mea
> culpa!), but I don't really see the value in doing that at this point.

this the crux of my point though ... if we want fcntl to be transparent,
then that includes making OFD locks "just work" in userspace.

the trouble is that glibc only does fcntl->fcntl64, it doesn't do any
other cmd or arg translation.  that means users are forced to pick the
right cmd (FOO or FOO64) that matches the LFS build mode.  i.e. they
can't use FOO w/LFS turned on, and they can't use FOO64 w/LFS turned
off.  otherwise there's a mismatch in the struct flock.

imo, we should either adapt our documentation (manual & man page) to
match reality, or we should bite the bullet and commit to doing the
heavy lifting with fcntl.
-mike
Jeff Layton Aug. 17, 2016, 8:05 p.m. UTC | #16
On Thu, 2016-08-18 at 07:59 +1200, Michael Kerrisk (man-pages) wrote:
> On 08/18/2016 07:15 AM, Jeff Layton wrote:
> > 
> > On Wed, 2016-08-17 at 11:43 -0700, Mike Frysinger wrote:
> > > 
> > > On 17 Aug 2016 10:47, Jeff Layton wrote:
> > > > 
> > > > 
> > > > The Linux kernel expects a flock64 structure whenever you use OFD locks
> > > > with fcntl64. Unfortunately, you can currently build a 32-bit program
> > > > that passes in a struct flock when it calls fcntl64.
> > > > 
> > > > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is also
> > > > defined, so that the build fails in this situation rather than
> > > > producing a broken binary.
> > > 
> > > this seems to be going against the glibc API/guarantees we've provided
> > > before (or at least tried to promise), and what the fcntl(2) man page
> > > says now.  namely, we haven't documented F_GETLK64 or struct flock64,
> > > with the expectation that the user just calls fcntl() with a struct
> > > flock.  in fact, the man page even goes so far as to discourage people
> > > from using the *64 variants.
> > > 
> > > it should be possible using our existing LFS framework to make the OFD
> > > cmds available even to 32-bit apps (where sizeof(off_t) == 32).  but
> > > maybe the usage of F_GETLK64/struct flock64/etc... in the real world
> > > has made it hard to put that genie back in the bottle ?  we'd have to
> > > version the current fcntl symbol, create a new fcntl symbol that does
> > > 32->64 munging, and add a new fcntl64 symbol that we'd transparently
> > > rewrite to when LFS is turned on.
> > > -mike
> > 
> > There should be no need to use struct flock64 explicitly, and there is
> > already a proposed patch to fix the manpage accordingly.
> > 
> > What we _do_ want to ensure is that large file offsets are in use if
> > the application wants to use OFD locks (either by virtue of being on a
> > 64 bit arch, or by defining _FILE_OFFSET_BITS=64).
> > 
> > In principle, we could try to fix it up so that the kernel can handle
> > OFD locks with legacy struct flock. That would mean adding
> > F_OFD_SETLK64 and friends in both the kernel and glibc, and we'd have
> > to ensure that legacy kernel+new glibc is handled sanely (and vice-
> > versa). That's a lot of effort (and more risk for breakage) to handle a
> > use case that I'm not sure even exists. This approach is much simpler,
> > and we'll just be breaking at build time a case that was already broken
> > at runtime.
> 
> Requiring _FILE_OFFSET_BITS=64 to use OFD locks on 32 bits seems rather
> ugly. So, in the ideal world, the solution in the preceding paragraph
> seems preferable. It's more work, but don't we have some precedents here
> that can be used as patterns? However, I'm not sure I feel very strongly
> about it all, since as you say the use case may not even exist.
> 
> > 
> > In hindsight, I wish I had just introduced F_OFD_SETLK64 and friends to
> > make them work with legacy struct flock when I did these patches (mea
> > culpa!), but I don't really see the value in doing that at this point.
> 
> Well, no one else spotted it at the time either :-).
> 
> Cheers,
> 
> Michael
> 
> 

Sounds nice, but that is a bit more difficult. Let's explore it
though...

The big question is: how do we do that without breaking applications
that are currently working?

The way it works now is that when you define _FILE_OFFSET_BITS=64 and
call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for your
struct flock, and F_SETLK64 for the F_SETLK.

So, suppose we do the same here -- define a set of F_OFD_SETLK64
constants, and then pass those in in the same way using glibc, and
teach the kernel to handle things in the same way.

The problem there is that we'd be silently changing the behavior for
applications that are working. Applications that are using F_OFD_SETLK
and passing in a flock64 structure would be broken. We need to preserve
how F_OFD_SETLK behaves today for applications that do set
_FILE_OFFSET_BITS=64.

OTOH, we could define a set of new F_OFD_*32 constants and use those to
indicate to the kernel that this is a legacy struct flock. That would
actually work, though you'd need to have a new kernel and build against
a new glibc.

If you built with a new glibc and run against an old kernel, then you'd
(presumably) fail with -EINVAL at runtime in the problematic case.

Maybe that is the best option though...thoughts?
Mike Frysinger Aug. 17, 2016, 8:37 p.m. UTC | #17
On 17 Aug 2016 16:05, Jeff Layton wrote:
> The way it works now is that when you define _FILE_OFFSET_BITS=64 and
> call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for your
> struct flock, and F_SETLK64 for the F_SETLK.

does it ?  doesn't seem like it does to me.  here's glibc's fcntl.c:
	io/fcntl.c - generic stub that sets ENOSYS
	sysdeps/unix/sysv/linux/fcntl.c - just calls syscall(fcntl)
	sysdeps/unix/sysv/linux/generic/wordsize-32/fcntl.c - just calls syscall(fcntl64)
	sysdeps/unix/sysv/linux/i386/fcntl.c - same as above
	<all the other 32-bit arches include the i386 file>

the kernel is where it gets interesting:
	fs/compat.c:
		COMPAT_SYSCALL_DEFINE3(fcntl):
			rejects all 64-bit commands w/EINVAL
			passes all other calls to compat_sys_fcntl64
		COMPAT_SYSCALL_DEFINE3(fcntl64):
			rewrites 32-bit flock struct to 64-bit flock struct
			passes args to sys_fcntl 
	fs/fcntl.c:
		SYSCALL_DEFINE3(fcntl):
			passes all args to do_fcntl
		SYSCALL_DEFINE3(fcntl64):
			handles 64-bit flock commands
			passes all others commands to do_fcntl
		do_fcntl:
			handles all commands using native sized flock struct

so for a 32-bit system (e.g. i386), you must match LFS & command usage.
if LFS is turned on, then using 32-bit commands w/struct flock fails.
if LFS is turned off, then using 64-bit commands w/struct flock fails.
-mike
Andreas Schwab Aug. 17, 2016, 8:52 p.m. UTC | #18
On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote:

> Why would the F_SETLK operation work with a struct flock64 in
> _FILE_OFFSET_BITS == 64 mode?  I think the kernel still expects a 32-bit
> struct.

With _F_O_B == 64, F_SETLK is actually F_SETLK64.

Andreas.
Jeff Layton Aug. 17, 2016, 8:57 p.m. UTC | #19
On Wed, 2016-08-17 at 13:37 -0700, Mike Frysinger wrote:
> On 17 Aug 2016 16:05, Jeff Layton wrote:
> > 
> > The way it works now is that when you define _FILE_OFFSET_BITS=64 and
> > call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for your
> > struct flock, and F_SETLK64 for the F_SETLK.
> 
> does it ?  doesn't seem like it does to me.  here's glibc's fcntl.c:
> 	io/fcntl.c - generic stub that sets ENOSYS
> 	sysdeps/unix/sysv/linux/fcntl.c - just calls syscall(fcntl)
> 	sysdeps/unix/sysv/linux/generic/wordsize-32/fcntl.c - just calls syscall(fcntl64)
> 	sysdeps/unix/sysv/linux/i386/fcntl.c - same as above
> 	<all the other 32-bit arches include the i386 file>
> 

Ok, I was being a little cavalier with my description. This is what
really happens (from x86 struct flock definition):

struct flock
  {
    short int l_type;   /* Type of lock: F_RDLCK, F_WRLCK, or F_UNLCK.  */
    short int l_whence; /* Where `l_start' is relative to (like `lseek').  */
#ifndef __USE_FILE_OFFSET64
    __off_t l_start;    /* Offset where the lock begins.  */
    __off_t l_len;      /* Size of the locked area; zero means until EOF.  */
#else
    __off64_t l_start;  /* Offset where the lock begins.  */
    __off64_t l_len;    /* Size of the locked area; zero means until EOF.  */
#endif
    __pid_t l_pid;      /* Process holding the lock.  */
  };

So, l_start and l_len get redefined into larger sizes when LFS is
enabled. The F_GETLK/F_SETLK/F_SETLKW are also redefined to their *64
equivalents in that case using the preprocessor.

> the kernel is where it gets interesting:

Yes indeed. It's quite the twisty maze...

> 	fs/compat.c:
> > 		COMPAT_SYSCALL_DEFINE3(fcntl):
> > 			rejects all 64-bit commands w/EINVAL
> 			passes all other calls to compat_sys_fcntl64
> 		COMPAT_SYSCALL_DEFINE3(fcntl64):
> > 			rewrites 32-bit flock struct to 64-bit flock struct
> 			passes args to sys_fcntl 
> 	fs/fcntl.c:
> > 		SYSCALL_DEFINE3(fcntl):
> 			passes all args to do_fcntl
> 		SYSCALL_DEFINE3(fcntl64):
> > 			handles 64-bit flock commands
> > 			passes all others commands to do_fcntl
> > 		do_fcntl:
> > 			handles all commands using native sized flock struct
> 
> so for a 32-bit system (e.g. i386), you must match LFS & command usage.
> if LFS is turned on, then using 32-bit commands w/struct flock fails.
> if LFS is turned off, then using 64-bit commands w/struct flock fails.
> -mike

Yes. The command is what tells the kernel the sort of struct flock it
has been given. The problem here is that we assume that it's
sizeof(struct flock64), but it could a non-LFS struct flock.
Cyril Hrubis Aug. 17, 2016, 9:30 p.m. UTC | #20
Hi!
> the trouble is that glibc only does fcntl->fcntl64, it doesn't do any
> other cmd or arg translation.  that means users are forced to pick the
> right cmd (FOO or FOO64) that matches the LFS build mode.  i.e. they
> can't use FOO w/LFS turned on, and they can't use FOO64 w/LFS turned
> off.  otherwise there's a mismatch in the struct flock.

One way to fix that would be to add fcntl32() that does the flock <->
flock64 translation for F_OFD_XXX and calls the generall fcntl()
implementation. Then select between fcntl() and fctnt32() on 32bit
architectures based on _FILE_OFFSET_BITS in the preprocessor in the
fcntl header. That way we wouldn't have to touch the kernel at all.
Mike Frysinger Aug. 17, 2016, 9:35 p.m. UTC | #21
On 17 Aug 2016 16:57, Jeff Layton wrote:
> On Wed, 2016-08-17 at 13:37 -0700, Mike Frysinger wrote:
> > On 17 Aug 2016 16:05, Jeff Layton wrote:
> > > 
> > > The way it works now is that when you define _FILE_OFFSET_BITS=64 and
> > > call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for your
> > > struct flock, and F_SETLK64 for the F_SETLK.
> > 
> > does it ?  doesn't seem like it does to me.  here's glibc's fcntl.c:
> > 	io/fcntl.c - generic stub that sets ENOSYS
> > 	sysdeps/unix/sysv/linux/fcntl.c - just calls syscall(fcntl)
> > 	sysdeps/unix/sysv/linux/generic/wordsize-32/fcntl.c - just calls syscall(fcntl64)
> > 	sysdeps/unix/sysv/linux/i386/fcntl.c - same as above
> > 	<all the other 32-bit arches include the i386 file>
> > 
> 
> Ok, I was being a little cavalier with my description. This is what
> really happens (from x86 struct flock definition):
> 
> struct flock
>   {
>     short int l_type;   /* Type of lock: F_RDLCK, F_WRLCK, or F_UNLCK.  */
>     short int l_whence; /* Where `l_start' is relative to (like `lseek').  */
> #ifndef __USE_FILE_OFFSET64
>     __off_t l_start;    /* Offset where the lock begins.  */
>     __off_t l_len;      /* Size of the locked area; zero means until EOF.  */
> #else
>     __off64_t l_start;  /* Offset where the lock begins.  */
>     __off64_t l_len;    /* Size of the locked area; zero means until EOF.  */
> #endif
>     __pid_t l_pid;      /* Process holding the lock.  */
>   };
> 
> So, l_start and l_len get redefined into larger sizes when LFS is
> enabled. The F_GETLK/F_SETLK/F_SETLKW are also redefined to their *64
> equivalents in that case using the preprocessor.

ah i forgot about that in the glibc header.  so it's not as grimm as
i was thinking, and explains how glibc has been providing the API so
the user doesn't have to explicitly pick the 64-bit types.

in order to do the same for OFD, we'd need to go the LFS route (i.e.
add fcntl64 and transparent rewrites in glibc).  or we can just run
with your idea ... i'm warmer to it now that i see we only have to
tell the user "enable LFS support" rather than "use the flock64
struct".
-mike
Jeff Layton Aug. 17, 2016, 9:48 p.m. UTC | #22
On Wed, 2016-08-17 at 14:35 -0700, Mike Frysinger wrote:
> On 17 Aug 2016 16:57, Jeff Layton wrote:
> > 
> > On Wed, 2016-08-17 at 13:37 -0700, Mike Frysinger wrote:
> > > 
> > > On 17 Aug 2016 16:05, Jeff Layton wrote:
> > > > 
> > > > 
> > > > The way it works now is that when you define
> > > > _FILE_OFFSET_BITS=64 and
> > > > call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for
> > > > your
> > > > struct flock, and F_SETLK64 for the F_SETLK.
> > > 
> > > does it ?  doesn't seem like it does to me.  here's glibc's
> > > fcntl.c:
> > > 	io/fcntl.c - generic stub that sets ENOSYS
> > > 	sysdeps/unix/sysv/linux/fcntl.c - just calls syscall(fcntl)
> > > 	sysdeps/unix/sysv/linux/generic/wordsize-32/fcntl.c - just
> > > calls syscall(fcntl64)
> > > 	sysdeps/unix/sysv/linux/i386/fcntl.c - same as above
> > > 	<all the other 32-bit arches include the i386 file>
> > > 
> > 
> > Ok, I was being a little cavalier with my description. This is what
> > really happens (from x86 struct flock definition):
> > 
> > struct flock
> >   {
> >     short int l_type;   /* Type of lock: F_RDLCK, F_WRLCK, or
> > F_UNLCK.  */
> >     short int l_whence; /* Where `l_start' is relative to (like
> > `lseek').  */
> > #ifndef __USE_FILE_OFFSET64
> >     __off_t l_start;    /* Offset where the lock begins.  */
> >     __off_t l_len;      /* Size of the locked area; zero means
> > until EOF.  */
> > #else
> >     __off64_t l_start;  /* Offset where the lock begins.  */
> >     __off64_t l_len;    /* Size of the locked area; zero means
> > until EOF.  */
> > #endif
> >     __pid_t l_pid;      /* Process holding the lock.  */
> >   };
> > 
> > So, l_start and l_len get redefined into larger sizes when LFS is
> > enabled. The F_GETLK/F_SETLK/F_SETLKW are also redefined to their
> > *64
> > equivalents in that case using the preprocessor.
> 
> ah i forgot about that in the glibc header.  so it's not as grimm as
> i was thinking, and explains how glibc has been providing the API so
> the user doesn't have to explicitly pick the 64-bit types.
> 
> in order to do the same for OFD, we'd need to go the LFS route (i.e.
> add fcntl64 and transparent rewrites in glibc).  or we can just run
> with your idea ... i'm warmer to it now that i see we only have to
> tell the user "enable LFS support" rather than "use the flock64
> struct".
> -mike

Sorry yeah, I should have done a better job of explaining it, but it is
(necessarily) rather complex...

I figure that most people only haven't enabled LFS support in that
situation by mistake. I really have a hard time thinking of when you'd
want to explicitly _avoid_ using LFS support and still use OFD locks.

So yeah, I think what I proposed before would probably be fine. But now
that Michael pushed the issue, it's dawned on me that we may be able to
get away with supporting it better if we turn the compatability
mechanism on its head and use F_OFD_*32 constants in the non-LFS case.

I'm building a kernel with a test patch now. I'll either post that or a
revised version of the earlier one in another day or two once I've done
a bit more exploration of that approach.

Thanks everyone for having a look at this so far. It's been helpful...
Florian Weimer Aug. 18, 2016, 8:44 a.m. UTC | #23
On 08/17/2016 09:20 PM, Jeff Layton wrote:

> It is, but it's preferable to unexpected behavior at runtime. I think
> it's entirely reasonable to require large file offsets in order to use
> OFD locks, but I'm willing to be convinced otherwise if there are use
> cases that you know of that this will break.

I have thought about it some more and I agree that removing the 
definitions for !64-bit is a viable course of action.

We should deprecate F_SETLK64 and struct lock64, too, and document that 
you should use _FILE_OFFSET_BITS == 64 if you need such locks.  That, 
and make fcntl type-safe (for both C and C++ with sufficiently recent GCC).

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Weimer Aug. 18, 2016, 8:45 a.m. UTC | #24
On 08/17/2016 10:52 PM, Andreas Schwab wrote:
> On Aug 17 2016, Florian Weimer <fweimer@redhat.com> wrote:
>
>> Why would the F_SETLK operation work with a struct flock64 in
>> _FILE_OFFSET_BITS == 64 mode?  I think the kernel still expects a 32-bit
>> struct.
>
> With _F_O_B == 64, F_SETLK is actually F_SETLK64.

Indeed, I missed that.  I think removing the file-private lock 
definitions in 32 bit mode is the closest we can get to mirror this 
behavior.

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Weimer Aug. 18, 2016, 8:57 a.m. UTC | #25
On 08/17/2016 10:57 PM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 13:37 -0700, Mike Frysinger wrote:
>> On 17 Aug 2016 16:05, Jeff Layton wrote:
>>>
>>> The way it works now is that when you define _FILE_OFFSET_BITS=64 and
>>> call fcntl(fd, F_SETLK, fl) glibc swaps in a struct flock64 for your
>>> struct flock, and F_SETLK64 for the F_SETLK.
>>
>> does it ?  doesn't seem like it does to me.  here's glibc's fcntl.c:
>> 	io/fcntl.c - generic stub that sets ENOSYS
>> 	sysdeps/unix/sysv/linux/fcntl.c - just calls syscall(fcntl)
>> 	sysdeps/unix/sysv/linux/generic/wordsize-32/fcntl.c - just calls syscall(fcntl64)
>> 	sysdeps/unix/sysv/linux/i386/fcntl.c - same as above
>> 	<all the other 32-bit arches include the i386 file>
>>
>
> Ok, I was being a little cavalier with my description. This is what
> really happens (from x86 struct flock definition):
>
> struct flock
>   {
>     short int l_type;   /* Type of lock: F_RDLCK, F_WRLCK, or F_UNLCK.  */
>     short int l_whence; /* Where `l_start' is relative to (like `lseek').  */
> #ifndef __USE_FILE_OFFSET64
>     __off_t l_start;    /* Offset where the lock begins.  */
>     __off_t l_len;      /* Size of the locked area; zero means until EOF.  */
> #else
>     __off64_t l_start;  /* Offset where the lock begins.  */
>     __off64_t l_len;    /* Size of the locked area; zero means until EOF.  */
> #endif
>     __pid_t l_pid;      /* Process holding the lock.  */
>   };
>
> So, l_start and l_len get redefined into larger sizes when LFS is
> enabled. The F_GETLK/F_SETLK/F_SETLKW are also redefined to their *64
> equivalents in that case using the preprocessor.

Note that LFS and 64-bit off_t are separate.  Only LFS 
(_LARGEFILE_SOURCE, _LARGEFILE64_SOURCE) is implied by _GNU_SOURCE.

We do not really have a working fcntl for _LARGEFILE64_SOURCE and 
_FILE_OFFSET_BITS == 32.

Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Schwab Aug. 18, 2016, 8:58 a.m. UTC | #26
On Aug 18 2016, Florian Weimer <fweimer@redhat.com> wrote:

> We should deprecate F_SETLK64 and struct lock64, too, and document that
> you should use _FILE_OFFSET_BITS == 64 if you need such locks.  That, and
> make fcntl type-safe (for both C and C++ with sufficiently recent GCC).

There is also _LARGEFILE64_SOURCE which enables the *64 API without
making it the default.

Andreas.
Florian Weimer Aug. 18, 2016, 9 a.m. UTC | #27
On 08/17/2016 11:48 PM, Jeff Layton wrote:
> So yeah, I think what I proposed before would probably be fine. But now
> that Michael pushed the issue, it's dawned on me that we may be able to
> get away with supporting it better if we turn the compatability
> mechanism on its head and use F_OFD_*32 constants in the non-LFS case.

That's rather confusing to programmers, though.

We then have:

F_OFD_SETLK     always 64-bit
F_SETLK         32-bit or 64-bit
F_SETLK64       always 64-bit (not recommended)
F_OFD_SETLK32   always 32-bit

Until we make fcntl type-safe, this doesn't really fly, I'm afraid.

Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cyril Hrubis Aug. 23, 2016, 11:03 a.m. UTC | #28
Hi!
> > So yeah, I think what I proposed before would probably be fine. But now
> > that Michael pushed the issue, it's dawned on me that we may be able to
> > get away with supporting it better if we turn the compatability
> > mechanism on its head and use F_OFD_*32 constants in the non-LFS case.
> 
> That's rather confusing to programmers, though.
> 
> We then have:
> 
> F_OFD_SETLK     always 64-bit
> F_SETLK         32-bit or 64-bit
> F_SETLK64       always 64-bit (not recommended)
> F_OFD_SETLK32   always 32-bit

It's even worse, the F_OFD_SETLK32 in the proposed patch behaves exactly
as F_SETLK so it's 32-bit or 64-bit depending on sizeof(long) in the
kernel, that is because the compat fcntl64 converts struct flock from
userspace to kernel struct flock and just call sys_fcntl() with the cmd
it has. So in the end if you call fcntl with F_OFD_SETLK32 on 64bit
kernel it expects flock64.
Jeff Layton Aug. 23, 2016, 11:36 a.m. UTC | #29
On Tue, 2016-08-23 at 13:03 +0200, Cyril Hrubis wrote:
> Hi!
> > 
> > > 
> > > So yeah, I think what I proposed before would probably be fine.
> > > But now
> > > that Michael pushed the issue, it's dawned on me that we may be
> > > able to
> > > get away with supporting it better if we turn the compatability
> > > mechanism on its head and use F_OFD_*32 constants in the non-LFS
> > > case.
> > 
> > That's rather confusing to programmers, though.
> > 
> > We then have:
> > 
> > F_OFD_SETLK     always 64-bit
> > F_SETLK         32-bit or 64-bit
> > F_SETLK64       always 64-bit (not recommended)
> > F_OFD_SETLK32   always 32-bit
> 
> It's even worse, the F_OFD_SETLK32 in the proposed patch behaves
> exactly
> as F_SETLK so it's 32-bit or 64-bit depending on sizeof(long) in the
> kernel, that is because the compat fcntl64 converts struct flock from
> userspace to kernel struct flock and just call sys_fcntl() with the
> cmd
> it has. So in the end if you call fcntl with F_OFD_SETLK32 on 64bit
> kernel it expects flock64.
> 

To be clear, that approach was NAK'ed by Christoph (and I think that's
a good thing, actually -- one more nail in the non-LFS coffin). I think
what we want merged into glibc is this patch that I sent late on
Thursday:

    [glibc PATCHv2] fcntl: don't define OFD lock constants for 32-bit builds with small file offsets

We'll also need your patch to the fcntl(2) manpage as well, but that's
a separate problem.
Cyril Hrubis Aug. 23, 2016, 11:38 a.m. UTC | #30
Hi!
> To be clear, that approach was NAK'ed by Christoph (and I think that's
> a good thing, actually -- one more nail in the non-LFS coffin). I think
> what we want merged into glibc is this patch that I sent late on
> Thursday:
> 
>     [glibc PATCHv2] fcntl: don't define OFD lock constants for 32-bit builds with small file offsets
> 
> We'll also need your patch to the fcntl(2) manpage as well, but that's
> a separate problem.

Hmm, the patch for man-pages should be updated as well. We need
somethign as:

...
This lock  type  is  Linux-specific,  and  available since Linux 3.15.
On 32bit platform _FILE_OFFSET_BITS must defined to 64 (before including
any header files) to make these locks available.
...

Michael should I send updated patch or will you take care of this?
Michael Kerrisk (man-pages) Aug. 23, 2016, 9:10 p.m. UTC | #31
On 08/23/2016 11:38 PM, Cyril Hrubis wrote:
> Hi!
>> To be clear, that approach was NAK'ed by Christoph (and I think that's
>> a good thing, actually -- one more nail in the non-LFS coffin). I think
>> what we want merged into glibc is this patch that I sent late on
>> Thursday:
>>
>>     [glibc PATCHv2] fcntl: don't define OFD lock constants for 32-bit builds with small file offsets
>>
>> We'll also need your patch to the fcntl(2) manpage as well, but that's
>> a separate problem.
> 
> Hmm, the patch for man-pages should be updated as well. We need
> somethign as:
> 
> ...
> This lock  type  is  Linux-specific,  and  available since Linux 3.15.
> On 32bit platform _FILE_OFFSET_BITS must defined to 64 (before including
> any header files) to make these locks available.
> ...
> 
> Michael should I send updated patch or will you take care of this?

I can do it, or you can. At the moment, I'm just holding off until I 
see what is accepted into glibc.

Cheers,

Michael
Cyril Hrubis Nov. 14, 2016, 1:45 p.m. UTC | #32
Hi!
> >> To be clear, that approach was NAK'ed by Christoph (and I think that's
> >> a good thing, actually -- one more nail in the non-LFS coffin). I think
> >> what we want merged into glibc is this patch that I sent late on
> >> Thursday:
> >>
> >>     [glibc PATCHv2] fcntl: don't define OFD lock constants for 32-bit builds with small file offsets
> >>
> >> We'll also need your patch to the fcntl(2) manpage as well, but that's
> >> a separate problem.
> > 
> > Hmm, the patch for man-pages should be updated as well. We need
> > somethign as:
> > 
> > ...
> > This lock  type  is  Linux-specific,  and  available since Linux 3.15.
> > On 32bit platform _FILE_OFFSET_BITS must defined to 64 (before including
> > any header files) to make these locks available.
> > ...
> > 
> > Michael should I send updated patch or will you take care of this?
> 
> I can do it, or you can. At the moment, I'm just holding off until I 
> see what is accepted into glibc.

Ping.

Is there any resolve for this?
Florian Weimer Nov. 22, 2016, 6:41 p.m. UTC | #33
On 11/14/2016 02:45 PM, Cyril Hrubis wrote:

> Is there any resolve for this?

I have a prototype patch for a type-safe <fcntl.h>, but it is currently 
C only and ran into some unexpected GCC issue.

The main problem is that we currently lack the ability test it because 
we have no framework to check for compiler warnings from test cases.

Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/manual/examples/ofdlocks.c b/manual/examples/ofdlocks.c
index ba4f0ef4d237..6df18ce5c368 100644
--- a/manual/examples/ofdlocks.c
+++ b/manual/examples/ofdlocks.c
@@ -15,6 +15,7 @@ 
    along with this program; if not, see <http://www.gnu.org/licenses/>.
 */
 
+/* Note that this must be built with -D_FILE_OFFSET_BITS=64 on 32-bit arch */
 #define _GNU_SOURCE
 #include <stdio.h>
 #include <sys/types.h>
diff --git a/manual/llio.texi b/manual/llio.texi
index 019dea2c3189..99c700833641 100644
--- a/manual/llio.texi
+++ b/manual/llio.texi
@@ -3907,9 +3907,11 @@  descriptor.
 
 Open file description locks use the same @code{struct flock} as
 process-associated locks as an argument (@pxref{File Locks}) and the
-macros for the @code{command} values are also declared in the header file
-@file{fcntl.h}. To use them, the macro @code{_GNU_SOURCE} must be
-defined prior to including any header file.
+macros for the @code{command} values are also declared in the header
+file @file{fcntl.h}. To use them, the macro @code{_GNU_SOURCE} must be
+defined prior to including any header file. Additionally, if building on
+a 32-bit architecture, then large file offsets must also be enabled
+by defining @code{_FILE_OFFSET_BITS == 64}.
 
 In contrast to process-associated locks, any @code{struct flock} used as
 an argument to open file description lock commands must have the @code{l_pid}
diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
index 7e5b0aecdcb4..7f3c9fef627f 100644
--- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
+++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
@@ -127,11 +127,18 @@ 
    This means that they are inherited across fork or clone with CLONE_FILES
    like BSD (flock) locks, and they are only released automatically when the
    last reference to the the file description against which they were acquired
-   is put. */
+   is put.
+
+   Note that the kernel does not support the legacy struct flock on 32-bit
+   arches with OFD locks. On those arches you need define both _GNU_SOURCE
+   and _FILE_OFFSET_BITS=64.
+   */
 #ifdef __USE_GNU
-# define F_OFD_GETLK	36
-# define F_OFD_SETLK	37
-# define F_OFD_SETLKW	38
+# if __WORDSIZE != 32 || defined __USE_FILE_OFFSET64
+#  define F_OFD_GETLK	36
+#  define F_OFD_SETLK	37
+#  define F_OFD_SETLKW	38
+# endif
 #endif
 
 #ifdef __USE_LARGEFILE64