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

On Tue, May 11, 2010 at 6:33 AM, Jeff Layton <[email protected]> wrote:
> 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.
>

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 <[email protected]>
> _______________________________________________
> linux-cifs-client mailing list
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>