From patchwork Mon Aug 17 20:31:43 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Shirish Pargaonkar X-Patchwork-Id: 31531 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.samba.org (fn.samba.org [216.83.154.106]) by bilbo.ozlabs.org (Postfix) with ESMTP id 79F89B707B for ; Tue, 18 Aug 2009 06:31:51 +1000 (EST) Received: from fn.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id E1D9D46680; Mon, 17 Aug 2009 14:25:27 -0600 (MDT) X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on fn.samba.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.8 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.2.5 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from mail-yw0-f193.google.com (mail-yw0-f193.google.com [209.85.211.193]) by lists.samba.org (Postfix) with ESMTP id A70ED4667E for ; Mon, 17 Aug 2009 14:25:21 -0600 (MDT) Received: by ywh31 with SMTP id 31so4099514ywh.4 for ; Mon, 17 Aug 2009 13:31:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=vjmTGyuD6yzUJ4eKtgsDwmdaS6c6tfAtRZMQ/YuwL4w=; b=Vg069tARIOpnKRBjRmDaHwkbPUr3jaCADj/A7IuNraGsRPbSVa/evl7LAFZTDEu1c6 79lUltG7ZGXbPrMw0ndG96fQre/E1c0Iuc58/K+UTH8HBg/mTaJdItRXPk91J8Z76IL/ 2t7sinps/sX3kW3mnFz+TBFp3WLN4aaiH1okI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=p9VxDcUm5suJHUbiYTEoyNG7t9Iok3khflIcXZgXjRxTsm15Q/GuSabX5oMQ7UhAap 4eyV0eXDtBqoH2mpK/6l2ZmqyqpjkiuMGyN/l1jaPCDrrM0mMl/Q7F9Bpkgb24jldDHx uDuDjIZhEYid3VL/7GE7zf/BHVRSr5N8q9AeY= MIME-Version: 1.0 Received: by 10.101.55.7 with SMTP id h7mr3959363ank.116.1250541103579; Mon, 17 Aug 2009 13:31:43 -0700 (PDT) In-Reply-To: <20090817155052.27c7a9b0@tlielax.poochiereds.net> References: <4a4634330908160938k4ea9a68ey496c7aed861c543b@mail.gmail.com> <20090817155052.27c7a9b0@tlielax.poochiereds.net> Date: Mon, 17 Aug 2009 15:31:43 -0500 Message-ID: <4a4634330908171331n28e7d9fcif14b606f935271f8@mail.gmail.com> From: Shirish Pargaonkar To: Jeff Layton Cc: linux-cifs-client@lists.samba.org Subject: Re: [linux-cifs-client] [patch] prevent slab corruption by fixing race codition in cifs X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces@lists.samba.org Errors-To: linux-cifs-client-bounces@lists.samba.org 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 On Mon, Aug 17, 2009 at 2:50 PM, Jeff Layton wrote: > On Sun, 16 Aug 2009 11:38:57 -0500 > Shirish Pargaonkar 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: [](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: [](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: [](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 > 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)) {