[linux-cifs-client] [PATCH] cifs: add comments explaining cifs_new_fileinfo behavior

[Shirish Pargaonkar]

On Wed, May 12, 2010 at 4:58 AM, Suresh Jayaraman <sjayaraman at suse.de> wrote:
> On 05/11/2010 07:42 PM, Jeff Layton wrote:
>> On Tue, 11 May 2010 18:18:08 +0530
>> Suresh Jayaraman <sjayaraman at suse.de> wrote:
>>
>>>>>> The comments help, but this code has bothered me for some time. Is it
>>>>>> possible for the create to return success and for something else to
>>>>>> happen such that the cifs_open is never called? I'd imagine that it is,
>>>>>> and if so, then this this open file will be "leaked".
>>>>>
>>>>> I asked Shirish exactly the same question while discussing in the
>>>>> #samba-technical irc channel. He does not see a leak, but thought you or
>>>>> Steve will have a better idea..
>>>>>
>>>>> Steve:  is such a situation not possible at all?
>>>>>
>>>>
>>>> I'm pretty sure it is possible and may even be somewhat likely.
>>>> Consider this situation. vfs_create gets called from
>>>> __open_namei_create. Something like this:
>>>>
>>>> do_last
>>>>   __open_namei_create
>>>>     vfs_create
>>>>        inode create operation
>>>>
>>>> ...after this, __open_namei_create calls may_open to check permissions
>>>> and can return an error. If that occurs, then I don't think the open op
>>>> will ever be called.
>>>>
>>>> I think you can probably reproduce this by doing something like this:
>>>>
>>>> Have samba export a world-writable directory. Mount up the share with a
>>>> user's credentials. Make sure that unix extensions are enabled. Have a
>>>> different user do something like this into a file on the mount:
>>>>
>>>>     echo foo > /path/to/share/testfile
>>>>
>>>> It's probable that the file will be created, but the open-for-write
>>>> permission check will fail and the open file will be left dangling.
>>>>
>>>
>>> A quick test shows it is not leaking atleast in this case. What happens is:
>>>   cifs_lookup() to the file returns NULL
>>>   cifs_posix_open()
>>>     CIFSPOSIXCreate() (file gets created)
>>>     cifs_new_fileinfo() (updated the openFileList)
>>>   lookup_instantiate_filp (gets the filep, calls cifs_open)
>>>   followed by a cifs_close
>>>
>>> On the wire, I see SET_PATH_INFO with Level Of Interest set to Set File
>>> Posix Open call followed by a Close.
>>>
>>
>> Hmm ok...sounds like the create on lookup stuff might be getting in the
>> way of reproducing this (even though you said that cifs_lookup returned
>
> I actually intended to mean cifs_lookup figured out file is not present.
> Yes, create on lookup prevents this from occuring in this case.
>
>> NULL). Maybe could do better reproducing this with a program that does
>> an open(...O_EXCL|O_CREAT|O_WRONLY....) or something? The O_EXCL makes
>> it fall through to cifs_create which doesn't do lookup_instantiate_filp.
>
> I tried this.
>
> open("f_creat", O_CREAT | O_WRONLY | O_TRUNC, 0644)
>
> and
>
> open("f_excl_creat", O_CREAT | O_WRONLY | O_EXCL, 0644)
>
> In both the cases create on lookup comes into play.. I'm starting to
> think whether cifs_create will never get called directly since it seemed
> to be wrapped.
>
>> In any case, I think the problem is valid. Clearly nothing will clean
>> these up if cifs_open is never called after cifs_create is...right?
>>
>
> Yes, absolutely that was my thinking too. But I'm yet to see a way to
> crack this in practice..
>
>
> Thanks,
>
> --
> Suresh Jayaraman
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client at lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Can this error be induced with unix extensions turned off?