[linux-cifs-client] [linux-cifs-client[[patch] Attempt #2 to handle null nameidata
Jeff Layton
jlayton at samba.org
Thu Apr 8 13:58:53 MDT 2010
On Thu, 8 Apr 2010 14:40:47 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton at samba.org> wrote:
> > On Wed, 7 Apr 2010 11:19:10 -0500
> > shirishpargaonkar at gmail.com wrote:
> >
> >> While creating a file on a server which supports unix extensions
> >> such as Samba, if a file is being created which does not supply
> >> nameidata (i.e. nd is null), cifs client can oops when calling
> >> cifs_posix_open.
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar at gmail.com>
> >> Reported-by: Eugene Teo <eugeneteo at kernel.sg>
> >> ---
> >>
> >
> > This patch is hideous, but I suppose it's the best that can be done
> > short of rewriting this code to have a sane API.
> >
> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >> index 88e2bc4..efb8772 100644
> >> --- a/fs/cifs/cifsproto.h
> >> +++ b/fs/cifs/cifsproto.h
> >> @@ -95,8 +95,10 @@ extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
> >> __u16 fileHandle, struct file *file,
> >> struct vfsmount *mnt, unsigned int oflags);
> >> extern int cifs_posix_open(char *full_path, struct inode **pinode,
> >> - struct vfsmount *mnt, int mode, int oflags,
> >> - __u32 *poplock, __u16 *pnetfid, int xid);
> >> + struct vfsmount *mnt,
> >> + struct super_block *sb,
> >> + int mode, int oflags,
> >> + __u32 *poplock, __u16 *pnetfid, int xid);
> >> extern void cifs_unix_basic_to_fattr(struct cifs_fattr *fattr,
> >> FILE_UNIX_BASIC_INFO *info,
> >> struct cifs_sb_info *cifs_sb);
> >> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> >> index 6ccf726..9e9d48f 100644
> >> --- a/fs/cifs/dir.c
> >> +++ b/fs/cifs/dir.c
> >> @@ -183,13 +183,14 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
> >> }
> >>
> >> int cifs_posix_open(char *full_path, struct inode **pinode,
> >> - struct vfsmount *mnt, int mode, int oflags,
> >> - __u32 *poplock, __u16 *pnetfid, int xid)
> >> + struct vfsmount *mnt, struct super_block *sb,
> >> + int mode, int oflags,
> >> + __u32 *poplock, __u16 *pnetfid, int xid)
> >> {
> >> int rc;
> >> FILE_UNIX_BASIC_INFO *presp_data;
> >> __u32 posix_flags = 0;
> >> - struct cifs_sb_info *cifs_sb = CIFS_SB(mnt->mnt_sb);
> >> + struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> >> struct cifs_fattr fattr;
> >>
> >> cFYI(1, ("posix open %s", full_path));
> >> @@ -242,7 +243,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >>
> >> /* get new inode and set it up */
> >> if (*pinode == NULL) {
> >> - *pinode = cifs_iget(mnt->mnt_sb, &fattr);
> >> + *pinode = cifs_iget(sb, &fattr);
> >> if (!*pinode) {
> >> rc = -ENOMEM;
> >> goto posix_open_ret;
> >> @@ -251,7 +252,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >> cifs_fattr_to_inode(*pinode, &fattr);
> >> }
> >>
> >> - cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
> >> + if (mnt)
> >> + cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
> >>
> >
> > The cifs_create codepath closes the file (via CIFSSMBClose) when nd is
> > NULL. The NULL mnt case here is analogous, right? Shouldn't it also
> > CIFSSMBClose the file?
>
> Jeff, not sure I understand. In case of null nd, cifs_create will
> close the file
> whether posix open or regular open (or legacy open) gets called or not.
>
Ahh ok, I see it now. Man this code is hard to follow. It really badly
needs cleanup such that the functions have clearly defined jobs and
behaviors.
We'll need to take this patch in the interim though to fix the
immediate oops.
> >
> >> posix_open_ret:
> >> kfree(presp_data);
> >> @@ -315,13 +317,14 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> >> if (nd && (nd->flags & LOOKUP_OPEN))
> >> oflags = nd->intent.open.flags;
> >> else
> >> - oflags = FMODE_READ;
> >> + oflags = FMODE_READ | SMB_O_CREAT;
> >>
> >> if (tcon->unix_ext && (tcon->ses->capabilities & CAP_UNIX) &&
> >> (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> >> le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> >> - rc = cifs_posix_open(full_path, &newinode, nd->path.mnt,
> >> - mode, oflags, &oplock, &fileHandle, xid);
> >> + rc = cifs_posix_open(full_path, &newinode,
> >> + nd ? nd->path.mnt : NULL,
> >> + inode->i_sb, mode, oflags, &oplock, &fileHandle, xid);
> >> /* EIO could indicate that (posix open) operation is not
> >> supported, despite what server claimed in capability
> >> negotation. EREMOTE indicates DFS junction, which is not
> >> @@ -678,6 +681,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >> (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
> >> (nd->intent.open.flags & O_CREAT)) {
> >> rc = cifs_posix_open(full_path, &newInode, nd->path.mnt,
> >> + parent_dir_inode->i_sb,
> >> nd->intent.open.create_mode,
> >> nd->intent.open.flags, &oplock,
> >> &fileHandle, xid);
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index 3d8f8a9..503a459 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -297,10 +297,12 @@ int cifs_open(struct inode *inode, struct file *file)
> >> (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> >> le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> >> int oflags = (int) cifs_posix_convert_flags(file->f_flags);
> >> + oflags |= SMB_O_CREAT;
> >> /* can not refresh inode info since size could be stale */
> >> rc = cifs_posix_open(full_path, &inode, file->f_path.mnt,
> >> - cifs_sb->mnt_file_mode /* ignored */,
> >> - oflags, &oplock, &netfid, xid);
> >> + inode->i_sb,
> >> + cifs_sb->mnt_file_mode /* ignored */,
> >> + oflags, &oplock, &netfid, xid);
> >> if (rc == 0) {
> >> cFYI(1, ("posix open succeeded"));
> >> /* no need for special case handling of setting mode
> >> @@ -512,8 +514,9 @@ reopen_error_exit:
> >> int oflags = (int) cifs_posix_convert_flags(file->f_flags);
> >> /* can not refresh inode info since size could be stale */
> >> rc = cifs_posix_open(full_path, NULL, file->f_path.mnt,
> >> - cifs_sb->mnt_file_mode /* ignored */,
> >> - oflags, &oplock, &netfid, xid);
> >> + inode->i_sb,
> >> + cifs_sb->mnt_file_mode /* ignored */,
> >> + oflags, &oplock, &netfid, xid);
> >> if (rc == 0) {
> >> cFYI(1, ("posix reopen succeeded"));
> >> goto reopen_success;
> >> _______________________________________________
> >> linux-cifs-client mailing list
> >> linux-cifs-client at lists.samba.org
> >> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >>
> >
> > Ugly, but I suppose it's all that can reasonably be done short of
> > making all of this code use a more sane API.
> >
> > --
> > Jeff Layton <jlayton at samba.org>
> >
>
--
Jeff Layton <jlayton at samba.org>
More information about the linux-cifs-client
mailing list