linux-kernel - Re: [PATCH v2 5/5] btrfs: enable swap file support

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 linux-cve-announce 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]

Message-ID: <20141124220302.GA5785@mew.dhcp4.washington.edu>
Date:	Mon, 24 Nov 2014 14:03:02 -0800
From:	Omar Sandoval <osandov@...ndov.com>
To:	Filipe David Manana <fdmanana@...il.com>,
	David Sterba <dsterba@...e.cz>, linux-btrfs@...r.kernel.org
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, linux-nfs@...r.kernel.org,
	Trond Myklebust <trond.myklebust@...marydata.com>,
	Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH v2 5/5] btrfs: enable swap file support

On Sat, Nov 22, 2014 at 12:03:57PM -0800, Omar Sandoval wrote:
> On Fri, Nov 21, 2014 at 07:00:45PM +0100, David Sterba wrote:
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
> > > +			pr_err("BTRFS: swapfile is compresed");
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > 
> > I think the preallocated extents should be refused as well. This means
> > the filesystem has enough space to hold the data but it would still have
> > to go through the allocation and could in turn stress the memory
> > management code that triggered the swapping activity in the first place.
> > 
> > Though it's probably still possible to reach such corner case even with
> > fully allocated nodatacow file, this should be reviewed anyway.
> > 
> I'll definitely take a closer look at this. In particular,
> btrfs_get_blocks_direct and btrfs_get_extent do allocations in some cases which
> I'll look into.
> 
Alright, I took a look at this. My understanding is that a PREALLOC extent
represents a region on disk that has already been allocated but isn't in use
yet, but please correct me if I'm wrong. Judging by this comment in
btrfs_get_blocks_direct, we don't have to worry about PREALLOC extents in
general:

/*
 * We don't allocate a new extent in the following cases
 *
 * 1) The inode is marked as NODATACOW.  In this case we'll just use the
 * existing extent.
 * 2) The extent is marked as PREALLOC.  We're good to go here and can
 * just use the extent.
 *
 */

A couple of other considerations that cropped up:

- btrfs_get_extent does a bunch of extra work if the extent is not cached in
  the extent map tree that would be nice to avoid when swapping
- We might still have to do a COW if the swap file is in a snapshot

We can avoid the btrfs_get_extent by pinning the extents in memory one way or
another in btrfs_swap_activate.

The snapshot issue is a little tricker to resolve. I see a few options:

1. Just do the COW and hope for the best
2. As part of btrfs_swap_activate, COW any shared extents. If a snapshot
happens while a swap file is active, we'll fall back to 1.
3. Clobber any swap file extents which are in a snapshot, i.e., always use the
existing extent.

I'm partial to 3, as it's the simplest approach, and I don't think it makes
much sense for a swap file to be in a snapshot anyways. I'd appreciate any
comments that anyone might have.

-- 
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/