From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1432675AbdDYWFj (ORCPT <rfc822;w@1wt.eu>);
        Tue, 25 Apr 2017 18:05:39 -0400
Received: from mga14.intel.com ([192.55.52.115]:56122 "EHLO mga14.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1430964AbdDYWFb (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 25 Apr 2017 18:05:31 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.37,251,1488873600"; 
   d="scan'208";a="78838572"
Message-ID: <1493157929.3209.113.camel@linux.intel.com>
Subject: Re: [PATCH -mm] mm, swap: Fix swap space leak in error path of
 swap_free_entries()
From: Tim Chen <tim.c.chen@linux.intel.com>
To: Andrew Morton <akpm@linux-foundation.org>,
        "Huang, Ying" <ying.huang@intel.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Tim Chen <tim.c.chen@intel.com>, Rik van Riel <riel@redhat.com>,
        Hugh Dickins <hughd@google.com>, Shaohua Li <shli@kernel.org>,
        Minchan Kim <minchan@kernel.org>
Date: Tue, 25 Apr 2017 15:05:29 -0700
In-Reply-To: <20170425143718.d05d4f5020b266dfdd61ed9c@linux-foundation.org>
References: <20170421124739.24534-1-ying.huang@intel.com>
         <20170425143718.d05d4f5020b266dfdd61ed9c@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.18.5.2 (3.18.5.2-1.fc23) 
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

>