linux-ext4 - Re: [PATCH v2] iomap: report collisions between directio and buffered writes to userspace

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: <20171117225640.GA11918@dhcp-10-11-180-142.int.fusionio.com>
Date:   Fri, 17 Nov 2017 14:56:41 -0800
From:   Liu Bo <bo.li.liu@...cle.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     xfs <linux-xfs@...r.kernel.org>, Ilya Dryomov <idryomov@...il.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Dave Chinner <david@...morbit.com>,
        Brian Foster <bfoster@...hat.com>,
        holger@...lied-asynchrony.com,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        linux-btrfs <linux-btrfs@...r.kernel.org>
Subject: Re: [PATCH v2] iomap: report collisions between directio and
 buffered writes to userspace

On Fri, Nov 17, 2017 at 11:39:25AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@...cle.com>
> 
> If two programs simultaneously try to write to the same part of a file
> via direct IO and buffered IO, there's a chance that the post-diowrite
> pagecache invalidation will fail on the dirty page.  When this happens,
> the dio write succeeded, which means that the page cache is no longer
> coherent with the disk!
> 
> Programs are not supposed to mix IO types and this is a clear case of
> data corruption, so store an EIO which will be reflected to userspace
> during the next fsync.  Replace the WARN_ON with a ratelimited pr_crit
> so that the developers have /some/ kind of breadcrumb to track down the
> offending program(s) and file(s) involved.
>

Looks good to me, thanks for addressing the warning.

Reviewed-by: Liu Bo <bo.li.liu@...cle.com>

Thanks,

-liubo

> Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
> ---
>  fs/direct-io.c     |   24 +++++++++++++++++++++++-
>  fs/iomap.c         |   12 ++++++++++--
>  include/linux/fs.h |    1 +
>  3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98fe132..ef5d12a 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -219,6 +219,27 @@ static inline struct page *dio_get_page(struct dio *dio,
>  	return dio->pages[sdio->head];
>  }
>  
> +/*
> + * Warn about a page cache invalidation failure during a direct io write.
> + */
> +void dio_warn_stale_pagecache(struct file *filp)
> +{
> +	static DEFINE_RATELIMIT_STATE(_rs, 30 * HZ, DEFAULT_RATELIMIT_BURST);
> +	char pathname[128];
> +	struct inode *inode = file_inode(filp);
> +	char *path;
> +
> +	errseq_set(&inode->i_mapping->wb_err, -EIO);
> +	if (__ratelimit(&_rs)) {
> +		path = file_path(filp, pathname, sizeof(pathname));
> +		if (IS_ERR(path))
> +			path = "(unknown)";
> +		pr_crit("Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!\n");
> +		pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid,
> +			current->comm);
> +	}
> +}
> +
>  /**
>   * dio_complete() - called when all DIO BIO I/O has been completed
>   * @offset: the byte offset in the file of the completed operation
> @@ -290,7 +311,8 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
>  		err = invalidate_inode_pages2_range(dio->inode->i_mapping,
>  					offset >> PAGE_SHIFT,
>  					(offset + ret - 1) >> PAGE_SHIFT);
> -		WARN_ON_ONCE(err);
> +		if (err)
> +			dio_warn_stale_pagecache(dio->iocb->ki_filp);
>  	}
>  
>  	if (!(dio->flags & DIO_SKIP_DIO_COUNT))
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 5011a96..028f329 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -753,7 +753,8 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  		err = invalidate_inode_pages2_range(inode->i_mapping,
>  				offset >> PAGE_SHIFT,
>  				(offset + dio->size - 1) >> PAGE_SHIFT);
> -		WARN_ON_ONCE(err);
> +		if (err)
> +			dio_warn_stale_pagecache(iocb->ki_filp);
>  	}
>  
>  	inode_dio_end(file_inode(iocb->ki_filp));
> @@ -1012,9 +1013,16 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	if (ret)
>  		goto out_free_dio;
>  
> +	/*
> +	 * Try to invalidate cache pages for the range we're direct
> +	 * writing.  If this invalidation fails, tough, the write will
> +	 * still work, but racing two incompatible write paths is a
> +	 * pretty crazy thing to do, so we don't support it 100%.
> +	 */
>  	ret = invalidate_inode_pages2_range(mapping,
>  			start >> PAGE_SHIFT, end >> PAGE_SHIFT);
> -	WARN_ON_ONCE(ret);
> +	if (ret)
> +		dio_warn_stale_pagecache(iocb->ki_filp);
>  	ret = 0;
>  
>  	if (iov_iter_rw(iter) == WRITE && !is_sync_kiocb(iocb) &&
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2690864..0e5f060 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2976,6 +2976,7 @@ enum {
>  };
>  
>  void dio_end_io(struct bio *bio);
> +void dio_warn_stale_pagecache(struct file *filp);
>  
>  ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
>  			     struct block_device *bdev, struct iov_iter *iter,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html