Patchwork cifs: add comments explaining cifs_new_fileinfo behavior

login
register
mail settings
Submitter Suresh Jayaraman
Date May 10, 2010, 2:30 p.m.
Message ID <1273501805-17754-1-git-send-email-sjayaraman@suse.de>
Download mbox | patch
Permalink /patch/52089/
State New
Headers show

Comments

Suresh Jayaraman - May 10, 2010, 2:30 p.m.
The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().

Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
--
 fs/cifs/dir.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
Shirish Pargaonkar - May 10, 2010, 2:34 p.m.
On Mon, May 10, 2010 at 9:30 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
>
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> --
>  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.
> + */
>  struct cifsFileInfo *
>  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
>                  struct file *file, struct vfsmount *mnt, unsigned int oflags)
> @@ -251,6 +257,10 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>                cifs_fattr_to_inode(*pinode, &fattr);
>        }
>
> +       /*
> +        * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
> +        * file->private_data.
> +        */
>        if (mnt)
>                cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>
> @@ -466,8 +476,12 @@ cifs_create_set_dentry:
>                /* mknod case - do not leave file open */
>                CIFSSMBClose(xid, tcon, fileHandle);
>        } else if (!(posix_create) && (newinode)) {
> -                       cifs_new_fileinfo(newinode, fileHandle, NULL,
> -                                               nd->path.mnt, oflags);
> +               /*
> +                * cifs_fill_filedata() takes care of setting cifsFileInfo
> +                * pointer to file->private_data.
> +                */
> +               cifs_new_fileinfo(newinode, fileHandle, NULL, nd->path.mnt,
> +                                 oflags);
>        }
>  cifs_create_out:
>        kfree(buf);
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Acked-by: Shirish S. Pargaonkar <shirishpargaonkar@gmail.com>
Jeff Layton - May 11, 2010, 10:47 a.m.
On Mon, 10 May 2010 20:00:05 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> --
>  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.
> + */
>  struct cifsFileInfo *
>  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
>  		  struct file *file, struct vfsmount *mnt, unsigned int oflags)
> @@ -251,6 +257,10 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  		cifs_fattr_to_inode(*pinode, &fattr);
>  	}
>  
> +	/*
> +	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
> +	 * file->private_data.
> +	 */
>  	if (mnt)
>  		cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>  
> @@ -466,8 +476,12 @@ cifs_create_set_dentry:
>  		/* mknod case - do not leave file open */
>  		CIFSSMBClose(xid, tcon, fileHandle);
>  	} else if (!(posix_create) && (newinode)) {
> -			cifs_new_fileinfo(newinode, fileHandle, NULL,
> -						nd->path.mnt, oflags);
> +		/*
> +		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> +		 * pointer to file->private_data.
> +		 */
> +		cifs_new_fileinfo(newinode, fileHandle, NULL, nd->path.mnt,
> +				  oflags);
>  	}
>  cifs_create_out:
>  	kfree(buf);
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 

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 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.
Suresh Jayaraman - May 11, 2010, 11:03 a.m.
On 05/11/2010 04:17 PM, Jeff Layton wrote:
> On Mon, 10 May 2010 20:00:05 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
>>
>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>> --
>>  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 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 - May 11, 2010, 11:33 a.m.
On Tue, 11 May 2010 16:33:44 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 05/11/2010 04:17 PM, Jeff Layton wrote:
> > On Mon, 10 May 2010 20:00:05 +0530
> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> > 
> >> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> >>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> >> --
> >>  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,
>
Suresh Jayaraman - May 11, 2010, 12:48 p.m.
On 05/11/2010 05:03 PM, Jeff Layton wrote:
> On Tue, 11 May 2010 16:33:44 +0530
> Suresh Jayaraman <sjayaraman@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,
Shirish Pargaonkar - May 11, 2010, 1:57 p.m.
On Tue, May 11, 2010 at 6:33 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Tue, 11 May 2010 16:33:44 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>
>> On 05/11/2010 04:17 PM, Jeff Layton wrote:
>> > On Mon, 10 May 2010 20:00:05 +0530
>> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
>> >
>> >> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
>> >>
>> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>> >> --
>> >>  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.
>

If may_open can return error, cifs_posix_open will/is_likely to fail
at the server as well right?
In which case, so could other forms of open, so there would not be
an inode (allocated) and cifsFileInfo structure (created).

> 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 <jlayton@samba.org>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
Jeff Layton - May 11, 2010, 2:07 p.m.
On Tue, 11 May 2010 08:57:32 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, May 11, 2010 at 6:33 AM, Jeff Layton <jlayton@samba.org> wrote:
> > On Tue, 11 May 2010 16:33:44 +0530
> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >
> >> On 05/11/2010 04:17 PM, Jeff Layton wrote:
> >> > On Mon, 10 May 2010 20:00:05 +0530
> >> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> >
> >> >> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> >> >>
> >> >> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> >> >> --
> >> >>  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.
> >
> 
> If may_open can return error, cifs_posix_open will/is_likely to fail
> at the server as well right?
> In which case, so could other forms of open, so there would not be
> an inode (allocated) and cifsFileInfo structure (created).
> 

