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

On Wed,  7 Apr 2010 11:19:10 -0500
[email protected] 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 <[email protected]>
> Reported-by: Eugene Teo <[email protected]>
> ---
> 

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
> [email protected]
> 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 <[email protected]>