nfs: fix memory leak in error path of nfs4_do_reclaim

[PATCH] nfs: fix memory leak in error path of nfs4_do_reclaim

Posted by Li Lingfeng 1 year, 3 months ago

Commit c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in
nfs4_do_reclaim()") separate out the freeing of the state owners from
nfs4_purge_state_owners() and finish it outside the rcu lock.
However, the error path is omitted. As a result, the state owners in
"freeme" will not be released.
Fix it by adding freeing in the error path.

Fixes: c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in nfs4_do_reclaim()")
Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
 fs/nfs/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 877f682b45f2..30aba1dedaba 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1957,6 +1957,7 @@ static int nfs4_do_reclaim(struct nfs_client *clp, const struct nfs4_state_recov
 				set_bit(ops->owner_flag_bit, &sp->so_flags);
 				nfs4_put_state_owner(sp);
 				status = nfs4_recovery_handle_error(clp, status);
+				nfs4_free_state_owners(&freeme);
 				return (status != 0) ? status : -EAGAIN;
 			}
 
-- 
2.31.1

Re: [PATCH] nfs: fix memory leak in error path of nfs4_do_reclaim

Posted by Trond Myklebust 1 year, 3 months ago

On Wed, 2024-09-04 at 20:34 +0800, Li Lingfeng wrote:
> Commit c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in
> nfs4_do_reclaim()") separate out the freeing of the state owners from
> nfs4_purge_state_owners() and finish it outside the rcu lock.
> However, the error path is omitted. As a result, the state owners in
> "freeme" will not be released.
> Fix it by adding freeing in the error path.
> 
> Fixes: c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in
> nfs4_do_reclaim()")
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
>  fs/nfs/nfs4state.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 877f682b45f2..30aba1dedaba 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1957,6 +1957,7 @@ static int nfs4_do_reclaim(struct nfs_client
> *clp, const struct nfs4_state_recov
>  				set_bit(ops->owner_flag_bit, &sp-
> >so_flags);
>  				nfs4_put_state_owner(sp);
>  				status =
> nfs4_recovery_handle_error(clp, status);
> +				nfs4_free_state_owners(&freeme);
>  				return (status != 0) ? status : -
> EAGAIN;
>  			}
>  

Nice catch! Yes, that leak has been there for quite a while. Thanks for
finding it.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfs: fix memory leak in error path of nfs4_do_reclaim

Posted by Anna Schumaker 1 year, 3 months ago

On Wed, Sep 4, 2024 at 10:39 AM Trond Myklebust <trondmy@kernel.org> wrote:
>
> On Wed, 2024-09-04 at 20:34 +0800, Li Lingfeng wrote:
> > Commit c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in
> > nfs4_do_reclaim()") separate out the freeing of the state owners from
> > nfs4_purge_state_owners() and finish it outside the rcu lock.
> > However, the error path is omitted. As a result, the state owners in
> > "freeme" will not be released.
> > Fix it by adding freeing in the error path.
> >
> > Fixes: c77e22834ae9 ("NFSv4: Fix a potential sleep while atomic in
> > nfs4_do_reclaim()")
> > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> > ---
> >  fs/nfs/nfs4state.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index 877f682b45f2..30aba1dedaba 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -1957,6 +1957,7 @@ static int nfs4_do_reclaim(struct nfs_client
> > *clp, const struct nfs4_state_recov
> >                               set_bit(ops->owner_flag_bit, &sp-
> > >so_flags);
> >                               nfs4_put_state_owner(sp);
> >                               status =
> > nfs4_recovery_handle_error(clp, status);
> > +                             nfs4_free_state_owners(&freeme);
> >                               return (status != 0) ? status : -
> > EAGAIN;
> >                       }
> >
>
> Nice catch! Yes, that leak has been there for quite a while. Thanks for
> finding it.

Looks good to me, too. I've applied this for v6.12 (with a cc stable
so it gets backported)

Anna

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>