| Message ID | 1273501805-17754-1-git-send-email-sjayaraman@suse.de |
|---|---|
| State | New |
| Headers | show |
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>
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.
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,
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, >
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,
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 >
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 ;).
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?
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 >
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...)
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> >
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.
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.
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,
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?
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,
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);
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(-)