linux-ext4 - Re: [PATCH] resize2fs: fix ENOSPC corruption case

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: <20090520113748.GC24836@mit.edu>
Date:	Wed, 20 May 2009 07:37:48 -0400
From:	Theodore Tso <tytso@....edu>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] resize2fs: fix ENOSPC corruption case

On Mon, May 18, 2009 at 04:30:29PM -0500, Eric Sandeen wrote:
> 
> Index: e2fsprogs/lib/ext2fs/extent.c
> ===================================================================
> --- e2fsprogs.orig/lib/ext2fs/extent.c
> +++ e2fsprogs/lib/ext2fs/extent.c
> @@ -1068,16 +1068,17 @@ errcode_t ext2fs_extent_insert(ext2_exte
>  
>  	retval = ext2fs_extent_replace(handle, 0, extent);
>  	if (retval)
> -		goto errout;
> +		goto errout_delete;
>  
>  	retval = update_path(handle);
>  	if (retval)
> -		goto errout;
> +		goto errout_delete;
>  
>  	return 0;
>  
> -errout:
> +errout_delete:
>  	ext2fs_extent_delete(handle, 0);
> +errout:
>  	return retval;
>  }


Instead adding an errout_delete and changing what errout means, why
not just change:

			retval = extent_node_split(handle);
			if (retval)
-				goto errout;
+				return retval;
			path = handle->path + handle->level;

I also took a quick scan of extent_node_split(), and I'm not 100%
convinced it handles all of its error cases sanely.  But that should
be the focus of a different patch....


> @@ -1239,16 +1240,17 @@ again:
>  #ifdef DEBUG
>  		printf("(re/un)mapping last block in extent\n");
>  #endif
> -		extent.e_len--;
> -		retval = ext2fs_extent_replace(handle, 0, &extent);
> -		if (retval)
> -			goto done;
> +		/* Make sure insert works before replacing old extent */
>  		if (physical) {
>  			retval = ext2fs_extent_insert(handle,
>  					EXT2_EXTENT_INSERT_AFTER, &newextent);
>  			if (retval)
>  				goto done;
>  		}
> +		extent.e_len--;
> +		retval = ext2fs_extent_replace(handle, 0, &extent);
> +		if (retval)
> +			goto done;
>  	} else if (logical == extent.e_lblk) {

Have you tested this by hand, using the tst_extents test progam?  Code
inspection says that ext2fs_extent_insert leaves extent handle
pointing at the same place, but I admit the semantics need to be
better documented and tested to make sure they are correct in all
situations.  The extent code is new enough and tricky enough that I'm
always cautious about changes to it.

Looks good, though.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html