[3.8.y.z,extended,stable] Patch "ext4: fix premature freeing of partial clusters split across leaf blocks" has been added to staging queue

This is a note to let you know that I have just added a patch titled

    ext4: fix premature freeing of partial clusters split across leaf blocks

to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree 
which can be found at:

 http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.8.y-queue

This patch is scheduled to be released in version 3.8.13.22.

If you, or anyone else, feels it should not be added to this tree, please 
reply to this email.

For more information about the 3.8.y.z tree, see
https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable

Thanks.
-Kamal

------

From bbc128e90ae5c1af34a25d40cdda91038fde3468 Mon Sep 17 00:00:00 2001
From: Eric Whitney <enwlinux@gmail.com>
Date: Tue, 1 Apr 2014 19:49:30 -0400
Subject: ext4: fix premature freeing of partial clusters split across leaf
 blocks

commit ad6599ab3ac98a4474544086e048ce86ec15a4d1 upstream.

Xfstests generic/311 and shared/298 fail when run on a bigalloc file
system.  Kernel error messages produced during the tests report that
blocks to be freed are already on the to-be-freed list.  When e2fsck
is run at the end of the tests, it typically reports bad i_blocks and
bad free blocks counts.

The bug that causes these failures is located in ext4_ext_rm_leaf().
Code at the end of the function frees a partial cluster if it's not
shared with an extent remaining in the leaf.  However, if all the
extents in the leaf have been removed, the code dereferences an
invalid extent pointer (off the front of the leaf) when the check for
sharing is made.  This generally has the effect of unconditionally
freeing the partial cluster, which leads to the observed failures
when the partial cluster is shared with the last extent in the next
leaf.

Fix this by attempting to free the cluster only if extents remain in
the leaf.  Any remaining partial cluster will be freed if possible
when the next leaf is processed or when leaf removal is complete.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 fs/ext4/extents.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

--
1.9.1

On Thu, 2014-04-17 at 16:30 -0700, Kamal Mostafa wrote:
> This is a note to let you know that I have just added a patch titled
> 
>     ext4: fix premature freeing of partial clusters split across leaf blocks
> 
> to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree 

This patch won't cleanly apply to 3.8 without a prerequisite that has
now been dropped from the queue (ext4: make punch hole code path work
with bigalloc).

So this patch (ext4: fix premature freeing) will also be dropped from
the 3.8-stable queue, unless one of the ext4 folks would like to provide
a backport which applies to 3.8-stable's fs/ext4/extents.c[0], or advice
about what that "if partial_cluster" line should look like when all is
said and done.

Thanks,

 -Kamal

[0] http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=blob;f=fs/ext4/extents.c;h=8b5c2646786fbe10cb559d8636544350cad9274c;hb=refs/heads/linux-3.8.y-queue#l2614



