Re: [patch] prevent slab corruption by fixing race codition in cifs

> On Mon, 17 Aug 2009 15:31:43 -0500
> Shirish Pargaonkar <shirishpargaonkar@...> 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@...>
>>
>
> 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.
>