Patchset to add asynchronous open/close to master

[simo]

On Wed, 2012-06-20 at 18:58 +0200, Volker Lendecke wrote: 
> On Wed, Jun 20, 2012 at 09:24:05AM -0700, Jeremy Allison wrote:
> > On Wed, Jun 20, 2012 at 11:41:05AM +0200, Volker Lendecke wrote:
> > > 
> > > In a commit you mentioned that it has a race condition
> > > fixed. Can you explain that? I am very worried that we put
> > > an open potentially creating a file into a thread and (as it
> > > seems to me) we fix up potential screwups later with a
> > > fchown. This to me far too badly smells like a potential
> > > race condition. The only way I see to correctly do this is
> > > by a helper process that does our normal become_user and not
> > > switch back to become_root(). Threads for my taste are too
> > > dangerous for this.
> > 
> > The open is only done in the thread if the flags are
> > set to O_CREAT, so we are only creating a file.
> > I cache the current euid and guid at the time the
> > VFS_OPEN is invoked, so we know who should own the
> > file.
> > 
> > In addition, an idea from a conversation I had with
> > you means I add in O_EXCL into the flags - even if
> > the client did not specify this. I save off the
> > initial flags the client requested.
> > 
> > So the open inside the thread can only create a file,
> > never open an existing file. If the open fails with
> > EEXIST, then we know we were raced, and so once
> > the thread terminates and the open call is rescheduled
> > I retry the open synchronously using the default path. 
> > 
> > If the open succeeded, we *know* we created the file.
> > So once the thread terminates I do an fstat() on the
> > returned fd and look at the created uid/gid.
> > 
> > If smbd did not change uids (the normal case) then
> > we are done. If it did and the threaded open hit
> > the race condition, then the uid on the file
> > will be different. If so, then we need to fix up
> > using the fchown call.
> > 
> > There is a slight complication where the parent
> > directory has the SETGID bit set so the owning
> > gid needs to be the parent directory gid, not
> > the gid from the process token, but there is
> > code to look at and handle that in the module.
> > 
> > Hope that explains things. As you know, the
> > open path emulating an CreateFile always has
> > some races due to there being no kernel-level
> > CreateFile call and emulated primitive not being
> > atomic.
> > 
> > I'm satisfied with this code coping with the
> > issues correctly, so I'd like you to trust
> > me on this, if you're comfortable with this
> > explaination.
> 
> The explanation fully describes what the patch is doing. It
> is just that I disagree that we should do it this way. It
> can happen that files are created where a normal create
> would not have been possible due to kernel-based
> permissions. Imagine a become_root() at an inopportune time
> while an async open as a non-privileged user is just about
> to be scheduled. This is a classic time-of-check -
> time-of-use race from my point of view.

I had exactly the same concern Volker, but you put it down much more
nicely :)

Simo.


-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>