lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 21 Aug 2020 08:21:45 +0800
From:   Gao Xiang <hsiangkao@...hat.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Carlos Maiolino <cmaiolino@...hat.com>,
        Eric Sandeen <esandeen@...hat.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        Rafael Aquini <aquini@...hat.com>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH v2] mm, THP, swap: fix allocating cluster for swapfile by
 mistake

Hi Dave,

On Fri, Aug 21, 2020 at 09:34:46AM +1000, Dave Chinner wrote:
> On Thu, Aug 20, 2020 at 12:53:23PM +0800, Gao Xiang wrote:
> > SWP_FS is used to make swap_{read,write}page() go through
> > the filesystem, and it's only used for swap files over
> > NFS. So, !SWP_FS means non NFS for now, it could be either
> > file backed or device backed. Something similar goes with
> > legacy SWP_FILE.
> > 
> > So in order to achieve the goal of the original patch,
> > SWP_BLKDEV should be used instead.
> > 
> > FS corruption can be observed with SSD device + XFS +
> > fragmented swapfile due to CONFIG_THP_SWAP=y.
> > 
> > I reproduced the issue with the following details:
> > 
> > Environment:
> > QEMU + upstream kernel + buildroot + NVMe (2 GB)
> > 
> > Kernel config:
> > CONFIG_BLK_DEV_NVME=y
> > CONFIG_THP_SWAP=y
> 
> Ok, so at it's core this is a swap file extent versus THP swap
> cluster alignment issue?

I think yes.

> 
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 6c26916e95fd..2937daf3ca02 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1074,7 +1074,7 @@ int get_swap_pages(int n_goal, swp_entry_t swp_entries[], int entry_size)
> >  			goto nextsi;
> >  		}
> >  		if (size == SWAPFILE_CLUSTER) {
> > -			if (!(si->flags & SWP_FS))
> > +			if (si->flags & SWP_BLKDEV)
> >  				n_ret = swap_alloc_cluster(si, swp_entries);
> >  		} else
> >  			n_ret = scan_swap_map_slots(si, SWAP_HAS_CACHE,
> 
> IOWs, if you don't make this change, does the corruption problem go
> away if you align swap extents in iomap_swapfile_add_extent() to
> (SWAPFILE_CLUSTER * PAGE_SIZE) instead of just PAGE_SIZE?
> 
> I.e. if the swapfile extents are aligned correctly to huge page swap
> cluster size and alignment, does the swap clustering optimisations
> for swapping THP pages work correctly? And, if so, is there any
> performance benefit we get from enabling proper THP swap clustering
> on swapfiles?
> 

Yeah, I once think about some similiar thing as well. My thought for now is

 - First, SWAP THP doesn't claim to support such swapfile for now.
   And the original author tried to explicitly avoid the whole thing in

   f0eea189e8e9 ("mm, THP, swap: Don't allocate huge cluster for file backed swap device")

   So such thing would be considered as some new feature and need
   more testing at least. But for now I think we just need a quick
   fix to fix the commit f0eea189e8e9 to avoid regression and for
   backport use.

 - It is hard for users to control swapfile in
   SWAPFILE_CLUSTER * PAGE_SIZE extents, especially users'
   disk are fragmented or have some on-disk metadata limitation or
   something. It's very hard for users to utilize this and arrange
   their swapfile physical addr alignment and fragments for now.

So my point is, if it's considered in the future (supporting SWAP
THP swapfile), it needs more carefully consideration and decision
(e.g. stability, performance, simplicity, etc). For now, it's just
a exist regression which fixes the original fix, and finish
the original author claim.

Thanks,
Gao Xiang

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@...morbit.com
> 

Powered by blists - more mailing lists