diff mbox

[linux-cifs-client[[patch] Attempt #2 to handle null nameidata

Message ID 1270657150-2845-1-git-send-email-shirishpargaonkar@gmail.com
State New
Headers show

Commit Message

Shirish Pargaonkar April 7, 2010, 4:19 p.m. UTC 
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@gmail.com>
Reported-by: Eugene Teo <eugeneteo@kernel.sg>
---

Comments

Jeff Layton April 8, 2010, 7:34 p.m. UTC | #1 
On Wed,  7 Apr 2010 11:19:10 -0500
shirishpargaonkar@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@gmail.com>
> Reported-by: Eugene Teo <eugeneteo@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?

>  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@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.
Shirish Pargaonkar April 8, 2010, 7:40 p.m. UTC | #2 
On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
> On Wed,  7 Apr 2010 11:19:10 -0500
> shirishpargaonkar@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@gmail.com>
>> Reported-by: Eugene Teo <eugeneteo@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.

Regards,

Shirish

>
>>  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@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@samba.org>
>
Jeff Layton April 8, 2010, 7:58 p.m. UTC | #3 
On Thu, 8 Apr 2010 14:40:47 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
> > On Wed,  7 Apr 2010 11:19:10 -0500
> > shirishpargaonkar@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@gmail.com>
> >> Reported-by: Eugene Teo <eugeneteo@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@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@samba.org>
> >
>
Suresh Jayaraman April 29, 2010, 10:26 a.m. UTC | #4 
On 04/09/2010 01:28 AM, Jeff Layton wrote:
> On Thu, 8 Apr 2010 14:40:47 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> 
>> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
>>> On Wed, �7 Apr 2010 11:19:10 -0500
>>> shirishpargaonkar@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@gmail.com>
>>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>

> 
> We'll need to take this patch in the interim though to fix the
> immediate oops.
> 

Do we need to Cc -stable as well as the issue seem to be reproducible on
kernel versions > 2.6.29-rc6?

Thanks,
Jeff Layton April 29, 2010, 11:16 a.m. UTC | #5 
On Thu, 29 Apr 2010 15:56:02 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 04/09/2010 01:28 AM, Jeff Layton wrote:
> > On Thu, 8 Apr 2010 14:40:47 -0500
> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> > 
> >> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
> >>> On Wed, �7 Apr 2010 11:19:10 -0500
> >>> shirishpargaonkar@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@gmail.com>
> >>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>
> 
> > 
> > We'll need to take this patch in the interim though to fix the
> > immediate oops.
> > 
> 
> Do we need to Cc -stable as well as the issue seem to be reproducible on
> kernel versions > 2.6.29-rc6?
> 
> Thanks,
> 

Can someone clarify how to reproduce this oops? I think that the only
place where this function gets called with NULL nameidata is from nfsd
and the export ops for cifs are just stubs. Has this actually been seen
in the field or was it just found via inspection?
Shirish Pargaonkar April 29, 2010, 1:51 p.m. UTC | #6 
On Thu, Apr 29, 2010 at 6:16 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Thu, 29 Apr 2010 15:56:02 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>
>> On 04/09/2010 01:28 AM, Jeff Layton wrote:
>> > On Thu, 8 Apr 2010 14:40:47 -0500
>> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>> >
>> >> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
>> >>> On Wed, �7 Apr 2010 11:19:10 -0500
>> >>> shirishpargaonkar@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@gmail.com>
>> >>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>
>>
>> >
>> > We'll need to take this patch in the interim though to fix the
>> > immediate oops.
>> >
>>
>> Do we need to Cc -stable as well as the issue seem to be reproducible on
>> kernel versions > 2.6.29-rc6?
>>
>> Thanks,
>>
>
> Can someone clarify how to reproduce this oops? I think that the only
> place where this function gets called with NULL nameidata is from nfsd
> and the export ops for cifs are just stubs. Has this actually been seen
> in the field or was it just found via inspection?
>
> --
> Jeff Layton <jlayton@samba.org>
>

As far as I know, by inspection.
Eugene, can you please comment on this?

Regards,

Shirish
Eugene Teo April 29, 2010, 2:12 p.m. UTC | #7 
On Thu, Apr 29, 2010 at 9:51 PM, Shirish Pargaonkar
<shirishpargaonkar@gmail.com> wrote:
> On Thu, Apr 29, 2010 at 6:16 AM, Jeff Layton <jlayton@samba.org> wrote:
>> On Thu, 29 Apr 2010 15:56:02 +0530
>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>
>>> On 04/09/2010 01:28 AM, Jeff Layton wrote:
>>> > On Thu, 8 Apr 2010 14:40:47 -0500
>>> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>>> >
>>> >> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
>>> >>> On Wed, �7 Apr 2010 11:19:10 -0500
>>> >>> shirishpargaonkar@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@gmail.com>
>>> >>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>
>>>
>>> >
>>> > We'll need to take this patch in the interim though to fix the
>>> > immediate oops.
>>> >
>>>
>>> Do we need to Cc -stable as well as the issue seem to be reproducible on
>>> kernel versions > 2.6.29-rc6?
>>>
>>> Thanks,
>>>
>>
>> Can someone clarify how to reproduce this oops? I think that the only
>> place where this function gets called with NULL nameidata is from nfsd
>> and the export ops for cifs are just stubs. Has this actually been seen
>> in the field or was it just found via inspection?
>>
>> --
>> Jeff Layton <jlayton@samba.org>
>
> As far as I know, by inspection.
> Eugene, can you please comment on this?

It was found by inspection. Did not attempt to reproduce the issue.

Eugene
Jeff Layton April 29, 2010, 2:29 p.m. UTC | #8 
On Thu, 29 Apr 2010 22:12:16 +0800
Eugene Teo <eugeneteo@kernel.sg> wrote:

> On Thu, Apr 29, 2010 at 9:51 PM, Shirish Pargaonkar
> <shirishpargaonkar@gmail.com> wrote:
> > On Thu, Apr 29, 2010 at 6:16 AM, Jeff Layton <jlayton@samba.org> wrote:
> >> On Thu, 29 Apr 2010 15:56:02 +0530
> >> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >>
> >>> On 04/09/2010 01:28 AM, Jeff Layton wrote:
> >>> > On Thu, 8 Apr 2010 14:40:47 -0500
> >>> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> >>> >
> >>> >> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org> wrote:
> >>> >>> On Wed, �7 Apr 2010 11:19:10 -0500
> >>> >>> shirishpargaonkar@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@gmail.com>
> >>> >>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>
> >>>
> >>> >
> >>> > We'll need to take this patch in the interim though to fix the
> >>> > immediate oops.
> >>> >
> >>>
> >>> Do we need to Cc -stable as well as the issue seem to be reproducible on
> >>> kernel versions > 2.6.29-rc6?
> >>>
> >>> Thanks,
> >>>
> >>
> >> Can someone clarify how to reproduce this oops? I think that the only
> >> place where this function gets called with NULL nameidata is from nfsd
> >> and the export ops for cifs are just stubs. Has this actually been seen
> >> in the field or was it just found via inspection?
> >>
> >> --
> >> Jeff Layton <jlayton@samba.org>
> >
> > As far as I know, by inspection.
> > Eugene, can you please comment on this?
> 
> It was found by inspection. Did not attempt to reproduce the issue.
> 
> Eugene
> 

Ok. I don't think this is actually exploitable. Certainly something
that should be fixed in case CIFS ever is exportable via nfsd, but not
worthy of a CVE.

On a related note... the sb->s_export_ops ought to be NULL in all cases
until CIFS actually has export ops that are functional. The current
situation probably tricks nfsd into thinking that CIFS is exportable
when it really isn't. That's likely to be very confusing for users.
Steve French April 29, 2010, 4:38 p.m. UTC | #9 
On Thu, Apr 29, 2010 at 9:29 AM, Jeff Layton <jlayton@samba.org> wrote:

> On Thu, 29 Apr 2010 22:12:16 +0800
> Eugene Teo <eugeneteo@kernel.sg> wrote:
>
> > On Thu, Apr 29, 2010 at 9:51 PM, Shirish Pargaonkar
> > <shirishpargaonkar@gmail.com> wrote:
> > > On Thu, Apr 29, 2010 at 6:16 AM, Jeff Layton <jlayton@samba.org>
> wrote:
> > >> On Thu, 29 Apr 2010 15:56:02 +0530
> > >> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> > >>
> > >>> On 04/09/2010 01:28 AM, Jeff Layton wrote:
> > >>> > On Thu, 8 Apr 2010 14:40:47 -0500
> > >>> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> > >>> >
> > >>> >> On Thu, Apr 8, 2010 at 2:34 PM, Jeff Layton <jlayton@samba.org>
> wrote:
> > >>> >>> On Wed, �7 Apr 2010 11:19:10 -0500
> > >>> >>> shirishpargaonkar@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@gmail.com>
> > >>> >>>> Reported-by: Eugene Teo <eugeneteo@kernel.sg>
> > >>>
> > >>> >
> > >>> > We'll need to take this patch in the interim though to fix the
> > >>> > immediate oops.
> > >>> >
> > >>>
> > >>> Do we need to Cc -stable as well as the issue seem to be reproducible
> on
> > >>> kernel versions > 2.6.29-rc6?
> > >>>
> > >>> Thanks,
> > >>>
> > >>
> > >> Can someone clarify how to reproduce this oops? I think that the only
> > >> place where this function gets called with NULL nameidata is from nfsd
> > >> and the export ops for cifs are just stubs. Has this actually been
> seen
> > >> in the field or was it just found via inspection?
> > >>
> > >> --
> > >> Jeff Layton <jlayton@samba.org>
> > >
> > > As far as I know, by inspection.
> > > Eugene, can you please comment on this?
> >
> > It was found by inspection. Did not attempt to reproduce the issue.
> >
> > Eugene
> >
>
> Ok. I don't think this is actually exploitable. Certainly something
> that should be fixed in case CIFS ever is exportable via nfsd, but not
> worthy of a CVE.
>
>
Yes - that is what we talked about earlier.    Looks like nfs client
has the same problem too (dereferences, potentially null nd)
but in practice can't get there (nfsd over nfs)


> On a related note... the sb->s_export_ops ought to be NULL in all cases
> until CIFS actually has export ops that are functional. The current
> situation probably tricks nfsd into thinking that CIFS is exportable
> when it really isn't. That's likely to be very confusing for users.
>
>
I don't think it matters much, the use case is nfs server
reexporting resources over cifs mounts on servers that don't
have nfs (via a cifs mount) - not sure if it is worth just
doing this in smb2 (where we have persistent file ids anyway,
which may be useful for this) or should do it in both
diff mbox

Patch

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);
 
 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;