Patchset to add asynchronous open/close to master

[Volker Lendecke]

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.

With best regards,

Volker Lendecke

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de