FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed to apply to 3.14-stable tree

FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed to apply to 3.14-stable tree

[gregkh]

The patch below does not apply to the 3.14-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

>From 06bd3c36a733ac27962fea7d6f47168841376824 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Sun, 24 Apr 2016 00:56:03 -0400
Subject: [PATCH] ext4: fix data exposure after a crash

Huang has reported that in his powerfail testing he is seeing stale
block contents in some of recently allocated blocks although he mounts
ext4 in data=ordered mode. After some investigation I have found out
that indeed when delayed allocation is used, we don't add inode to
transaction's list of inodes needing flushing before commit. Originally
we were doing that but commit f3b59291a69d removed the logic with a
flawed argument that it is not needed.

The problem is that although for delayed allocated blocks we write their
contents immediately after allocating them, there is no guarantee that
the IO scheduler or device doesn't reorder things and thus transaction
allocating blocks and attaching them to inode can reach stable storage
before actual block contents. Actually whenever we attach freshly
allocated blocks to inode using a written extent, we should add inode to
transaction's ordered inode list to make sure we properly wait for block
contents to be written before committing the transaction. So that is
what we do in this patch. This also handles other cases where stale data
exposure was possible - like filling hole via mmap in
data=ordered,nodelalloc mode.

The only exception to the above rule are extending direct IO writes where
blkdev_direct_IO() waits for IO to complete before increasing i_size and
thus stale data exposure is not possible. For now we don't complicate
the code with optimizing this special case since the overhead is pretty
low. In case this is observed to be a performance problem we can always
handle it using a special flag to ext4_map_blocks().

CC: stable@vger.kernel.org
Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 981a1fc30eaa..250c2df04a92 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -684,6 +684,21 @@ out_sem:
 		ret = check_block_validity(inode, map);
 		if (ret != 0)
 			return ret;
+
+		/*
+		 * Inodes with freshly allocated blocks where contents will be
+		 * visible after transaction commit must be on transaction's
+		 * ordered data list.
+		 */
+		if (map->m_flags & EXT4_MAP_NEW &&
+		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
+		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
+		    !IS_NOQUOTA(inode) &&
+		    ext4_should_order_data(inode)) {
+			ret = ext4_jbd2_file_inode(handle, inode);
+			if (ret)
+				return ret;
+		}
 	}
 	return retval;
 }