Not at all. may_open checks the inode permissions based on the local
fsuid, which has no real relation to the credentials used at the
server. That's the fundamental problem I'm trying to fix with
multisession mounts ;).
Jeff Layton - May 11, 2010, 2:12 p.m.
On Tue, 11 May 2010 18:18:08 +0530
Suresh Jayaraman <sjayaraman@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@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?
Shirish Pargaonkar - May 11, 2010, 2:45 p.m.
On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Tue, 11 May 2010 18:18:08 +0530
> Suresh Jayaraman <sjayaraman@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@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@samba.org>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
Jeff Layton - May 11, 2010, 3:06 p.m.
On Tue, 11 May 2010 09:45:54 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton@samba.org> wrote:
> > On Tue, 11 May 2010 18:18:08 +0530
> > Suresh Jayaraman <sjayaraman@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@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.
> 

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...)
Shirish Pargaonkar - May 11, 2010, 3:12 p.m.
On Tue, May 11, 2010 at 10:06 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Tue, 11 May 2010 09:45:54 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>
>> On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton@samba.org> wrote:
>> > On Tue, 11 May 2010 18:18:08 +0530
>> > Suresh Jayaraman <sjayaraman@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@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.
>>
>
> 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 <jlayton@samba.org>
>
Jeff Layton - May 11, 2010, 4:51 p.m.
On Tue, 11 May 2010 10:12:07 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, May 11, 2010 at 10:06 AM, Jeff Layton <jlayton@samba.org> wrote:
> > On Tue, 11 May 2010 09:45:54 -0500
> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> >
> >> On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton@samba.org> wrote:
> >> > On Tue, 11 May 2010 18:18:08 +0530
> >> > Suresh Jayaraman <sjayaraman@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@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.
> >>
> >
> > 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?
> 

Yes, I believe so.
Jeff Layton - May 11, 2010, 5:14 p.m.
On Tue, 11 May 2010 12:51:41 -0400
Jeff Layton <jlayton@samba.org> wrote:

> On Tue, 11 May 2010 10:12:07 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> 
> > On Tue, May 11, 2010 at 10:06 AM, Jeff Layton <jlayton@samba.org> wrote:
> > > On Tue, 11 May 2010 09:45:54 -0500
> > > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> > >
> > >> On Tue, May 11, 2010 at 9:12 AM, Jeff Layton <jlayton@samba.org> wrote:
> > >> > On Tue, 11 May 2010 18:18:08 +0530
> > >> > Suresh Jayaraman <sjayaraman@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@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.
> > >>
> > >
> > > 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?
> > 
> 
> Yes, I believe so.
> 
> 

I should clarify though...

Since lookup_instantiate_filp creates a fully instantiated file
pointer, it will be cleaned up anyway if there is a problem. Basically
the error path will do a fput on it, so there should be no need to do
anything special for that if you do this correctly.
Suresh Jayaraman - May 12, 2010, 9:58 a.m.
On 05/11/2010 07:42 PM, Jeff Layton wrote:
> On Tue, 11 May 2010 18:18:08 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>>>>> 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.
>>
> 
> 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..


Thanks,
Shirish Pargaonkar - May 12, 2010, 3:15 p.m.
On Wed, May 12, 2010 at 4:58 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> On 05/11/2010 07:42 PM, Jeff Layton wrote:
>> On Tue, 11 May 2010 18:18:08 +0530
>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>
>>>>>> 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.
>>>
>>
>> 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..
>
>
> Thanks,
>
> --
> Suresh Jayaraman
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Can this error be induced with unix extensions turned off?
Suresh Jayaraman - May 13, 2010, 2:51 p.m.
On 05/12/2010 08:45 PM, Shirish Pargaonkar wrote:
> On Wed, May 12, 2010 at 4:58 AM, Suresh Jayaraman <sjayaraman@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,

Patch

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.
+ */
 struct cifsFileInfo *
 cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
 		  struct file *file, struct vfsmount *mnt, unsigned int oflags)
@@ -251,6 +257,10 @@  int cifs_posix_open(char *full_path, struct inode **pinode,
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
+	/*
+	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
+	 * file->private_data.
+	 */
 	if (mnt)
 		cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
 
@@ -466,8 +476,12 @@  cifs_create_set_dentry:
 		/* mknod case - do not leave file open */
 		CIFSSMBClose(xid, tcon, fileHandle);
 	} else if (!(posix_create) && (newinode)) {
-			cifs_new_fileinfo(newinode, fileHandle, NULL,
-						nd->path.mnt, oflags);
+		/*
+		 * cifs_fill_filedata() takes care of setting cifsFileInfo
+		 * pointer to file->private_data.
+		 */
+		cifs_new_fileinfo(newinode, fileHandle, NULL, nd->path.mnt,
+				  oflags);
 	}
 cifs_create_out:
 	kfree(buf);