Re: [PATCH] cifs: add comments explaining cifs_new_fileinfo behavior

On Tue, May 11, 2010 at 10:06 AM, Jeff Layton <[email protected]> wrote:
> On Tue, 11 May 2010 09:45:54 -0500
> Shirish Pargaonkar <[email protected]> wrote:
>
>> On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <[email protected]> wrote:
>> > On Tue, 11 May 2010 18:18:08 +0530
>> > Suresh Jayaraman <[email protected]> wrote:
>> >
>> >> On 05/11/2010 05:03 PM, Jeff Layton wrote:
>> >> > On Tue, 11 May 2010 16:33:44 +0530
>> >> > Suresh Jayaraman <[email protected]> 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.
>>
>
> When I say "leaked" I don't mean that they are never cleaned up (though
> I'm not certain that they are), but rather that they outlive the open
> syscall even though it failed.
>
> We should be cleaning these up when the open fails. It's incorrect to
> assume that cifs_open will always be called after cifs_create on an
> open(...O_CREAT...)

And along with cleaning them up, we should close the file at the server also?

>
> --
> Jeff Layton <[email protected]>
>