Patchwork prevent slab corruption by fixing race codition in cifs

login
register
mail settings
Submitter Shirish Pargaonkar
Date Aug. 17, 2009, 8:31 p.m.
Message ID <4a4634330908171331n28e7d9fcif14b606f935271f8@mail.gmail.com>
Download mbox | patch
Permalink /patch/31531/
State New
Headers show

Comments

Shirish Pargaonkar - Aug. 17, 2009, 8:31 p.m.
Jeff,

Hope this works.

 			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@gmail.com>


On Mon, Aug 17, 2009 at 2:50 PM, Jeff Layton<jlayton@redhat.com> wrote:
> On Sun, 16 Aug 2009 11:38:57 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>
>> This patch prevents a slab corruption like this.  During heavy stress,
>> it is possible that
>> cifs_close will free up cifsFileInfo while due to delayed writes,
>> wrtPending of that
>> cifsFileInfo gets updated (decremented), cifsFileInfo either freed or
>> freed and allocated to another process.
>>
>>
>> Slab corruption: start=ffff8101e28e3818, len=256
>> Redzone: 0x5a2cf071/0x5a2cf071.
>> Last user: [<ffffffff88276ec4>](cifs_close+0x224/0x2c2 [cifs])
>> 060: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6a 6b 6b 6b
>> Prev obj: start=ffff8101e28e3700, len=256
>> Redzone: 0x170fc2a5/0x170fc2a5.
>> Last user: [<ffffffff882784fa>](cifs_open+0x348/0x6d9 [cifs])
>> 000: b8 d3 44 e2 01 81 ff ff a0 e2 c7 e1 01 81 ff ff
>> 010: 68 f5 fa dc 01 81 ff ff 10 47 d4 dd 01 81 ff ff
>> Next obj: start=ffff8101e28e3930, len=256
>> Redzone: 0x170fc2a5/0x170fc2a5.
>> Last user: [<ffffffff882784fa>](cifs_open+0x348/0x6d9 [cifs])
>> 000: 18 b8 2a e3 01 81 ff ff b8 a3 5a e0 01 81 ff ff
>> 010: 68 f5 36 65 02 81 ff ff 40 99 a1 dc 01 81 ff ff
>
> Can you resend this patch with it inlined into the email? I can't
> reasonably comment on it with it sent as a binary attachment.
>
> Thanks,
> --
> Jeff Layton <jlayton@redhat.com>
>
Jeff Layton - Aug. 18, 2009, 11:15 a.m.
On Mon, 17 Aug 2009 15:31:43 -0500
Shirish Pargaonkar <shirishpargaonkar@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@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.
Shirish Pargaonkar - Aug. 18, 2009, 3:23 p.m.
On Tue, Aug 18, 2009 at 6:15 AM, Jeff Layton<jlayton@redhat.com> wrote:
> On Mon, 17 Aug 2009 15:31:43 -0500
> Shirish Pargaonkar <shirishpargaonkar@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.

May be we could do handle wrtPending within lock like GlobalSMBSeslock
and perhaps get rid of msleep while waiting for wrtPending to become 0.
(add some code, loose some code)

>
>>               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@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.
>

I think this needs, some re-designing the code right?

> --
> Jeff Layton <jlayton@redhat.com>
>
Jeff Layton - Aug. 18, 2009, 4:43 p.m.
On Tue, 18 Aug 2009 10:23:09 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Tue, Aug 18, 2009 at 6:15 AM, Jeff Layton<jlayton@redhat.com> wrote:
> > On Mon, 17 Aug 2009 15:31:43 -0500
> > Shirish Pargaonkar <shirishpargaonkar@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.
> 
> May be we could do handle wrtPending within lock like GlobalSMBSeslock
> and perhaps get rid of msleep while waiting for wrtPending to become 0.
> (add some code, loose some code)
> 

My vote would be no. It's time to stop overloading existing locks like
this. The more I look at the open filehandle code, the more I'm
convinced that it's just a fundamentally poor design.

If you want to go that route, then I think you need to first step back
and document the locking rules for the open filehandle code. What data
structures does the GlobalSMBSesLock protect?

> >
> >>               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@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.
> >
> 
> I think this needs, some re-designing the code right?
> 

My vote would be "yes". Redesign and simplify the code rather than
adding in new hacks to work around the flaws in the existing design.

Redesigning it with actual refcounting (and not this half-assed
wrtPending stuff) seems like a much better approach.

Patch

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