Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c2679cde30fc64bb1df1a0bfea49c1efdbb77ec4.camel@kernel.org>
Subject: Re: [PATCH v3 3/4] nfsd: close race between unhashing and LRU
 addition
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>, Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Mon, 31 Oct 2022 06:01:59 -0400
In-Reply-To: <166716630911.13915.14442550645061536898@noble.neil.brown.name>
References: <20221028185712.79863-1-jlayton@kernel.org>
        , <20221028185712.79863-4-jlayton@kernel.org>
        , <08778EE0-CBDC-467B-ACA6-9D8E6719E1BB@oracle.com>
         <166716630911.13915.14442550645061536898@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-10-31 at 08:45 +1100, NeilBrown wrote:
> On Sat, 29 Oct 2022, Chuck Lever III wrote:
> >=20
> > > On Oct 28, 2022, at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > >=20
> > > The list_lru_add and list_lru_del functions use list_empty checks to =
see
> > > whether the object is already on the LRU. That's fine in most cases, =
but
> > > we occasionally repurpose nf_lru after unhashing. It's possible for a=
n
> > > LRU removal to remove it from a different list altogether if we lose =
a
> > > race.
> >=20
> > I've never seen that happen. lru field re-use is actually used in other
> > places in the kernel. Shouldn't we try to find and fix such races?
> >=20
> > Wasn't the idea to reduce the complexity of nfsd_file_put ?
> >=20
>=20
> I think the nfsd filecache is different from those "other places"
> because of nfsd_file_close_inode() and related code.  If I understand
> correctly, nfsd can remove a file from the filecache while it is still
> in use.  In this case it needs to be unhashed and removed from the lru -
> and then added to a dispose list - while it might still be active for
> some IO request.
>=20
> Prior to Commit 8d0d254b15cc ("nfsd: fix nfsd_file_unhash_and_dispose")
> unhash_and_dispose() wouldn't add to the dispose list unless the
> refcount was one.  I'm not sure how racy this test was, but it would
> mean that it is unlikely for an nfsd_file to be added to the dispose list
> if it was still in use.
>=20

I'm pretty sure that was racy. By the time you go to put it on the
dispose list, the refcount may no longer be one since it can be
incremented at any time.

> After that commit it seems more likely that a nfsd_file will be added to
> a dispose list while it is in use.
> As we add/remove things to a dispose list without a lock, we need to be
> careful that no other thread will add the nfsd_file to an lru at the
> same time.  refcounts will no longer provide that protection.  Maybe we
> should restore the refcount protection, or maybe we need a bit as Jeff
> has added here.
>=20

If we have an entry on a list, then it can't be added to the LRU (since
the list_empty check would fail). The real danger is that we could end
up trying to remove it from the LRU and inadvertantly remove it from a
private dispose list instead. Since that is done without a lock, we
could easily corrupt memory.

--=20
Jeff Layton <jlayton@kernel.org>