From patchwork Wed Apr 30 13:31:47 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 4093911 Return-Path: X-Original-To: patchwork-cifs-client@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 906689F38E for ; Wed, 30 Apr 2014 13:32:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8FF9B202AE for ; Wed, 30 Apr 2014 13:32:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8378720148 for ; Wed, 30 Apr 2014 13:32:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933549AbaD3NcB (ORCPT ); Wed, 30 Apr 2014 09:32:01 -0400 Received: from mail-qa0-f46.google.com ([209.85.216.46]:50591 "EHLO mail-qa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933706AbaD3NcA (ORCPT ); Wed, 30 Apr 2014 09:32:00 -0400 Received: by mail-qa0-f46.google.com with SMTP id w8so1608365qac.19 for ; Wed, 30 Apr 2014 06:31:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=Ul7DEgLfI7qZt6yMDclCPq92blHc5Pq6r88EBltX3Rw=; b=S7M5cW6wBANcvPTMRZwOt2BwvI4o1t1l/fu45kCNGDiL2Vcij/xPEvHum17gkCrafd opbs1+4JjZUytUWc1FoMiFFhYGfmb4u6o/xRxyPUoPuVCx+xDL1If0Z3aVkjj6vwwlON 4yw6/Gj3a6K+Lq3Aq+wFGqkERJxujPhGuXX1kYW1gl+TFAExR1jfQcKtgbmH2ckXJG2I lw+YcJ/s56oguSg5Tsqt67KTy9WyEUOG6ueUjcXTap8BBv/ZOq1u2vtHPayOL2HjiCwm obAMUExX6FflyIAKVz6V10OVvostv8+rzhADdhijAd7gqTG5rBV9QJjsfrOm8IMULzo5 zXtw== X-Gm-Message-State: ALoCoQkmka/FvB39CIK+wQ95oMvzGBF1M4rbsKSnrCtvWMHOLlzk/kDN8G6iZCZWV1+CREOLs6IU X-Received: by 10.224.165.139 with SMTP id i11mr5252101qay.94.1398864719623; Wed, 30 Apr 2014 06:31:59 -0700 (PDT) Received: from tlielax.poochiereds.net ([2001:470:8:d63:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id n105sm30733850qgd.7.2014.04.30.06.31.58 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 30 Apr 2014 06:31:58 -0700 (PDT) From: Jeff Layton To: smfrench@gmail.com Cc: linux-cifs@vger.kernel.org, Tetsuo Handa Subject: [PATCH v2 3/4] cifs: fix potential races in cifs_revalidate_mapping Date: Wed, 30 Apr 2014 09:31:47 -0400 Message-Id: <1398864708-18185-4-git-send-email-jlayton@poochiereds.net> X-Mailer: git-send-email 1.9.0 In-Reply-To: <1398864708-18185-1-git-send-email-jlayton@poochiereds.net> References: <1398864708-18185-1-git-send-email-jlayton@poochiereds.net> Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The handling of the CIFS_INO_INVALID_MAPPING flag is racy. It's possible for two tasks to attempt to revalidate the mapping at the same time. The first sees that CIFS_INO_INVALID_MAPPING is set. It clears the flag and then calls invalidate_inode_pages2 to start shooting down the pagecache. While that's going on, another task checks the flag and sees that it's clear. It then ends up trusting the pagecache to satisfy a read when it shouldn't. Fix this by adding a bitlock to ensure that the clearing of the flag is atomic with respect to the actual cache invalidation. Also, move the other existing users of cifs_invalidate_mapping to use a new cifs_zap_mapping() function that just sets the INVALID_MAPPING bit and then uses the standard codepath to handle the invalidation. Signed-off-by: Jeff Layton --- fs/cifs/cifsfs.h | 1 + fs/cifs/cifsglob.h | 1 + fs/cifs/file.c | 12 ++++++------ fs/cifs/inode.c | 50 +++++++++++++++++++++++++++++++++++++++++--------- 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h index 40d5a4df1e38..b7cf9ee7654b 100644 --- a/fs/cifs/cifsfs.h +++ b/fs/cifs/cifsfs.h @@ -68,6 +68,7 @@ extern int cifs_revalidate_file(struct file *filp); extern int cifs_revalidate_dentry(struct dentry *); extern int cifs_invalidate_mapping(struct inode *inode); extern int cifs_revalidate_mapping(struct inode *inode); +extern int cifs_zap_mapping(struct inode *inode); extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *); extern int cifs_setattr(struct dentry *, struct iattr *); diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 69da55b750e7..630e0f4b2c66 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1118,6 +1118,7 @@ struct cifsInodeInfo { #define CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2 (2) /* Downgrade oplock to L2 */ #define CIFS_INO_DELETE_PENDING (3) /* delete pending on server */ #define CIFS_INO_INVALID_MAPPING (4) /* pagecache is invalid */ +#define CIFS_INO_LOCK (5) /* lock bit for synchronization */ unsigned long flags; spinlock_t writers_lock; unsigned int writers; /* Number of writers on this inode */ diff --git a/fs/cifs/file.c b/fs/cifs/file.c index c8a570e87900..208f56eca4bf 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -335,7 +335,7 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, spin_unlock(&cifs_file_list_lock); if (fid->purge_cache) - cifs_invalidate_mapping(inode); + cifs_zap_mapping(inode); file->private_data = cfile; return cfile; @@ -1529,7 +1529,7 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u32 type, */ if (!CIFS_CACHE_WRITE(CIFS_I(inode)) && CIFS_CACHE_READ(CIFS_I(inode))) { - cifs_invalidate_mapping(inode); + cifs_zap_mapping(inode); cifs_dbg(FYI, "Set no oplock for inode=%p due to mand locks\n", inode); CIFS_I(inode)->oplock = 0; @@ -2218,7 +2218,7 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end, file->f_path.dentry->d_name.name, datasync); if (!CIFS_CACHE_READ(CIFS_I(inode))) { - rc = cifs_invalidate_mapping(inode); + rc = cifs_zap_mapping(inode); if (rc) { cifs_dbg(FYI, "rc: %d during invalidate phase\n", rc); rc = 0; /* don't care about it in fsync */ @@ -2649,7 +2649,7 @@ cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov, * request comes - break it on the client to prevent reading * an old data. */ - cifs_invalidate_mapping(inode); + cifs_zap_mapping(inode); cifs_dbg(FYI, "Set no oplock for inode=%p after a write operation\n", inode); cinode->oplock = 0; @@ -3112,7 +3112,7 @@ int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma) xid = get_xid(); if (!CIFS_CACHE_READ(CIFS_I(inode))) { - rc = cifs_invalidate_mapping(inode); + rc = cifs_zap_mapping(inode); if (rc) return rc; } @@ -3670,7 +3670,7 @@ void cifs_oplock_break(struct work_struct *work) if (!CIFS_CACHE_READ(cinode)) { rc = filemap_fdatawait(inode->i_mapping); mapping_set_error(inode->i_mapping, rc); - cifs_invalidate_mapping(inode); + cifs_zap_mapping(inode); } cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc); } diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index ff420b275777..9ff8df8b4d84 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "cifsfs.h" #include "cifspdu.h" @@ -1762,29 +1763,60 @@ int cifs_invalidate_mapping(struct inode *inode) { int rc = 0; - struct cifsInodeInfo *cifs_i = CIFS_I(inode); - - clear_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags); if (inode->i_mapping && inode->i_mapping->nrpages != 0) { rc = invalidate_inode_pages2(inode->i_mapping); - if (rc) { + if (rc) cifs_dbg(VFS, "%s: could not invalidate inode %p\n", __func__, inode); - set_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags); - } } cifs_fscache_reset_inode_cookie(inode); return rc; } +/** + * cifs_wait_bit_killable - helper for functions that are sleeping on bit locks + * @word: long word containing the bit lock + */ +static int +cifs_wait_bit_killable(void *word) +{ + if (fatal_signal_pending(current)) + return -ERESTARTSYS; + freezable_schedule_unsafe(); + return 0; +} + int cifs_revalidate_mapping(struct inode *inode) { - if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags)) - return cifs_invalidate_mapping(inode); - return 0; + int rc; + unsigned long *flags = &CIFS_I(inode)->flags; + + rc = wait_on_bit_lock(flags, CIFS_INO_LOCK, cifs_wait_bit_killable, + TASK_KILLABLE); + if (rc) + return rc; + + if (test_and_clear_bit(CIFS_INO_INVALID_MAPPING, flags)) { + rc = cifs_invalidate_mapping(inode); + if (rc) + set_bit(CIFS_INO_INVALID_MAPPING, flags); + } + + clear_bit_unlock(CIFS_INO_LOCK, flags); + smp_mb__after_clear_bit(); + wake_up_bit(flags, CIFS_INO_LOCK); + + return rc; +} + +int +cifs_zap_mapping(struct inode *inode) +{ + set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags); + return cifs_revalidate_mapping(inode); } int cifs_revalidate_file_attr(struct file *filp)