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

The testing, so far looks good, the same tests that detected this slab
memory corruption
have run without any errors/corruption for more than 24 hours.

On Tue, Aug 25, 2009 at 4:23 PM, Dave Kleikamp<[email protected]> wrote:
> On Tue, 2009-08-18 at 12:43 -0400, Jeff Layton wrote:
>> On Tue, 18 Aug 2009 10:23:09 -0500
>> Shirish Pargaonkar <[email protected]> wrote:
>>
>> > On Tue, Aug 18, 2009 at 6:15 AM, Jeff Layton<[email protected]> wrote:
>
>> > > 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.
>
> How's this?  Untested so far:
>
> cifs: Replace wrtPending with a real reference count
>
> Currently, cifs_close() tries to wait until all I/O is complete and then
> frees the file private data.  If I/O does not completely in a reasonable
> amount of time it frees the structure anyway, leaving a potential use-
> after-free situation.
>
> This patch changes the wrtPending counter to a complete reference count and
> lets the last user free the structure.
>
> WARNING: compile tested only at this time
>
> Signed-off-by: Dave Kleikamp <[email protected]>
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 6941c22..7dfe084 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -607,7 +607,7 @@ static struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
>                return get_cifs_acl_by_path(cifs_sb, path, pacllen);
>
>        pntsd = get_cifs_acl_by_fid(cifs_sb, open_file->netfid, pacllen);
> -       atomic_dec(&open_file->wrtPending);
> +       cifsFileInfo_put(open_file);
>        return pntsd;
>  }
>
> @@ -665,7 +665,7 @@ static int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen,
>                return set_cifs_acl_by_path(cifs_sb, path, pnntsd, acllen);
>
>        rc = set_cifs_acl_by_fid(cifs_sb, open_file->netfid, pnntsd, acllen);
> -       atomic_dec(&open_file->wrtPending);
> +       cifsFileInfo_put(open_file);
>        return rc;
>  }
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 6084d63..e3b1161 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -351,11 +351,24 @@ struct cifsFileInfo {
>        bool closePend:1;       /* file is marked to close */
>        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 */
> +       atomic_t count;         /* reference count */
>        struct mutex fh_mutex; /* prevents reopen race after dead ses*/
>        struct cifs_search_info srch_inf;
>  };
>
> +/* Take a reference on the file private data */
> +static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
> +{
> +       atomic_inc(&cifs_file->count);
> +}
> +
> +/* Release a reference on the file private data */
> +static inline void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> +{
> +       if (atomic_dec_and_test(&cifs_file->count))
> +               kfree(cifs_file);
> +}
> +
>  /*
>  * One of these for each file inode
>  */
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 4326ffd..a6424cf 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -153,7 +153,7 @@ cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
>        mutex_init(&pCifsFile->fh_mutex);
>        mutex_init(&pCifsFile->lock_mutex);
>        INIT_LIST_HEAD(&pCifsFile->llist);
> -       atomic_set(&pCifsFile->wrtPending, 0);
> +       atomic_set(&pCifsFile->count, 1);
>
>        /* set the following in open now
>                        pCifsFile->pfile = file; */
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c34b7f8..fa7beac 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -53,11 +53,9 @@ static inline struct cifsFileInfo *cifs_init_private(
>        private_data->pInode = inode;
>        private_data->invalidHandle = false;
>        private_data->closePend = false;
> -       /* we have to track num writers to the inode, since writepages
> -       does not tell us which handle the write is for so there can
> -       be a close (overlapping with write) of the filehandle that
> -       cifs_writepages chose to use */
> -       atomic_set(&private_data->wrtPending, 0);
> +       /* Initialize reference count to one.  The private data is
> +       freed on the release of the last reference */
> +       atomic_set(&private_data->count, 1);
>
>        return private_data;
>  }
> @@ -643,7 +641,7 @@ int cifs_close(struct inode *inode, struct file *file)
>                        if (!pTcon->need_reconnect) {
>                                write_unlock(&GlobalSMBSeslock);
>                                timeout = 2;
> -                               while ((atomic_read(&pSMBFile->wrtPending) != 0)
> +                               while ((atomic_read(&pSMBFile->count) != 1)
>                                        && (timeout <= 2048)) {
>                                        /* Give write a better chance to get to
>                                        server ahead of the close.  We do not
> @@ -657,8 +655,6 @@ int cifs_close(struct inode *inode, struct file *file)
>                                        msleep(timeout);
>                                        timeout *= 4;
>                                }
> -                               if (atomic_read(&pSMBFile->wrtPending))
> -                                       cERROR(1, ("close with pending write"));
>                                if (!pTcon->need_reconnect &&
>                                    !pSMBFile->invalidHandle)
>                                        rc = CIFSSMBClose(xid, pTcon,
> @@ -681,24 +677,7 @@ int cifs_close(struct inode *inode, struct file *file)
>                list_del(&pSMBFile->flist);
>                list_del(&pSMBFile->tlist);
>                write_unlock(&GlobalSMBSeslock);
> -               timeout = 10;
> -               /* We waited above to give the SMBWrite a chance to issue
> -                  on the wire (so we do not get SMBWrite returning EBADF
> -                  if writepages is racing with close.  Note that writepages
> -                  does not specify a file handle, so it is possible for a file
> -                  to be opened twice, and the application close the "wrong"
> -                  file handle - in these cases we delay long enough to allow
> -                  the SMBWrite to get on the wire before the SMB Close.
> -                  We allow total wait here over 45 seconds, more than
> -                  oplock break time, and more than enough to allow any write
> -                  to complete on the server, or to time out on the client */
> -               while ((atomic_read(&pSMBFile->wrtPending) != 0)
> -                               && (timeout <= 50000)) {
> -                       cERROR(1, ("writes pending, delay free of handle"));
> -                       msleep(timeout);
> -                       timeout *= 8;
> -               }
> -               kfree(file->private_data);
> +               cifsFileInfo_put(file->private_data);
>                file->private_data = NULL;
>        } else
>                rc = -EBADF;
> @@ -1236,7 +1215,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode)
>                        if (!open_file->invalidHandle) {
>                                /* found a good file */
>                                /* lock it so it will not be closed on us */
> -                               atomic_inc(&open_file->wrtPending);
> +                               cifsFileInfo_get(open_file);
>                                read_unlock(&GlobalSMBSeslock);
>                                return open_file;
>                        } /* else might as well continue, and look for
> @@ -1276,7 +1255,7 @@ refind_writable:
>                if (open_file->pfile &&
>                    ((open_file->pfile->f_flags & O_RDWR) ||
>                     (open_file->pfile->f_flags & O_WRONLY))) {
> -                       atomic_inc(&open_file->wrtPending);
> +                       cifsFileInfo_get(open_file);
>
>                        if (!open_file->invalidHandle) {
>                                /* found a good writable file */
> @@ -1293,7 +1272,7 @@ refind_writable:
>                                else { /* start over in case this was deleted */
>                                       /* since the list could be modified */
>                                        read_lock(&GlobalSMBSeslock);
> -                                       atomic_dec(&open_file->wrtPending);
> +                                       cifsFileInfo_put(open_file);
>                                        goto refind_writable;
>                                }
>                        }
> @@ -1309,7 +1288,7 @@ refind_writable:
>                        read_lock(&GlobalSMBSeslock);
>                        /* can not use this handle, no write
>                           pending on this one after all */
> -                       atomic_dec(&open_file->wrtPending);
> +                       cifsFileInfo_put(open_file);
>
>                        if (open_file->closePend) /* list could have changed */
>                                goto refind_writable;
> @@ -1373,7 +1352,7 @@ 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);
> +               cifsFileInfo_put(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 +1541,7 @@ retry:
>                                                   bytes_to_write, offset,
>                                                   &bytes_written, iov, n_iov,
>                                                   long_op);
> -                               atomic_dec(&open_file->wrtPending);
> +                               cifsFileInfo_put(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..1f09c76 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -800,7 +800,7 @@ set_via_filehandle:
>        if (open_file == NULL)
>                CIFSSMBClose(xid, pTcon, netfid);
>        else
> -               atomic_dec(&open_file->wrtPending);
> +               cifsFileInfo_put(open_file);
>  out:
>        return rc;
>  }
> @@ -1635,7 +1635,7 @@ 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);
> +               cifsFileInfo_put(open_file);
>                cFYI(1, ("SetFSize for attrs rc = %d", rc));
>                if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
>                        unsigned int bytes_written;
> @@ -1790,7 +1790,7 @@ 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);
> +               cifsFileInfo_put(open_file);
>        } else {
>                rc = CIFSSMBUnixSetPathInfo(xid, pTcon, full_path, args,
>                                    cifs_sb->local_nls,
>
> --
> David Kleikamp
> IBM Linux Technology Center
>
>