diff mbox series

[05/10] NFS: Fix memory leaks in nfs_pageio_stop_mirroring()

Message ID 20200401185652.1904777-6-trondmy@kernel.org
State New, archived
Headers show
Series NFS: Fix a number of memory leaks and use-after-free | expand

Commit Message

trondmy@kernel.org April 1, 2020, 6:56 p.m. UTC
From: Trond Myklebust <trond.myklebust@hammerspace.com>

If we just set the mirror count to 1 without first clearing out
the mirrors, we can leak queued up requests.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/pagelist.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Anna Schumaker April 2, 2020, 4:14 p.m. UTC | #1
Hi Trond,

On 4/1/20 2:56 PM, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> If we just set the mirror count to 1 without first clearing out
> the mirrors, we can leak queued up requests.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/pagelist.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 99eb839c5778..1fd4862217e0 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -900,15 +900,6 @@ static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
>  	pgio->pg_mirror_count = mirror_count;
>  }
>  
> -/*
> - * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
> - */
> -void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> -{
> -	pgio->pg_mirror_count = 1;
> -	pgio->pg_mirror_idx = 0;
> -}
> -
>  static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
>  {
>  	pgio->pg_mirror_count = 1;
> @@ -1334,6 +1325,14 @@ void nfs_pageio_cond_complete(struct nfs_pageio_descriptor *desc, pgoff_t index)
>  	}
>  }
>  
> +/*
> + * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
> + */
> +void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> +{
> +	nfs_pageio_complete(pgio);
> +}
> +

Would it make sense to replace calls to nfs_pageio_stop_mirroring() with nfs_pageio_complete() instead?

Anna

>  int __init nfs_init_nfspagecache(void)
>  {
>  	nfs_page_cachep = kmem_cache_create("nfs_page",
Trond Myklebust April 2, 2020, 4:54 p.m. UTC | #2
On Thu, 2020-04-02 at 12:14 -0400, Anna Schumaker wrote:
> Hi Trond,
> 
> On 4/1/20 2:56 PM, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If we just set the mirror count to 1 without first clearing out
> > the mirrors, we can leak queued up requests.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/pagelist.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index 99eb839c5778..1fd4862217e0 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
> > @@ -900,15 +900,6 @@ static void nfs_pageio_setup_mirroring(struct
> > nfs_pageio_descriptor *pgio,
> >  	pgio->pg_mirror_count = mirror_count;
> >  }
> >  
> > -/*
> > - * nfs_pageio_stop_mirroring - stop using mirroring (set mirror
> > count to 1)
> > - */
> > -void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> > -{
> > -	pgio->pg_mirror_count = 1;
> > -	pgio->pg_mirror_idx = 0;
> > -}
> > -
> >  static void nfs_pageio_cleanup_mirroring(struct
> > nfs_pageio_descriptor *pgio)
> >  {
> >  	pgio->pg_mirror_count = 1;
> > @@ -1334,6 +1325,14 @@ void nfs_pageio_cond_complete(struct
> > nfs_pageio_descriptor *desc, pgoff_t index)
> >  	}
> >  }
> >  
> > +/*
> > + * nfs_pageio_stop_mirroring - stop using mirroring (set mirror
> > count to 1)
> > + */
> > +void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
> > +{
> > +	nfs_pageio_complete(pgio);
> > +}
> > +
> 
> Would it make sense to replace calls to nfs_pageio_stop_mirroring()
> with nfs_pageio_complete() instead?
> 

No. Let's keep the wrapper, since it makes the writeback code easier to
read (it expresses the intent more clearly).

> Anna
> 
> >  int __init nfs_init_nfspagecache(void)
> >  {
> >  	nfs_page_cachep = kmem_cache_create("nfs_page",
diff mbox series

Patch

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 99eb839c5778..1fd4862217e0 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -900,15 +900,6 @@  static void nfs_pageio_setup_mirroring(struct nfs_pageio_descriptor *pgio,
 	pgio->pg_mirror_count = mirror_count;
 }
 
-/*
- * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
- */
-void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
-{
-	pgio->pg_mirror_count = 1;
-	pgio->pg_mirror_idx = 0;
-}
-
 static void nfs_pageio_cleanup_mirroring(struct nfs_pageio_descriptor *pgio)
 {
 	pgio->pg_mirror_count = 1;
@@ -1334,6 +1325,14 @@  void nfs_pageio_cond_complete(struct nfs_pageio_descriptor *desc, pgoff_t index)
 	}
 }
 
+/*
+ * nfs_pageio_stop_mirroring - stop using mirroring (set mirror count to 1)
+ */
+void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio)
+{
+	nfs_pageio_complete(pgio);
+}
+
 int __init nfs_init_nfspagecache(void)
 {
 	nfs_page_cachep = kmem_cache_create("nfs_page",