[PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Huang, Ying]

From: Huang Ying <ying.huang@intel.com>

In swapcache_free_entries(), if swap_info_get_cont() return NULL,
something wrong occurs for the swap entry.  But we should still
continue to free the following swap entries in the array instead of
skip them to avoid swap space leak.  This is just problem in error
path, where system may be in an inconsistent state, but it is still
good to fix it.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Tim Chen <tim.c.chen@intel.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shaohua Li <shli@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
---
 mm/swapfile.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 178130880b90..71890061f653 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
 		p = swap_info_get_cont(entries[i], prev);
 		if (p)
 			swap_entry_free(p, entries[i]);
-		else
-			break;
 		prev = p;
 	}
 	if (p)
-- 
2.11.0

[PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Huang, Ying]

From: Huang Ying <ying.huang@intel.com>

In swapcache_free_entries(), if swap_info_get_cont() return NULL,
something wrong occurs for the swap entry.  But we should still
continue to free the following swap entries in the array instead of
skip them to avoid swap space leak.  This is just problem in error
path, where system may be in an inconsistent state, but it is still
good to fix it.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Tim Chen <tim.c.chen@intel.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shaohua Li <shli@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
---
 mm/swapfile.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 178130880b90..71890061f653 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
 		p = swap_info_get_cont(entries[i], prev);
 		if (p)
 			swap_entry_free(p, entries[i]);
-		else
-			break;
 		prev = p;
 	}
 	if (p)
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Tim Chen]

On Fri, 2017-04-21 at 20:47 +0800, Huang, Ying wrote:
> From: Huang Ying <ying.huang@intel.com>
> 
> In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> something wrong occurs for the swap entry.  But we should still
> continue to free the following swap entries in the array instead of
> skip them to avoid swap space leak.  This is just problem in error
> path, where system may be in an inconsistent state, but it is still
> good to fix it.
> 

Acked-by: Tim Chen <tim.c.chen@linux.intel.com>

> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Tim Chen <tim.c.chen@intel.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> ---

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Tim Chen]

On Fri, 2017-04-21 at 20:47 +0800, Huang, Ying wrote:
> From: Huang Ying <ying.huang@intel.com>
> 
> In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> something wrong occurs for the swap entry.A A But we should still
> continue to free the following swap entries in the array instead of
> skip them to avoid swap space leak.A A This is just problem in error
> path, where system may be in an inconsistent state, but it is still
> good to fix it.
> 

Acked-by: Tim Chen <tim.c.chen@linux.intel.com>

> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Tim Chen <tim.c.chen@intel.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> ---

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Andrew Morton]

On Fri, 21 Apr 2017 20:47:39 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:

> From: Huang Ying <ying.huang@intel.com>
> 
> In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> something wrong occurs for the swap entry.  But we should still
> continue to free the following swap entries in the array instead of
> skip them to avoid swap space leak.  This is just problem in error
> path, where system may be in an inconsistent state, but it is still
> good to fix it.
> 
> ...
>
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>  		p = swap_info_get_cont(entries[i], prev);
>  		if (p)
>  			swap_entry_free(p, entries[i]);
> -		else
> -			break;
>  		prev = p;

So now prev==NULL.  Will this code get the locking correct in
swap_info_get_cont()?  I think so, but please double-check.

>  	}
>  	if (p)

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Andrew Morton]

On Fri, 21 Apr 2017 20:47:39 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:

> From: Huang Ying <ying.huang@intel.com>
> 
> In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> something wrong occurs for the swap entry.  But we should still
> continue to free the following swap entries in the array instead of
> skip them to avoid swap space leak.  This is just problem in error
> path, where system may be in an inconsistent state, but it is still
> good to fix it.
> 
> ...
>
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>  		p = swap_info_get_cont(entries[i], prev);
>  		if (p)
>  			swap_entry_free(p, entries[i]);
> -		else
> -			break;
>  		prev = p;

So now prev==NULL.  Will this code get the locking correct in
swap_info_get_cont()?  I think so, but please double-check.

>  	}
>  	if (p)


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Tim Chen]

On Tue, 2017-04-25 at 14:37 -0700, Andrew Morton wrote:
> On Fri, 21 Apr 2017 20:47:39 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:
> 
> > 
> > From: Huang Ying <ying.huang@intel.com>
> > 
> > In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> > something wrong occurs for the swap entry.  But we should still
> > continue to free the following swap entries in the array instead of
> > skip them to avoid swap space leak.  This is just problem in error
> > path, where system may be in an inconsistent state, but it is still
> > good to fix it.
> > 
> > ...
> > 
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
> >  		p = swap_info_get_cont(entries[i], prev);
> >  		if (p)
> >  			swap_entry_free(p, entries[i]);
> > -		else
> > -			break;
> >  		prev = p;
> So now prev==NULL.  Will this code get the locking correct in
> swap_info_get_cont()?  I think so, but please double-check.
> 

There are 4 possible cases, and I checked that the logic
in swap_info_get_cont do the expected:

entries[i]
valid?		prev	 	Expected swap_info_get_cont behavior
---------------------------------------------------------------------
NO		NULL		Return NULL p, Do nothing on lock/unlock
NO		NON-NULL	Return NULL p, Unlock prev 
YES		NULL		Return non-NULL p, lock p
YES		NON-NULL	Return non-NULL p, (p != prev) unlock prev and lock p 
						   (p == prev) do nothing on lock/unlock

Thanks.

Tim

> 

Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of swap_free_entries()

[Tim Chen]

On Tue, 2017-04-25 at 14:37 -0700, Andrew Morton wrote:
> On Fri, 21 Apr 2017 20:47:39 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:
> 
> > 
> > From: Huang Ying <ying.huang@intel.com>
> > 
> > In swapcache_free_entries(), if swap_info_get_cont() return NULL,
> > something wrong occurs for the swap entry.A A But we should still
> > continue to free the following swap entries in the array instead of
> > skip them to avoid swap space leak.A A This is just problem in error
> > path, where system may be in an inconsistent state, but it is still
> > good to fix it.
> > 
> > ...
> > 
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1079,8 +1079,6 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
> > A 		p = swap_info_get_cont(entries[i], prev);
> > A 		if (p)
> > A 			swap_entry_free(p, entries[i]);
> > -		else
> > -			break;
> > A 		prev = p;
> So now prev==NULL.A A Will this code get the locking correct in
> swap_info_get_cont()?A A I think so, but please double-check.
> 

There are 4 possible cases, and I checked that the logic
in swap_info_get_cont do the expected:

entries[i]
valid?		prev	A 	Expected swap_info_get_cont behavior
---------------------------------------------------------------------
NO		NULL		Return NULL p, Do nothing on lock/unlock
NO		NON-NULL	Return NULL p, Unlock prevA 
YES		NULL		Return non-NULL p, lock p
YES		NON-NULL	Return non-NULL p, (p != prev) unlock prev and lock pA 
						A  A (p == prev) do nothing on lock/unlock

Thanks.

Tim

> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>