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

Shirish Pargaonkar shirishpargaonkar at gmail.com
Tue May 11 08:45:54 MDT 2010


On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton at samba.org> wrote:
> On Tue, 11 May 2010 18:18:08 +0530
> Suresh Jayaraman <sjayaraman at suse.de> wrote:
>
>> On 05/11/2010 05:03 PM, Jeff Layton wrote:
>> > On Tue, 11 May 2010 16:33:44 +0530
>> > Suresh Jayaraman <sjayaraman at suse.de> wrote:
>> >
>> >>>>
>> >>>> +/*
>> >>>> + * When called with struct file pointer set to NULL, there is no way we could
>> >>>> + * update file->private_data, but getting it stuck on openFileList provides a
>> >>>> + * way to access it from cifs_fill_filedata and thereby set file->private_data
>> >>>> + * from cifs_open.
>> >>>> + */
>> >>
>> >>>
>> >>> 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.
>>
>>
>> Thanks,
>>
>
> 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
> 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.
>
> 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?
>

It is possible, during unmount, cifs goes through openFileList, freeing up
structures on the list, have to delve into code to affirm this either way.
If cifs is not doing it, it should start doing it.

> --
> Jeff Layton <jlayton at samba.org>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client at lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>


More information about the linux-cifs-client mailing list