@@ -1289,15 +1304,6 @@ static int ext4_write_end(struct file *file,
 	int i_size_changed = 0;
 
 	trace_ext4_write_end(inode, pos, len, copied);
-	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
-		ret = ext4_jbd2_file_inode(handle, inode);
-		if (ret) {
-			unlock_page(page);
-			put_page(page);
-			goto errout;
-		}
-	}
-
 	if (ext4_has_inline_data(inode)) {
 		ret = ext4_write_inline_data_end(inode, pos, len,
 						 copied, page);

RE: FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed to apply to 3.14-stable tree

[HUANG Weller (CM/ESW12-CN)]

Hi backports team,

Could you please apply the below patch to the 3.14-stable tree ?

Thanks.
Best regards

 Weller HUANG



> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, June 05, 2016 5:47 AM
> To: jack@suse.cz; HUANG Weller (CM/ESW12-CN)
> <Weller.Huang@cn.bosch.com>; tytso@mit.edu
> Cc: stable@vger.kernel.org
> Subject: FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed to
> apply to 3.14-stable tree
> 
> 
> The patch below does not apply to the 3.14-stable tree.
> If someone wants it applied there, or to any other stable or longterm tree, then
> please email the backport, including the original git commit id to
> <stable@vger.kernel.org>.
> 
> thanks,
> 
> greg k-h
> 
> ------------------ original commit in Linus's tree ------------------
> 
> From 06bd3c36a733ac27962fea7d6f47168841376824 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Sun, 24 Apr 2016 00:56:03 -0400
> Subject: [PATCH] ext4: fix data exposure after a crash
> 
> Huang has reported that in his powerfail testing he is seeing stale block contents in
> some of recently allocated blocks although he mounts
> ext4 in data=ordered mode. After some investigation I have found out that indeed
> when delayed allocation is used, we don't add inode to transaction's list of inodes
> needing flushing before commit. Originally we were doing that but commit
> f3b59291a69d removed the logic with a flawed argument that it is not needed.
> 
> The problem is that although for delayed allocated blocks we write their contents
> immediately after allocating them, there is no guarantee that the IO scheduler or
> device doesn't reorder things and thus transaction allocating blocks and attaching
> them to inode can reach stable storage before actual block contents. Actually
> whenever we attach freshly allocated blocks to inode using a written extent, we
> should add inode to transaction's ordered inode list to make sure we properly wait
> for block contents to be written before committing the transaction. So that is what
> we do in this patch. This also handles other cases where stale data exposure was
> possible - like filling hole via mmap in data=ordered,nodelalloc mode.
> 
> The only exception to the above rule are extending direct IO writes where
> blkdev_direct_IO() waits for IO to complete before increasing i_size and thus stale
> data exposure is not possible. For now we don't complicate the code with
> optimizing this special case since the overhead is pretty low. In case this is
> observed to be a performance problem we can always handle it using a special
> flag to ext4_map_blocks().
> 
> CC: stable@vger.kernel.org
> Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
> Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 981a1fc30eaa..250c2df04a92
> 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -684,6 +684,21 @@ out_sem:
>  		ret = check_block_validity(inode, map);
>  		if (ret != 0)
>  			return ret;
> +
> +		/*
> +		 * Inodes with freshly allocated blocks where contents will be
> +		 * visible after transaction commit must be on transaction's
> +		 * ordered data list.
> +		 */
> +		if (map->m_flags & EXT4_MAP_NEW &&
> +		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
> +		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
> +		    !IS_NOQUOTA(inode) &&
> +		    ext4_should_order_data(inode)) {
> +			ret = ext4_jbd2_file_inode(handle, inode);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  	return retval;
>  }
> @@ -1289,15 +1304,6 @@ static int ext4_write_end(struct file *file,
>  	int i_size_changed = 0;
> 
>  	trace_ext4_write_end(inode, pos, len, copied);
> -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> -		ret = ext4_jbd2_file_inode(handle, inode);
> -		if (ret) {
> -			unlock_page(page);
> -			put_page(page);
> -			goto errout;
> -		}
> -	}
> -
>  	if (ext4_has_inline_data(inode)) {
>  		ret = ext4_write_inline_data_end(inode, pos, len,
>  						 copied, page);

RE: FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed to apply to 3.14-stable tree

[HUANG Weller (CM/ESW12-CN)]

Hi Arend,

I am sorry for this misunderstanding.
Yes, I remember Jan Kara told me before.

Best regards

 Weller HUANG



> -----Original Message-----
> From: Arend van Spriel [mailto:arend.vanspriel@broadcom.com]
> Sent: Monday, June 06, 2016 1:17 PM
> To: HUANG Weller (CM/ESW12-CN) <Weller.Huang@cn.bosch.com>;
> gregkh@linuxfoundation.org; jack@suse.cz; tytso@mit.edu;
> backports@vger.kernel.org
> Cc: stable@vger.kernel.org; Behme Dirk (CM/ESO2)
> <Dirk.Behme@de.bosch.com>; Juergens Dirk (CM/ESO2)
> <Dirk.Juergens@de.bosch.com>
> Subject: Re: FAILED: patch "[PATCH] ext4: fix data exposure after a crash" failed
> to apply to 3.14-stable tree
> 
> On 06-06-16 02:02, HUANG Weller (CM/ESW12-CN) wrote:
> > Hi backports team,
> >
> > Could you please apply the below patch to the 3.14-stable tree ?
> 
> Hi Huang,
> 
> I suspect you are talking to the wrong crowd. Backports is a project to generate a
> source package of latest kernel code which can be compiled against older target
> kernels.
> 
> What is meant in Greg's (automated) response is that the patch should be
> backported manually so it applies to 3.14-stable tree. This is something Jan Kara
> should take care of if he cares about getting this patch in 3.14-stable.
> 
> Regards,
> Arend
> 
> > Thanks.
> > Best regards
> >
> >  Weller HUANG
> >
> >
> >
> >> -----Original Message-----
> >> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> >> Sent: Sunday, June 05, 2016 5:47 AM
> >> To: jack@suse.cz; HUANG Weller (CM/ESW12-CN)
> >> <Weller.Huang@cn.bosch.com>; tytso@mit.edu
> >> Cc: stable@vger.kernel.org
> >> Subject: FAILED: patch "[PATCH] ext4: fix data exposure after a
> >> crash" failed to apply to 3.14-stable tree
> >>
> >>
> >> The patch below does not apply to the 3.14-stable tree.
> >> If someone wants it applied there, or to any other stable or longterm
> >> tree, then please email the backport, including the original git
> >> commit id to <stable@vger.kernel.org>.
> >>
> >> thanks,
> >>
> >> greg k-h
> >>
> >> ------------------ original commit in Linus's tree ------------------
> >>
> >> From 06bd3c36a733ac27962fea7d6f47168841376824 Mon Sep 17 00:00:00
> >> 2001
> >> From: Jan Kara <jack@suse.cz>
> >> Date: Sun, 24 Apr 2016 00:56:03 -0400
> >> Subject: [PATCH] ext4: fix data exposure after a crash
> >>
> >> Huang has reported that in his powerfail testing he is seeing stale
> >> block contents in some of recently allocated blocks although he
> >> mounts
> >> ext4 in data=ordered mode. After some investigation I have found out
> >> that indeed when delayed allocation is used, we don't add inode to
> >> transaction's list of inodes needing flushing before commit.
> >> Originally we were doing that but commit f3b59291a69d removed the logic with
> a flawed argument that it is not needed.
> >>
> >> The problem is that although for delayed allocated blocks we write
> >> their contents immediately after allocating them, there is no
> >> guarantee that the IO scheduler or device doesn't reorder things and
> >> thus transaction allocating blocks and attaching them to inode can
> >> reach stable storage before actual block contents. Actually whenever
> >> we attach freshly allocated blocks to inode using a written extent,
> >> we should add inode to transaction's ordered inode list to make sure
> >> we properly wait for block contents to be written before committing
> >> the transaction. So that is what we do in this patch. This also handles other
> cases where stale data exposure was possible - like filling hole via mmap in
> data=ordered,nodelalloc mode.
> >>
> >> The only exception to the above rule are extending direct IO writes
> >> where
> >> blkdev_direct_IO() waits for IO to complete before increasing i_size
> >> and thus stale data exposure is not possible. For now we don't
> >> complicate the code with optimizing this special case since the
> >> overhead is pretty low. In case this is observed to be a performance
> >> problem we can always handle it using a special flag to ext4_map_blocks().
> >>
> >> CC: stable@vger.kernel.org
> >> Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
> >> Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> >> Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> >> Signed-off-by: Jan Kara <jack@suse.cz>
> >> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> >>
> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index
> >> 981a1fc30eaa..250c2df04a92
> >> 100644
> >> --- a/fs/ext4/inode.c
> >> +++ b/fs/ext4/inode.c
> >> @@ -684,6 +684,21 @@ out_sem:
> >>  		ret = check_block_validity(inode, map);
> >>  		if (ret != 0)
> >>  			return ret;
> >> +
> >> +		/*
> >> +		 * Inodes with freshly allocated blocks where contents will be
> >> +		 * visible after transaction commit must be on transaction's
> >> +		 * ordered data list.
> >> +		 */
> >> +		if (map->m_flags & EXT4_MAP_NEW &&
> >> +		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
> >> +		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
> >> +		    !IS_NOQUOTA(inode) &&
> >> +		    ext4_should_order_data(inode)) {
> >> +			ret = ext4_jbd2_file_inode(handle, inode);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >>  	}
> >>  	return retval;
> >>  }
> >> @@ -1289,15 +1304,6 @@ static int ext4_write_end(struct file *file,
> >>  	int i_size_changed = 0;
> >>
> >>  	trace_ext4_write_end(inode, pos, len, copied);
> >> -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> >> -		ret = ext4_jbd2_file_inode(handle, inode);
> >> -		if (ret) {
> >> -			unlock_page(page);
> >> -			put_page(page);
> >> -			goto errout;
> >> -		}
> >> -	}
> >> -
> >>  	if (ext4_has_inline_data(inode)) {
> >>  		ret = ext4_write_inline_data_end(inode, pos, len,
> >>  						 copied, page);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe backports"
> > in
> >

[PATCH 3.14] ext4: fix data exposure after a crash

[George G. Davis]

From: Jan Kara <jack@suse.cz>

From: Jan Kara <jack@suse.cz>

commit 06bd3c36a733ac27962fea7d6f47168841376824 upstream

Huang has reported that in his powerfail testing he is seeing stale
block contents in some of recently allocated blocks although he mounts
ext4 in data=ordered mode. After some investigation I have found out
that indeed when delayed allocation is used, we don't add inode to
transaction's list of inodes needing flushing before commit. Originally
we were doing that but commit f3b59291a69d removed the logic with a
flawed argument that it is not needed.

The problem is that although for delayed allocated blocks we write their
contents immediately after allocating them, there is no guarantee that
the IO scheduler or device doesn't reorder things and thus transaction
allocating blocks and attaching them to inode can reach stable storage
before actual block contents. Actually whenever we attach freshly
allocated blocks to inode using a written extent, we should add inode to
transaction's ordered inode list to make sure we properly wait for block
contents to be written before committing the transaction. So that is
what we do in this patch. This also handles other cases where stale data
exposure was possible - like filling hole via mmap in
data=ordered,nodelalloc mode.

The only exception to the above rule are extending direct IO writes where
blkdev_direct_IO() waits for IO to complete before increasing i_size and
thus stale data exposure is not possible. For now we don't complicate
the code with optimizing this special case since the overhead is pretty
low. In case this is observed to be a performance problem we can always
handle it using a special flag to ext4_map_blocks().

Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
[weller: fix conflict with 3.14 kernel]
Signed-off-by: weller huang <weller.huang@cn.bosch.com>
Signed-off-by: George G. Davis <george_davis@mentor.com>
---
gdavis: Confirmed that backport conflicts are due to lack of upstream
	commits c86d8db ("ext4: implement allocation of pre-zeroed
	blocks") and 09cbfea ("mm, fs: get rid of PAGE_CACHE_* and
	page_cache_{get,release} macros") in v3.14.37. The conflict
	resolution therefore appears to be correct.
---
 fs/ext4/inode.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 58001fc..d33a80e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -695,6 +695,20 @@ has_zeroout:
 		int ret = check_block_validity(inode, map);
 		if (ret != 0)
 			return ret;
+
+		/*
+		 * Inodes with freshly allocated blocks where contents will be
+		 * visible after transaction commit must be on transaction's
+		 * ordered data list.
+		 */
+		if (map->m_flags & EXT4_MAP_NEW &&
+		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
+		    !IS_NOQUOTA(inode) &&
+		    ext4_should_order_data(inode)) {
+			ret = ext4_jbd2_file_inode(handle, inode);
+			if (ret)
+				return ret;
+		}
 	}
 	return retval;
 }
@@ -1059,15 +1073,6 @@ static int ext4_write_end(struct file *file,
 	int i_size_changed = 0;
 
 	trace_ext4_write_end(inode, pos, len, copied);
-	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
-		ret = ext4_jbd2_file_inode(handle, inode);
-		if (ret) {
-			unlock_page(page);
-			page_cache_release(page);
-			goto errout;
-		}
-	}
-
 	if (ext4_has_inline_data(inode)) {
 		ret = ext4_write_inline_data_end(inode, pos, len,
 						 copied, page);
-- 
1.9.3

Re: [PATCH 3.14] ext4: fix data exposure after a crash

[Jan Kara]

The backport looks good to me.

								Honza

On Tue 28-06-16 18:31:48, George G. Davis wrote:
> From: Jan Kara <jack@suse.cz>
> 
> From: Jan Kara <jack@suse.cz>
> 
> commit 06bd3c36a733ac27962fea7d6f47168841376824 upstream
> 
> Huang has reported that in his powerfail testing he is seeing stale
> block contents in some of recently allocated blocks although he mounts
> ext4 in data=ordered mode. After some investigation I have found out
> that indeed when delayed allocation is used, we don't add inode to
> transaction's list of inodes needing flushing before commit. Originally
> we were doing that but commit f3b59291a69d removed the logic with a
> flawed argument that it is not needed.
> 
> The problem is that although for delayed allocated blocks we write their
> contents immediately after allocating them, there is no guarantee that
> the IO scheduler or device doesn't reorder things and thus transaction
> allocating blocks and attaching them to inode can reach stable storage
> before actual block contents. Actually whenever we attach freshly
> allocated blocks to inode using a written extent, we should add inode to
> transaction's ordered inode list to make sure we properly wait for block
> contents to be written before committing the transaction. So that is
> what we do in this patch. This also handles other cases where stale data
> exposure was possible - like filling hole via mmap in
> data=ordered,nodelalloc mode.
> 
> The only exception to the above rule are extending direct IO writes where
> blkdev_direct_IO() waits for IO to complete before increasing i_size and
> thus stale data exposure is not possible. For now we don't complicate
> the code with optimizing this special case since the overhead is pretty
> low. In case this is observed to be a performance problem we can always
> handle it using a special flag to ext4_map_blocks().
> 
> Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
> Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> [weller: fix conflict with 3.14 kernel]
> Signed-off-by: weller huang <weller.huang@cn.bosch.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> gdavis: Confirmed that backport conflicts are due to lack of upstream
> 	commits c86d8db ("ext4: implement allocation of pre-zeroed
> 	blocks") and 09cbfea ("mm, fs: get rid of PAGE_CACHE_* and
> 	page_cache_{get,release} macros") in v3.14.37. The conflict
> 	resolution therefore appears to be correct.
> ---
>  fs/ext4/inode.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 58001fc..d33a80e 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -695,6 +695,20 @@ has_zeroout:
>  		int ret = check_block_validity(inode, map);
>  		if (ret != 0)
>  			return ret;
> +
> +		/*
> +		 * Inodes with freshly allocated blocks where contents will be
> +		 * visible after transaction commit must be on transaction's
> +		 * ordered data list.
> +		 */
> +		if (map->m_flags & EXT4_MAP_NEW &&
> +		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
> +		    !IS_NOQUOTA(inode) &&
> +		    ext4_should_order_data(inode)) {
> +			ret = ext4_jbd2_file_inode(handle, inode);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  	return retval;
>  }
> @@ -1059,15 +1073,6 @@ static int ext4_write_end(struct file *file,
>  	int i_size_changed = 0;
>  
>  	trace_ext4_write_end(inode, pos, len, copied);
> -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> -		ret = ext4_jbd2_file_inode(handle, inode);
> -		if (ret) {
> -			unlock_page(page);
> -			page_cache_release(page);
> -			goto errout;
> -		}
> -	}
> -
>  	if (ext4_has_inline_data(inode)) {
>  		ret = ext4_write_inline_data_end(inode, pos, len,
>  						 copied, page);
> -- 
> 1.9.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3.14] ext4: fix data exposure after a crash

[Greg KH]

On Tue, Jun 28, 2016 at 06:31:48PM -0400, George G. Davis wrote:
> From: Jan Kara <jack@suse.cz>
> 
> From: Jan Kara <jack@suse.cz>
> 
> commit 06bd3c36a733ac27962fea7d6f47168841376824 upstream

What about a 4.4-stable version of this?  I don't want to take this into
3.14-stable without that.

thanks,

greg k-h

RE: [PATCH 3.14] ext4: fix data exposure after a crash

[HUANG Weller (CM/ESW12-CN)]

Hello Mr. Davis and Mr. Kara,

I checked the latest 3.14 source code 3.14.79. I didn't found the below patch.

It should be there , right ?



> -----Original Message-----
> From: Jan Kara [mailto:jack@suse.cz]
> Sent: Wednesday, June 29, 2016 3:46 PM
> To: George G. Davis <george_davis@mentor.com>
> Cc: stable@vger.kernel.org; Greg KH <gregkh@linuxfoundation.org>; Behme Dirk
> (CM/ESO2) <Dirk.Behme@de.bosch.com>; Jan Kara <jack@suse.cz>; Theodore
> Ts'o <tytso@mit.edu>; HUANG Weller (CM/ESW12-CN)
> <Weller.Huang@cn.bosch.com>
> Subject: Re: [PATCH 3.14] ext4: fix data exposure after a crash
> 
> The backport looks good to me.
> 
> 								Honza
> 
> On Tue 28-06-16 18:31:48, George G. Davis wrote:
> > From: Jan Kara <jack@suse.cz>
> >
> > From: Jan Kara <jack@suse.cz>
> >
> > commit 06bd3c36a733ac27962fea7d6f47168841376824 upstream
> >
> > Huang has reported that in his powerfail testing he is seeing stale
> > block contents in some of recently allocated blocks although he mounts
> > ext4 in data=ordered mode. After some investigation I have found out
> > that indeed when delayed allocation is used, we don't add inode to
> > transaction's list of inodes needing flushing before commit.
> > Originally we were doing that but commit f3b59291a69d removed the
> > logic with a flawed argument that it is not needed.
> >
> > The problem is that although for delayed allocated blocks we write
> > their contents immediately after allocating them, there is no
> > guarantee that the IO scheduler or device doesn't reorder things and
> > thus transaction allocating blocks and attaching them to inode can
> > reach stable storage before actual block contents. Actually whenever
> > we attach freshly allocated blocks to inode using a written extent, we
> > should add inode to transaction's ordered inode list to make sure we
> > properly wait for block contents to be written before committing the
> > transaction. So that is what we do in this patch. This also handles
> > other cases where stale data exposure was possible - like filling hole
> > via mmap in data=ordered,nodelalloc mode.
> >
> > The only exception to the above rule are extending direct IO writes
> > where
> > blkdev_direct_IO() waits for IO to complete before increasing i_size
> > and thus stale data exposure is not possible. For now we don't
> > complicate the code with optimizing this special case since the
> > overhead is pretty low. In case this is observed to be a performance
> > problem we can always handle it using a special flag to ext4_map_blocks().
> >
> > Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
> > Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> > Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > [weller: fix conflict with 3.14 kernel]
> > Signed-off-by: weller huang <weller.huang@cn.bosch.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> > gdavis: Confirmed that backport conflicts are due to lack of upstream
> > 	commits c86d8db ("ext4: implement allocation of pre-zeroed
> > 	blocks") and 09cbfea ("mm, fs: get rid of PAGE_CACHE_* and
> > 	page_cache_{get,release} macros") in v3.14.37. The conflict
> > 	resolution therefore appears to be correct.
> > ---
> >  fs/ext4/inode.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 58001fc..d33a80e
> > 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -695,6 +695,20 @@ has_zeroout:
> >  		int ret = check_block_validity(inode, map);
> >  		if (ret != 0)
> >  			return ret;
> > +
> > +		/*
> > +		 * Inodes with freshly allocated blocks where contents will be
> > +		 * visible after transaction commit must be on transaction's
> > +		 * ordered data list.
> > +		 */
> > +		if (map->m_flags & EXT4_MAP_NEW &&
> > +		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
> > +		    !IS_NOQUOTA(inode) &&
> > +		    ext4_should_order_data(inode)) {
> > +			ret = ext4_jbd2_file_inode(handle, inode);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	}
> >  	return retval;
> >  }
> > @@ -1059,15 +1073,6 @@ static int ext4_write_end(struct file *file,
> >  	int i_size_changed = 0;
> >
> >  	trace_ext4_write_end(inode, pos, len, copied);
> > -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> > -		ret = ext4_jbd2_file_inode(handle, inode);
> > -		if (ret) {
> > -			unlock_page(page);
> > -			page_cache_release(page);
> > -			goto errout;
> > -		}
> > -	}
> > -
> >  	if (ext4_has_inline_data(inode)) {
> >  		ret = ext4_write_inline_data_end(inode, pos, len,
> >  						 copied, page);
> > --
> > 1.9.3
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH 3.14] ext4: fix data exposure after a crash

[Jan Kara]

On Fri 06-10-17 10:04:41, HUANG Weller (CM/ESW12-CN) wrote:
> Hello Mr. Davis and Mr. Kara,
> 
> I checked the latest 3.14 source code 3.14.79. I didn't found the below patch.
> 
> It should be there , right ?

Well, I'm not sure who runs the 3.14 stable tree (it's not listed at
kernel.org). It's up to him to pick up patches...

								Honza

> > -----Original Message-----
> > From: Jan Kara [mailto:jack@suse.cz]
> > Sent: Wednesday, June 29, 2016 3:46 PM
> > To: George G. Davis <george_davis@mentor.com>
> > Cc: stable@vger.kernel.org; Greg KH <gregkh@linuxfoundation.org>; Behme Dirk
> > (CM/ESO2) <Dirk.Behme@de.bosch.com>; Jan Kara <jack@suse.cz>; Theodore
> > Ts'o <tytso@mit.edu>; HUANG Weller (CM/ESW12-CN)
> > <Weller.Huang@cn.bosch.com>
> > Subject: Re: [PATCH 3.14] ext4: fix data exposure after a crash
> > 
> > The backport looks good to me.
> > 
> > 								Honza
> > 
> > On Tue 28-06-16 18:31:48, George G. Davis wrote:
> > > From: Jan Kara <jack@suse.cz>
> > >
> > > From: Jan Kara <jack@suse.cz>
> > >
> > > commit 06bd3c36a733ac27962fea7d6f47168841376824 upstream
> > >
> > > Huang has reported that in his powerfail testing he is seeing stale
> > > block contents in some of recently allocated blocks although he mounts
> > > ext4 in data=ordered mode. After some investigation I have found out
> > > that indeed when delayed allocation is used, we don't add inode to
> > > transaction's list of inodes needing flushing before commit.
> > > Originally we were doing that but commit f3b59291a69d removed the
> > > logic with a flawed argument that it is not needed.
> > >
> > > The problem is that although for delayed allocated blocks we write
> > > their contents immediately after allocating them, there is no
> > > guarantee that the IO scheduler or device doesn't reorder things and
> > > thus transaction allocating blocks and attaching them to inode can
> > > reach stable storage before actual block contents. Actually whenever
> > > we attach freshly allocated blocks to inode using a written extent, we
> > > should add inode to transaction's ordered inode list to make sure we
> > > properly wait for block contents to be written before committing the
> > > transaction. So that is what we do in this patch. This also handles
> > > other cases where stale data exposure was possible - like filling hole
> > > via mmap in data=ordered,nodelalloc mode.
> > >
> > > The only exception to the above rule are extending direct IO writes
> > > where
> > > blkdev_direct_IO() waits for IO to complete before increasing i_size
> > > and thus stale data exposure is not possible. For now we don't
> > > complicate the code with optimizing this special case since the
> > > overhead is pretty low. In case this is observed to be a performance
> > > problem we can always handle it using a special flag to ext4_map_blocks().
> > >
> > > Fixes: f3b59291a69d0b734be1fc8be489fef2dd846d3d
> > > Reported-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> > > Tested-by: "HUANG Weller (CM/ESW12-CN)" <Weller.Huang@cn.bosch.com>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > [weller: fix conflict with 3.14 kernel]
> > > Signed-off-by: weller huang <weller.huang@cn.bosch.com>
> > > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > > ---
> > > gdavis: Confirmed that backport conflicts are due to lack of upstream
> > > 	commits c86d8db ("ext4: implement allocation of pre-zeroed
> > > 	blocks") and 09cbfea ("mm, fs: get rid of PAGE_CACHE_* and
> > > 	page_cache_{get,release} macros") in v3.14.37. The conflict
> > > 	resolution therefore appears to be correct.
> > > ---
> > >  fs/ext4/inode.c | 23 ++++++++++++++---------
> > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 58001fc..d33a80e
> > > 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -695,6 +695,20 @@ has_zeroout:
> > >  		int ret = check_block_validity(inode, map);
> > >  		if (ret != 0)
> > >  			return ret;
> > > +
> > > +		/*
> > > +		 * Inodes with freshly allocated blocks where contents will be
> > > +		 * visible after transaction commit must be on transaction's
> > > +		 * ordered data list.
> > > +		 */
> > > +		if (map->m_flags & EXT4_MAP_NEW &&
> > > +		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
> > > +		    !IS_NOQUOTA(inode) &&
> > > +		    ext4_should_order_data(inode)) {
> > > +			ret = ext4_jbd2_file_inode(handle, inode);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > >  	}
> > >  	return retval;
> > >  }
> > > @@ -1059,15 +1073,6 @@ static int ext4_write_end(struct file *file,
> > >  	int i_size_changed = 0;
> > >
> > >  	trace_ext4_write_end(inode, pos, len, copied);
> > > -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> > > -		ret = ext4_jbd2_file_inode(handle, inode);
> > > -		if (ret) {
> > > -			unlock_page(page);
> > > -			page_cache_release(page);
> > > -			goto errout;
> > > -		}
> > > -	}
> > > -
> > >  	if (ext4_has_inline_data(inode)) {
> > >  		ret = ext4_write_inline_data_end(inode, pos, len,
> > >  						 copied, page);
> > > --
> > > 1.9.3
> > >
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Ext4 support for LTS kernels (was: Re: [PATCH 3.14] ext4: fix data exposure after a crash)

[Theodore Ts'o]

On Mon, Oct 09, 2017 at 01:05:37PM +0200, Jan Kara wrote:
> On Fri 06-10-17 10:04:41, HUANG Weller (CM/ESW12-CN) wrote:
> > I checked the latest 3.14 source code 3.14.79. I didn't found the below patch.
> 
> Well, I'm not sure who runs the 3.14 stable tree (it's not listed at
> kernel.org). It's up to him to pick up patches...

The announcement of 3.14's End of Life as a stable kernel series was
announced in September 2016.  As far as I know, the fact that 3.14 is
no longer listed on kernel.org means that no one is maintaining 3.14
after Greg K-H declared it to be EOL'ed.

On a side note, I just recently finished an effort to update the ext4
patches for 3.14, 4.1, 4.4, and 4.9.  I've sent those backports and
cherry pick requests to stable@vger.kernel.org.  For details of my
work, at:

https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git

There are testing notes in the signed git tags:

ext4-4.9.54-1
   - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.9.54-1
ext4-4.4.91-1
   - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.4.91-1
ext4-4.1.44-1
   - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.1.44-1
ext4-3.18.74-1
   - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-3.18.74-1

.... and on a "when Ted has time / interest basis", the ext4-3.18,
ext4-4.1, ext4-4.4, and ext-4.9 branches in the above repo will be
updated with a rebase against the latest LTS kernel and whatever
patches are needed to minimize xfstests failures that haven't yet
gotten accepted into the LTS kernel versions.

Getting changes into android-common and/or various SOC's BSP kernels
is left as an exercise to the reader.

I suspect that at some point soon, when 4.14 becomes released and
becomes a LTS kernel, I'm going to drop ext4-4.1 and replace it with
ext4-4.14, since very few people seem to be using the 4.1 LTS kernel,
and my time / effort is limited.  If someone is interested in helping
out, please contact me.

					- Ted

Re: Ext4 support for LTS kernels (was: Re: [PATCH 3.14] ext4: fix data exposure after a crash)

[Greg KH]

On Mon, Oct 09, 2017 at 09:20:04AM -0400, Theodore Ts'o wrote:
> On Mon, Oct 09, 2017 at 01:05:37PM +0200, Jan Kara wrote:
> > On Fri 06-10-17 10:04:41, HUANG Weller (CM/ESW12-CN) wrote:
> > > I checked the latest 3.14 source code 3.14.79. I didn't found the below patch.
> > 
> > Well, I'm not sure who runs the 3.14 stable tree (it's not listed at
> > kernel.org). It's up to him to pick up patches...
> 
> The announcement of 3.14's End of Life as a stable kernel series was
> announced in September 2016.  As far as I know, the fact that 3.14 is
> no longer listed on kernel.org means that no one is maintaining 3.14
> after Greg K-H declared it to be EOL'ed.

Yes, 3.14 is dead :)

> On a side note, I just recently finished an effort to update the ext4
> patches for 3.14, 4.1, 4.4, and 4.9.  I've sent those backports and
> cherry pick requests to stable@vger.kernel.org.  For details of my
> work, at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
> 
> There are testing notes in the signed git tags:
> 
> ext4-4.9.54-1
>    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.9.54-1
> ext4-4.4.91-1
>    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.4.91-1
> ext4-4.1.44-1
>    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.1.44-1
> ext4-3.18.74-1
>    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-3.18.74-1
> 
> .... and on a "when Ted has time / interest basis", the ext4-3.18,
> ext4-4.1, ext4-4.4, and ext-4.9 branches in the above repo will be
> updated with a rebase against the latest LTS kernel and whatever
> patches are needed to minimize xfstests failures that haven't yet
> gotten accepted into the LTS kernel versions.

Thanks so much for this work.  I think I've gotten most of these for
4.9, 4.4, and 3.18, I've emailed about the ones that didn't apply.

> Getting changes into android-common and/or various SOC's BSP kernels
> is left as an exercise to the reader.

android-common syncs with the lts stable releases, so they should get
picked up by the SoC BSP kernels that way.  As long as your SoC company
is sane...

thanks,

greg k-h

Re: Ext4 support for LTS kernels (was: Re: [PATCH 3.14] ext4: fix data exposure after a crash)

[Greg KH]

On Tue, Oct 10, 2017 at 08:17:10PM +0200, Greg KH wrote:
> On Mon, Oct 09, 2017 at 09:20:04AM -0400, Theodore Ts'o wrote:
> > On Mon, Oct 09, 2017 at 01:05:37PM +0200, Jan Kara wrote:
> > > On Fri 06-10-17 10:04:41, HUANG Weller (CM/ESW12-CN) wrote:
> > > > I checked the latest 3.14 source code 3.14.79. I didn't found the below patch.
> > > 
> > > Well, I'm not sure who runs the 3.14 stable tree (it's not listed at
> > > kernel.org). It's up to him to pick up patches...
> > 
> > The announcement of 3.14's End of Life as a stable kernel series was
> > announced in September 2016.  As far as I know, the fact that 3.14 is
> > no longer listed on kernel.org means that no one is maintaining 3.14
> > after Greg K-H declared it to be EOL'ed.
> 
> Yes, 3.14 is dead :)
> 
> > On a side note, I just recently finished an effort to update the ext4
> > patches for 3.14, 4.1, 4.4, and 4.9.  I've sent those backports and
> > cherry pick requests to stable@vger.kernel.org.  For details of my
> > work, at:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
> > 
> > There are testing notes in the signed git tags:
> > 
> > ext4-4.9.54-1
> >    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.9.54-1
> > ext4-4.4.91-1
> >    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.4.91-1
> > ext4-4.1.44-1
> >    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-4.1.44-1
> > ext4-3.18.74-1
> >    - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/tag/?h=ext4-3.18.74-1
> > 
> > .... and on a "when Ted has time / interest basis", the ext4-3.18,
> > ext4-4.1, ext4-4.4, and ext-4.9 branches in the above repo will be
> > updated with a rebase against the latest LTS kernel and whatever
> > patches are needed to minimize xfstests failures that haven't yet
> > gotten accepted into the LTS kernel versions.
> 
> Thanks so much for this work.  I think I've gotten most of these for
> 4.9, 4.4, and 3.18, I've emailed about the ones that didn't apply.

I dug in your branches above, and found the one missing patch for 3.18.
So I think I am all caught up now.

If not, please let me know.

thanks,

greg k-h