From patchwork Mon Aug 8 13:19:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 9268361 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 60B17607D6 for ; Mon, 8 Aug 2016 13:19:32 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 52A49283ED for ; Mon, 8 Aug 2016 13:19:32 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 47655283EF; Mon, 8 Aug 2016 13:19:32 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A388D283ED for ; Mon, 8 Aug 2016 13:19:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752513AbcHHNTV (ORCPT ); Mon, 8 Aug 2016 09:19:21 -0400 Received: from mail-qk0-f179.google.com ([209.85.220.179]:36415 "EHLO mail-qk0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752077AbcHHNTT (ORCPT ); Mon, 8 Aug 2016 09:19:19 -0400 Received: by mail-qk0-f179.google.com with SMTP id v123so188300073qkh.3 for ; Mon, 08 Aug 2016 06:19:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=z38dEXvh8Tf2Z1oGH4kTXKZooHA4TinilPKhjcWr9S4=; b=TgvUW9ea4KHRaIBGrq3n7gnaOUi/ACnur+gttowJy+cEa9xQhBbJQVcka+wT5JjgtH 97ntpZH3kYfwEoO3zu/zHz5GjOwE9X8RNijGpcWAFp1sF0B6KLRVpkFC9jSTGzMV2uPa p/pdeSkqAHxKxVZm+ESc3An7sgAYI6C+sM6+jlowK2BeDjTjii1t57MocdCvYgoykTSO hAj8GVMZWSFV/6GdmY/04du8KkfooN/2XKay4rtSZU6JcSqT2E26TsRBWaoHAN9vkj83 77A8x9qR1hVC1isqY5yodD0Gwu9dF0tSH3rQIrhHbbFqPDzbpBO8icllhRfMA8MabT0x jEog== X-Gm-Message-State: AEkooutyHoGNXrM5bIofGXloL9lXykBL8Kj9ZqGJw1b6HK8mlDR2BwBSfbu32Mo61a+l4A77 X-Received: by 10.55.22.29 with SMTP id g29mr27962943qkh.267.1470662358001; Mon, 08 Aug 2016 06:19:18 -0700 (PDT) Received: from cpe-2606-A000-1125-405B-1A5E-FFF-FE12-8671.dyn6.twc.com (cpe-2606-A000-1125-405B-1A5E-FFF-FE12-8671.dyn6.twc.com. [2606:a000:1125:405b:1a5e:fff:fe12:8671]) by smtp.gmail.com with ESMTPSA id r5sm17466254qkf.34.2016.08.08.06.19.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 08 Aug 2016 06:19:17 -0700 (PDT) Message-ID: <1470662355.844.10.camel@redhat.com> Subject: Re: [PATCH v2] nfsd: Fix race between FREE_STATEID and LOCK From: Jeff Layton To: Chuck Lever , linux-nfs@vger.kernel.org Date: Mon, 08 Aug 2016 09:19:15 -0400 In-Reply-To: <1470608556.2975.8.camel@redhat.com> References: <20160807185024.11705.10864.stgit@klimt.1015granger.net> <1470608556.2975.8.camel@redhat.com> X-Mailer: Evolution 3.20.4 (3.20.4-1.fc24) Mime-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sun, 2016-08-07 at 18:22 -0400, Jeff Layton wrote: > On Sun, 2016-08-07 at 14:53 -0400, Chuck Lever wrote: > > > > When running LTP's nfslock01 test, the Linux client can send a LOCK > > and a FREE_STATEID request at the same time. The LOCK uses the same > > lockowner as the stateid sent in the FREE_STATEID request. > > > > The outcome is: > > > > Frame 115025 C FREE_STATEID stateid 2/A > > Frame 115026 C LOCK offset 672128 len 64 > > Frame 115029 R FREE_STATEID NFS4_OK > > Frame 115030 R LOCK stateid 3/A Oh, to be clear here -- I assume this a lk_is_new lock (with an open stateid in it). Right? > > Frame 115034 C WRITE stateid 0/A offset 672128 len 64 > > Frame 115038 R WRITE NFS4ERR_BAD_STATEID > > > > In other words, the server returns stateid A in a successful LOCK > > reply, but it has already released it. Subsequent uses of the > > stateid fail. > > > > To address this, protect the generation check in nfsd4_free_stateid > > with the st_mutex. This should guarantee that only one of two > > outcomes occurs: either LOCK returns a fresh valid stateid, or > > FREE_STATEID returns NFS4ERR_LOCKS_HELD. > > > > Reported-by: Alexey Kodanev > > Fix-suggested-by: Jeff Layton > > Signed-off-by: Chuck Lever > > --- > >  fs/nfsd/nfs4state.c |   19 ++++++++++++------- > >  1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index b921123..07dc1aa 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -4911,19 +4911,20 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, > > struct nfsd4_compound_state *cstate, > >   ret = nfserr_locks_held; > >   break; > >   case NFS4_LOCK_STID: > > + atomic_inc(&s->sc_count); > > + spin_unlock(&cl->cl_lock); > > + stp = openlockstateid(s); > > + mutex_lock(&stp->st_mutex); > >   ret = check_stateid_generation(stateid, &s- > > > > > > sc_stateid, 1); > >   if (ret) > > - break; > > - stp = openlockstateid(s); > > + goto out_mutex_unlock; > >   ret = nfserr_locks_held; > >   if (check_for_locks(stp->st_stid.sc_file, > >       lockowner(stp- > > >st_stateowner))) > > - break; > > - WARN_ON(!unhash_lock_stateid(stp)); > > - spin_unlock(&cl->cl_lock); > > - nfs4_put_stid(s); > > + goto out_mutex_unlock; > > + release_lock_stateid(stp); > >   ret = nfs_ok; > > - goto out; > > + goto out_mutex_unlock; > >   case NFS4_REVOKED_DELEG_STID: > >   dp = delegstateid(s); > >   list_del_init(&dp->dl_recall_lru); > > @@ -4937,6 +4938,10 @@ out_unlock: > >   spin_unlock(&cl->cl_lock); > >  out: > >   return ret; > > +out_mutex_unlock: > > + mutex_unlock(&stp->st_mutex); > > + nfs4_put_stid(s); > > + goto out; > >  } > >   > >  static inline int > > > >   > > Looks good to me. > > Reviewed-by: Jeff Layton Hmm...I think this is not a complete fix though. We also need something like this patch: --------------[snip]--------------- [PATCH] nfsd: don't return an already-unhashed lock stateid after taking mutex nfsd4_lock will take the st_mutex before working with the stateid it gets, but between the time when we drop the cl_lock and take the mutex, the stateid could become unhashed (a'la FREE_STATEID). If that happens the lock stateid returned to the client will be forgotten. Fix this by first moving the st_mutex acquisition into lookup_or_create_lock_state. Then, have it check to see if the lock stateid is still hashed after taking the mutex. If it's not, then put the stateid and try the find/create again. Signed-off-by: Jeff Layton --- fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 5d6a28af0f42..1235b1661703 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -5653,7 +5653,7 @@ static __be32 lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, struct nfs4_ol_stateid *ost, struct nfsd4_lock *lock, - struct nfs4_ol_stateid **lst, bool *new) + struct nfs4_ol_stateid **plst, bool *new) { __be32 status; struct nfs4_file *fi = ost->st_stid.sc_file; @@ -5661,7 +5661,9 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, struct nfs4_client *cl = oo->oo_owner.so_client; struct inode *inode = d_inode(cstate->current_fh.fh_dentry); struct nfs4_lockowner *lo; + struct nfs4_ol_stateid *lst; unsigned int strhashval; + bool hashed; lo = find_lockowner_str(cl, &lock->lk_new_owner); if (!lo) { @@ -5677,12 +5679,27 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, goto out; } - *lst = find_or_create_lock_stateid(lo, fi, inode, ost, new); - if (*lst == NULL) { +retry: + lst = find_or_create_lock_stateid(lo, fi, inode, ost, new); + if (lst == NULL) { status = nfserr_jukebox; goto out; } + + mutex_lock(&lst->st_mutex); + + /* See if it's still hashed to avoid race with FREE_STATEID */ + spin_lock(&cl->cl_lock); + hashed = list_empty(&lst->st_perfile); + spin_unlock(&cl->cl_lock); + + if (!hashed) { + mutex_unlock(&lst->st_mutex); + nfs4_put_stid(&lst->st_stid); + goto retry; + } status = nfs_ok; + *plst = lst; out: nfs4_put_stateowner(&lo->lo_owner); return status; @@ -5752,8 +5769,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, goto out; status = lookup_or_create_lock_state(cstate, open_stp, lock, &lock_stp, &new); - if (status == nfs_ok) - mutex_lock(&lock_stp->st_mutex); } else { status = nfs4_preprocess_seqid_op(cstate, lock->lk_old_lock_seqid,