Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
From:   Benjamin Coddington <bcodding@redhat.com>
To:     trondmy@kernel.org
Cc:     Anna Schumaker <Anna.Schumaker@netapp.com>,
        linux-nfs@vger.kernel.org, Scott Mayhew <smayhew@redhat.com>
Subject: Re: [PATCH] pNFS: Fix a hang in nfs4_evict_inode()
Date:   Wed, 01 Nov 2023 09:20:45 -0400
Message-ID: <92B7CF2E-26C4-412A-AAED-B06F6B9B4383@redhat.com>
In-Reply-To: <20231008182019.12842-1-trondmy@kernel.org>
References: <20231008182019.12842-1-trondmy@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Trond,

On 8 Oct 2023, at 14:20, trondmy@kernel.org wrote:

> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> We are not allowed to call pnfs_mark_matching_lsegs_return() without
> also holding a reference to the layout header, since doing so could lea=
d
> to the reference count going to zero when we call
> pnfs_layout_remove_lseg(). This again can lead to a hang when we get to=

> nfs4_evict_inode() and are unable to clear the layout pointer.
>
> pnfs_layout_return_unused_byserver() is guilty of this behaviour, and
> has been seen to trigger the refcount warning prior to a hang.
>
> Fixes: b6d49ecd1081 ("NFSv4: Fix a pNFS layout related use-after-free r=
ace when freeing the inode")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/pnfs.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 63904a372b2f..21a365357629 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -2638,31 +2638,44 @@ pnfs_should_return_unused_layout(struct pnfs_la=
yout_hdr *lo,
>  	return mode =3D=3D 0;
>  }
>
> -static int
> -pnfs_layout_return_unused_byserver(struct nfs_server *server, void *da=
ta)
> +static int pnfs_layout_return_unused_byserver(struct nfs_server *serve=
r,
> +					      void *data)
>  {
>  	const struct pnfs_layout_range *range =3D data;
> +	const struct cred *cred;
>  	struct pnfs_layout_hdr *lo;
>  	struct inode *inode;
> +	nfs4_stateid stateid;
> +	enum pnfs_iomode iomode;
> +
>  restart:
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(lo, &server->layouts, plh_layouts) {
> -		if (!pnfs_layout_can_be_returned(lo) ||
> +		inode =3D lo->plh_inode;
> +		if (!inode || !pnfs_layout_can_be_returned(lo) ||
>  		    test_bit(NFS_LAYOUT_RETURN_REQUESTED, &lo->plh_flags))
>  			continue;
> -		inode =3D lo->plh_inode;
>  		spin_lock(&inode->i_lock);
> -		if (!pnfs_should_return_unused_layout(lo, range)) {
> +		if (!lo->plh_inode ||
> +		    !pnfs_should_return_unused_layout(lo, range)) {
>  			spin_unlock(&inode->i_lock);
>  			continue;
>  		}
> +		pnfs_get_layout_hdr(lo);

We're getting a crash with the nfs_inode.layout =3D=3D NULL in writeback.=


We haven't bisected to this yet, but I think this change is exposing the
case where the pnfs_layout_hdr refcount goes to zero, but we can still fi=
nd
it here on server->layouts, and bump the refcount incorrectly.

Plausible?  We can send a fix or test one..

Ben