Patchset to add asynchronous open/close to master

[simo]

On Wed, 2012-06-20 at 09:24 -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.

Sorry I do not understand how this works properly.

A) what assures you have the right to actually create the file in the
directory if you are not setting the whole credentials (uid,gid and all
secondary gids) as the process credentials for the open ?

B) Why do you have a comment that Posix ACL inheritance doesn't apply ?
Is it because we assume that all additional ACLs are properly set and we
need to care exclusively about setting the right user/group owner ?

> 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.

I just need to understand how do you know the open will always
succeed/fail properly given the parent directory ACL and the user's
group memberships, if the open my happen in a different user context,
what am I missing ?

Simo.

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