| Message ID | 1471445251-2450-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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).
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
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
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...
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
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.
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
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.
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!
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
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
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.
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.
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
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
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?
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
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.
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.
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.
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
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...
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
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
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
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.
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
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.
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.
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?
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
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?
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
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
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(-)