> which can be found at:
> 
>  http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.8.y-queue
> 
> This patch is scheduled to be released in version 3.8.13.22.
> 
> If you, or anyone else, feels it should not be added to this tree, please 
> reply to this email.
> 
> For more information about the 3.8.y.z tree, see
> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
> 
> Thanks.
> -Kamal
> 
> ------
> 
> From bbc128e90ae5c1af34a25d40cdda91038fde3468 Mon Sep 17 00:00:00 2001
> From: Eric Whitney <enwlinux@gmail.com>
> Date: Tue, 1 Apr 2014 19:49:30 -0400
> Subject: ext4: fix premature freeing of partial clusters split across leaf
>  blocks
> 
> commit ad6599ab3ac98a4474544086e048ce86ec15a4d1 upstream.
> 
> Xfstests generic/311 and shared/298 fail when run on a bigalloc file
> system.  Kernel error messages produced during the tests report that
> blocks to be freed are already on the to-be-freed list.  When e2fsck
> is run at the end of the tests, it typically reports bad i_blocks and
> bad free blocks counts.
> 
> The bug that causes these failures is located in ext4_ext_rm_leaf().
> Code at the end of the function frees a partial cluster if it's not
> shared with an extent remaining in the leaf.  However, if all the
> extents in the leaf have been removed, the code dereferences an
> invalid extent pointer (off the front of the leaf) when the check for
> sharing is made.  This generally has the effect of unconditionally
> freeing the partial cluster, which leads to the observed failures
> when the partial cluster is shared with the last extent in the next
> leaf.
> 
> Fix this by attempting to free the cluster only if extents remain in
> the leaf.  Any remaining partial cluster will be freed if possible
> when the next leaf is processed or when leaf removal is complete.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> ---
>  fs/ext4/extents.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7b22105..37f6849 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2646,10 +2646,15 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
>  		err = ext4_ext_correct_indexes(handle, inode, path);
> 
>  	/*
> -	 * Free the partial cluster only if the current extent does not
> -	 * reference it. Otherwise we might free used cluster.
> +	 * If there's a partial cluster and at least one extent remains in
> +	 * the leaf, free the partial cluster if it isn't shared with the
> +	 * current extent.  If there's a partial cluster and no extents
> +	 * remain in the leaf, it can't be freed here.  It can only be
> +	 * freed when it's possible to determine if it's not shared with
> +	 * any other extent - when the next leaf is processed or when space
> +	 * removal is complete.
>  	 */
> -	if (*partial_cluster > 0 &&
> +	if (*partial_cluster > 0 && eh->eh_entries &&
>  	    (EXT4_B2C(sbi, ext4_ext_pblock(ex) + ex_ee_len - 1) !=
>  	     *partial_cluster)) {
>  		int flags = EXT4_FREE_BLOCKS_FORGET;
> --
> 1.9.1
>

On Fri, Apr 18, 2014 at 10:23:27AM -0700, Kamal Mostafa wrote:
> On Thu, 2014-04-17 at 16:30 -0700, Kamal Mostafa wrote:
> > This is a note to let you know that I have just added a patch titled
> > 
> >     ext4: fix premature freeing of partial clusters split across leaf blocks
> > 
> > to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree 
> 
> This patch won't cleanly apply to 3.8 without a prerequisite that has
> now been dropped from the queue (ext4: make punch hole code path work
> with bigalloc).
> 
> So this patch (ext4: fix premature freeing) will also be dropped from
> the 3.8-stable queue, unless one of the ext4 folks would like to provide
> a backport which applies to 3.8-stable's fs/ext4/extents.c[0], or advice
> about what that "if partial_cluster" line should look like when all is
> said and done.

I believe the problem which this patches is trying to address only
happens if punch_hole is enabled for bigalloc. (That is, it should
never happen if we are just truncating the file), so this should be
OK.

Eric, can you confirm?

						- Ted

* tytso@mit.edu <tytso@mit.edu>:
> On Fri, Apr 18, 2014 at 10:23:27AM -0700, Kamal Mostafa wrote:
> > On Thu, 2014-04-17 at 16:30 -0700, Kamal Mostafa wrote:
> > > This is a note to let you know that I have just added a patch titled
> > > 
> > >     ext4: fix premature freeing of partial clusters split across leaf blocks
> > > 
> > > to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree 
> > 
> > This patch won't cleanly apply to 3.8 without a prerequisite that has
> > now been dropped from the queue (ext4: make punch hole code path work
> > with bigalloc).
> > 
> > So this patch (ext4: fix premature freeing) will also be dropped from
> > the 3.8-stable queue, unless one of the ext4 folks would like to provide
> > a backport which applies to 3.8-stable's fs/ext4/extents.c[0], or advice
> > about what that "if partial_cluster" line should look like when all is
> > said and done.
> 
> I believe the problem which this patches is trying to address only
> happens if punch_hole is enabled for bigalloc. (That is, it should
> never happen if we are just truncating the file), so this should be
> OK.
> 
> Eric, can you confirm?
> 

Dropping this patch from Ubuntu's 3.8 stable queue is fine.  It fixes a bug
introduced during the 3.10 release cycle that can be triggered by
truncation as well as hole punching on bigalloc file systems (see d23142c627
upstream).

By inspection, the relevant code in 3.8 contains an expression -
ex >= EXT_FIRST_EXTENT(eh) - that should be equivalent in effect to the one
added by my patch.  Removal of that expression by d23142c627 caused the bug.

The xfstests that exposed the bug during 3.14 regression, generic/311 and
shared/298, did not exist at the time of the 3.8 or 3.10 releases.  I built
a 3.8 kernel and ran those tests on it - they both passed as expected.

Please let me know if you have any more questions.

Eric

On Fri, 2014-04-18 at 17:32 -0400, Eric Whitney wrote:
> * tytso@mit.edu <tytso@mit.edu>:
> > On Fri, Apr 18, 2014 at 10:23:27AM -0700, Kamal Mostafa wrote:
> > > On Thu, 2014-04-17 at 16:30 -0700, Kamal Mostafa wrote:
> > > > This is a note to let you know that I have just added a patch titled
> > > > 
> > > >     ext4: fix premature freeing of partial clusters split across leaf blocks
> > > > 
> > > > to the linux-3.8.y-queue branch of the 3.8.y.z extended stable tree 
> > > 
> > > This patch won't cleanly apply to 3.8 without a prerequisite that has
> > > now been dropped from the queue (ext4: make punch hole code path work
> > > with bigalloc).
> > > 
> > > So this patch (ext4: fix premature freeing) will also be dropped from
> > > the 3.8-stable queue, unless one of the ext4 folks would like to provide
> > > a backport which applies to 3.8-stable's fs/ext4/extents.c[0], or advice
> > > about what that "if partial_cluster" line should look like when all is
> > > said and done.
> > 
> > I believe the problem which this patches is trying to address only
> > happens if punch_hole is enabled for bigalloc. (That is, it should
> > never happen if we are just truncating the file), so this should be
> > OK.
> > 
> > Eric, can you confirm?
> > 
> 
> Dropping this patch from Ubuntu's 3.8 stable queue is fine.  It fixes a bug
> introduced during the 3.10 release cycle that can be triggered by
> truncation as well as hole punching on bigalloc file systems (see d23142c627
> upstream).
> 
> By inspection, the relevant code in 3.8 contains an expression -
> ex >= EXT_FIRST_EXTENT(eh) - that should be equivalent in effect to the one
> added by my patch.  Removal of that expression by d23142c627 caused the bug.
> 
> The xfstests that exposed the bug during 3.14 regression, generic/311 and
> shared/298, did not exist at the time of the 3.8 or 3.10 releases.  I built
> a 3.8 kernel and ran those tests on it - they both passed as expected.
> 
> Please let me know if you have any more questions.
> 
> Eric


Thanks Eric, and thanks again Ted and Lukas.

 -Kamal