Re: [PATCH RESEND 3/3] udf: leave udf_next_aext() outputs alone at the end of the extent list

[Jan Kara]

On Fri 02-10-26 12:26:27, Matthias Goergens wrote:
> udf_next_aext() decodes each allocation descriptor straight into the
> caller's eloc, elen and etype, and follows a continuation descriptor
> into the next allocation extent.  If that allocation extent holds no
> descriptors, it returns 0 for the end of the list, but by then eloc,
> elen and etype describe the continuation descriptor itself: the block
> of the empty allocation extent, one block long, type 3.
> 
> The kernel creates such lists itself: udf_delete_aext() leaves the last
> allocation extent of a list empty when it removes its only descriptor,
> and udf_do_extend_file() and udf_extend_file() already handle a list
> that ends in an empty one.
> 
> Two callers use the outputs after a return of 0.  udf_discard_prealloc()
> walks to the last extent and, if it is a preallocation, deletes it with
> udf_delete_aext() and frees eloc/elen.  When an empty allocation extent
> follows, udf_delete_aext() removes the continuation and frees the empty
> block, and udf_discard_prealloc() then frees that block a second time
> instead of the preallocated blocks.  On a space bitmap the preallocated
> blocks are leaked and the free block count drifts.  On an unallocated
> space table the second free adds a second free extent for the same
> block, which is later handed out twice: fsx as run by generic/091 and
> generic/263 ends up with two parts of its test file in one block and
> reads back bad data.
> 
> udf_table_prealloc_blocks() can likewise take an empty allocation extent
> at the end of the table's own list for a free extent starting at the
> goal block.
> 
> Only update the outputs once a descriptor other than a continuation has
> been found.  The other callers use the outputs only on a positive
> return, or not at all, with one exception: when udf_table_free_blocks()
> appends a new extent, it keeps the partition reference of whatever eloc
> last held, which for a table whose list is a single continuation to an
> empty allocation extent would now be uninitialised.  Take it from the
> freed blocks instead.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

Good catch! But I'd consider this mostly a bug in udf_discard_prealloc().
If udf_next_aext() returns value <= 0, you cannot assume anything about the
content of eloc & elen, including whether their value was or wasn't
clobbered. So IMO a nicer fix is to fix udf_discard_prealloc() to use
tmpeloc & tmpelen which should fix the problem as well.

								Honza

> ---
> Reproducer, with fsx from fstests and mkudffs from udftools:
> 
>   truncate --size=2G udf.img
>   mkudffs --blocksize=512 --space=unalloctable udf.img
>   mount -t udf -o loop udf.img /mnt
>   fsx -N 10000 -l 500000 -r 4096 -t 512 -w 512 -Z -R -W /mnt/junk
> 
> Without this patch fsx stops with READ BAD DATA after about 9850
> operations, in every run; with it, all 10000 operations complete.
> With --space=unallocbitmap fsx completes either way.
> 
>  fs/udf/balloc.c |  1 +
>  fs/udf/inode.c  | 21 +++++++++++++++++----
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index 2ec577b4321c..d863ee201cbf 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -457,6 +457,7 @@ static void udf_table_free_blocks(struct super_block *sb,
>  
>  		int adsize;
>  
> +		eloc.partitionReferenceNum = bloc->partitionReferenceNum;
>  		eloc.logicalBlockNum = start;
>  		elen = EXT_RECORDED_ALLOCATED |
>  			(count << sb->s_blocksize_bits);
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 71386e7ac796..0e55f749bc48 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -2266,22 +2266,35 @@ void udf_write_aext(struct inode *inode, struct extent_position *epos,
>  
>  /*
>   * Returns 1 on success, -errno on error, 0 on hit EOF.
> + *
> + * eloc, elen and etype are only updated when the next allocation descriptor
> + * was found.  In particular, when a chain of indirect extents ends in an
> + * empty one, following the trailing CONTINUE descriptor and hitting EOF must
> + * not clobber them with the location and length of that CONTINUE: callers
> + * keep using the last real extent's values after a 0 return, e.g. to discard
> + * its preallocation.
>   */
>  int udf_next_aext(struct inode *inode, struct extent_position *epos,
>  		  struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
>  		  int inc)
>  {
> +	struct kernel_lb_addr tloc;
> +	uint32_t tlen;
> +	int8_t ttype;
>  	unsigned int indirections = 0;
>  	int ret = 0;
>  	udf_pblk_t block;
>  
>  	while (1) {
> -		ret = udf_current_aext(inode, epos, eloc, elen,
> -				       etype, inc);
> +		ret = udf_current_aext(inode, epos, &tloc, &tlen, &ttype, inc);
>  		if (ret <= 0)
>  			return ret;
> -		if (*etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
> +		if (ttype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
> +			*eloc = tloc;
> +			*elen = tlen;
> +			*etype = ttype;
>  			return ret;
> +		}
>  
>  		if (++indirections > UDF_MAX_INDIR_EXTS) {
>  			udf_err(inode->i_sb,
> @@ -2290,7 +2303,7 @@ int udf_next_aext(struct inode *inode, struct extent_position *epos,
>  			return -EFSCORRUPTED;
>  		}
>  
> -		epos->block = *eloc;
> +		epos->block = tloc;
>  		epos->offset = sizeof(struct allocExtDesc);
>  		brelse(epos->bh);
>  		block = udf_get_lb_pblock(inode->i_sb, &epos->block, 0);
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR