[PATCH] mm: fix nr_rotate_swap leak in swapon() error case

[PATCH] mm: fix nr_rotate_swap leak in swapon() error case

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

If swapon() fails after incrementing nr_rotate_swap, we don't decrement
it and thus effectively leak it. Make sure we decrement it if we
incremented it.

Fixes: 81a0298bdfab ("mm, swap: don't use VMA based swap readahead if HDD is used as swap")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Based on v4.17-rc5.

 mm/swapfile.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index cc2cf04d9018..78a015fcec3b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3112,6 +3112,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	unsigned long *frontswap_map = NULL;
 	struct page *page = NULL;
 	struct inode *inode = NULL;
+	bool inced_nr_rotate_swap = false;
 
 	if (swap_flags & ~SWAP_FLAGS_VALID)
 		return -EINVAL;
@@ -3215,8 +3216,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 			cluster = per_cpu_ptr(p->percpu_cluster, cpu);
 			cluster_set_null(&cluster->index);
 		}
-	} else
+	} else {
 		atomic_inc(&nr_rotate_swap);
+		inced_nr_rotate_swap = true;
+	}
 
 	error = swap_cgroup_swapon(p->type, maxpages);
 	if (error)
@@ -3307,6 +3310,8 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 	vfree(swap_map);
 	kvfree(cluster_info);
 	kvfree(frontswap_map);
+	if (inced_nr_rotate_swap)
+		atomic_dec(&nr_rotate_swap);
 	if (swap_file) {
 		if (inode && S_ISREG(inode->i_mode)) {
 			inode_unlock(inode);
-- 
2.17.0

Re: [PATCH] mm: fix nr_rotate_swap leak in swapon() error case

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Wed, 2018-05-16 at 10:56 -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> If swapon() fails after incrementing nr_rotate_swap, we don't
> decrement
> it and thus effectively leak it. Make sure we decrement it if we
> incremented it.

My first inclination when reading this patch was
"surely there must be a better way to structure
the error code", but given that swap on rotating
media increments this count, while swap on SSD
does not, and the rotating media and SSD code
paths are 90% the same, my first inclination
was wrong.

Thanks for catching and fixing that bug.

> Fixes: 81a0298bdfab ("mm, swap: don't use VMA based swap readahead if
> HDD is used as swap")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Rik van Riel <riel@surriel.com>

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] mm: fix nr_rotate_swap leak in swapon() error case

[Huang, Ying]

Omar Sandoval <osandov@osandov.com> writes:

> From: Omar Sandoval <osandov@fb.com>
>
> If swapon() fails after incrementing nr_rotate_swap, we don't decrement
> it and thus effectively leak it. Make sure we decrement it if we
> incremented it.
>
> Fixes: 81a0298bdfab ("mm, swap: don't use VMA based swap readahead if HDD is used as swap")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Good catch!  Thanks!

Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

Best Regards,
Huang, Ying

> ---
> Based on v4.17-rc5.
>
>  mm/swapfile.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index cc2cf04d9018..78a015fcec3b 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3112,6 +3112,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>  	unsigned long *frontswap_map = NULL;
>  	struct page *page = NULL;
>  	struct inode *inode = NULL;
> +	bool inced_nr_rotate_swap = false;
>  
>  	if (swap_flags & ~SWAP_FLAGS_VALID)
>  		return -EINVAL;
> @@ -3215,8 +3216,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>  			cluster = per_cpu_ptr(p->percpu_cluster, cpu);
>  			cluster_set_null(&cluster->index);
>  		}
> -	} else
> +	} else {
>  		atomic_inc(&nr_rotate_swap);
> +		inced_nr_rotate_swap = true;
> +	}
>  
>  	error = swap_cgroup_swapon(p->type, maxpages);
>  	if (error)
> @@ -3307,6 +3310,8 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>  	vfree(swap_map);
>  	kvfree(cluster_info);
>  	kvfree(frontswap_map);
> +	if (inced_nr_rotate_swap)
> +		atomic_dec(&nr_rotate_swap);
>  	if (swap_file) {
>  		if (inode && S_ISREG(inode->i_mode)) {
>  			inode_unlock(inode);