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

[Suresh Jayaraman]

On 05/12/2010 08:45 PM, Shirish Pargaonkar wrote:
> 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:
>>>>> 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..
>>
> 
> Can this error be induced with unix extensions turned off?

No, it doesn't seem so.. cifs_open comes into rescue this time..


Thanks,


-- 
Suresh Jayaraman