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

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?

-- 
Jeff Layton <[email protected]>