[patch] cifs: accidentally creating empty files

[Steve French]

On Thu, Oct 13, 2011 at 12:27 PM, Jeff Layton <jlayton at samba.org> wrote:
> On Wed, 28 Sep 2011 00:28:10 +0300
> Dan Carpenter <dan.carpenter at oracle.com> wrote:
>
>> This solves a problem for where a user without write permissions
>> was creating empty files.  This function was supposed to do a lookup
>> only, the create happens later.
>>
>
> Not quite. This uses the open-intent goop in the VFS layer that Al Viro
> is trying to get rid of. The idea here is that doing a lookup just to
> do an open is a waste, when you could just attempt the open. There's no
> real reason to exempt creates from that if cifs used a sane permissions
> model...
>
>> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
>>
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index 72d448b..8515afe 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -566,11 +566,13 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>>               if (nd && !(nd->flags & LOOKUP_DIRECTORY) &&
>>                    (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
>>                    (nd->intent.open.file->f_flags & O_CREAT)) {
>> +                     unsigned int f_flags;
>> +
>> +                     f_flags = (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);
>> +                                     f_flags, &oplock, &fileHandle, xid);
>
>        ...so this makes it only do the open at lookup time if the
>        file already exists.
>
>        I suspect the real problem here is that cifs is trying to
>        enforce permissions on the client, which happens after the
>        lookup.
>
>        If the client simply allowed the server to handle the
>        permissions (and used the right credentials for each user),

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.


-- 
Thanks,

Steve