Re: [RFC-PATCH] nfsd: when unhashing openowners, increment openowner's refcount

From: Alex Lyakas <alex@zadara.com>
To: bfields@fieldses.org, chuck.lever@oracle.com
Cc: linux-nfs@vger.kernel.org, Shyam Kaushik <shyam@zadara.com>
Subject: Re: [RFC-PATCH] nfsd: when unhashing openowners, increment openowner's refcount
Date: Sun, 25 Aug 2019 13:12:34 +0300	[thread overview]
Message-ID: <CAOcd+r0bXefi79dnwrwsDN1OecScfTjc8DYS5_9A8D5XKrh7QQ@mail.gmail.com> (raw)
In-Reply-To: <1566406146-7887-1-git-send-email-alex@zadara.com>

Hi Bruce, Chuck,

You are listed as maintainers of nfsd. Can you please take a look at
the below patch?

Thanks,
Alex.

On Wed, Aug 21, 2019 at 7:50 PM Alex Lyakas <alex@zadara.com> wrote:
>
> release_openowner() expects an extra refcnt taken for the openowner,
> which it is releasing.
>
>  With nfsd_inject_forget_client_openowners() and nfsd_inject_forget_openowners(),
> we unhash openowners and collect them into a reaplist. Later we call
> nfsd_reap_openowners(), which calls release_openowner(), which releases all openowner's stateids.
> Each OPEN stateid holds a refcnt on the openowner. Therefore, after releasing
> the last OPEN stateid via its sc_free function, which is nfs4_free_ol_stateid,
> nfs4_put_stateowner() will be called, which will realize its the last
> refcnt for the openowner. As a result, openowner will be freed.
> But later, release_openowner() will go ahead and call release_last_closed_stateid()
> and nfs4_put_stateowner() on the same openowner which was just released.
> This corrupts memory and causes random crashes.
>
> After we fixed this, we confirmed that the openowner is not freed
> prematurely. It is freed by release_openowner() final call
> to nfs4_put_stateowner().
>
> However, we still get (other) random crashes and memory corruptions
> when nfsd_inject_forget_client_openowners() and
> nfsd_inject_forget_openowners().
> According to our analysis, we don't see any other refcount issues.
> Can anybody from the community review these flows for other potentials issues?
>
> Signed-off-by: Alex Lyakas <alex@zadara.com>
> ---
>  fs/nfsd/nfs4state.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7857942..4e9afca 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7251,6 +7251,7 @@ static u64 nfsd_foreach_client_lock(struct nfs4_client *clp, u64 max,
>                         func(oop);
>                         if (collect) {
>                                 atomic_inc(&clp->cl_rpc_users);
> +                               nfs4_get_stateowner(&oop->oo_owner);
>                                 list_add(&oop->oo_perclient, collect);
>                         }
>                 }
> --
> 1.9.1
>