[linux-cifs-client] [patch] prevent slab corruption by fixing race codition in cifs

[Jeff Layton]

On Mon, 17 Aug 2009 15:31:43 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:

> Jeff,
> 
> Hope this works.
> 

Thanks for sending it. Comments inline below:

> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 6084d63..9ad3258 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -349,6 +349,7 @@ struct cifsFileInfo {
>  	struct mutex lock_mutex;
>  	struct list_head llist; /* list of byte range locks we have. */
>  	bool closePend:1;	/* file is marked to close */
> +	bool closed:1;		/* file is closed */
>  	bool invalidHandle:1;	/* file closed via session abend */
>  	bool messageMode:1;	/* for pipes: message vs byte mode */
>  	atomic_t wrtPending;   /* handle in use - defer close */
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c34b7f8..f1ae25c 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -698,7 +698,10 @@ int cifs_close(struct inode *inode, struct file *file)
>  			msleep(timeout);
>  			timeout *= 8;
>  		}
> -		kfree(file->private_data);
> +		if (atomic_read(&pSMBFile->wrtPending) == 0)
> +			kfree(file->private_data);
> +		else
> +			pSMBFile->closed = true;

This is racy. It's possible that wrtPending will change just after you
check it but before you kfree or set closed to true.

>  		file->private_data = NULL;
>  	} else
>  		rc = -EBADF;
> @@ -1293,7 +1296,10 @@ refind_writable:
>  				else { /* start over in case this was deleted */
>  				       /* since the list could be modified */
>  					read_lock(&GlobalSMBSeslock);
> -					atomic_dec(&open_file->wrtPending);
> +					if (atomic_dec_and_test(
> +							&open_file->wrtPending)
> +							&& open_file->closed)
> +						kfree(open_file);
>  					goto refind_writable;
>  				}
>  			}
> @@ -1309,10 +1315,12 @@ refind_writable:
>  			read_lock(&GlobalSMBSeslock);
>  			/* can not use this handle, no write
>  			   pending on this one after all */
> -			atomic_dec(&open_file->wrtPending);
> -
> -			if (open_file->closePend) /* list could have changed */
> +			if (atomic_dec_and_test(&open_file->wrtPending)
> +					&& open_file->closed) {
> +				kfree(open_file);
>  				goto refind_writable;
> +			} else if (open_file->closePend) /* list could */
> +				goto refind_writable;	 /* have changed */
>  			/* else we simply continue to the next entry. Thus
>  			   we do not loop on reopen errors.  If we
>  			   can not reopen the file, for example if we
> @@ -1373,7 +1381,9 @@ static int cifs_partialpagewrite(struct page
> *page, unsigned from, unsigned to)
>  	if (open_file) {
>  		bytes_written = cifs_write(open_file->pfile, write_data,
>  					   to-from, &offset);
> -		atomic_dec(&open_file->wrtPending);
> +		if (atomic_dec_and_test(&open_file->wrtPending)
> +				&& open_file->closed)
> +			kfree(open_file);
>  		/* Does mm or vfs already set times? */
>  		inode->i_atime = inode->i_mtime = current_fs_time(inode->i_sb);
>  		if ((bytes_written > 0) && (offset))
> @@ -1562,7 +1572,9 @@ retry:
>  						   bytes_to_write, offset,
>  						   &bytes_written, iov, n_iov,
>  						   long_op);
> -				atomic_dec(&open_file->wrtPending);
> +				if (atomic_dec_and_test(&open_file->wrtPending)
> +						&& open_file->closed)
> +					kfree(open_file);
>  				cifs_update_eof(cifsi, offset, bytes_written);
> 
>  				if (rc || bytes_written < bytes_to_write) {
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 82d8383..3b4c27d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -799,8 +799,11 @@ set_via_filehandle:
> 
>  	if (open_file == NULL)
>  		CIFSSMBClose(xid, pTcon, netfid);
> -	else
> -		atomic_dec(&open_file->wrtPending);
> +	else {
> +		if (atomic_dec_and_test(&open_file->wrtPending)
> +					&& open_file->closed)
> +			kfree(open_file);
> +	}
>  out:
>  	return rc;
>  }
> @@ -1635,7 +1638,9 @@ cifs_set_file_size(struct inode *inode, struct
> iattr *attrs,
>  		__u32 npid = open_file->pid;
>  		rc = CIFSSMBSetFileSize(xid, pTcon, attrs->ia_size, nfid,
>  					npid, false);
> -		atomic_dec(&open_file->wrtPending);
> +		if (atomic_dec_and_test(&open_file->wrtPending)
> +					&& open_file->closed)
> +			kfree(open_file);
>  		cFYI(1, ("SetFSize for attrs rc = %d", rc));
>  		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
>  			unsigned int bytes_written;
> @@ -1790,7 +1795,9 @@ cifs_setattr_unix(struct dentry *direntry,
> struct iattr *attrs)
>  		u16 nfid = open_file->netfid;
>  		u32 npid = open_file->pid;
>  		rc = CIFSSMBUnixSetFileInfo(xid, pTcon, args, nfid, npid);
> -		atomic_dec(&open_file->wrtPending);
> +		if (atomic_dec_and_test(&open_file->wrtPending)
> +				&& open_file->closed)
> +			kfree(open_file);
>  	} else {
>  		rc = CIFSSMBUnixSetPathInfo(xid, pTcon, full_path, args,
>  				    cifs_sb->local_nls,
> 
> 
> signed-off by: Shirish Pargaonkar <shirishpargaonkar at gmail.com>
> 

I'm less than thrilled with this patch. This looks like like it's
layering more complexity onto a codepath that is already far too
complex for what it does.

Is it not possible to just use regular old refcounting for the open
filehandles? i.e. have each user of the filehandle take a reference,
and the last one to put it does the actual close. That seems like a
much better approach to me than all of this crazy flag business.

-- 
Jeff Layton <jlayton at redhat.com>