* [PATCH 0/2] udf: fix hole append when File Tail exists @ 2021-01-09 22:40 Steve Magnani 2021-01-09 22:40 ` [PATCH 1/2] " Steve Magnani 2021-01-09 22:40 ` [PATCH 2/2] udf: fix File Tail reclaim following hole append Steve Magnani 0 siblings, 2 replies; 4+ messages in thread From: Steve Magnani @ 2021-01-09 22:40 UTC (permalink / raw) To: Jan Kara; +Cc: linux-kernel, Steven J . Magnani From: Steven J. Magnani <magnani@ieee.org> Fixes: fa33cdbf3ece ("udf: Fix incorrect final NOT_ALLOCATED (hole) extent length") The fix for incorrect final NOT_ALLOCATED (hole) extent length did not consider the possibility that an ALLOCATED_NOT_RECORDED extent ("File Tail") may follow the extents describing the file body. A ftruncate() that adds a hole to the end of such a file now causes the File Tail to grow into the block that follows it...even if that block is already in use. The block is not marked as allocated (in the space bitmap) as part of this process and so can later end up being double- allocated for some other use (i.e., an ICB or file/directory data). Other side-effects in this scenario include a failure to reclaim allocated File Tail blocks when the file is released, and associated misreporting of the number of recorded blocks in the file's Extended File Entry. The kernel does not give any indication of any of these issues. However, an attempt to read the file in Windows 10 fails with "The file or directory is corrupted and unreadable." The script below creates a toy UDF filesystem to illustrate the problem. The filesystem can be dd'd to a flash disk partition of the same size to observe how Windows handles the corruption. --- #!/bin/bash # Tool to illustrate / test fix for bugs in UDF driver # related to creation of an end-of-file hole. # Developed using mkudffs from udftools 2.2. # Terminology: # LSN == Logical Sector Number (media / volume relative) # LBN == Logical Block Number (UDF partition relative) TEST_UDFFS=/tmp/test.udf # Changing these may alter filesystem layout and/or invalidate the test UDFFS_SIZE=1M # --size argument to 'truncate' command UDF_BLOCKSIZE=512 PD_LSN=98 # Expected UDF Partition Descriptor location EFE_LSN=261 # Location of Extended File Entry under test SBD_LSN=257 # Location of Space Bitmap Descriptor require() { local APP_REALPATH=`which $1` local PACKAGE_NAME=${2:-$1} if [ -z "$APP_REALPATH" ] ; then echo This test requires $1. Please install the $PACKAGE_NAME package. exit 1 fi } # "Quiet" dd command ddq() { dd $* 2> /dev/null } # Extract an 8-bit unsigned value at a specified byte offset ($2) # of a specified LSN ($1). Hex format can be output by passing x for $3. # extract8() { local format=${3:-u} # Default to unsigned int local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq bs=1 skip=$2 count=1 | od -A none -t ${format}1` [ -z "$value" ] && value=0 # Fail in a sane manner echo -n $value } # Extract a 16-bit little-endian unsigned value at a specified byte offset ($2) # of a specified LSN ($1). Hex format can be output by passing x for $3. # extract16() { local format=${3:-u} # Default to unsigned int local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq bs=1 skip=$2 count=2 | od -A none -t ${format}2 --endian=little` [ -z "$value" ] && value=0 # Fail in a sane manner echo -n $value } # Extract a 32-bit little-endian unsigned value at a specified byte offset ($2) # of a specified LSN ($1). Hex format can be output by passing x for $3. # extract32() { local format=${3:-u} # Default to unsigned int local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq bs=1 skip=$2 count=4 | od -A none -t ${format}4 --endian=little` [ -z "$value" ] && value=0 # Fail in a sane manner echo -n $value } # Extract a 64-bit little-endian unsigned value at a specified byte offset ($2) # of a specified LSN ($1). Hex format can be output by passing x for $3. # extract64() { local format=${3:-u} # Default to unsigned int local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq bs=1 skip=$2 count=8 | od -A none -t ${format}8 --endian=little` [ -z "$value" ] && value=0 # Fail in a sane manner echo -n $value } read_extent_start_lbn() { local extent_lbn_offset=$((220 + $2 * 16)) extract32 $1 $extent_lbn_offset } # $1 == LSN of EFE # $2 == Extent index of interest read_extent_type() { local extent_type_offset=$((219 + $2 * 16)) local type_byte=`extract8 $1 $extent_type_offset` expr $type_byte / 64 } # $1 == LSN of EFE # $2 == Extent index of interest read_extent_len() { local extent_len_offset=$((216 + $2 * 16)) local extent_typelen=`extract32 $1 $extent_len_offset` local extent_type=`read_extent_type $1 $2` echo $(($extent_typelen & 0x3FFFFFFF)) } # $1 == LSN of EFE # $2 == Extent index of interest # $3 == Expected length field (including type) of extent - 8 lowercase hex digits verify_extent_typelen() { local extent_len_offset=$((216 + $2 * 16)) local found_extent_len=`extract32 $1 $extent_len_offset x` if [ $found_extent_len != $3 ] ; then echo FAILURE: expected extent[$2] type/length $3, but EFE has $found_extent_len exitcode=1 fi } # $1 = LSN # $2 = Expected tag ID value (decimal) - i.e., 266 for EFE require_tag_id() { local found_tag=`extract16 $1 0` if [ $found_tag -ne $2 ] ; then echo Expected tag $2 in LSN $1, found $found_tag. echo Unexpected test conditions - results must be verified manually exit 1 fi } gen_provoker_data() { openssl rand 1536 ddq if=/dev/zero bs=$UDF_BLOCKSIZE count=1 openssl rand 16384 ddq if=/dev/zero bs=2102 count=1 PROVOKER_RECORDED_BLOCKS=$(( (1536 + $UDF_BLOCKSIZE - 1) / $UDF_BLOCKSIZE )) PROVOKER_RECORDED_BLOCKS=$(( $PROVOKER_RECORDED_BLOCKS + ( 16384 + $UDF_BLOCKSIZE - 1 ) / $UDF_BLOCKSIZE )) PROVOKER_NUM_EXTENTS=4 } # $1 == loopback UDF filesystem to create make_test_udf() { rm -f $1 truncate --size=$UDFFS_SIZE $1 mkudffs --label=SPARSE --uid=$UID --blocksize=$UDF_BLOCKSIZE $1 > /dev/null mkdir -p /tmp/testudf.mnt echo -n Mounting test UDF filesystem... sudo mount $1 /tmp/testudf.mnt echo cp --sparse=always /tmp/provoker.$$ /tmp/testudf.mnt/provoker sync sudo umount /tmp/testudf.mnt rmdir /tmp/testudf.mnt echo Test UDF filesystem generated in $1. } ### MAIN require openssl require mkudffs udftools if [ -e $TEST_UDFFS ] ; then echo $TEST_UDFFS already exists - please dismount and remove it exit 1 fi gen_provoker_data > /tmp/provoker.$$ make_test_udf $TEST_UDFFS # Verify hardcoded LSNs involved in testing require_tag_id $PD_LSN 5 require_tag_id $SBD_LSN 264 require_tag_id $EFE_LSN 266 # Make sure EFE is for the created file PROVOKER_LEN=`ls -l /tmp/provoker.$$ | cut -d' ' -f5` EFE_FILE_INFO_LEN=`extract64 $EFE_LSN 56` rm -f /tmp/provoker.$$ if [ $PROVOKER_LEN -ne $EFE_FILE_INFO_LEN ] ; then echo Expected file info len $PROVOKER_LEN in EFE at LSN $EFE_LSN, found $EFE_FILE_INFO_LEN. echo Unexpected test conditions - results must be verified manually exit 1 fi EFE_ALLOC_DESC_LEN=`extract32 $EFE_LSN 212` EFE_NUM_EXTENTS=$(($EFE_ALLOC_DESC_LEN / 16)) if [ $EFE_NUM_EXTENTS -ne $PROVOKER_NUM_EXTENTS ] ; then echo Expected $PROVOKER_NUM_EXTENTS file extents, but EFE says $EFE_NUM_EXTENTS echo Unexpected test conditions - results must be verified manually exit 1 fi exitcode=0 EFE_RECORDED_BLOCKS=`extract64 $EFE_LSN 72` if [ $EFE_RECORDED_BLOCKS -ne $PROVOKER_RECORDED_BLOCKS ] ; then echo FAILURE: expected $PROVOKER_RECORDED_BLOCKS recorded blocks, but EFE says $EFE_RECORDED_BLOCKS exitcode=1 fi verify_extent_typelen $EFE_LSN 0 00000600 # Recorded, 3 blocks verify_extent_typelen $EFE_LSN 1 80000200 # Not allocated, 512 bytes verify_extent_typelen $EFE_LSN 2 00004000 # Recorded, 32 blocks verify_extent_typelen $EFE_LSN 3 80000836 # Not allocated, 2102 bytes if [ `read_extent_type $EFE_LSN 3` -ne 2 ] ; then # Verify that space bitmap considers last block of the final (allocated) extent used LAST_EXTENT_FIRST_LBN=`read_extent_start_lbn $EFE_LSN 3` LAST_EXTENT_BYTE_LEN=`read_extent_len $EFE_LSN 3` LAST_EXTENT_BLOCK_LEN=$(( ($LAST_EXTENT_BYTE_LEN + $UDF_BLOCKSIZE - 1) / $UDF_BLOCKSIZE )) LAST_EXTENT_LAST_LBN=$(($LAST_EXTENT_FIRST_LBN + $LAST_EXTENT_BLOCK_LEN - 1)) BITMAP_BYTE_INDEX=$(($LAST_EXTENT_LAST_LBN / 8)) LAST_LBN_MASK=$((1 << ($LAST_EXTENT_LAST_LBN - 8 * $BITMAP_BYTE_INDEX) )) SBD_BYTE_INDEX=$((24 + $BITMAP_BYTE_INDEX)) BITMAP_BYTE=`extract8 $SBD_LSN $SBD_BYTE_INDEX` if [ $(($BITMAP_BYTE & $LAST_LBN_MASK)) -ne 0 ] ; then echo FAILURE: Space bitmap indicates LBN $LAST_EXTENT_LAST_LBN is free, but it is in use exitcode=1 fi fi [ $exitcode -eq 0 ] && echo Test PASSED. exit $exitcode ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] udf: fix hole append when File Tail exists 2021-01-09 22:40 [PATCH 0/2] udf: fix hole append when File Tail exists Steve Magnani @ 2021-01-09 22:40 ` Steve Magnani 2021-01-14 17:02 ` Jan Kara 2021-01-09 22:40 ` [PATCH 2/2] udf: fix File Tail reclaim following hole append Steve Magnani 1 sibling, 1 reply; 4+ messages in thread From: Steve Magnani @ 2021-01-09 22:40 UTC (permalink / raw) To: Jan Kara; +Cc: linux-kernel, Steven J . Magnani From: Steven J. Magnani <magnani@ieee.org> When an ALLOCATED_NOT_RECORDED extent ("File Tail") follows the extents describing a file body, don't (improperly) try to make use of it as part of appending a hole to the file. Fixes: fa33cdbf3ece ("udf: Fix incorrect final NOT_ALLOCATED (hole) extent length") Signed-off-by: Steven J. Magnani <magnani@ieee.org> --- --- a/fs/udf/inode.c 2020-12-28 20:51:29.000000000 -0600 +++ b/fs/udf/inode.c 2021-01-02 17:00:39.794157840 -0600 @@ -520,8 +520,7 @@ static int udf_do_extend_file(struct ino prealloc_loc = last_ext->extLocation; prealloc_len = last_ext->extLength; /* Mark the extent as a hole */ - last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED | - (last_ext->extLength & UDF_EXTENT_LENGTH_MASK); + last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED; last_ext->extLocation.logicalBlockNum = 0; last_ext->extLocation.partitionReferenceNum = 0; } @@ -626,7 +625,6 @@ static void udf_do_extend_final_block(st static int udf_extend_file(struct inode *inode, loff_t newsize) { - struct extent_position epos; struct kernel_lb_addr eloc; uint32_t elen; @@ -639,6 +637,7 @@ static int udf_extend_file(struct inode struct kernel_long_ad extent; int err = 0; int within_final_block; + loff_t hole_size = 0; if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT) adsize = sizeof(struct short_ad); @@ -648,7 +647,8 @@ static int udf_extend_file(struct inode BUG(); etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset); - within_final_block = (etype != -1); + within_final_block = (etype == (EXT_RECORDED_ALLOCATED >> 30)) || + (etype == (EXT_NOT_RECORDED_NOT_ALLOCATED >> 30)); if ((!epos.bh && epos.offset == udf_file_entry_alloc_offset(inode)) || (epos.bh && epos.offset == sizeof(struct allocExtDesc))) { @@ -658,9 +658,15 @@ static int udf_extend_file(struct inode extent.extLocation.partitionReferenceNum = 0; extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED; } else { + int8_t bmap_etype = etype; epos.offset -= adsize; etype = udf_next_aext(inode, &epos, &extent.extLocation, &extent.extLength, 0); + if ((bmap_etype == -1) && + (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30))) { + /* offset is relative to prealloc extent end */ + hole_size = extent.extLength; + } extent.extLength |= etype << 30; } @@ -674,9 +680,9 @@ static int udf_extend_file(struct inode udf_do_extend_final_block(inode, &epos, &extent, partial_final_block); } else { - loff_t add = ((loff_t)offset << sb->s_blocksize_bits) | + hole_size += ((loff_t)offset << sb->s_blocksize_bits) | partial_final_block; - err = udf_do_extend_file(inode, &epos, &extent, add); + err = udf_do_extend_file(inode, &epos, &extent, hole_size); } if (err < 0) @@ -700,7 +706,7 @@ static sector_t inode_getblk(struct inod loff_t lbcount = 0, b_off = 0; udf_pblk_t newblocknum, newblock; sector_t offset = 0; - int8_t etype; + int8_t etype = -1; struct udf_inode_info *iinfo = UDF_I(inode); udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum; int lastblock = 0; @@ -729,14 +735,22 @@ static sector_t inode_getblk(struct inod cur_epos.bh = next_epos.bh; } - lbcount += elen; - prev_epos.block = cur_epos.block; cur_epos.block = next_epos.block; prev_epos.offset = cur_epos.offset; cur_epos.offset = next_epos.offset; + /* Corner case: preallocated extent that stops short of + * desired block + */ + if ((etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) && + ((lbcount + elen) <= b_off)) { + etype = -1; + break; + } + + lbcount += elen; etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1); if (etype == -1) break; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] udf: fix hole append when File Tail exists 2021-01-09 22:40 ` [PATCH 1/2] " Steve Magnani @ 2021-01-14 17:02 ` Jan Kara 0 siblings, 0 replies; 4+ messages in thread From: Jan Kara @ 2021-01-14 17:02 UTC (permalink / raw) To: Steve Magnani; +Cc: Jan Kara, linux-kernel On Sat 09-01-21 16:40:53, Steve Magnani wrote: > From: Steven J. Magnani <magnani@ieee.org> > > When an ALLOCATED_NOT_RECORDED extent ("File Tail") follows the > extents describing a file body, don't (improperly) try to make use of it > as part of appending a hole to the file. > > Fixes: fa33cdbf3ece ("udf: Fix incorrect final NOT_ALLOCATED (hole) extent length") > Signed-off-by: Steven J. Magnani <magnani@ieee.org> Thanks for the fix! One comment below. > --- a/fs/udf/inode.c 2020-12-28 20:51:29.000000000 -0600 > +++ b/fs/udf/inode.c 2021-01-02 17:00:39.794157840 -0600 > @@ -520,8 +520,7 @@ static int udf_do_extend_file(struct ino > prealloc_loc = last_ext->extLocation; > prealloc_len = last_ext->extLength; > /* Mark the extent as a hole */ > - last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED | > - (last_ext->extLength & UDF_EXTENT_LENGTH_MASK); > + last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED; > last_ext->extLocation.logicalBlockNum = 0; > last_ext->extLocation.partitionReferenceNum = 0; > } > @@ -626,7 +625,6 @@ static void udf_do_extend_final_block(st > > static int udf_extend_file(struct inode *inode, loff_t newsize) > { > - > struct extent_position epos; > struct kernel_lb_addr eloc; > uint32_t elen; > @@ -639,6 +637,7 @@ static int udf_extend_file(struct inode > struct kernel_long_ad extent; > int err = 0; > int within_final_block; > + loff_t hole_size = 0; > > if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT) > adsize = sizeof(struct short_ad); > @@ -648,7 +647,8 @@ static int udf_extend_file(struct inode > BUG(); > > etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset); > - within_final_block = (etype != -1); > + within_final_block = (etype == (EXT_RECORDED_ALLOCATED >> 30)) || > + (etype == (EXT_NOT_RECORDED_NOT_ALLOCATED >> 30)); > > if ((!epos.bh && epos.offset == udf_file_entry_alloc_offset(inode)) || > (epos.bh && epos.offset == sizeof(struct allocExtDesc))) { > @@ -658,9 +658,15 @@ static int udf_extend_file(struct inode > extent.extLocation.partitionReferenceNum = 0; > extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED; > } else { > + int8_t bmap_etype = etype; > epos.offset -= adsize; > etype = udf_next_aext(inode, &epos, &extent.extLocation, > &extent.extLength, 0); > + if ((bmap_etype == -1) && > + (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30))) { > + /* offset is relative to prealloc extent end */ > + hole_size = extent.extLength; Rather than introducing 'hole_size', I think we can just update 'offset' here? > + } > extent.extLength |= etype << 30; > } > ... > @@ -729,14 +735,22 @@ static sector_t inode_getblk(struct inod > cur_epos.bh = next_epos.bh; > } > > - lbcount += elen; > - > prev_epos.block = cur_epos.block; > cur_epos.block = next_epos.block; > > prev_epos.offset = cur_epos.offset; > cur_epos.offset = next_epos.offset; > > + /* Corner case: preallocated extent that stops short of > + * desired block > + */ > + if ((etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) && > + ((lbcount + elen) <= b_off)) { > + etype = -1; > + break; > + } > + > + lbcount += elen; Honestly, I don't like this special case and it seems rather arbitrary. Also any EXT_NOT_RECORDED_ALLOCATED inside the file would now break the block lookup while previously it was gracefully handled. Also I'm not sure why it's event needed - even if inode_getblk() returns returns -1 and the 'extent' loaded by udf_next_aext() ends up being EXT_NOT_RECORDED_ALLOCATED, we will end up passing it to udf_do_extend_file() which recognizes it as preallocation extent and will insert a hole extent before it? Am I missing something? Honza > etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1); > if (etype == -1) > break; > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] udf: fix File Tail reclaim following hole append 2021-01-09 22:40 [PATCH 0/2] udf: fix hole append when File Tail exists Steve Magnani 2021-01-09 22:40 ` [PATCH 1/2] " Steve Magnani @ 2021-01-09 22:40 ` Steve Magnani 1 sibling, 0 replies; 4+ messages in thread From: Steve Magnani @ 2021-01-09 22:40 UTC (permalink / raw) To: Jan Kara; +Cc: linux-kernel, Steven J . Magnani From: Steven J. Magnani <magnani@ieee.org> Adjust bookkeeping during creation of an end-of-file hole to ensure that any File Tail (preallocated extent) is reclaimed when the file is released. This also ensures that the file's Extended File Entry is populated with the proper count of recorded blocks. Fixes: fa33cdbf3ece ("udf: Fix incorrect final NOT_ALLOCATED (hole) extent length") Signed-off-by: Steven J. Magnani <magnani@ieee.org> --- --- a/fs/udf/inode.c 2020-12-28 20:51:29.000000000 -0600 +++ b/fs/udf/inode.c 2021-01-03 07:04:05.759911829 -0600 @@ -535,6 +535,7 @@ static int udf_do_extend_file(struct ino add = new_block_bytes; new_block_bytes -= add; last_ext->extLength += add; + iinfo->i_lenExtents += add; } if (fake) { @@ -571,6 +572,7 @@ static int udf_do_extend_file(struct ino last_ext->extLength, 1); if (err) return err; + iinfo->i_lenExtents += add; count++; } if (new_block_bytes) { @@ -580,6 +582,7 @@ static int udf_do_extend_file(struct ino last_ext->extLength, 1); if (err) return err; + iinfo->i_lenExtents += new_block_bytes; count++; } @@ -682,7 +685,6 @@ static int udf_extend_file(struct inode if (err < 0) goto out; err = 0; - iinfo->i_lenExtents = newsize; out: brelse(epos.bh); return err; ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-14 17:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-09 22:40 [PATCH 0/2] udf: fix hole append when File Tail exists Steve Magnani 2021-01-09 22:40 ` [PATCH 1/2] " Steve Magnani 2021-01-14 17:02 ` Jan Kara 2021-01-09 22:40 ` [PATCH 2/2] udf: fix File Tail reclaim following hole append Steve Magnani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).