[linux-cifs-client] [patch] prevent slab corruption by fixing race codition in cifs
Shirish Pargaonkar
shirishpargaonkar at gmail.com
Tue Aug 18 09:23:09 MDT 2009
On Tue, Aug 18, 2009 at 6:15 AM, Jeff Layton<jlayton at redhat.com> wrote:
> 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.
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 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.
>
I think this needs, some re-designing the code right?
> --
> Jeff Layton <jlayton at redhat.com>
>
More information about the linux-cifs-client
mailing list