Re: [glibc PATCH] fcntl: put F_OFD_* constants under #ifdef __USE_FILE_OFFSET64

[Jeff Layton <jlayton at redhat dot com>]

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.

-- 
Jeff Layton <jlayton@redhat.com>