linux-ext4 - Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

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: <20180704004923.GT19934@dastard>
Date:   Wed, 4 Jul 2018 10:49:23 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Ross Zwisler <ross.zwisler@...ux.intel.com>
Cc:     Jan Kara <jack@...e.cz>, Dan Williams <dan.j.williams@...el.com>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Christoph Hellwig <hch@....de>, linux-nvdimm@...ts.01.org,
        Jeff Moyer <jmoyer@...hat.com>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v3 2/2] ext4: handle layout changes to pinned DAX mappings

On Mon, Jul 02, 2018 at 11:29:12AM -0600, Ross Zwisler wrote:
> Follow the lead of xfs_break_dax_layouts() and add synchronization between
> operations in ext4 which remove blocks from an inode (hole punch, truncate
> down, etc.) and pages which are pinned due to DAX DMA operations.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> Reviewed-by: Jan Kara <jack@...e.cz>
> Reviewed-by: Lukas Czerner <lczerner@...hat.com>
> ---
> 
> Changes since v2:
>  * Added a comment to ext4_insert_range() explaining why we don't call
>    ext4_break_layouts(). (Jan)

Which I think is wrong and will cause data corruption.

> @@ -5651,6 +5663,11 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
>  			LLONG_MAX);
>  	if (ret)
>  		goto out_mmap;
> +	/*
> +	 * We don't need to call ext4_break_layouts() because we aren't
> +	 * removing any blocks from the inode.  We are just changing their
> +	 * offset by inserting a hole.
> +	 */

The entire point of these leases is so that a thrid party can
directly access the blocks underlying the file. That means they are
keeping their own file offset<->disk block mapping internally, and
they are assuming that it is valid for as long as they hold the
lease. If the filesystem modifies the extent map - even something
like a shift here which changes the offset<->disk block mapping -
the userspace app now has a stale mapping and so the lease *must be
broken* to tell it that it's mappings are now stale and it needs to
refetch them.

If the app doesn't refetch it's mappings after something like a
shift, it will be reading and writing data from the wrong file
offset, and that will lead to the app silently corrupting it's data.
IOWs, layouts need to be broken by any operation that modifies the
extent map in any way, not just those operations that remove blocks.

Cheers,

Dave.

-- 
Dave Chinner
david@...morbit.com