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

On Tue, 11 May 2010 16:33:44 +0530
Suresh Jayaraman <[email protected]> wrote:

> On 05/11/2010 04:17 PM, Jeff Layton wrote:
> > On Mon, 10 May 2010 20:00:05 +0530
> > Suresh Jayaraman <[email protected]> wrote:
> > 
> >> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> >>
> >> Signed-off-by: Suresh Jayaraman <[email protected]>
> >> --
> >>  fs/cifs/dir.c |   18 ++++++++++++++++--
> >>  1 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> >> index d791d07..bd363df 100644
> >> --- a/fs/cifs/dir.c
> >> +++ b/fs/cifs/dir.c
> >> @@ -129,6 +129,12 @@ cifs_bp_rename_retry:
> >>  	return full_path;
> >>  }
> >>  
> >> +/*
> >> + * 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.

> > I think we should be using lookup_instantiate_filp here instead and
> > doing this similarly to the open on lookup code. There is some
> > precedent for this -- fuse_create_open does this and it gets called via
> > the .create operation.
> > 
> 
> I thought about this briefly but was not sure whether it can be called
> from cifs_create. I'll need to dig more.
> 
> 
> Thanks,
> 


-- 
Jeff Layton <[email protected]>