From patchwork Thu Jun 26 19:12:16 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 4430481 Return-Path: X-Original-To: patchwork-linux-nfs@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 03F289F2C8 for ; Thu, 26 Jun 2014 19:14:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0D00F201BB for ; Thu, 26 Jun 2014 19:14:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 103A02024D for ; Thu, 26 Jun 2014 19:14:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751772AbaFZTOh (ORCPT ); Thu, 26 Jun 2014 15:14:37 -0400 Received: from mail-qc0-f175.google.com ([209.85.216.175]:39626 "EHLO mail-qc0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751764AbaFZTOg (ORCPT ); Thu, 26 Jun 2014 15:14:36 -0400 Received: by mail-qc0-f175.google.com with SMTP id i8so3592637qcq.34 for ; Thu, 26 Jun 2014 12:14:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=BotFmKgI+QaDkNqsACQ/AwaE04dlKf5ieZBCQt3wX/Y=; b=Q2X/WEJ+QECF1TP68sOyd9Cz0EySq6Z7JkGNcurB9cqouZSYsgeXPPxnI1piITZZy6 HGq3HlUGoTAJ413VQSPAdM7OS70AJ1Wwkpjqi8C11QYJ2xSN9ye5r3MnxJ42QHivA8es gkt5dhO/12FtU1IH3sWu9BfzYX8piuP6yE0H4mdcNg/io7mO2L/hYI7SeDzItDTJek4s oH+gpH8BgzFT7esWVgqAruW7mO0ja1ctgLbhM0ueQe+XCZRets2RVlXkpDaXrj7CQq0v eDEdrWqkgHaqvIuyGASkNT0Qqh0FCAg4sChkvj5pFP9eahgFCNRKc/ofXvhMpuCc4HlD GPgg== X-Gm-Message-State: ALoCoQn4QftDQpnYmPYAn4RlkZCmvP3LhuWT1/OaWecYdb+lSVNu0z45pgs03pIC0HXP4O5PuBCO X-Received: by 10.140.48.161 with SMTP id o30mr24574824qga.68.1403810075404; Thu, 26 Jun 2014 12:14:35 -0700 (PDT) Received: from tlielax.poochiereds.net ([2001:470:8:d63:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id 88sm4763039qgh.5.2014.06.26.12.14.33 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 26 Jun 2014 12:14:34 -0700 (PDT) From: Jeff Layton To: bfields@fieldses.org Cc: linux-nfs@vger.kernel.org Subject: [PATCH v2 036/117] nfsd: Protect the nfs4_file delegation fields using the fi_lock Date: Thu, 26 Jun 2014 15:12:16 -0400 Message-Id: <1403810017-16062-37-git-send-email-jlayton@primarydata.com> X-Mailer: git-send-email 1.9.3 In-Reply-To: <1403810017-16062-1-git-send-email-jlayton@primarydata.com> References: <1403810017-16062-1-git-send-email-jlayton@primarydata.com> Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_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 Move more of the delegation fields to be protected by the fi_lock. It's more granular than the state_lock and in later patches we'll want to be able to rely on it in addition to the state_lock. Also, the current code in nfs4_setlease calls vfs_setlease and uses the client_mutex to ensure that it doesn't disappear before we can hash the delegation. With the client_mutex gone, we'll have a potential race condition. It's possible that the delegation could be recalled after we acquire the lease but before we ever get around to hashing it. If that happens, then we'd have a nfs4_file that *thinks* it has a delegation, when it actually has none. Attempt to acquire a delegation. If that succeeds, take the state_lock and recheck to make sure the lease is still there. If it is, then take the fi_lock and set up the rest of the delegation fields. This prevents the race because the delegation break workqueue job must also take the state_lock. Signed-off-by: Trond Myklebust Signed-off-by: Jeff Layton --- fs/nfsd/nfs4state.c | 51 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index e75cb6da1e64..01970b3cf593 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -658,6 +658,8 @@ nfs4_put_delegation(struct nfs4_delegation *dp) static void nfs4_put_deleg_lease(struct nfs4_file *fp) { + lockdep_assert_held(&state_lock); + if (!fp->fi_lease) return; if (atomic_dec_and_test(&fp->fi_delegees)) { @@ -677,11 +679,10 @@ static void hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) { lockdep_assert_held(&state_lock); + lockdep_assert_held(&fp->fi_lock); dp->dl_stid.sc_type = NFS4_DELEG_STID; - spin_lock(&fp->fi_lock); list_add(&dp->dl_perfile, &fp->fi_delegations); - spin_unlock(&fp->fi_lock); list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations); } @@ -692,17 +693,17 @@ unhash_delegation(struct nfs4_delegation *dp) struct nfs4_file *fp = dp->dl_file; spin_lock(&state_lock); + spin_lock(&fp->fi_lock); list_del_init(&dp->dl_perclnt); list_del_init(&dp->dl_recall_lru); - spin_lock(&fp->fi_lock); list_del_init(&dp->dl_perfile); spin_unlock(&fp->fi_lock); - spin_unlock(&state_lock); if (fp) { nfs4_put_deleg_lease(fp); - put_nfs4_file(fp); dp->dl_file = NULL; } + spin_unlock(&state_lock); + put_nfs4_file(fp); } @@ -3440,7 +3441,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp) { struct nfs4_file *fp = dp->dl_file; struct file_lock *fl; - int status; + int status = 0; fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); if (!fl) @@ -3448,15 +3449,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp) fl->fl_file = find_readable_file(fp); status = vfs_setlease(fl->fl_file, fl->fl_type, &fl); if (status) - goto out_free; + goto out_fput; + spin_lock(&state_lock); + /* Did the lease get broken before we took the lock? */ + status = -EAGAIN; + if (!file_has_lease(fl->fl_file)) + goto out_unlock; + spin_lock(&fp->fi_lock); + /* Race breaker */ + if (fp->fi_lease) { + status = 0; + atomic_inc(&fp->fi_delegees); + hash_delegation_locked(dp, fp); + spin_unlock(&fp->fi_lock); + goto out_unlock; + } fp->fi_lease = fl; fp->fi_deleg_file = fl->fl_file; atomic_set(&fp->fi_delegees, 1); - spin_lock(&state_lock); hash_delegation_locked(dp, fp); + spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); return 0; -out_free: +out_unlock: + spin_unlock(&state_lock); +out_fput: if (fl->fl_file) fput(fl->fl_file); locks_free_lock(fl); @@ -3465,19 +3482,27 @@ out_free: static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp) { + int status = 0; + if (fp->fi_had_conflict) return -EAGAIN; get_nfs4_file(fp); + spin_lock(&state_lock); + spin_lock(&fp->fi_lock); dp->dl_file = fp; - if (!fp->fi_lease) + if (!fp->fi_lease) { + spin_unlock(&fp->fi_lock); + spin_unlock(&state_lock); return nfs4_setlease(dp); - spin_lock(&state_lock); + } atomic_inc(&fp->fi_delegees); if (fp->fi_had_conflict) { - spin_unlock(&state_lock); - return -EAGAIN; + status = -EAGAIN; + goto out_unlock; } hash_delegation_locked(dp, fp); +out_unlock: + spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); return 0; }