[linux-cifs-client] [PATCH] cifs: add comments explaining cifs_new_fileinfo behavior

[Jeff Layton]

On Tue, 11 May 2010 08:57:32 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:

> On Tue, May 11, 2010 at 6:33 AM, Jeff Layton <jlayton at samba.org> wrote:
> > On Tue, 11 May 2010 16:33:44 +0530
> > Suresh Jayaraman <sjayaraman at 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 at suse.de> wrote:
> >> >
> >> >> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> >> >>
> >> >> Signed-off-by: Suresh Jayaraman <sjayaraman at 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 <jlayton at samba.org>