From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932857AbaBACH7 (ORCPT ); Fri, 31 Jan 2014 21:07:59 -0500 Received: from mail-pd0-f178.google.com ([209.85.192.178]:58653 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932450AbaBACH5 (ORCPT ); Fri, 31 Jan 2014 21:07:57 -0500 Date: Fri, 31 Jan 2014 18:07:16 -0800 (PST) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Weijie Yang cc: linux-kernel , Linux-MM , Andrew Morton , hughd@google.com, Minchan Kim , shli@fusionio.com, Bob Liu , k.kozlowski@samsung.com, weijie.yang.kh@gmail.com Subject: Re: [PATCH] mm/swap: fix race on swap_info reuse between swapoff and swapon In-Reply-To: <000001cf0cfd$6d251640$476f42c0$%yang@samsung.com> Message-ID: References: <000001cf0cfd$6d251640$476f42c0$%yang@samsung.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 9 Jan 2014, Weijie Yang wrote: > swapoff clear swap_info's SWP_USED flag prematurely and free its resources > after that. A concurrent swapon will reuse this swap_info while its previous > resources are not cleared completely. > > These late freed resources are: > - p->percpu_cluster > - swap_cgroup_ctrl[type] > - block_device setting > - inode->i_flags &= ~S_SWAPFILE > > This patch clear SWP_USED flag after all its resources freed, so that swapon > can reuse this swap_info by alloc_swap_info() safely. > > Signed-off-by: Weijie Yang Acked-by: Hugh Dickins Cc: stable@vger.kernel.org I've now read through the thread at last, and think this (or akpm's mm-swap-fix-race-on-swap_info-reuse-between-swapoff-and-swapon.patch more clearly commented version) is the best of the patches on offer. I agree that it fixes Krzysztof's set_blocksize issue among others, and I prefer this one to his. Largely because I dislike swapon_mutex: it has always felt like one lock too many, so, contrary to akpm, I'm usually (perhaps irrationally) resistant to extending its use. swapon_mutex came into existence (as swapon_sem in 2.6.6) to handle a very specific might_sleep issue where /proc/swaps was using swap_lock. I may have abused it myself since in swapoff, not sure offhand: but think of it as proc_swaps_mutex, that's what it's really about. I'm sorry for derailing the previous discussion with my set_blocksize doubts: I still don't understand what that's all about, but we didn't get any clarification, and I now accept that it's safer to go on doing what we've always done there - plus these fixes. I think the use of swap_lock below is actually unnecessary, isn't it? This is the only piece of code that might be writing to p->flags at this point, and if another piece of code catches the before state or the after state, so what? But let's go ahead with mm-swap-fix-race-on-swap_info-reuse-between-swapoff-and-swapon.patch as is: no need to remove every redundancy (there is more near here!), and I may be playing too trickily. Thanks for the patch: I'll explain in a separate response why I prefer this to your later 2/8 version. Hugh > --- > mm/swapfile.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 612a7c9..89071c3 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1922,7 +1922,6 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile) > p->swap_map = NULL; > cluster_info = p->cluster_info; > p->cluster_info = NULL; > - p->flags = 0; > frontswap_map = frontswap_map_get(p); > spin_unlock(&p->lock); > spin_unlock(&swap_lock); > @@ -1948,6 +1947,16 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile) > mutex_unlock(&inode->i_mutex); > } > filp_close(swap_file, NULL); > + > + /* > + * clear SWP_USED flag after all resources freed > + * so that swapon can reuse this swap_info in alloc_swap_info() safely > + * it is ok to not hold p->lock after we cleared its SWP_WRITEOK > + */ > + spin_lock(&swap_lock); > + p->flags = 0; > + spin_unlock(&swap_lock); > + > err = 0; > atomic_inc(&proc_poll_event); > wake_up_interruptible(&proc_poll_wait); > -- > 1.7.10.4