From patchwork Thu Jul 17 19:12:29 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 4578331 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6E5F1C0514 for ; Thu, 17 Jul 2014 19:12:50 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 68FC92017D for ; Thu, 17 Jul 2014 19:12:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6A78D201BA for ; Thu, 17 Jul 2014 19:12:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755085AbaGQTMp (ORCPT ); Thu, 17 Jul 2014 15:12:45 -0400 Received: from mail-qa0-f51.google.com ([209.85.216.51]:51876 "EHLO mail-qa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755057AbaGQTMn (ORCPT ); Thu, 17 Jul 2014 15:12:43 -0400 Received: by mail-qa0-f51.google.com with SMTP id k15so2187326qaq.38 for ; Thu, 17 Jul 2014 12:12:43 -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=DjFhRyP63LkdEdKv+X8r4UYHbmvcVpDF14jA15f0xWg=; b=ADUwyYM2T1KtelXGKHmnA6DKwRsfVQk+x9OAkGWD+T3c1lYJCXQXyXsbPbWwhlOjE0 fcxnQRHQyahd5zz0jqDqLmOGuu/TIt/6wbxGdTizmhAlF2WwYNIwSR6kOtbl4np26hmg IRW6nXy3JEnZE3dwnUlBCEYW+bBIKt3ynFudzFb2oTEdRh+6dBHAmkk183Q/SxZ2YLpO E4SCclUM7QlOON6fTSFafoQdY0EhdRNeBHiUj7dkfHCMqOotJo5vVgsHKvyfM5IY5Pjw aRIVJ2ujjRLolIZksHaiOaPabOVYpfJP9BPjkHPLqgXQBaIT7KhehQQvsqmjFG5x4bqj DfAA== X-Gm-Message-State: ALoCoQnEiE8wTzQiw5KUhKdurCc8zKx/8ZcrKW3RORbpfxKqyUpMep++0bqXZPXAM8tb4nBRpSQB X-Received: by 10.224.0.79 with SMTP id 15mr58382915qaa.56.1405624363103; Thu, 17 Jul 2014 12:12:43 -0700 (PDT) Received: from tlielax.poochiereds.net ([2001:470:8:d63:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id 98sm3678250qgh.5.2014.07.17.12.12.41 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 17 Jul 2014 12:12:42 -0700 (PDT) From: Jeff Layton To: bfields@fieldses.org Cc: hch@infradead.org, linux-nfs@vger.kernel.org Subject: [PATCH 1/6] nfsd: Protect the nfs4_file delegation fields using the fi_lock Date: Thu, 17 Jul 2014 15:12:29 -0400 Message-Id: <1405624354-22388-2-git-send-email-jlayton@primarydata.com> X-Mailer: git-send-email 1.9.3 In-Reply-To: <1405624354-22388-1-git-send-email-jlayton@primarydata.com> References: <1405624354-22388-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, 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 spinlocks and then check to see if the file has had a conflict show up since then. If it has, then we assume that the lease is no longer valid and that we shouldn't hand out a delegation. Signed-off-by: Trond Myklebust Signed-off-by: Jeff Layton --- fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index fd4deb049ddf..9bc896720db3 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -624,6 +624,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)) { @@ -643,11 +645,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); } @@ -659,17 +660,18 @@ unhash_delegation(struct nfs4_delegation *dp) spin_lock(&state_lock); dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID; + 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); + if (fp) + put_nfs4_file(fp); } static void destroy_revoked_delegation(struct nfs4_delegation *dp) @@ -3143,8 +3145,8 @@ static void nfsd_break_deleg_cb(struct file_lock *fl) */ fl->fl_break_time = 0; - fp->fi_had_conflict = true; spin_lock(&fp->fi_lock); + fp->fi_had_conflict = true; list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) nfsd_break_one_deleg(dp); spin_unlock(&fp->fi_lock); @@ -3493,7 +3495,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(fp, NFS4_OPEN_DELEGATE_READ); if (!fl) @@ -3501,15 +3503,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); + spin_lock(&fp->fi_lock); + /* Did the lease get broken before we took the lock? */ + status = -EAGAIN; + if (fp->fi_had_conflict) + goto out_unlock; + /* Race breaker */ + if (fp->fi_lease) { + status = 0; + atomic_inc(&fp->fi_delegees); + hash_delegation_locked(dp, fp); + 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(&fp->fi_lock); + spin_unlock(&state_lock); +out_fput: if (fl->fl_file) fput(fl->fl_file); locks_free_lock(fl); @@ -3518,19 +3536,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; }