linux-ext4 - Re: [PATCH v7 04/22] buffer: set errors in mapping at the time that the error occurs

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: <20170626081931.i6jxjel5p2alq6rn@eorzea.usersys.redhat.com>
Date:   Mon, 26 Jun 2017 10:19:31 +0200
From:   Carlos Maiolino <cmaiolino@...hat.com>
To:     Jeff Layton <jlayton@...hat.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: [PATCH v7 04/22] buffer: set errors in mapping at the time that
 the error occurs

On Fri, Jun 16, 2017 at 03:34:09PM -0400, Jeff Layton wrote:
> I noticed on xfs that I could still sometimes get back an error on fsync
> on a fd that was opened after the error condition had been cleared.
> 
> The problem is that the buffer code sets the write_io_error flag and
> then later checks that flag to set the error in the mapping. That flag
> perisists for quite a while however. If the file is later opened with
> O_TRUNC, the buffers will then be invalidated and the mapping's error
> set such that a subsequent fsync will return error. I think this is
> incorrect, as there was no writeback between the open and fsync.
> 
> Add a new mark_buffer_write_io_error operation that sets the flag and
> the error in the mapping at the same time. Replace all calls to
> set_buffer_write_io_error with mark_buffer_write_io_error, and remove
> the places that check this flag in order to set the error in the
> mapping.
> 
> This sets the error in the mapping earlier, at the time that it's first
> detected.
> 
> Signed-off-by: Jeff Layton <jlayton@...hat.com>
> Reviewed-by: Jan Kara <jack@...e.cz>
> ---
>  fs/buffer.c                 | 20 +++++++++++++-------
>  fs/gfs2/lops.c              |  2 +-
>  include/linux/buffer_head.h |  1 +
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 

Reviewed-by: Carlos Maiolino <cmaiolino@...hat.com>

> diff --git a/fs/buffer.c b/fs/buffer.c
> index 7b4f4bfde91e..4d5d03b42e11 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -178,7 +178,7 @@ void end_buffer_write_sync(struct buffer_head *bh, int uptodate)
>  		set_buffer_uptodate(bh);
>  	} else {
>  		buffer_io_error(bh, ", lost sync page write");
> -		set_buffer_write_io_error(bh);
> +		mark_buffer_write_io_error(bh);
>  		clear_buffer_uptodate(bh);
>  	}
>  	unlock_buffer(bh);
> @@ -352,8 +352,7 @@ void end_buffer_async_write(struct buffer_head *bh, int uptodate)
>  		set_buffer_uptodate(bh);
>  	} else {
>  		buffer_io_error(bh, ", lost async page write");
> -		mapping_set_error(page->mapping, -EIO);
> -		set_buffer_write_io_error(bh);
> +		mark_buffer_write_io_error(bh);
>  		clear_buffer_uptodate(bh);
>  		SetPageError(page);
>  	}
> @@ -481,8 +480,6 @@ static void __remove_assoc_queue(struct buffer_head *bh)
>  {
>  	list_del_init(&bh->b_assoc_buffers);
>  	WARN_ON(!bh->b_assoc_map);
> -	if (buffer_write_io_error(bh))
> -		mapping_set_error(bh->b_assoc_map, -EIO);
>  	bh->b_assoc_map = NULL;
>  }
>  
> @@ -1181,6 +1178,17 @@ void mark_buffer_dirty(struct buffer_head *bh)
>  }
>  EXPORT_SYMBOL(mark_buffer_dirty);
>  
> +void mark_buffer_write_io_error(struct buffer_head *bh)
> +{
> +	set_buffer_write_io_error(bh);
> +	/* FIXME: do we need to set this in both places? */
> +	if (bh->b_page && bh->b_page->mapping)
> +		mapping_set_error(bh->b_page->mapping, -EIO);
> +	if (bh->b_assoc_map)
> +		mapping_set_error(bh->b_assoc_map, -EIO);
> +}
> +EXPORT_SYMBOL(mark_buffer_write_io_error);
> +
>  /*
>   * Decrement a buffer_head's reference count.  If all buffers against a page
>   * have zero reference count, are clean and unlocked, and if the page is clean
> @@ -3266,8 +3274,6 @@ drop_buffers(struct page *page, struct buffer_head **buffers_to_free)
>  
>  	bh = head;
>  	do {
> -		if (buffer_write_io_error(bh) && page->mapping)
> -			mapping_set_error(page->mapping, -EIO);
>  		if (buffer_busy(bh))
>  			goto failed;
>  		bh = bh->b_this_page;
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 885d36e7a29f..1a9c2c08c1a1 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -182,7 +182,7 @@ static void gfs2_end_log_write_bh(struct gfs2_sbd *sdp, struct bio_vec *bvec,
>  		bh = bh->b_this_page;
>  	do {
>  		if (error)
> -			set_buffer_write_io_error(bh);
> +			mark_buffer_write_io_error(bh);
>  		unlock_buffer(bh);
>  		next = bh->b_this_page;
>  		size -= bh->b_size;
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index bd029e52ef5e..e0abeba3ced7 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -149,6 +149,7 @@ void buffer_check_dirty_writeback(struct page *page,
>   */
>  
>  void mark_buffer_dirty(struct buffer_head *bh);
> +void mark_buffer_write_io_error(struct buffer_head *bh);
>  void init_buffer(struct buffer_head *, bh_end_io_t *, void *);
>  void touch_buffer(struct buffer_head *bh);
>  void set_bh_page(struct buffer_head *bh,
> -- 
> 2.13.0
> 

-- 
Carlos