Re: [patch] cifs: accidentally creating empty files

[Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

On Sat, 29 Oct 2011 00:07:12 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Oct 28, 2011 at 10:44 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Fri, 28 Oct 2011 10:27:04 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Fri, Oct 28, 2011 at 2:20 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > On Wed, Oct 19, 2011 at 09:14:37PM -0500, Steve French wrote:
> >> >> Doesn't this force the create to happen later - rather than
> >> >> at lookup time where it belongs?
> >> >>
> >> >> if the issue is just noperm ... we should let this through if the user
> >> >> has local permissions.  Doubling the cost of a file create to Samba
> >> >> seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX
> >> >> doubles the roundrip, doubles the load on the server etc.) - the whole
> >> >> point of this is to let us do an "atomic" open or create operation
> >> >> and not have to split it into multiple requests.  In any case why would
> >> >> we do the open and then follow it with a create?
> >> >>
> >> >> Can we fix this to (at least) narrow the performance penalty.
> >> >>
> >> >
> >> > Yes, it does add another back and forth to file create...  It's hard/
> >> > impossible to check the permissions first and then decide whether to
> >> > pass the O_CREAT flag.  Maybe we could add an if statement to check
> >> > whether noperm was used and the noperm version could use the create
> >> > on lookup.
> >>
> >> Why is it hard to check local permissions? We have the local mode
> >> for the parent.
> >>
> >> Also note that the "intent" flags and the atomic create is not just
> >> about performance, but also making it atomic reduces some of the weird
> >> failure cases.
> >>
> >
> > We have the local mode for the parent, but we do not have the ownership
> > and mode for the file that has not yet been created. Because of the
> > special (ahem) way that cifs handles permissions, it's easily possible
> > for the ownership of the file not to match the user doing that create.
> > At that point, later operations on the file can easily fail.
> >
> > This was the primary reason for the multiuser patch series, and why I
> > still say that doing permissions checking on the client is a broken
> > model.
> 
> We don't have much choice - we have to allow permission
> checking on the client when client security context
> can't match security context on server - but I would like
> to make multiuser the default (especially if we
> can figure out a way to integrate winbind-like
> upcall for ntlmv2 auth) at least for the future (smb2 etc.)
> even if we can't change the default for cifs mounts.
> 

That's fine, but that doesn't solve Dan's immediate problem. We need to
either take his patch or something like it for 3.2 (and probably for
stable as well) or just rip this crap out altogether.

I'd favor the latter, for the following reason:

----------------------[snip]-------------------
        /*
         * O_EXCL: optimize away the lookup, but don't hash the dentry. Let
         * the VFS handle the create.
         */
        if (nd && (nd->flags & LOOKUP_EXCL)) {
                d_instantiate(direntry, NULL);
                rc = 0;
                goto lookup_out;
        }

	[...]

        if (pTcon->unix_ext) {
                if (nd && !(nd->flags & LOOKUP_DIRECTORY) &&
                     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
                     (nd->intent.open.file->f_flags & O_CREAT)) {
                        rc = cifs_posix_open(full_path, &newInode,
                                        parent_dir_inode->i_sb,
                                        nd->intent.open.create_mode,
                                        nd->intent.open.file->f_flags, &oplock,
                                        &fileHandle, xid);
----------------------[snip]-------------------

Ok, so we optimize away the lookup in the O_EXCL case (which is fine).

Then, we only attempt a posix open if O_CREAT was set (note that
regular SMB CREATE_ANDX is prohibited here -- why?). Now, with Dan's
patch, we'll be doing this only when O_CREAT is set, and only when the
file actually exists on the server (since we'll be clearing 

This really just makes no sense at all. The times when we can use this
are so vanishingly small that it makes more sense to just remove this
stuff. If you really want to do this, it needs a redesign from the
ground up, with a consideration of each possible case and determination
of whether it's better (and safe) to use an open instead of lookup in
that case.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>