diff mbox

[linux-cifs-client] Re: [GIT] CIFS Fix

Message ID 524f69650905211401s20617430se9aa5bfdc8cdc108@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve French May 21, 2009, 9:01 p.m. UTC 
Same thing but fixed the long line.

Unless problems are found (I will continue to test, to supplement was
Shirish is doing), I can put this in my tree, ASAP

The unrelated cifs_create/get_info possible file handle leak fix later
that I mentioned can be added later.

Comments

Luca Tettamanti May 21, 2009, 9:53 p.m. UTC | #1 
On Thu, May 21, 2009 at 04:01:15PM -0500, Steve French wrote:
> Same thing but fixed the long line.
> 
> Unless problems are found (I will continue to test, to supplement was
> Shirish is doing), I can put this in my tree, ASAP

I confirm that the patch fixes the bug.

Tested-By: Luca Tettamanti <kronos.it@gmail.com>

thanks,
Luca 

> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 11431ed..f49d684 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -225,6 +225,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  	if (!(oflags & FMODE_READ))
>  		write_only = true;
>  
> +	mode &= ~current_umask();
>  	rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
>  			pnetfid, presp_data, &oplock, full_path,
>  			cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> @@ -310,7 +311,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  		return -ENOMEM;
>  	}
>  
> -	mode &= ~current_umask();
>  	if (oplockEnabled)
>  		oplock = REQ_OPLOCK;
>  
> @@ -336,7 +336,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  			else /* success, no need to query */
>  				goto cifs_create_set_dentry;
>  		} else if ((rc != -EIO) && (rc != -EREMOTE) &&
> -			 (rc != -EOPNOTSUPP)) /* path not found or net err */
> +			 (rc != -EOPNOTSUPP) && (rc != -EINVAL))
>  			goto cifs_create_out;
>  		/* else fallthrough to retry, using older open call, this is
>  		   case where server does not support this SMB level, and
> @@ -609,7 +609,6 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  	int xid;
>  	int rc = 0; /* to get around spurious gcc warning, set to zero here */
>  	int oplock = 0;
> -	int mode;
>  	__u16 fileHandle = 0;
>  	bool posix_open = false;
>  	struct cifs_sb_info *cifs_sb;
> @@ -660,13 +659,12 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  
>  	if (pTcon->unix_ext) {
>  		if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
> -				(nd->flags & LOOKUP_OPEN)) {
> +		     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open) {
>  			if (!((nd->intent.open.flags & O_CREAT) &&
>  					(nd->intent.open.flags & O_EXCL))) {
> -				mode = nd->intent.open.create_mode &
> -						~current_umask();
>  				rc = cifs_posix_open(full_path, &newInode,
> -					parent_dir_inode->i_sb, mode,
> +					parent_dir_inode->i_sb,
> +					nd->intent.open.create_mode,
>  					nd->intent.open.flags, &oplock,
>  					&fileHandle, xid);
>  				/*
> @@ -681,6 +679,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  				 */
>  				if ((rc != -EINVAL) && (rc != -EOPNOTSUPP))
>  					posix_open = true;
> +				else
> +					pTcon->broken_posix_open = true;
>  			}
>  		}
>  		if (!posix_open)
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 38c06f8..302ea15 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -130,10 +130,6 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
>  			struct cifsFileInfo *pCifsFile, int oplock, u16 netfid)
>  {
>  
> -	file->private_data = kmalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
> -	if (file->private_data == NULL)
> -		return -ENOMEM;
> -	pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
>  	write_lock(&GlobalSMBSeslock);
>  
>  	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> @@ -184,6 +180,38 @@ psx_client_can_cache:
>  	return 0;
>  }
>  
> +static struct cifsFileInfo *
> +cifs_fill_filedata(struct file *file)
> +{
> +	struct list_head *tmp;
> +	struct cifsFileInfo *pCifsFile = NULL;
> +	struct cifsInodeInfo *pCifsInode = NULL;
> +
> +	/* search inode for this file and fill in file->private_data */
> +	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> +	read_lock(&GlobalSMBSeslock);
> +	list_for_each(tmp, &pCifsInode->openFileList) {
> +		pCifsFile = list_entry(tmp, struct cifsFileInfo, flist);
> +		if ((pCifsFile->pfile == NULL) &&
> +		    (pCifsFile->pid == current->tgid)) {
> +			/* mode set in cifs_create */
> +
> +			/* needed for writepage */
> +			pCifsFile->pfile = file;
> +			file->private_data = pCifsFile;
> +			break;
> +		}
> +	}
> +	read_unlock(&GlobalSMBSeslock);
> +
> +	if (file->private_data != NULL) {
> +		return pCifsFile;
> +	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
> +			cERROR(1, ("could not find file instance for "
> +				   "new file %p", file));
> +	return NULL;
> +}
> +
>  /* all arguments to this function must be checked for validity in caller */
>  static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
>  	struct cifsInodeInfo *pCifsInode, struct cifsFileInfo *pCifsFile,
> @@ -258,7 +286,6 @@ int cifs_open(struct inode *inode, struct file *file)
>  	struct cifsTconInfo *tcon;
>  	struct cifsFileInfo *pCifsFile;
>  	struct cifsInodeInfo *pCifsInode;
> -	struct list_head *tmp;
>  	char *full_path = NULL;
>  	int desiredAccess;
>  	int disposition;
> @@ -270,32 +297,12 @@ int cifs_open(struct inode *inode, struct file *file)
>  	cifs_sb = CIFS_SB(inode->i_sb);
>  	tcon = cifs_sb->tcon;
>  
> -	/* search inode for this file and fill in file->private_data */
>  	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> -	read_lock(&GlobalSMBSeslock);
> -	list_for_each(tmp, &pCifsInode->openFileList) {
> -		pCifsFile = list_entry(tmp, struct cifsFileInfo,
> -				       flist);
> -		if ((pCifsFile->pfile == NULL) &&
> -		    (pCifsFile->pid == current->tgid)) {
> -			/* mode set in cifs_create */
> -
> -			/* needed for writepage */
> -			pCifsFile->pfile = file;
> -
> -			file->private_data = pCifsFile;
> -			break;
> -		}
> -	}
> -	read_unlock(&GlobalSMBSeslock);
> -
> -	if (file->private_data != NULL) {
> -		rc = 0;
> +	pCifsFile = cifs_fill_filedata(file);
> +	if (pCifsFile) {
>  		FreeXid(xid);
> -		return rc;
> -	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
> -			cERROR(1, ("could not find file instance for "
> -				   "new file %p", file));
> +		return 0;
> +	}
>  
>  	full_path = build_path_from_dentry(file->f_path.dentry);
>  	if (full_path == NULL) {
> @@ -325,6 +332,7 @@ int cifs_open(struct inode *inode, struct file *file)
>  			/* no need for special case handling of setting mode
>  			   on read only files needed here */
>  
> +			pCifsFile = cifs_fill_filedata(file);
>  			cifs_posix_open_inode_helper(inode, file, pCifsInode,
>  						     pCifsFile, oplock, netfid);
>  			goto out;
Steve French May 21, 2009, 10:40 p.m. UTC | #2 
The following changes since commit 5805977e63a36ad56594a623f3bd2bebcb7db233:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/drm-2.6

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6.git
..BRANCH.NOT.VERIFIED..

Steve French (1):
      [CIFS] fix posix open regression

 fs/cifs/dir.c  |   14 ++++++------
 fs/cifs/file.c |   66 +++++++++++++++++++++++++++++++------------------------
 2 files changed, 44 insertions(+), 36 deletions(-)

On Thu, May 21, 2009 at 4:53 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Thu, May 21, 2009 at 04:01:15PM -0500, Steve French wrote:
>> Same thing but fixed the long line.
>>
>> Unless problems are found (I will continue to test, to supplement was
>> Shirish is doing), I can put this in my tree, ASAP
>
> I confirm that the patch fixes the bug.
>
> Tested-By: Luca Tettamanti <kronos.it@gmail.com>
Linus Torvalds May 22, 2009, 2:40 p.m. UTC | #3 
On Thu, 21 May 2009, Steve French wrote:
>
> The following changes since commit 5805977e63a36ad56594a623f3bd2bebcb7db233:
>   Linus Torvalds (1):
>         Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/drm-2.6
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6.git
> ..BRANCH.NOT.VERIFIED..
> 
> Steve French (1):
>       [CIFS] fix posix open regression

Thanks, pulled,

		Linus
diff mbox

Patch

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 11431ed..f49d684 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -225,6 +225,7 @@  int cifs_posix_open(char *full_path, struct inode **pinode,
 	if (!(oflags & FMODE_READ))
 		write_only = true;
 
+	mode &= ~current_umask();
 	rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
 			pnetfid, presp_data, &oplock, full_path,
 			cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
@@ -310,7 +311,6 @@  cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 		return -ENOMEM;
 	}
 
-	mode &= ~current_umask();
 	if (oplockEnabled)
 		oplock = REQ_OPLOCK;
 
@@ -336,7 +336,7 @@  cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 			else /* success, no need to query */
 				goto cifs_create_set_dentry;
 		} else if ((rc != -EIO) && (rc != -EREMOTE) &&
-			 (rc != -EOPNOTSUPP)) /* path not found or net err */
+			 (rc != -EOPNOTSUPP) && (rc != -EINVAL))
 			goto cifs_create_out;
 		/* else fallthrough to retry, using older open call, this is
 		   case where server does not support this SMB level, and
@@ -609,7 +609,6 @@  cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	int xid;
 	int rc = 0; /* to get around spurious gcc warning, set to zero here */
 	int oplock = 0;
-	int mode;
 	__u16 fileHandle = 0;
 	bool posix_open = false;
 	struct cifs_sb_info *cifs_sb;
@@ -660,13 +659,12 @@  cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 
 	if (pTcon->unix_ext) {
 		if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
-				(nd->flags & LOOKUP_OPEN)) {
+		     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open) {
 			if (!((nd->intent.open.flags & O_CREAT) &&
 					(nd->intent.open.flags & O_EXCL))) {
-				mode = nd->intent.open.create_mode &
-						~current_umask();
 				rc = cifs_posix_open(full_path, &newInode,
-					parent_dir_inode->i_sb, mode,
+					parent_dir_inode->i_sb,
+					nd->intent.open.create_mode,
 					nd->intent.open.flags, &oplock,
 					&fileHandle, xid);
 				/*
@@ -681,6 +679,8 @@  cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 				 */
 				if ((rc != -EINVAL) && (rc != -EOPNOTSUPP))
 					posix_open = true;
+				else
+					pTcon->broken_posix_open = true;
 			}
 		}
 		if (!posix_open)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 38c06f8..302ea15 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -130,10 +130,6 @@  static inline int cifs_posix_open_inode_helper(struct inode *inode,
 			struct cifsFileInfo *pCifsFile, int oplock, u16 netfid)
 {
 
-	file->private_data = kmalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
-	if (file->private_data == NULL)
-		return -ENOMEM;
-	pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
 	write_lock(&GlobalSMBSeslock);
 
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
@@ -184,6 +180,38 @@  psx_client_can_cache:
 	return 0;
 }
 
+static struct cifsFileInfo *
+cifs_fill_filedata(struct file *file)
+{
+	struct list_head *tmp;
+	struct cifsFileInfo *pCifsFile = NULL;
+	struct cifsInodeInfo *pCifsInode = NULL;
+
+	/* search inode for this file and fill in file->private_data */
+	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
+	read_lock(&GlobalSMBSeslock);
+	list_for_each(tmp, &pCifsInode->openFileList) {
+		pCifsFile = list_entry(tmp, struct cifsFileInfo, flist);
+		if ((pCifsFile->pfile == NULL) &&
+		    (pCifsFile->pid == current->tgid)) {
+			/* mode set in cifs_create */
+
+			/* needed for writepage */
+			pCifsFile->pfile = file;
+			file->private_data = pCifsFile;
+			break;
+		}
+	}
+	read_unlock(&GlobalSMBSeslock);
+
+	if (file->private_data != NULL) {
+		return pCifsFile;
+	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
+			cERROR(1, ("could not find file instance for "
+				   "new file %p", file));
+	return NULL;
+}
+
 /* all arguments to this function must be checked for validity in caller */
 static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
 	struct cifsInodeInfo *pCifsInode, struct cifsFileInfo *pCifsFile,
@@ -258,7 +286,6 @@  int cifs_open(struct inode *inode, struct file *file)
 	struct cifsTconInfo *tcon;
 	struct cifsFileInfo *pCifsFile;
 	struct cifsInodeInfo *pCifsInode;
-	struct list_head *tmp;
 	char *full_path = NULL;
 	int desiredAccess;
 	int disposition;
@@ -270,32 +297,12 @@  int cifs_open(struct inode *inode, struct file *file)
 	cifs_sb = CIFS_SB(inode->i_sb);
 	tcon = cifs_sb->tcon;
 
-	/* search inode for this file and fill in file->private_data */
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
-	read_lock(&GlobalSMBSeslock);
-	list_for_each(tmp, &pCifsInode->openFileList) {
-		pCifsFile = list_entry(tmp, struct cifsFileInfo,
-				       flist);
-		if ((pCifsFile->pfile == NULL) &&
-		    (pCifsFile->pid == current->tgid)) {
-			/* mode set in cifs_create */
-
-			/* needed for writepage */
-			pCifsFile->pfile = file;
-
-			file->private_data = pCifsFile;
-			break;
-		}
-	}
-	read_unlock(&GlobalSMBSeslock);
-
-	if (file->private_data != NULL) {
-		rc = 0;
+	pCifsFile = cifs_fill_filedata(file);
+	if (pCifsFile) {
 		FreeXid(xid);
-		return rc;
-	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
-			cERROR(1, ("could not find file instance for "
-				   "new file %p", file));
+		return 0;
+	}
 
 	full_path = build_path_from_dentry(file->f_path.dentry);
 	if (full_path == NULL) {
@@ -325,6 +332,7 @@  int cifs_open(struct inode *inode, struct file *file)
 			/* no need for special case handling of setting mode
 			   on read only files needed here */
 
+			pCifsFile = cifs_fill_filedata(file);
 			cifs_posix_open_inode_helper(inode, file, pCifsInode,
 						     pCifsFile, oplock, netfid);
 			goto out;