Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757589Ab3JQP7V (ORCPT ); Thu, 17 Oct 2013 11:59:21 -0400 Received: from mail-ie0-f179.google.com ([209.85.223.179]:33448 "EHLO mail-ie0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756986Ab3JQP7T convert rfc822-to-8bit (ORCPT ); Thu, 17 Oct 2013 11:59:19 -0400 MIME-Version: 1.0 In-Reply-To: References: <1381759136-8616-1-git-send-email-k.kozlowski@samsung.com> <1381836170.18389.13.camel@AMDC1943> Date: Thu, 17 Oct 2013 23:59:18 +0800 Message-ID: Subject: Re: [PATCH] swap: fix setting PAGE_SIZE blocksize during swapoff/swapon race From: Weijie Yang To: Hugh Dickins Cc: Krzysztof Kozlowski , Al Viro , Andrew Morton , Linux-Kernel , Linux-MM , Michal Hocko , Shaohua Li , Minchan Kim Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5822 Lines: 135 On Thu, Oct 17, 2013 at 11:48 PM, Weijie Yang wrote: > On Wed, Oct 16, 2013 at 1:19 AM, Hugh Dickins wrote: >> On Tue, 15 Oct 2013, Krzysztof Kozlowski wrote: >>> On Tue, 2013-10-15 at 02:59 -0700, Hugh Dickins wrote: >>> > On Mon, 14 Oct 2013, Krzysztof Kozlowski wrote: >>> > >>> > > Fix race between swapoff and swapon resulting in setting blocksize of >>> > > PAGE_SIZE for block devices during swapoff. >>> > > >>> > > The swapon modifies swap_info->old_block_size before acquiring >>> > > swapon_mutex. It reads block_size of bdev, stores it under >>> > > swap_info->old_block_size and sets new block_size to PAGE_SIZE. >>> > > >>> > > On the other hand the swapoff sets the device's block_size to >>> > > old_block_size after releasing swapon_mutex. >>> > > >>> > > This patch locks the swapon_mutex much earlier during swapon. It also >>> > > releases the swapon_mutex later during swapoff. >>> > > >>> > > The effect of race can be triggered by following scenario: >>> > > - One block swap device with block size of 512 >>> > > - thread 1: Swapon is called, swap is activated, >>> > > p->old_block_size = block_size(p->bdev); /512/ >>> > > block_size(p->bdev) = PAGE_SIZE; >>> > > Thread ends. >>> > > >>> > > - thread 2: Swapoff is called and it goes just after releasing the >>> > > swapon_mutex. The swap is now fully disabled except of setting the >>> > > block size to old value. The p->bdev->block_size is still equal to >>> > > PAGE_SIZE. >>> > > >>> > > - thread 3: New swapon is called. This swap is disabled so without >>> > > acquiring the swapon_mutex: >>> > > - p->old_block_size = block_size(p->bdev); /PAGE_SIZE (!!!)/ >>> > > - block_size(p->bdev) = PAGE_SIZE; >>> > > Swap is activated and thread ends. >>> > > >>> > > - thread 2: resumes work and sets blocksize to old value: >>> > > - set_blocksize(bdev, p->old_block_size) >>> > > But now the p->old_block_size is equal to PAGE_SIZE. >>> > > >>> > > The patch swap-fix-set_blocksize-race-during-swapon-swapoff does not fix >>> > > this particular issue. It reduces the possibility of races as the swapon >>> > > must overwrite p->old_block_size before acquiring swapon_mutex in >>> > > swapoff. >>> > > >>> > > Signed-off-by: Krzysztof Kozlowski >>> > >>> > Sorry you're being blown back and forth on this, but I say Nack to >>> > this version. I've not spent the time to check whether it ends up >>> > correct or not; but your original patch was appropriate to the bug, >>> > and this one is just unnecessary churn in my view. >>> >>> Hi, >>> >>> I still think my previous patch does not solve the issue entirely. >>> The call set_blocksize() in swapoff quite often sets PAGE_SIZE instead >>> of valid block size (e.g. 512). I trigger this with: >> >> PAGE_SIZE and 512 are equally valid block sizes, >> it's just hard to support both consistently at the same instant. >> >>> ------ >>> for i in `seq 1000` >>> do >>> swapoff /dev/sdc1 & >>> swapon /dev/sdc1 & >>> swapon /dev/sdc1 & >>> done >>> ------ >>> 10 seconds run of this script resulted in 50% of set_blocksize(PAGE_SIZE). >>> Although effect can only be observed after adding printks (block device is >>> released). >> >> But despite PAGE_SIZE being a valid block size, >> I agree that it's odd if you see variation there. >> >> Here's my guess: it looks as if the p->bdev test is inadequate, in the >> decision whether bad_swap should set_blocksize() or not: p->bdev is not >> usually reset when a swap_info_struct is released for reuse. >> >> Please try correcting that, either by resetting p->bdev where necessary, >> or by putting a better test in bad_swap: see if that fixes this oddity. >> >> I still much prefer your original little patch, >> to this extension of the use of swapon_mutex. >> >> However, a bigger question would be, why does swapoff have to set block >> size back to old_block_size anyway? That was introduced in 2.5.13 by >> >> (02/05/01 1.447.69.1) >> [PATCH] (1/6) blksize_size[] removal >> >> - preliminary cleanups: make sure that swapoff restores original block >> size, kill set_blocksize() (and use of __bread()) in multipath.c, >> reorder opening device and finding its block size in mtdblock.c. >> >> Al, not an urgent question, but is this swapoff old_block_size stuff >> still necessary? And can't swapon just use whatever bd_block_size is >> already in force? IIUC, it plays no part beyond the initial readpage >> of swap header. >> >> Thanks, >> Hugh > > Let me try to explain(and guess): > we have to set_block in swapon. the swap_header is PAGE_SIZE, if device's > blocksize is more than PAGE_SIZE, then the swap entry address on swapfile > would be not PAGE_SIZE aligned. or one swap page can not fill a block. > There maybe a problem for some device. > The set_blocksize() do the judgement work for swapon. > And may be some userland tools assume swap device blocksize is PAGE_SIZE？ > > issues here are more than this one: > After swap_info_struct is released for reuse in swapoff. > Its corresponding resources are released later, such as: > - swap_cgroup_swapoff(type); > - blkdev_put > - inode->i_flags &= ~S_SWAPFILE; > my code is 3.11 version. And in 3.12-rc5, free_percpu(p->percpu_cluster); is another issue that released later. > we need release(or clean) these resources before release swap_info_struct. > > to Krzysztof: I think it is better to add this handle to your patch > > regards -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/