from:"Steve Magnani"

Re: [PATCH] udf: fix the problem that the disc content is not displayed

2021-01-13 Thread Steve Magnani


On 2021-01-13 20:51, 常廉志 wrote:


On 2021-01-11 23:53, lianzhi chang wrote:


When the capacity of the disc is too large (assuming the 4.7G
specification), the disc (UDF file system) will be burned
multiple times in the windows (Multisession Usage). When the
remaining capacity of the CD is less than 300M (estimated
value, for reference only), open the CD in the Linux system,
the content of the CD is displayed as blank (the kernel will
say "No VRS found"). Windows can display the contents of the
CD normally.
Through analysis, in the "fs/udf/super.c": udf_check_vsd
function, the actual value of VSD_MAX_SECTOR_OFFSET may
be much larger than 0x80. According to the current code

l>ogic, it is found that the type of sbi->s_session is "__s32",

when the remaining capacity of the disc is less than 300M
(take a set of test values: sector=3154903040,
sbi->s_session=1540464, sb->s_blocksize_bits=11 ), the
calculation result of "sbi->s_session << sb->s_blocksize_bits"
will overflow. Therefore, it is necessary to convert the
type of s_session to "loff_t" (when udf_check_vsd starts,
assign a value to _sector, which is also converted in this
way), so that the result will not overflow, and then the
content of the disc can be displayed normally.

Signed-off-by: lianzhi chang 
---
fs/udf/super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 5bef3a68395d..6c3069cd1321 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -757,7 +757,7 @@ static int udf_check_vsd(struct super_block *sb)

if (nsr > 0)
return 1;
- else if (!bh && sector - (sbi->s_session << sb->s_blocksize_bits) 
==

+ else if (!bh && sector - ((loff_t)sbi->s_session <<
sb->s_blocksize_bits) ==
VSD_FIRST_SECTOR_OFFSET)
return -1;
else



Looks good. Perhaps consider factoring out the conversion (which also
occurs
earlier in the function) so that the complexity of this "else if" can 
be

reduced?




Reviewed-by: Steven J. Magnani 


Thank you very much! So, which one of the following methods do you 
think is better:


(1) Change the type of s_session in struct udf_sb_info to __s64. If you 
modify this way, it may involve some memory copy problems of the 
structure, and there are more modifications.


(2) Definition: loff_t sector_offset=sbi->s_session << 
sb->s_blocksize_bits, and then put sector_offset into the "else if" 
statement.


(3) Or is there any other better way?


I had #2 in mind.

 Steven J. Magnani   "I claim this network for MARS!
  Earthling, return my space modulator!"
 #include

Re: [PATCH] udf: fix the problem that the disc content is not displayed

2021-01-13 Thread Steve Magnani


On 2021-01-11 23:53, lianzhi chang wrote:

When the capacity of the disc is too large (assuming the 4.7G
specification), the disc (UDF file system) will be burned
multiple times in the windows (Multisession Usage). When the
remaining capacity of the CD is less than 300M (estimated
value, for reference only), open the CD in the Linux system,
the content of the CD is displayed as blank (the kernel will
say "No VRS found"). Windows can display the contents of the
CD normally.
Through analysis, in the "fs/udf/super.c": udf_check_vsd
function, the actual value of VSD_MAX_SECTOR_OFFSET may
be much larger than 0x80. According to the current code
logic, it is found that the type of sbi->s_session is "__s32",
 when the remaining capacity of the disc is less than 300M
(take a set of test values: sector=3154903040,
sbi->s_session=1540464, sb->s_blocksize_bits=11 ), the
calculation result of "sbi->s_session << sb->s_blocksize_bits"
 will overflow. Therefore, it is necessary to convert the
type of s_session to "loff_t" (when udf_check_vsd starts,
assign a value to _sector, which is also converted in this
way), so that the result will not overflow, and then the
content of the disc can be displayed normally.

Signed-off-by: lianzhi chang 
---
 fs/udf/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 5bef3a68395d..6c3069cd1321 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -757,7 +757,7 @@ static int udf_check_vsd(struct super_block *sb)

if (nsr > 0)
return 1;
-   else if (!bh && sector - (sbi->s_session << sb->s_blocksize_bits) ==
+	else if (!bh && sector - ((loff_t)sbi->s_session << 
sb->s_blocksize_bits) ==

VSD_FIRST_SECTOR_OFFSET)
return -1;
else



Looks good. Perhaps consider factoring out the conversion (which also 
occurs
earlier in the function) so that the complexity of this "else if" can be 
reduced?


Reviewed-by: Steven J. Magnani 

 Steven J. Magnani   "I claim this network for MARS!
  Earthling, return my space modulator!"

 #include

[PATCH 2/2] udf: fix File Tail reclaim following hole append

2021-01-09 Thread Steve Magnani

From: Steven J. Magnani 

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 
---
--- a/fs/udf/inode.c2020-12-28 20:51:29.0 -0600
+++ b/fs/udf/inode.c2021-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;

[PATCH 0/2] udf: fix hole append when File Tail exists

2021-01-09 Thread Steve Magnani

From: Steven J. Magnani 

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 & 0x3FFF))
}


# $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

[PATCH 1/2] udf: fix hole append when File Tail exists

2021-01-09 Thread Steve Magnani

From: Steven J. Magnani 

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 
---
--- a/fs/udf/inode.c2020-12-28 20:51:29.0 -0600
+++ b/fs/udf/inode.c2021-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, , , , );
-   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, , ,
  , 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, , ,
  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, , , add);
+   err = udf_do_extend_file(inode, , , 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, _epos, , , 1);
if (etype == -1)
break;

[PATCH 1/1] udf: fix silent AED tagLocation corruption

2021-01-07 Thread Steve Magnani

From: Steven J. Magnani 

When extending a file, make sure that the pointer to the last written
extent does not get adjusted into the header (tag) portion of an
Allocation Extent Descriptor. Otherwise, a subsequent call to
udf_update_exents() can clobber the AED's tagLocation field, replacing
it with the logical block number of a file extent.

Signed-off-by: Steven J. Magnani 
---
--- a/fs/udf/inode.c2020-12-28 20:51:29.0 -0600
+++ b/fs/udf/inode.c2021-01-01 19:20:37.163767576 -0600
@@ -547,11 +547,14 @@ static int udf_do_extend_file(struct ino
 
udf_write_aext(inode, last_pos, _ext->extLocation,
last_ext->extLength, 1);
-   /*
-* We've rewritten the last extent but there may be empty
-* indirect extent after it - enter it.
-*/
-   udf_next_aext(inode, last_pos, , , 0);
+
+   if (new_block_bytes || prealloc_len) {
+   /*
+* We've rewritten the last extent but there may be 
empty
+* indirect extent after it - enter it.
+*/
+   udf_next_aext(inode, last_pos, , , 0);
+   }
}
 
/* Managed to do everything necessary? */

[PATCH 0/1] udf: fix silent AED tagLocation corruption

2021-01-07 Thread Steve Magnani

From: Steven J. Magnani 

Certain scenarios involving enlargement of a large and/or fragmented file
result in corruption of the file's Allocation Extent Descriptor tag.
Once this has happened, attempts to read the file in Windows 10 fail with
"Data error (cyclic redundancy check)". A "chkdsk /f" in Windows causes
extents of the file described by the corrupted AED (and any subsequent
chained AEDs) to be truncated.

No problems are noted when the file is accessed in Linux.

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 bug in UDF driver
# that can result in corruption of Allocation Extent Descriptor tag location
# 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
AED_LSN=1343 # Location of Allocation Extent Descriptor under test

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 a 16-bit little-endian unsigned value at a specified byte offset ($2)
# of a specified LSN ($1)
# 
extract16()
{
local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq 
bs=1 skip=$2 count=2 | od -A none -t u2 --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)
# 
extract32()
{
local value=`ddq if=$TEST_UDFFS bs=$UDF_BLOCKSIZE skip=$1 count=1 | ddq 
bs=1 skip=$2 count=4 | od -A none -t u4 --endian=little`
[ -z "$value" ] && value=0   # Fail in a sane manner

echo -n $value
}


# $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()
{
ddq if=/dev/zero bs=$UDF_BLOCKSIZE count=1 of=/tmp/hole.$$

openssl rand 172032
cat /tmp/hole.$$
openssl rand 149504
cat /tmp/hole.$$
openssl rand 22528
cat /tmp/hole.$$
openssl rand 2048
cat /tmp/hole.$$
openssl rand 4096
cat /tmp/hole.$$
openssl rand 4096
cat /tmp/hole.$$
openssl rand 8192
cat /tmp/hole.$$
openssl rand 184320
cat /tmp/hole.$$
openssl rand 131072

rm -f /tmp/hole.$$
}

# $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 Mounting test UDF filesystem. Please provide the root password if 
prompted.
sudo mount $1 /tmp/testudf.mnt
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  # PD
require_tag_id $AED_LSN 258  # AED

LBN_BASE=`extract32 $PD_LSN 188`   # Partition Starting Location
AED_LBN=`expr $AED_LSN - $LBN_BASE`
AED_TAG_LOCATION=`extract32 $AED_LSN 12`

if [ $AED_TAG_LOCATION -ne $AED_LBN ] ; then
echo Test FAILED: expected AED tag location $AED_LBN, actual is 
$AED_TAG_LOCATION
exit 1
else
echo Test PASSED. AED tag location field is correct.
fi

 Steven J. Magnani   "I claim this network for MARS!
  Earthling, return my space modulator!"
 #include

Re: [PATCH v2] udf: reduce leakage of blocks related to named streams

2019-08-19 Thread Steve Magnani


Jan -


On 8/15/19 7:42 AM, Jan Kara wrote:

On Wed 14-08-19 07:50:02,  Steven J. Magnani  wrote:

Windows is capable of creating UDF files having named streams.
One example is the "Zone.Identifier" stream attached automatically
to files downloaded from a network. See:
   https://msdn.microsoft.com/en-us/library/dn392609.aspx

Modification of a file having one or more named streams in Linux causes
the stream directory to become detached from the file, essentially leaking
all blocks pertaining to the file's streams.

Fix by saving off information about an inode's streams when reading it,
for later use when its on-disk data is updated.

} else {
inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
(inode->i_sb->s_blocksize_bits - 9);
@@ -1498,6 +1502,16 @@ reread:
iinfo->i_lenEAttr = le32_to_cpu(efe->lengthExtendedAttr);
iinfo->i_lenAlloc = le32_to_cpu(efe->lengthAllocDescs);
iinfo->i_checkpoint = le32_to_cpu(efe->checkpoint);
+
+   /* Named streams */
+   iinfo->i_streamdir = (efe->streamDirectoryICB.extLength != 0);
+   iinfo->i_locStreamdir =
+   lelb_to_cpu(efe->streamDirectoryICB.extLocation);
+   iinfo->i_lenStreams = le64_to_cpu(efe->objectSize);
+   if (iinfo->i_lenStreams >= inode->i_size)
+   iinfo->i_lenStreams -= inode->i_size;
+   else
+   iinfo->i_lenStreams = 0;

Hum, maybe you could just have i_objectSize instead of i_lenStreams? You
use the field just to preserve objectSize anyway so there's no point in
complicating it.



I started making this change and found that it actually complicates things more,
by forcing the driver to update i_objectSize everywhere that i_size is changed.
Are you sure this is what you want?


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH] udf: reduce leakage of blocks related to named streams

2019-08-09 Thread Steve Magnani


Jan -

Thanks for the feedback.

On 8/9/19 8:05 AM, Jan Kara wrote:

On Wed 07-08-19 08:32:58,  Steven J. Magnani  wrote:

From: Steve Magnani 

Windows is capable of creating UDF files having named streams.
One example is the "Zone.Identifier" stream attached automatically
to files downloaded from a network. See:
   https://msdn.microsoft.com/en-us/library/dn392609.aspx

Modification of a file having one or more named streams in Linux causes
the stream directory to become detached from the file, essentially leaking
all blocks pertaining to the file's streams. Worse, an attempt to delete
the file causes its directory entry (FID) to be deleted, but because the
driver believes that a hard link to the file remains, the Extended File
Entry (EFE) and all extents of the file itself remain allocated. Since
there is no hard link, after the FID has been deleted all of these blocks
are unreachable (leaked).

...

For this case, this partial solution reduces the number of blocks leaked
during file deletion to just one (the EFE containing the stream data).



Thanks for the patch! I was thinking about this and rather than this
partial fix, I'd prefer to fail the last unlink of an inode with
a named-stream directory with EOPNOTSUPP. Later we can properly handle this
and walk the named-stream directory and remove all associated EFEs for the
named streams. After all named-stream directories are restricted to not
have any subdirectories, hardlinks, or anything similarly fancy so the walk
should not be *that* hard to implement.


Maybe not but it's more work than I am able to take on anytime soon.
Absent a complete solution, how to handle this is a judgement call.
Since Windows seems to attach a Zone.Identifier stream to _all_ files
downloaded from a network, and since interchange between Windows and Linux
via USB Mass Storage is a somewhat common (or at least desirable) use case
for UDF, this issue seemed fairly serious from a user perspective. Leaking
all the blocks of a file on delete is pretty bad. Leaking a single block is
still bad, but much less so. One could argue that prohibiting deletion of
files with named streams is nearly as bad as leaking all the blocks - in
both cases, within Linux, none of the file's blocks can be reused.
That's pretty limiting for users. It's too limiting for my use cases.


Steven J. Magnani   "I claim this network for MARS!
www.digidescorp.com  Earthling, return my space modulator!"

#include

Re: [PATCH] udf: prevent allocation beyond UDF partition

2019-07-31 Thread Steve Magnani


On 7/31/19 4:59 AM, Jan Kara wrote:

On Sun 28-07-19 14:19:12, Steve Magnani wrote:

The UDF bitmap allocation code assumes that a recorded
Unallocated Space Bitmap is compliant with ECMA-167 4/13,
which requires that pad bytes between the end of the bitmap
and the end of a logical block are all zero.

When a recorded bitmap does not comply with this requirement,
for example one padded with FF to the block boundary instead
of 00, the allocator may "allocate" blocks that are outside
the UDF partition extent. This can result in UDF volume descriptors
being overwritten by file data or by partition-level descriptors,
and in extreme cases, even in scribbling on a subsequent disk partition.

Add a check that the block selected by the allocator actually
resides within the UDF partition extent.

Signed-off-by: Steven J. Magnani 

Thanks for the patch! Added to my tree. I've just slightly modified the
patch to also output error message about filesystem corruption.

Honza



Thanks Jan. Ror the record, it appears that Windows chkdsk has a bug in its
analysis of a space bitmaps. If the last block of a UDF partition falls
in the middle of a bitmap byte, chkdsk reports spurious errors if the bits
in that byte that _don't_ correspond to UDF partition blocks are zero.

To maximize interoperability it would appear that it's best to format such
that UDF partition sizes are always a multiple of 8 blocks.

Note to non-UDF wonks reading this, a UDF partition is a sub-extent of a
disk partition. So achieving the multiple-of-8-blocks involves a change to
mkudffs code.


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[PATCH] udf: prevent allocation beyond UDF partition

2019-07-28 Thread Steve Magnani

The UDF bitmap allocation code assumes that a recorded 
Unallocated Space Bitmap is compliant with ECMA-167 4/13,
which requires that pad bytes between the end of the bitmap 
and the end of a logical block are all zero.

When a recorded bitmap does not comply with this requirement,
for example one padded with FF to the block boundary instead
of 00, the allocator may "allocate" blocks that are outside
the UDF partition extent. This can result in UDF volume descriptors
being overwritten by file data or by partition-level descriptors,
and in extreme cases, even in scribbling on a subsequent disk partition.

Add a check that the block selected by the allocator actually
resides within the UDF partition extent.

Signed-off-by: Steven J. Magnani 

--- a/fs/udf/balloc.c   2019-07-26 11:35:28.249563705 -0500
+++ b/fs/udf/balloc.c   2019-07-28 13:11:25.061431597 -0500
@@ -325,6 +325,13 @@ got_block:
newblock = bit + (block_group << (sb->s_blocksize_bits + 3)) -
(sizeof(struct spaceBitmapDesc) << 3);
 
+   if (newblock >= sbi->s_partmaps[partition].s_partition_len) {
+   /* Ran off the end of the bitmap,
+* and bits following are non-compliant (not all zero)
+*/
+   goto error_return;
+   }
+
if (!udf_clear_bit(bit, bh->b_data)) {
udf_debug("bit already cleared for block %d\n", bit);
goto repeat;

Re: UDF filesystem image with Write-Once UDF Access Type

2019-07-26 Thread Steve Magnani


Hi,

On 7/12/19 5:02 AM, Pali Rohár wrote:

In my opinion without support for additional layer, kernel should treat
UDF Write-Once Access Type as read-only mount for userspace. And not
classic read/write mount.

...

It seems that udf.ko does not support updating VAT table, so probably it
should treat also filesystem with VAT as read-only too.



I thinkb085fbe2ef7fa7489903c45271ae7b7a52b0f9ab  
,
 deployed in 4.20,
does both of the things you want.

One case I ran across today that Windows handles, but Linux doesn't,
is write-protection via flags in the DomainIdentifier fields of the
Logical Volume Descriptor and File Set Descriptor. Linux allows
RW mount when those are marked protected, but Windows forces RO mount.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH v2 2/2] udf: support 2048-byte spacing of VRS descriptors on 4K media

2019-07-11 Thread Steve Magnani


On 7/11/19 10:04 AM, Jan Kara wrote:

Thanks for the patches! I've added them to my tree and somewhat simplified
the logic since we don't really care about nsr 2 vs 3 or whether we
actually saw BEA or not. Everything seems to work fine for me but I'd
appreciate if you could doublecheck - the result is pushed out to

git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git for_next


Tested-by: Steven J. Magnani 

The rework is more permissive than what you had suggested initially
(conditioning acceptance of a noncompliant NSR on a preceding BEA).
I had also tried to code the original so that a malformed 2048-byte
interval VRS would not be accepted. But the simplifications do make
the code easier to follow...

Thanks,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [RFC] udf: 2.01 interoperability issues with Windows 10

2019-07-10 Thread Steve Magnani




On 7/9/19 4:04 PM, Pali Rohár wrote:

On Tuesday 09 July 2019 15:14:38 Steve Magnani wrote:

On 7/9/19 1:56 PM, Pali Rohár wrote:

Can grub2 recognize such disks?

I'm not sure what you're asking here. The physical interface to this drive is 
USB,

It is USB mass storage device? If yes, then grub2 should be able to
normally use. Read its content, etc... You can use "ls" grub command to
list content of disk with supported filesystem.


Yes, Mass Storage Bulk-Only Transport.




and it's not designed for general-purpose storage (or booting). That said, if 
you
have some grub2 commands you want me to run against this drive/partition let me 
know.

There is also some way for using grub's fs implementation to read disk
images. It is primary used by grub's automated tests. I do not know
right now how to use, I need to look grub documentation. But I have
already used it during implementation of UDF UUID in grub.


Grub is not recognizing my USB drive, i.e. 'ls' does not show usb0 as an option.
I tried 'insmod usb' but that made no difference. Maybe grub does not support my
USB 3.0 host controller, I will retry on a USB2 port when I have a chance.


Also can you check if libparted from git master branch can recognize
such disk? In following commit I added support for recognizing UDF
filesystem in libparted, it is only in git master branch, not released:

http://git.savannah.gnu.org/cgit/parted.git/commit/?id=8740cfcff3ea839dd6dc8650dec0a466e9870625

Build failed:
   In file included from pt-tools.c:114:0:
   pt-tools.c: In function 'pt_limit_lookup':
   pt-limit.gperf:78:1: error: function might be candidate for attribute 'pure' 
[-Werror=suggest-attribute=pure]

If you send me some other SHA to try I can attempt a rebuild.

Try to use top of master branch. That mentioned commit is already in git
master.

And if it still produce that error, compile without -Werror flag (or add
-Wno-error).


I had to configure with CFLAGS=-Wno-error.

It does not recognize Windows-formatted 4K-sector media:
  Disk /dev/sdb: 17.6TB
  Sector size (logical/physical): 4096B/4096B
  Partition Table: msdos
  Disk Flags:

  Number  Start   End SizeType File system  Flags
   1  1049kB  17.6TB  17.6TB  primary


It does recognize mkudffs-formatted media.




ISSUE 2: Inability of Windows chkdsk to analyze 4K-sector media
   formatted by mkudffs.
This is really bad :-(


It would be possible to work around this by tweaking mkudffs to
insert dummy BOOT2 components in between the BEA/NSR/TEA:

: 00 42 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .BEA01..
0800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00  .BOOT2..
1000: 00 4e 53 52 30 33 01 00 00 00 00 00 00 00 00 00  .NSR03..
1800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00  .BOOT2..
2000: 00 54 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .TEA01..

That would introduce a slight ECMA-167 nonconformity, but Linux
does not object and Windows actually performs better. I would
have to tweak udffsck though since I believe this could confuse
its automatic detection of medium block size.

I would like to avoid this hack. If chkdsk is unable to detect such
filesystem, it is really a good idea to let it do try doing filesystem
checks and recovery? You are saying that udfs.sys can recognize such
disk and mount it. I think this should be enough.

Fair enough, but it's also reasonable to assume the bugginess is
limited to the VRS corner case. AFAIK that's the only place in ECMA-167
where there is a difference in layout specific to 4K sectors.
With the BOOT2 band-aid chkdsk is able to analyze filesystems on 4Kn media.

Main problem with this hack is that it breaks detection of valid UDF
filesystems which use VRS for block size detection. I do not know which
implementation may use VRS for block size detection, but I do not see
anything wrong in this approach.


I went through this with udffsck. The VRS is not very helpful in
determining block size because the only time the block size can be
determined conclusively is when the interval between VRS components
is > 2048 bytes. With an interval of 2048 bytes, the only conclusion
that can be drawn is that blocks are no larger than 2048 bytes.


I use chkdsk frequently to double-check UDF generation firmware

Vojtěch wrote in his thesis that MS's chkdsk sometimes put UDF
filesystem into more broken state as before.


Yes, I have personally experienced this. I don't have chkdsk do
repairs any more. In my case the problem may be that chkdsk
poorly handles the cascading corruption that resulted from this:

https://lkml.org/lkml/2019/2/8/740


I am writing, and also udffsck work-in-progress.

Have you used some Vojtěch's parts? Or are you writing it from scratch?


A udffsck discussion should probably continue in another thread.
Here let me just say that I have been enhancing Vojtěch's code,
in this fork:

  https://github.com/smagnani/udftools

...as

Re: [RFC] udf: 2.01 interoperability issues with Windows 10

2019-07-09 Thread Steve Magnani

On 7/9/19 1:56 PM, Pali Rohár wrote:

Steve, can you describe what you mean by Advanced Format 4K sector size?

It is hard disk with 512 bytes logical sector size and 4096 bytes
physical sector size? Or do you mean hard disk which has both logical
and physical sector size equal to 4096 bytes?

Sorry, forgot that the term Advanced Format introduces ambiguity.
As far as the OSes are concerned the drive is 4Kn.

On Tuesday 09 July 2019 13:27:58 Steve Magnani wrote:

Hi,

Recently I have been exploring Advanced Format (4K sector size)
and high capacity aspects of UDF 2.01 support in Linux and
Windows 10. I thought it might be helpful to summarize my findings.

The good news is that I did not see any bugs in the Linux
ecosystem (kernel driver + mkudffs).

The not-so-good news is that Windows has some issues that affect
interoperability. One of my goals in posting this is to open a
discussion on whether changes should be made in the Linux UDF
ecosystem to accommodate these quirks.

My test setup includes the following software components:

* mkudffs 1.3 and 2.0

Can you do tests also with last version of mkudffs 2.1?

A very quick smoke test of a 16-ish TiB 4Kn partition seemed OK.

* kernel 4.15.0-43 and 4.15.0-52
* Windows 10 1803 17134.829
* chkdsk 10.0.17134.1
* udfs.sys 10.0.17134.648

ISSUE 1: Inability of the Linux UDF driver to mount 4K-sector
media formatted by Windows.

Can you check if udfinfo (from udftools) can recognize such disk?

It cannot:

$ ./udfinfo /dev/sdb1
udfinfo: Error: UDF Volume Recognition Sequence found but not Anchor Volume
Descriptor Pointer, maybe wrong --blocksize?
udfinfo: Error: Cannot process device '/dev/sdb1' as UDF disk

$ ./udfinfo --blocksize=4096 /dev/sdb1
udfinfo: Error: UDF Volume Recognition Sequence not found
udfinfo: Error: Cannot process device '/dev/sdb1' as UDF disk

$ ./udfinfo
udfinfo from udftools 2.1

And can blkid (from util-linux) recognize such disk as UDF with reading
all properties?

Seemingly:

$ blkid --info /dev/sdb1
/dev/sdb1: MINIMUM_IO_SIZE="4096"
PHYSICAL_SECTOR_SIZE="4096"
LOGICAL_SECTOR_SIZE="4096"

$ blkid --probe /dev/sdb1
/dev/sdb1: VOLUME_ID="UDF Volume"
UUID="0e131b3b20554446"
VOLUME_SET_ID="0E131B3B UDF Volume Set"
LABEL="WIN10_FORMATTED"
LOGICAL_VOLUME_ID="WIN10_FORMATTED"
VERSION="2.01"
TYPE="udf"
USAGE="filesystem"

$ blkid --version
blkid from util-linux 2.31.1 (libblkid 2.31.1, 19-Dec-2017)

Can grub2 recognize such disks?

I'm not sure what you're asking here. The physical interface to this drive is
USB,
and it's not designed for general-purpose storage (or booting). That said, if
you
have some grub2 commands you want me to run against this drive/partition let me
know.

Also can you check if libparted from git master branch can recognize
such disk? In following commit I added support for recognizing UDF
filesystem in libparted, it is only in git master branch, not released:

http://git.savannah.gnu.org/cgit/parted.git/commit/?id=8740cfcff3ea839dd6dc8650dec0a466e9870625

Build failed:
In file included from pt-tools.c:114:0:
pt-tools.c: In function 'pt_limit_lookup':
pt-limit.gperf:78:1: error: function might be candidate for attribute 'pure'
[-Werror=suggest-attribute=pure]

If you send me some other SHA to try I can attempt a rebuild.

ISSUE 2: Inability of Windows chkdsk to analyze 4K-sector media
formatted by mkudffs.
This is really bad :-(

It would be possible to work around this by tweaking mkudffs to
insert dummy BOOT2 components in between the BEA/NSR/TEA:

: 00 42 45 41 30 31 01 00 00 00 00 00 00 00 00 00 .BEA01..
0800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00 .BOOT2..
1000: 00 4e 53 52 30 33 01 00 00 00 00 00 00 00 00 00 .NSR03..
1800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00 .BOOT2..
2000: 00 54 45 41 30 31 01 00 00 00 00 00 00 00 00 00 .TEA01..

That would introduce a slight ECMA-167 nonconformity, but Linux
does not object and Windows actually performs better. I would
have to tweak udffsck though since I believe this could confuse
its automatic detection of medium block size.

I would like to avoid this hack. If chkdsk is unable to detect such
filesystem, it is really a good idea to let it do try doing filesystem
checks and recovery? You are saying that udfs.sys can recognize such
disk and mount it. I think this should be enough.

Fair enough, but it's also reasonable to assume the bugginess is
limited to the VRS corner case. AFAIK that's the only place in ECMA-167
where there is a difference in layout specific to 4K sectors.
With the BOOT2 band-aid chkdsk is able to analyze filesystems on 4Kn media.

[RFC] udf: 2.01 interoperability issues with Windows 10

2019-07-09 Thread Steve Magnani


Hi,

Recently I have been exploring Advanced Format (4K sector size)
and high capacity aspects of UDF 2.01 support in Linux and
Windows 10. I thought it might be helpful to summarize my findings.

The good news is that I did not see any bugs in the Linux
ecosystem (kernel driver + mkudffs).

The not-so-good news is that Windows has some issues that affect
interoperability. One of my goals in posting this is to open a
discussion on whether changes should be made in the Linux UDF
ecosystem to accommodate these quirks.

My test setup includes the following software components:

* mkudffs 1.3 and 2.0
* kernel 4.15.0-43 and 4.15.0-52
* Windows 10 1803 17134.829
* chkdsk 10.0.17134.1
* udfs.sys 10.0.17134.648


ISSUE 1: Inability of the Linux UDF driver to mount 4K-sector
 media formatted by Windows.

This is because the Windows ecosystem mishandles the ECMA-167
corner case that requires Volume Recognition Sequence components
to be placed at 4K intervals on 4K-sector media, instead of the
2K intervals required with smaller sectors. The Linux UDF driver
emits the following when presented with Windows-formatted media:

  UDF-fs: warning (device sdc1): udf_load_vrs: No VRS found
  UDF-fs: Scanning with blocksize 4096 failed

A hex dump of the VRS written by the Windows 10 'format' utility
yields this:

  : 00 42 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .BEA01..
  0800: 00 4e 53 52 30 33 01 00 00 00 00 00 00 00 00 00  .NSR03..
  1000: 00 54 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .TEA01..

We may want to consider tweaking the kernel UDF driver to
tolerate this quirk; if so a question is whether that should be
done automatically, only in response to a mount option or
module parameter, or only with some subsequent confirmation
that the medium was formatted by Windows.


ISSUE 2: Inability of Windows chkdsk to analyze 4K-sector media
 formatted by mkudffs.

This is another aspect of Windows' VRS corner case bug.
Formatting by mkudffs places the VRS components at the proper 4K
intervals. But the chkdsk utility looks for components at 2K
intervals. Not finding a component at byte offset 2048, chkdsk
decides that the media is RAW and cannot be checked. Note that
this bug affects chkdsk only; udfs.sys *does* recognize mkudffs-
formatted 4K-sector media and is able to mount it.

It would be possible to work around this by tweaking mkudffs to
insert dummy BOOT2 components in between the BEA/NSR/TEA:

  : 00 42 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .BEA01..
  0800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00  .BOOT2..
  1000: 00 4e 53 52 30 33 01 00 00 00 00 00 00 00 00 00  .NSR03..
  1800: 00 42 4f 4f 54 32 01 00 00 00 00 00 00 00 00 00  .BOOT2..
  2000: 00 54 45 41 30 31 01 00 00 00 00 00 00 00 00 00  .TEA01..

That would introduce a slight ECMA-167 nonconformity, but Linux
does not object and Windows actually performs better. I would
have to tweak udffsck though since I believe this could confuse
its automatic detection of medium block size.


ISSUE 3: Inability of the Windows UDF driver to mount media
 read-write when a maximally-sized space bitmap
 descriptor is present

I suspect this is an off-by-one error in udfs.sys relating to
the maximum number of blocks a space bitmap descriptor can
occupy. The bug causes UDF partitions that are close to 2 TiB
(512-sector media) or 16 TiB (4K-sector media) to be mounted
read-only, with no user-visible indication as to why.

It would be possible for mkudffs to print a warning when
formatting results in a space bitmap that occupies the maximum
number of blocks.


ISSUE 4: chkdsk reports spurious errors when space bitmap
 descriptor exceeds 32 MiB

Some permutations of this:

  * "Correcting errors in Space Bitmap Descriptor at block 0"
(with no prior mention of any errors)
  * "Space Bitmap Descriptor at block 32 is corrupt or unreadable"

This is actually one of the more crippling issues if one values
Windows' ability to check and repair UDF errors. A limit of
32 MiB on the space bitmap implies a UDF partition size of at
most 137 GB (not GiB) with 512-sector media or at most 1099 GB
with 4K-sector media.

Again, the most I think we could do is code mkudffs to warn of
this possibility. But a message that clearly conveys the issue
and what should be done to avoid it could be a little tricky to
construct.


Obviously the best solution would be for the Windows bugs to
get fixed. If anyone reading this can convey these details into
the Microsoft silo, that would be great.

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH 1/1] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length

2019-06-26 Thread Steve Magnani


Hi Jan,

On 6/25/19 5:30 AM, Jan Kara wrote:

On Tue 04-06-19 07:31:58, Steve Magnani wrote:

In some cases, using the 'truncate' command to extend a UDF file results
in a mismatch between the length of the file's extents (specifically, due
to incorrect length of the final NOT_ALLOCATED extent) and the information
(file) length. The discrepancy can prevent other operating systems
(i.e., Windows 10) from opening the file.

Two particular errors have been observed when extending a file:

1. The final extent is larger than it should be, having been rounded up
to a multiple of the block size.

B. The final extent is not shorter than it should be, due to not having
been updated when the file's information length was increased.

The first case could represent a design error, if coded intentionally
due to a misinterpretation of scantily-documented ECMA-167 "file tail"
rules. The standard specifies that the tail, if present, consists of
a sequence of "unrecorded and allocated" extents (only).

Signed-off-by: Steven J. Magnani 

Thanks for the testcase and the patch! I finally got to reading through
this in detail. In udf driver in Linux we are generally fine with the last
extent being rounded up to the block size. udf_truncate_tail_extent() is
generally responsible for truncating the last extent to appropriate size
once we are done with the inode. However there are two problems with this:

1) We used to do this inside udf_clear_inode() back in the old days but
then switched to a different scheme in commit 2c948b3f86e5f "udf: Avoid IO
in udf_clear_inode". So this actually breaks workloads where user calls
truncate(2) directly and there's no place where udf_truncate_tail_extent()
gets called.

2) udf_extend_file() sets i_lenExtents == i_size although the last extent
isn't properly rounded so even if udf_truncate_tail_extent() gets called
(which is actually the case for truncate(1) which does open, ftruncate,
close), it will think it has nothing to do and exit.

Now 2) is easily fixed by setting i_lenExtents to real length of extents we
have created. However that still leaves problem 1) which isn't easy to deal
with. After some though I think that your solution of making
udf_do_extend_file() always create appropriately sized extents makes
sense. However I dislike the calling convention you've chosen. When
udf_do_extend_file() needs to now byte length, then why not pass it to it
directly, instead of somewhat cumbersome "sector length + byte offset"
pair?

Will you update the patch please? Thanks!


That sounds reasonable, but at first glance I think it might be more 
confusing. The API as I reworked it now communicates two different 
(although related) things - the number of blocks that need to be added, 
and the number of bytes within the last block that are part of the file. 
This is able to cover both the corner case of extending within the last 
file block and extending beyond that:


partial_final_block = newsize & (sb->s_blocksize - 1);

/* File has extent covering the new size (could happen when extending
 * inside a block)? */
if (etype == -1) {
if (partial_final_block)
offset++;
} else {
/* Extending file within the last file block */
offset = 0;  /* Don't add any new blocks */
}

If it were as simple as passing to udf_do_extend_file() a loff_t 
specifying the number of bytes to add, including both full blocks and a 
final partial block, I would agree with you. But this isn't enough 
information for udf_do_extend_file() to know whether the final partial 
block requires a new block or not.


I will think about it some more. Maybe moving the 'extending within the 
last file block' case out to udf_extend_file() would help.


Steve

Re: [PATCH 1/1] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length

2019-06-19 Thread Steve Magnani


On 6/19/19 1:47 AM, Jan Kara wrote:

Hi Steve!

On Sun 16-06-19 11:28:46, Steve Magnani wrote:

On 6/4/19 7:31 AM, Steve Magnani wrote:


In some cases, using the 'truncate' command to extend a UDF file results
in a mismatch between the length of the file's extents (specifically, due
to incorrect length of the final NOT_ALLOCATED extent) and the information
(file) length. The discrepancy can prevent other operating systems
(i.e., Windows 10) from opening the file.

Two particular errors have been observed when extending a file:

1. The final extent is larger than it should be, having been rounded up
 to a multiple of the block size.

B. The final extent is shorter than it should be, due to not having
 been updated when the file's information length was increased.

Wondering if you've seen this, or if something got lost in a spam folder.

Sorry for not getting to you earlier. I've seen the patches and they look
reasonable to me. I just wanted to have a one more closer look but last
weeks were rather busy so I didn't get to it. I'll look into it this week.
Thanks a lot for debugging the problem and sending the fixes!

Honza


No worries. If you're short on time I'd suggest looking first at the ways
udf_do_extend_file() can be called via inode_getblk(). Those were harder for
me to follow so if there is a bug it's most likely in one of those paths.

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH 1/1] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length

2019-06-16 Thread Steve Magnani


Hi Jan,

On 6/4/19 7:31 AM, Steve Magnani wrote:


In some cases, using the 'truncate' command to extend a UDF file results
in a mismatch between the length of the file's extents (specifically, due
to incorrect length of the final NOT_ALLOCATED extent) and the information
(file) length. The discrepancy can prevent other operating systems
(i.e., Windows 10) from opening the file.

Two particular errors have been observed when extending a file:

1. The final extent is larger than it should be, having been rounded up
to a multiple of the block size.

B. The final extent is shorter than it should be, due to not having
been updated when the file's information length was increased.


Wondering if you've seen this, or if something got lost in a spam folder.

Regards,


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH 1/1] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length

2019-06-04 Thread Steve Magnani


On 6/4/19 7:31 AM, Steve Magnani wrote:

B. The final extent is not shorter than it should be, due to not having


Oops: should have been

B. The final extent is shorter than it should be, due to not having

[PATCH 1/1] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length

2019-06-04 Thread Steve Magnani

In some cases, using the 'truncate' command to extend a UDF file results
in a mismatch between the length of the file's extents (specifically, due
to incorrect length of the final NOT_ALLOCATED extent) and the information
(file) length. The discrepancy can prevent other operating systems
(i.e., Windows 10) from opening the file.

Two particular errors have been observed when extending a file:

1. The final extent is larger than it should be, having been rounded up
   to a multiple of the block size.

B. The final extent is not shorter than it should be, due to not having
   been updated when the file's information length was increased.

The first case could represent a design error, if coded intentionally
due to a misinterpretation of scantily-documented ECMA-167 "file tail"
rules. The standard specifies that the tail, if present, consists of
a sequence of "unrecorded and allocated" extents (only).

Signed-off-by: Steven J. Magnani 

--- a/fs/udf/inode.c2019-05-24 21:17:33.659704533 -0500
+++ b/fs/udf/inode.c2019-05-29 20:32:23.730129419 -0500
@@ -474,7 +474,8 @@ static struct buffer_head *udf_getblk(st
 static int udf_do_extend_file(struct inode *inode,
  struct extent_position *last_pos,
  struct kernel_long_ad *last_ext,
- sector_t blocks)
+ sector_t blocks,
+ unsigned long partial_final_block)
 {
sector_t add;
int count = 0, fake = !(last_ext->extLength & UDF_EXTENT_LENGTH_MASK);
@@ -486,7 +487,7 @@ static int udf_do_extend_file(struct ino
 
/* The previous extent is fake and we should not extend by anything
 * - there's nothing to do... */
-   if (!blocks && fake)
+   if (!blocks && !partial_final_block && fake)
return 0;
 
iinfo = UDF_I(inode);
@@ -524,6 +525,10 @@ static int udf_do_extend_file(struct ino
add = blocks;
blocks -= add;
last_ext->extLength += add << sb->s_blocksize_bits;
+   if (blocks == 0 && partial_final_block) {
+   last_ext->extLength -= sb->s_blocksize
+   - partial_final_block;
+   }
}
 
if (fake) {
@@ -566,6 +571,10 @@ static int udf_do_extend_file(struct ino
if (blocks) {
last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED |
(blocks << sb->s_blocksize_bits);
+   if (partial_final_block) {
+   last_ext->extLength -= sb->s_blocksize
+   - partial_final_block;
+   }
err = udf_add_aext(inode, last_pos, _ext->extLocation,
   last_ext->extLength, 1);
if (err)
@@ -605,6 +614,7 @@ static int udf_extend_file(struct inode
int8_t etype;
struct super_block *sb = inode->i_sb;
sector_t first_block = newsize >> sb->s_blocksize_bits, offset;
+   unsigned long partial_final_block;
int adsize;
struct udf_inode_info *iinfo = UDF_I(inode);
struct kernel_long_ad extent;
@@ -619,15 +629,17 @@ static int udf_extend_file(struct inode
 
etype = inode_bmap(inode, first_block, , , , );
 
+   partial_final_block = newsize & (sb->s_blocksize - 1);
+
/* File has extent covering the new size (could happen when extending
 * inside a block)? */
-   if (etype != -1)
-   return 0;
-   if (newsize & (sb->s_blocksize - 1))
-   offset++;
-   /* Extended file just to the boundary of the last file block? */
-   if (offset == 0)
-   return 0;
+   if (etype == -1) {
+   if (partial_final_block)
+   offset++;
+   } else {
+   /* Extending file within the last file block */
+   offset = 0;  /* Don't add any new blocks */
+   }
 
/* Truncate is extending the file by 'offset' blocks */
if ((!epos.bh && epos.offset == udf_file_entry_alloc_offset(inode)) ||
@@ -643,7 +655,8 @@ static int udf_extend_file(struct inode
  , 0);
extent.extLength |= etype << 30;
}
-   err = udf_do_extend_file(inode, , , offset);
+   err = udf_do_extend_file(inode, , , offset,
+partial_final_block);
if (err < 0)
goto out;
err = 0;
@@ -760,7 +773,7 @@ static sector_t inode_getblk(struct inod
startnum = (offset > 0);
}
/* Create extents for the hole between EOF and offset */
-   ret = udf_do_extend_file(inode, _epos, laarr, offset);
+   ret = udf_do_extend_file(inode, _epos, laarr, offset, 0);
if (ret < 0) {
*err = ret;
newblock

[PATCH 0/1] udf: Incorrect final NOT_ALLOCATED (hole) extent length

2019-06-04 Thread Steve Magnani

The following script reveals some errors in the final
NOT_RECORDED_NOT_ALLOCATED extent of a file following use of truncate(1)
to extend the file by adding or manipulating a hole at the end.

The script produces the following output:

 Now testing NOT ALLOCATED extent.
 Testing0 -->  300 : FAILED - bad extent type/length: expected 812C, 
actual 8200
 Testing  300 -->  301 : PASSED
 Testing  301 -->  302 : FAILED - bad extent type/length: expected 812E, 
actual 812D
 Testing  302 -->  511 : FAILED - bad extent type/length: expected 81FF, 
actual 812D
 Testing  511 -->  512 : FAILED - bad extent type/length: expected 8200, 
actual 812D
 Testing  512 -->  513 : FAILED - bad extent type/length: expected 8201, 
actual 8400
 Testing  513 -->  514 : PASSED
 Testing  514 --> 1023 : FAILED - bad extent type/length: expected 83FF, 
actual 8202
 Testing 1023 --> 1024 : FAILED - bad extent type/length: expected 8400, 
actual 8202
 Testing 1024 --> 1026 : FAILED - bad extent type/length: expected 8402, 
actual 8600
 Testing 1026 --> 1538 : FAILED - bad extent type/length: expected 8602, 
actual 8800
 Testing 1538 --> 4096 : PASSED
 Testing 4096 -->0 : PASSED
 Testing0 --> 4096 : PASSED
 Testing 4096 -->0 : PASSED
 Testing0 --> 4097 : FAILED - bad extent type/length: expected 80001001, 
actual 80001200
 Testing 4097 -->0 : PASSED

 Now testing RECORDED extent.
 Testing  512 -->  512 : PASSED
 Testing  512 -->  511 : PASSED
 Testing  511 -->  300 : PASSED
 Testing  300 -->  512 : FAILED - bad extent type/length: expected 0200, 
actual 012C

 Now testing NOT ALLOCATED beyond RECORDED.
 Testing  512 -->  513 : FAILED - bad extent type/length: expected 0200 
8001, actual 0200 8200
 Testing  513 -->  512 : PASSED
 Testing  512 -->  300 : PASSED
 Testing  300 -->  513 : FAILED - bad extent type/length: expected 0200 
8001, actual 0200 8200
 Testing  513 -->  300 : PASSED
 Testing  300 --> 1538 : FAILED - bad extent type/length: expected 0200 
8402, actual 0200 8600
 Testing 1538 -->0 : PASSED

 Now testing multiple NOT ALLOCATED.
 Testing0 --> 1073741312 : PASSED
 Testing 1073741312 -->0 : PASSED
 Testing0 --> 1073741313 : FAILED - bad extent type/length: expected 
BE00 8001, actual BE00 8200
 Testing 1073741313 -->0 : PASSED
 Testing0 --> 1073741824 : PASSED

#!/bin/bash

FS_SIZE=256K
FS_FILE=/tmp/test.udf
MNT=/mnt
ICB_LSN=261
XXD=/usr/bin/xxd

truncate_test()
{
local prev_size=`ls -l ${MNT}/truncate.test | cut -d' ' -f5`
printf "Testing %4u --> %4u : " $prev_size $1 
truncate --size=$1 ${MNT}/truncate.test
sync
local new_size=`ls -l ${MNT}/truncate.test | cut -d' ' -f5`

if [ $new_size -ne $1 ] ; then
echo FAILED - bad information length
else
local ext_type_and_len=`dd if=${FS_FILE} skip=${ICB_LSN} count=1 2> 
/dev/null | dd bs=1 skip=216 count=4 2> /dev/null | ${XXD} -g4 -e -u | cut 
-c11-18`
if [ "$ext_type_and_len" = "$2" ] ; then
if [ -z "$3" ] ; then
echo PASSED
else
ext_type_and_len=`dd if=${FS_FILE} skip=${ICB_LSN} count=1 2> 
/dev/null | dd bs=1 skip=232 count=4 2> /dev/null | ${XXD} -g4 -e -u | cut 
-c11-18`
if [ "$ext_type_and_len" = "$3" ] ; then
echo PASSED
else
echo FAILED - bad extent type/length: expected $2 $3, 
actual $2 $ext_type_and_len
fi
fi
else
echo FAILED - bad extent type/length: expected $2, actual 
$ext_type_and_len
fi
fi
}


### MAIN

rm -f $FS_FILE

truncate --size=${FS_SIZE} $FS_FILE
mkudffs --label=TRUNCATE --media-type=hd --uid=1000 --gid=1000 $FS_FILE > 
/dev/null
echo -n Mounting test filesystem...
sudo mount $FS_FILE $MNT -o loop
echo
touch ${MNT}/truncate.test

echo
echo Now testing NOT ALLOCATED extent.
truncate_test 300  812C
truncate_test 301  812D
truncate_test 302  812E
truncate_test 511  81FF
truncate_test 512  8200
truncate_test 513  8201
truncate_test 514  8202
truncate_test 1023 83FF
truncate_test 1024 8400
truncate_test 1026 8402
truncate_test 1538 8602
truncate_test 4096 80001000
truncate_test 0
truncate_test 4096 80001000
truncate_test 0
truncate_test 4097 80001001
truncate_test 0

dd if=/dev/zero of=${MNT}/truncate.test bs=512 count=1 2> /dev/null
echo
echo Now testing RECORDED extent.
truncate_test 512  0200
truncate_test 511  01FF
truncate_test 300  012C
truncate_test 512  0200
echo
echo Now testing NOT ALLOCATED beyond RECORDED.
truncate_test 513  0200 8001
truncate_test 512  0200 
truncate_test 300  012C
truncate_test 513  0200 8001
truncate_test 300  012C 
truncate_test 1538 0200 8402
truncate_test 0

Re: Possible UDF locking error?

2019-03-30 Thread Steve Magnani


Jan -

On 3/25/19 11:42 AM, Jan Kara wrote:

Hi!

On Sat 23-03-19 15:14:05, Steve Magnani wrote:

I have been hunting a UDF bug that occasionally results in generation
of an Allocation Extent Descriptor with an incorrect tagLocation. So
far I haven't been able to see a path through the code that could
cause that. But, I noticed some inconsistency in locking during
AED generation and wonder if it could result in random corruption.

The function udf_update_inode() has this general pattern:

   bh = udf_tgetblk(...);   // calls sb_getblk()
   lock_buffer(bh);
   memset(bh->b_data, 0, inode->i_sb->s_blocksize);
   // other code to populate FE/EFE data in the block
   set_buffer_uptodate(bh);
   unlock_buffer(bh);
   mark_buffer_dirty(bh);

This I can understand - the lock is held for as long as the buffer
contents are being assembled.

In contrast, udf_setup_indirect_aext(), which constructs an AED,
has this sequence:

   bh = udf_tgetblk(...);   // calls sb_getblk()
   lock_buffer(bh);
   memset(bh->b_data, 0, inode->i_sb->s_blocksize);

   set_buffer_uptodate(bh);
   unlock_buffer(bh);
   mark_buffer_dirty_inode(bh);

   // other code to populate AED data in the block

In this case the population of the block occurs without
the protection of the lock.

Because the block has been marked dirty, does this mean that
writeback could occur at any point during population?

Yes. Thanks for noticing this!


There is one path through udf_setup_indirect_aext() where
mark_buffer_dirty_inode() gets called again after population is
complete, which I suppose could heal a partial writeout, but there is
also another path in which the buffer does not get marked dirty again.

Generally, we add new extents to the created indirect extent which dirties
the buffer and that should fix the problem. But you are definitely right
that the code is suspicious and should be fixed. Will you send a patch?


I did a little archaeology to see how the code evolved to this point. 
It's been like this a long time.


I also did some research to understand why filesystems use lock_buffer() 
sometimes but not others. For example, the FAT driver never calls it. I 
ran across this thread from 2011:


   https://lkml.org/lkml/2011/5/16/402

...from which I conclude that while it is correct in a strict sense to 
hold a lock on a buffer any time its contents are being modified, 
performance considerations make it preferable (or at least reasonable) 
to make some modifications without a lock provided it's known that a 
subsequent write-out will "fix" any potential partial write out before 
anyone else tries to read the block. I doubt that UDF sees common use 
with DIF/DIX block devices, which might make a decision in favor of 
performance a little easier. Since the FAT driver doesn't contain 
Darrick's proposed changes I assume a decision was made that performance 
was more important there.


Certainly the call to udf_setup_indirect_aext() from udf_add_aext() 
meets those criteria. But udf_table_free_blocks() may not dirty the AED 
block.


So if this looks reasonable I will resend as a formal patch:

--- a/fs/udf/inode.c2019-03-30 11:28:38.637759458 -0500
+++ b/fs/udf/inode.c2019-03-30 11:33:00.357761250 -0500
@@ -1873,9 +1873,6 @@ int udf_setup_indirect_aext(struct inode
return -EIO;
lock_buffer(bh);
memset(bh->b_data, 0x00, sb->s_blocksize);
-   set_buffer_uptodate(bh);
-   unlock_buffer(bh);
-   mark_buffer_dirty_inode(bh, inode);
 
 	aed = (struct allocExtDesc *)(bh->b_data);

if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT)) {
@@ -1890,6 +1887,9 @@ int udf_setup_indirect_aext(struct inode
udf_new_tag(bh->b_data, TAG_IDENT_AED, ver, 1, block,
sizeof(struct tag));
 
+	set_buffer_uptodate(bh);

+   unlock_buffer(bh);
+
nepos.block = neloc;
nepos.offset = sizeof(struct allocExtDesc);
nepos.bh = bh;
@@ -1913,6 +1913,8 @@ int udf_setup_indirect_aext(struct inode
} else {
__udf_add_aext(inode, epos, ,
   sb->s_blocksize | EXT_NEXT_EXTENT_ALLOCDECS, 0);
+   /* Make sure completed AED gets written out */
+   mark_buffer_dirty_inode(nepos.bh, inode);
}
 
 	brelse(epos->bh);



 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com   Earthling, return my space modulator!"

 #include

Re: Possible UDF locking error?

2019-03-25 Thread Steve Magnani


On 3/25/19 11:42 AM, Jan Kara wrote:

Hi!

On Sat 23-03-19 15:14:05, Steve Magnani wrote:

...

In contrast, udf_setup_indirect_aext(), which constructs an AED,
has this sequence:

   bh = udf_tgetblk(...);   // calls sb_getblk()
   lock_buffer(bh);
   memset(bh->b_data, 0, inode->i_sb->s_blocksize);

   set_buffer_uptodate(bh);
   unlock_buffer(bh);
   mark_buffer_dirty_inode(bh);

   // other code to populate AED data in the block

In this case the population of the block occurs without
the protection of the lock.

Because the block has been marked dirty, does this mean that
writeback could occur at any point during population?

Yes. Thanks for noticing this!


There is one path through udf_setup_indirect_aext() where
mark_buffer_dirty_inode() gets called again after population is
complete, which I suppose could heal a partial writeout, but there is
also another path in which the buffer does not get marked dirty again.

Generally, we add new extents to the created indirect extent which dirties
the buffer and that should fix the problem. But you are definitely right
that the code is suspicious and should be fixed. Will you send a patch?

Honza



Sure. There's at least one other place where it looked like there might 
be a similar issue.


Steve

Possible UDF locking error?

2019-03-23 Thread Steve Magnani


Hi,

I have been hunting a UDF bug that occasionally results in generation
of an Allocation Extent Descriptor with an incorrect tagLocation. So
far I haven't been able to see a path through the code that could
cause that. But, I noticed some inconsistency in locking during
AED generation and wonder if it could result in random corruption.

The function udf_update_inode() has this general pattern:

  bh = udf_tgetblk(...);   // calls sb_getblk()
  lock_buffer(bh);
  memset(bh->b_data, 0, inode->i_sb->s_blocksize);
  
  // other code to populate FE/EFE data in the block
  
  set_buffer_uptodate(bh);

  unlock_buffer(bh);
  mark_buffer_dirty(bh);

This I can understand - the lock is held for as long as the buffer
contents are being assembled.

In contrast, udf_setup_indirect_aext(), which constructs an AED,
has this sequence:

  bh = udf_tgetblk(...);   // calls sb_getblk()
  lock_buffer(bh);
  memset(bh->b_data, 0, inode->i_sb->s_blocksize);

  set_buffer_uptodate(bh);
  unlock_buffer(bh);
  mark_buffer_dirty_inode(bh);

  // other code to populate AED data in the block

In this case the population of the block occurs without
the protection of the lock.

Because the block has been marked dirty, does this mean that
writeback could occur at any point during population?

There is one path through udf_setup_indirect_aext() where
mark_buffer_dirty_inode() gets called again after population is
complete, which I suppose could heal a partial writeout, but there is
also another path in which the buffer does not get marked dirty again.

Regards,
 
 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[RFC] udftools: steps towards fsck

2019-03-06 Thread Steve Magnani

(Please remove at least LKML when responding. Mailing lists are a 
scattershot attempt to reach others who might be interested in this 
topic since I'm not aware of any linux-udf mailing list. )


A few months ago I stumbled across an interesting bit of abandonware in 
the Sourceforge CVS repo that hosted UDF development through about 2004. 
Code that originated here eventually became the modern-day udftools:


    https://sourceforge.net/p/linux-udf/code/

The 'udf' module in that repo contains a program from 1999 named 
'chkudf', which appears to have been written by Rob Simms. Being from 
the Y2K era, the program has no awareness of anything beyond UDF2.01; in 
particular, its comprehension of VAT reflects UDF1.50 and not the 
revamped design introduced in UDF2.00. But it does have an ability to 
analyze the major UDF data structures and to walk the filesystem.


I've spent quite a bit of time enhancing and fixing bugs in this code, 
with a short term goal of being able to report damage to UDF2.01 
filesystems on "hard disk" (magnetic and SSD) media. It's not quite to 
the point of being release-ready, but I think the code is on the cusp of 
becoming useful to others so I wanted to get some feedback on the approach.


I posted a GIT port (via SVN) of the CVS repo here, including all the 
changes I've made so far:


    https://github.com/smagnani/chkudf.git

If you're interested in building the code you should be able to just run 
'make' within the chkudf folder. On Debian-derived systems you'll need 
libblkid-dev installed in order to build.


Some questions for consideration:

* Would a udffsck limited to checking of UDF2.01 and earlier on "hard 
disk" media be a sufficiently useful starting point to justify inclusion 
in udftools? Obviously a tool with such limitations would have to be 
particularly vigilant about ensuring that media-under-test doesn't 
exceed its capabilities.


* If so, do you think the chkudf implementation could qualify? It's not 
ready yet, but with an investment of some time and energy it could be 
made more functionally complete and (maybe more importantly) more 
user-friendly.


In part this is a question of whether the chkudf design can support 
enhancements to get (eventually) to UDF2.60 and optical media support, 
balanced against the many years without an open-source udffsck and not 
"letting the perfect become the enemy of the good."


* For any standards-based parser it's important to have examples of as 
many variations as possible (both normal and pathological) in order to 
ensure that corner cases and less common features are tested properly. 
Can anyone point me to any good sources of UDF data for testing? There 
are always commercial DVDs and Blu-Ray discs, of course, and I've 
cobbled together a few special cases by hand (i.e., a filesystem with 
directory cycles), but I have no examples with extended attributes or 
stream data. If I could find a DVD of Mac software in a resale shop 
would that help? [Side note, I've thought of enhancing chkudf to support 
a tool that would store all the UDF structures of a filesystem in a 
tarball that could be used to reconstitute that filesystem within a 
sparse file. Since none of the file contents would be stored the 
tarballs would be relatively small even if they represent terabyte-scale 
filesystems.


* Are there versions (or features) of UDF that are less important to 
support than others (1.50? Strategy 4096? Named streams? etc.) I know 
1.02, 2.01, and 2.50 are in wide use.


* What kinds of repairs are most important to implement? I was thinking 
that regeneration of the Logical Volume Integrity Descriptor and the 
unallocated space bitmap are both important and hopefully relatively 
straightforward. Beyond that...recovering ICBs to "lost+found"?



My 2 cents:
I didn't write this program. There are things I would have done 
differently, but to this point I have tried to work within the existing 
design and code style. After becoming more aware of differences between 
the various UDF standards (in particular, the increase in complexity 
since 2.01) and the many errata involved, I have a gut feeling that an 
implementation in a language that supports inheritance might be a lot 
more manageable over the long term - but it's not something I've spent a 
lot of time thinking about. I've only recently become aware of 
UDFclient, and haven't had time to look over its design yet. And, I can 
see the potential for followon utilities such as a filesystem resizer - 
which might argue for making more of the code library-based and not so 
heavy on printed output.


Bottom line...udffsck has to start somewhere, could it start with chkudf?

Thanks for reading.

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH][udf-next] udf: don't call mark_buffer_dirty on a null bh pointer

2019-02-20 Thread Steve Magnani



> On Feb 20, 2019, at 3:50 AM, Jan Kara  wrote:
> 
>> On Tue 19-02-19 08:17:09, Steve Magnani wrote:
>>> On 2/19/19 8:02 AM, Jan Kara wrote:
>>>> On Tue 19-02-19 11:44:03, Colin King wrote:
>>>> From: Colin Ian King 
>>>> 
>>>> There is a null check on the pointer bh to avoid a null pointer dereference
>>>> on bh->b_data however later bh is passed to mark_buffer_dirty that can also
>>>> cause a null pointer dereference on bh.  Avoid this potential null pointer
>>>> dereference by moving the call to mark_buffer_dirty inside the null checked
>>>> block.
>>>> 
>>>> Fixes: e8b4274735e4 ("udf: finalize integrity descriptor before writeback")
>>>> Signed-off-by: Colin Ian King 
>>> Thanks for the patch! In fact it is the 'if (bh)' check that's
>>> unnecessarily defensive (we cannot have sbi->s_lvid_dirty and
>>> !sbi->s_lvid_bh). So I'll just drop that check (attached patch).
>>> 
>>>Honza
>>> 
>>>> ---
>>>>  fs/udf/super.c | 12 ++--
>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/fs/udf/super.c b/fs/udf/super.c
>>>> index a6940d90bedd..b7e9a83d39db 100644
>>>> --- a/fs/udf/super.c
>>>> +++ b/fs/udf/super.c
>>>> @@ -2336,13 +2336,13 @@ static int udf_sync_fs(struct super_block *sb, int 
>>>> wait)
>>>>  lvid = (struct logicalVolIntegrityDesc *)bh->b_data;
>>>>  udf_finalize_lvid(lvid);
>>>> -}
>>>> -/*
>>>> - * Blockdevice will be synced later so we don't have to submit
>>>> - * the buffer for IO
>>>> - */
>>>> -mark_buffer_dirty(bh);
>>>> +/*
>>>> + * Blockdevice will be synced later so we don't have
>>>> + * to submit the buffer for IO
>>>> + */
>>>> +mark_buffer_dirty(bh);
>>>> +}
>>>>  sbi->s_lvid_dirty = 0;
>>>>  }
>>>>  mutex_unlock(>s_alloc_mutex);
>>>> -- 
>>>> 2.20.1
>>>> 
>> Reviewed-by: Steven J. Magnani 
> 
> Is this Reviewed-by for my fixup or the Colin's? Because I've decided to
> rather remove the 'if (bh)' check completely since it is pointless...
> 
>Honza
> -- 

Sorry, I realized on rereading that this could be ambiguous. The R-B is for 
your patch.

Steve

Re: [PATCH][udf-next] udf: don't call mark_buffer_dirty on a null bh pointer

2019-02-19 Thread Steve Magnani


On 2/19/19 8:02 AM, Jan Kara wrote:

On Tue 19-02-19 11:44:03, Colin King wrote:

From: Colin Ian King 

There is a null check on the pointer bh to avoid a null pointer dereference
on bh->b_data however later bh is passed to mark_buffer_dirty that can also
cause a null pointer dereference on bh.  Avoid this potential null pointer
dereference by moving the call to mark_buffer_dirty inside the null checked
block.

Fixes: e8b4274735e4 ("udf: finalize integrity descriptor before writeback")
Signed-off-by: Colin Ian King 

Thanks for the patch! In fact it is the 'if (bh)' check that's
unnecessarily defensive (we cannot have sbi->s_lvid_dirty and
!sbi->s_lvid_bh). So I'll just drop that check (attached patch).

Honza


---
  fs/udf/super.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index a6940d90bedd..b7e9a83d39db 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2336,13 +2336,13 @@ static int udf_sync_fs(struct super_block *sb, int wait)
  
  			lvid = (struct logicalVolIntegrityDesc *)bh->b_data;

udf_finalize_lvid(lvid);
-   }
  
-		/*

-* Blockdevice will be synced later so we don't have to submit
-* the buffer for IO
-*/
-   mark_buffer_dirty(bh);
+   /*
+* Blockdevice will be synced later so we don't have
+* to submit the buffer for IO
+*/
+   mark_buffer_dirty(bh);
+   }
sbi->s_lvid_dirty = 0;
}
mutex_unlock(>s_alloc_mutex);
--
2.20.1


Reviewed-by: Steven J. Magnani 

Doh! Thanks for the catch Colin.


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[PATCH] udf: disallow RW mount without valid integrity descriptor

2019-02-11 Thread Steve Magnani

Refuse to mount a volume read-write without a coherent Logical Volume
Integrity Descriptor, because we can't generate truly unique IDs without 
one.

This fixes a bug where all inodes created on a UDF filesystem following
mount without a coherent LVID are assigned UID 0.

Signed-off-by: Steven J. Magnani 
---
--- a/fs/udf/super.c2018-11-16 06:43:00.622344354 -0600
+++ b/fs/udf/super.c2019-02-11 08:08:00.478331631 -0600
@@ -1474,6 +1474,17 @@ static int udf_load_logicalvol(struct su
if (lvd->integritySeqExt.extLength)
udf_load_logicalvolint(sb, leea_to_cpu(lvd->integritySeqExt));
ret = 0;
+
+   if (!sbi->s_lvid_bh) {
+   /* We can't generate UIDs without a valid LVID */
+   if (sb_rdonly(sb))
+   UDF_SET_FLAG(sb, UDF_FLAG_RW_INCOMPAT);
+   else {
+   udf_warn(sb, "Damaged or missing LVID, forcing "
+"readonly mount\n");
+   ret = -EACCES;
+   }
+   }
 out_bh:
brelse(bh);
return ret;

Re: [PATCH 0/2] udf: Fix corrupt on-disk integrity descriptor following sync()

2019-02-08 Thread Steve Magnani


On 2/8/19 11:34 AM, Steve Magnani wrote:

In cases where the next unique ID or file/directory count of the
in-core Logical Volume Integrity Descriptor have been updated,
a sync() causes a (corrupt) LVID with a stale CRC to be written to disk.

Ordinarily, this is corrected by an update of the on-disk LVID that occurs
when the filesystem is unmounted. However, if power is lost or the
filesystem resides on a device that is surprise-removed, the corrupt LVID
remains and can complicate later remounting or fsck/chkdsk.


"Complicate later remounting" turns out to be an understatement.
Actually it seems that this one event can trigger more silent corruption
of the filesystem on future remounts.

The driver will mount without complaint a filesystem that lacks a valid LVID.
But in this scenario, every time lvid_get_unique_id() is called to generate
an ID for a new inode or link, it returns zero. It appears that callers
happily assign this zero to all new inodes or links, with no indication to
the user or in the syslog that a problem is brewing.

I lost the entire contents of a flash disk to what was probably an
instance of this kind of corruption, when I told Windows chkdsk to
repair it.

I'll try to solve that in a separate followon patchset.
The simplest solution I can think of is to force read-only mount when
no LVID is available.

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[PATCH 1/2] udf: factor out LVID finalization for reuse

2019-02-08 Thread Steve Magnani

Centralize timestamping and CRC/checksum updating of the in-core
Logical Volume Integrity Descriptor, in preparation for adding
a third site where this functionality is needed.

Signed-off-by: Steven J. Magnani 
---
--- a/fs/udf/super.c2019-02-08 10:30:00.397978609 -0600
+++ b/fs/udf/super.c2019-02-08 10:39:14.462529517 -0600
@@ -1771,6 +1771,17 @@ static int udf_load_vrs(struct super_blo
return 1;
 }
 
+static void udf_finalize_lvid(struct logicalVolIntegrityDesc *lvid)
+{
+   udf_time_to_disk_stamp(>recordingDateAndTime,
+   CURRENT_TIME);
+
+   lvid->descTag.descCRC = cpu_to_le16(
+   crc_itu_t(0, (char *)lvid + sizeof(struct tag),
+   le16_to_cpu(lvid->descTag.descCRCLength)));
+   lvid->descTag.tagChecksum = udf_tag_checksum(>descTag);
+}
+
 static void udf_open_lvid(struct super_block *sb)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
@@ -1787,15 +1798,9 @@ static void udf_open_lvid(struct super_b
 
lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
-   udf_time_to_disk_stamp(>recordingDateAndTime,
-   CURRENT_TIME);
lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
 
-   lvid->descTag.descCRC = cpu_to_le16(
-   crc_itu_t(0, (char *)lvid + sizeof(struct tag),
-   le16_to_cpu(lvid->descTag.descCRCLength)));
-
-   lvid->descTag.tagChecksum = udf_tag_checksum(>descTag);
+   udf_finalize_lvid(lvid);
mark_buffer_dirty(bh);
sbi->s_lvid_dirty = 0;
mutex_unlock(>s_alloc_mutex);
@@ -1816,7 +1821,6 @@ static void udf_close_lvid(struct super_
lvidiu = udf_sb_lvidiu(sbi);
lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
-   udf_time_to_disk_stamp(>recordingDateAndTime, CURRENT_TIME);
if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
@@ -1825,11 +1829,7 @@ static void udf_close_lvid(struct super_
lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
 
-   lvid->descTag.descCRC = cpu_to_le16(
-   crc_itu_t(0, (char *)lvid + sizeof(struct tag),
-   le16_to_cpu(lvid->descTag.descCRCLength)));
-
-   lvid->descTag.tagChecksum = udf_tag_checksum(>descTag);
+   udf_finalize_lvid(lvid);
mark_buffer_dirty(bh);
sbi->s_lvid_dirty = 0;
mutex_unlock(>s_alloc_mutex);

[PATCH 2/2] udf: finalize integrity descriptor before writeback

2019-02-08 Thread Steve Magnani

Make sure the CRC and tag checksum of the Logical Volume Integrity
Descriptor are valid before the structure is written out to disk.
Otherwise, unless the filesystem is unmounted gracefully, the on-disk
LVID will be invalid - which is unnecessary filesystem damage.

Signed-off-by: Steven J. Magnani 
---
--- a/fs/udf/super.c2019-02-08 10:39:48.351039434 -0600
+++ b/fs/udf/super.c2019-02-08 10:42:26.017400020 -0600
@@ -1856,8 +1856,8 @@ u64 lvid_get_unique_id(struct super_bloc
if (!(++uniqueID & 0x))
uniqueID += 16;
lvhd->uniqueID = cpu_to_le64(uniqueID);
+   udf_updated_lvid(sb);
mutex_unlock(>s_alloc_mutex);
-   mark_buffer_dirty(bh);
 
return ret;
 }
@@ -2154,11 +2154,20 @@ static int udf_sync_fs(struct super_bloc
 
mutex_lock(>s_alloc_mutex);
if (sbi->s_lvid_dirty) {
+   struct buffer_head *bh = sbi->s_lvid_bh;
+
+   if (bh) {
+   struct logicalVolIntegrityDesc *lvid;
+
+   lvid = (struct logicalVolIntegrityDesc *)bh->b_data;
+   udf_finalize_lvid(lvid);
+   }
+
/*
 * Blockdevice will be synced later so we don't have to submit
 * the buffer for IO
 */
-   mark_buffer_dirty(sbi->s_lvid_bh);
+   mark_buffer_dirty(bh);
sb->s_dirt = 0;
sbi->s_lvid_dirty = 0;
}

[PATCH 0/2] udf: Fix corrupt on-disk integrity descriptor following sync()

2019-02-08 Thread Steve Magnani

In cases where the next unique ID or file/directory count of the
in-core Logical Volume Integrity Descriptor have been updated,
a sync() causes a (corrupt) LVID with a stale CRC to be written to disk.

Ordinarily, this is corrected by an update of the on-disk LVID that occurs
when the filesystem is unmounted. However, if power is lost or the
filesystem resides on a device that is surprise-removed, the corrupt LVID
remains and can complicate later remounting or fsck/chkdsk.

(Lack of) i_version handling in udf

2017-12-22 Thread Steve Magnani


Jan,

Re: [PATCH v4 00/19] fs: rework and optimize i_version handling in filesystems

On 12/22/2017 06:05 AM, Jeff Layton wrote:


The inode->i_version field is supposed to be a value that changes
whenever there is any data or metadata change to the inode. Some
filesystems use it internally to detect directory changes during
readdir. knfsd will use it if the filesystem has MS_I_VERSION set. IMA
will also use it to optimize away some remeasurement if it's available.
NFS and AFS just use it to store an opaque change attribute from the
server.

Only btrfs, ext4, and xfs increment it for data changes. Because of
this, these filesystems must log the inode to disk whenever the
i_version counter changes. That has a non-zero performance impact,
especially on write-heavy workloads, because we end up dirtying the
inode metadata on every write, not just when the times change.

It turns out though that none of these users of i_version require that
it change on every change to the file. The only real requirement is that
it be different if something changed since the last time we queried for
it.

If we keep track of when something queries the value, we can avoid
bumping the counter and an on-disk update when nothing else has changed
if no one has queried it since it was last incremented.


This happened to cross my radar, which made me think...is it a problem
that the UDF driver does not have any references to i_version at all?
I suppose R/W UDF is a small subset of the normal use cases, but the driver
does try to support it.

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

(Lack of) i_version handling in udf

2017-12-22 Thread Steve Magnani


Jan,

Re: [PATCH v4 00/19] fs: rework and optimize i_version handling in filesystems

On 12/22/2017 06:05 AM, Jeff Layton wrote:


The inode->i_version field is supposed to be a value that changes
whenever there is any data or metadata change to the inode. Some
filesystems use it internally to detect directory changes during
readdir. knfsd will use it if the filesystem has MS_I_VERSION set. IMA
will also use it to optimize away some remeasurement if it's available.
NFS and AFS just use it to store an opaque change attribute from the
server.

Only btrfs, ext4, and xfs increment it for data changes. Because of
this, these filesystems must log the inode to disk whenever the
i_version counter changes. That has a non-zero performance impact,
especially on write-heavy workloads, because we end up dirtying the
inode metadata on every write, not just when the times change.

It turns out though that none of these users of i_version require that
it change on every change to the file. The only real requirement is that
it be different if something changed since the last time we queried for
it.

If we keep track of when something queries the value, we can avoid
bumping the counter and an on-disk update when nothing else has changed
if no one has queried it since it was last incremented.


This happened to cross my radar, which made me think...is it a problem
that the UDF driver does not have any references to i_version at all?
I suppose R/W UDF is a small subset of the normal use cases, but the driver
does try to support it.

Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-16 Thread Steve Magnani


Jan -

On 10/16/2017 09:17 AM, Jan Kara wrote:

...
If you agree with these changes, I'll update your patch and merge it to my
tree (along with the other two changes which look good to me).


I think your suggestions are OK. In an ideal world we could work to 
reduce the many signed/unsigned conversions, which make me dizzy. I 
think the ones that really cause problems are large unsigned int values 
which are converted to int and then either to sector_t (except on 32-bit 
systems without LBDAF) or to unsigned long on 64-bit systems.


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-16 Thread Steve Magnani


Jan -

On 10/16/2017 09:17 AM, Jan Kara wrote:

...
If you agree with these changes, I'll update your patch and merge it to my
tree (along with the other two changes which look good to me).


I think your suggestions are OK. In an ideal world we could work to 
reduce the many signed/unsigned conversions, which make me dizzy. I 
think the ones that really cause problems are large unsigned int values 
which are converted to int and then either to sector_t (except on 32-bit 
systems without LBDAF) or to unsigned long on 64-bit systems.


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[PATCH v2 0/3] udf: Fix some signed/unsigned conversion issues

2017-10-12 Thread Steve Magnani

The UDF driver has several points at which conversion between unsigned and
signed types cause (or could cause) problems. On 64-bit systems,
conversion of block addresses larger than 0x7FFF (>= 1 TiB when
blocksize is 512 bytes) can involve undesired sign extension that corrupts
the block address value. This is known to cause the following problems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

Additionally, large unsigned values can be printed as negative numbers,
for example:

  Partition (0 type 1511) starts at physical 460, block length -1779968542

Take care to use unsigned types to store UDF block addresses and to use
format specifiers that match the signedness of the values they are to
print.

Changes since V1:
* Separated printing fixes from sign extension fixes
* Implemented suggested udf_pblk_t typedef for representation of
  block addresses and use it in place of 'uint32_t' for changes in this
  patch series
* Converted some uint32_t block address variables in inode_getblk() to
  udf_pblk_t
* Fixed additional signed/unsigned type mismatches in:
  - udf_table_new_block()
  - udf_fileident_read()
  - udf_new_inode()
  - udf_split_extents()
  - udf_getblk()
  - udf_do_extend_file()
  - udf_bread()
  - udf_setsize()
  - udf_setup_indirect_aext()
  - udf_add_aext()
  - extent_trunc()

Signed-off-by: Steven J. Magnani

[PATCH v2 0/3] udf: Fix some signed/unsigned conversion issues

2017-10-12 Thread Steve Magnani

The UDF driver has several points at which conversion between unsigned and
signed types cause (or could cause) problems. On 64-bit systems,
conversion of block addresses larger than 0x7FFF (>= 1 TiB when
blocksize is 512 bytes) can involve undesired sign extension that corrupts
the block address value. This is known to cause the following problems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

Additionally, large unsigned values can be printed as negative numbers,
for example:

  Partition (0 type 1511) starts at physical 460, block length -1779968542

Take care to use unsigned types to store UDF block addresses and to use
format specifiers that match the signedness of the values they are to
print.

Changes since V1:
* Separated printing fixes from sign extension fixes
* Implemented suggested udf_pblk_t typedef for representation of
  block addresses and use it in place of 'uint32_t' for changes in this
  patch series
* Converted some uint32_t block address variables in inode_getblk() to
  udf_pblk_t
* Fixed additional signed/unsigned type mismatches in:
  - udf_table_new_block()
  - udf_fileident_read()
  - udf_new_inode()
  - udf_split_extents()
  - udf_getblk()
  - udf_do_extend_file()
  - udf_bread()
  - udf_setsize()
  - udf_setup_indirect_aext()
  - udf_add_aext()
  - extent_trunc()

Signed-off-by: Steven J. Magnani

[PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-12 Thread Steve Magnani

Large (> 1 TiB) UDF filesystems appear subject to several problems when
mounted on 64-bit systems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

Per suggestion by Jan Kara, introduce a udf_pblk_t type for representation
of UDF block addresses. Ultimately, all driver functions that manipulate
UDF block addresses should use this type; for now, deployment is limited
to functions with actual or potential sign extension issues.

Changes to udf_readdir() and udf_block_map() address the issues noted
above; other changes address potential similar issues uncovered during
audit of the driver code.

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/balloc.c b/fs/udf/balloc.c
--- a/fs/udf/balloc.c   2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/balloc.c   2017-10-11 13:08:47.969313878 -0500
@@ -218,16 +218,17 @@ out:
return alloc_count;
 }
 
-static int udf_bitmap_new_block(struct super_block *sb,
+static udf_pblk_t udf_bitmap_new_block(struct super_block *sb,
struct udf_bitmap *bitmap, uint16_t partition,
uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
-   int newbit, bit = 0, block, block_group, group_start;
+   int newbit, bit = 0;
+   udf_pblk_t block, block_group, group_start;
int end_goal, nr_groups, bitmap_nr, i;
struct buffer_head *bh = NULL;
char *ptr;
-   int newblock = 0;
+   udf_pblk_t newblock = 0;
 
*err = -ENOSPC;
mutex_lock(>s_alloc_mutex);
@@ -545,13 +546,14 @@ static int udf_table_prealloc_blocks(str
return alloc_count;
 }
 
-static int udf_table_new_block(struct super_block *sb,
+static udf_pblk_t udf_table_new_block(struct super_block *sb,
   struct inode *table, uint16_t partition,
   uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
uint32_t spread = 0x, nspread = 0x;
-   uint32_t newblock = 0, adsize;
+   udf_pblk_t newblock = 0;
+   uint32_t adsize;
uint32_t elen, goal_elen = 0;
struct kernel_lb_addr eloc, uninitialized_var(goal_eloc);
struct extent_position epos, goal_epos;
@@ -700,12 +702,12 @@ inline int udf_prealloc_blocks(struct su
return allocated;
 }
 
-inline int udf_new_block(struct super_block *sb,
+inline udf_pblk_t udf_new_block(struct super_block *sb,
 struct inode *inode,
 uint16_t partition, uint32_t goal, int *err)
 {
struct udf_part_map *map = _SB(sb)->s_partmaps[partition];
-   int block;
+   udf_pblk_t block;
 
if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP)
block = udf_bitmap_new_block(sb,
diff -uprN a/fs/udf/dir.c b/fs/udf/dir.c
--- a/fs/udf/dir.c  2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/dir.c  2017-10-11 08:01:39.376578063 -0500
@@ -43,7 +43,7 @@ static int udf_readdir(struct file *file
struct udf_fileident_bh fibh = { .sbh = NULL, .ebh = NULL};
struct fileIdentDesc *fi = NULL;
struct fileIdentDesc cfi;
-   int block, iblock;
+   udf_pblk_t block, iblock;
loff_t nf_pos;
int flen;
unsigned char *fname = NULL, *copy_name = NULL;
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/directory.c2017-10-11 12:02:27.226674863 -0500
@@ -26,7 +26,8 @@ struct fileIdentDesc *udf_fileident_read
 sector_t *offset)
 {
struct fileIdentDesc *fi;
-   int i, num, block;
+   int i, num;
+   udf_pblk_t block;
struct buffer_head *tmp, *bha[16];
struct udf_inode_info *iinfo = UDF_I(dir);
 
diff -uprN a/fs/udf/ialloc.c b/fs/udf/ialloc.c
--- a/fs/udf/ialloc.c   2017-10-07 12:06:18.749230803 -0500
+++ b/fs/udf/ialloc.c   2017-10-11 12:25:21.255500184 -0500
@@ -50,7 +50,7 @@ struct inode *udf_new_inode(struct inode
struct super_block *sb = dir->i_sb;
struct udf_sb_info *sbi = UDF_SB(sb);
struct inode *inode;
-   int block;
+   udf_pblk_t block;
uint32_t start = UDF_I(dir)->i_location.logicalBlockNum;
struct udf_inode_info *iinfo;
struct udf_inode_info *dinfo = UDF_I(dir);
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/inode.c2017-10-11 12:49:03.972233635 -0500
@@ -52,7 +52,7 @@ static int udf_alloc_i_data(struct inode
 static sector_t inode_getblk(struct inode *, sector_t, int *, int *);
 static

[PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-12 Thread Steve Magnani

Large (> 1 TiB) UDF filesystems appear subject to several problems when
mounted on 64-bit systems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

Per suggestion by Jan Kara, introduce a udf_pblk_t type for representation
of UDF block addresses. Ultimately, all driver functions that manipulate
UDF block addresses should use this type; for now, deployment is limited
to functions with actual or potential sign extension issues.

Changes to udf_readdir() and udf_block_map() address the issues noted
above; other changes address potential similar issues uncovered during
audit of the driver code.

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/balloc.c b/fs/udf/balloc.c
--- a/fs/udf/balloc.c   2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/balloc.c   2017-10-11 13:08:47.969313878 -0500
@@ -218,16 +218,17 @@ out:
return alloc_count;
 }
 
-static int udf_bitmap_new_block(struct super_block *sb,
+static udf_pblk_t udf_bitmap_new_block(struct super_block *sb,
struct udf_bitmap *bitmap, uint16_t partition,
uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
-   int newbit, bit = 0, block, block_group, group_start;
+   int newbit, bit = 0;
+   udf_pblk_t block, block_group, group_start;
int end_goal, nr_groups, bitmap_nr, i;
struct buffer_head *bh = NULL;
char *ptr;
-   int newblock = 0;
+   udf_pblk_t newblock = 0;
 
*err = -ENOSPC;
mutex_lock(>s_alloc_mutex);
@@ -545,13 +546,14 @@ static int udf_table_prealloc_blocks(str
return alloc_count;
 }
 
-static int udf_table_new_block(struct super_block *sb,
+static udf_pblk_t udf_table_new_block(struct super_block *sb,
   struct inode *table, uint16_t partition,
   uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
uint32_t spread = 0x, nspread = 0x;
-   uint32_t newblock = 0, adsize;
+   udf_pblk_t newblock = 0;
+   uint32_t adsize;
uint32_t elen, goal_elen = 0;
struct kernel_lb_addr eloc, uninitialized_var(goal_eloc);
struct extent_position epos, goal_epos;
@@ -700,12 +702,12 @@ inline int udf_prealloc_blocks(struct su
return allocated;
 }
 
-inline int udf_new_block(struct super_block *sb,
+inline udf_pblk_t udf_new_block(struct super_block *sb,
 struct inode *inode,
 uint16_t partition, uint32_t goal, int *err)
 {
struct udf_part_map *map = _SB(sb)->s_partmaps[partition];
-   int block;
+   udf_pblk_t block;
 
if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP)
block = udf_bitmap_new_block(sb,
diff -uprN a/fs/udf/dir.c b/fs/udf/dir.c
--- a/fs/udf/dir.c  2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/dir.c  2017-10-11 08:01:39.376578063 -0500
@@ -43,7 +43,7 @@ static int udf_readdir(struct file *file
struct udf_fileident_bh fibh = { .sbh = NULL, .ebh = NULL};
struct fileIdentDesc *fi = NULL;
struct fileIdentDesc cfi;
-   int block, iblock;
+   udf_pblk_t block, iblock;
loff_t nf_pos;
int flen;
unsigned char *fname = NULL, *copy_name = NULL;
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/directory.c2017-10-11 12:02:27.226674863 -0500
@@ -26,7 +26,8 @@ struct fileIdentDesc *udf_fileident_read
 sector_t *offset)
 {
struct fileIdentDesc *fi;
-   int i, num, block;
+   int i, num;
+   udf_pblk_t block;
struct buffer_head *tmp, *bha[16];
struct udf_inode_info *iinfo = UDF_I(dir);
 
diff -uprN a/fs/udf/ialloc.c b/fs/udf/ialloc.c
--- a/fs/udf/ialloc.c   2017-10-07 12:06:18.749230803 -0500
+++ b/fs/udf/ialloc.c   2017-10-11 12:25:21.255500184 -0500
@@ -50,7 +50,7 @@ struct inode *udf_new_inode(struct inode
struct super_block *sb = dir->i_sb;
struct udf_sb_info *sbi = UDF_SB(sb);
struct inode *inode;
-   int block;
+   udf_pblk_t block;
uint32_t start = UDF_I(dir)->i_location.logicalBlockNum;
struct udf_inode_info *iinfo;
struct udf_inode_info *dinfo = UDF_I(dir);
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/inode.c2017-10-11 12:49:03.972233635 -0500
@@ -52,7 +52,7 @@ static int udf_alloc_i_data(struct inode
 static sector_t inode_getblk(struct inode *, sector_t, int *, int *);
 static int8_t

[PATCH v2 3/3] udf: Fix some sign-conversion warnings

2017-10-12 Thread Steve Magnani

Fix some warnings that appear when compiling with -Wconversion.
A sub-optimal choice of variable type leads to warnings about
conversion in both directions between unsigned and signed.

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/directory.c2017-10-11 12:02:27.226674863 -0500
@@ -52,7 +52,7 @@ struct fileIdentDesc *udf_fileident_read
}
 
if (fibh->eoffset == dir->i_sb->s_blocksize) {
-   int lextoffset = epos->offset;
+   uint32_t lextoffset = epos->offset;
unsigned char blocksize_bits = dir->i_sb->s_blocksize_bits;
 
if (udf_next_aext(dir, epos, eloc, elen, 1) !=
@@ -111,7 +111,7 @@ struct fileIdentDesc *udf_fileident_read
memcpy((uint8_t *)cfi, (uint8_t *)fi,
   sizeof(struct fileIdentDesc));
} else if (fibh->eoffset > dir->i_sb->s_blocksize) {
-   int lextoffset = epos->offset;
+   uint32_t lextoffset = epos->offset;
 
if (udf_next_aext(dir, epos, eloc, elen, 1) !=
(EXT_RECORDED_ALLOCATED >> 30))
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/inode.c2017-10-11 12:49:03.972233635 -0500
@@ -480,7 +480,7 @@ static int udf_do_extend_file(struct ino
int count = 0, fake = !(last_ext->extLength & UDF_EXTENT_LENGTH_MASK);
struct super_block *sb = inode->i_sb;
struct kernel_lb_addr prealloc_loc = {};
-   int prealloc_len = 0;
+   uint32_t prealloc_len = 0;
struct udf_inode_info *iinfo;
int err;
 
@@ -1193,7 +1193,7 @@ int udf_setsize(struct inode *inode, lof
 {
int err;
struct udf_inode_info *iinfo;
-   int bsize = i_blocksize(inode);
+   unsigned int bsize = i_blocksize(inode);
 
if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
  S_ISLNK(inode->i_mode)))

[PATCH v2 3/3] udf: Fix some sign-conversion warnings

2017-10-12 Thread Steve Magnani

Fix some warnings that appear when compiling with -Wconversion.
A sub-optimal choice of variable type leads to warnings about
conversion in both directions between unsigned and signed.

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/directory.c2017-10-11 12:02:27.226674863 -0500
@@ -52,7 +52,7 @@ struct fileIdentDesc *udf_fileident_read
}
 
if (fibh->eoffset == dir->i_sb->s_blocksize) {
-   int lextoffset = epos->offset;
+   uint32_t lextoffset = epos->offset;
unsigned char blocksize_bits = dir->i_sb->s_blocksize_bits;
 
if (udf_next_aext(dir, epos, eloc, elen, 1) !=
@@ -111,7 +111,7 @@ struct fileIdentDesc *udf_fileident_read
memcpy((uint8_t *)cfi, (uint8_t *)fi,
   sizeof(struct fileIdentDesc));
} else if (fibh->eoffset > dir->i_sb->s_blocksize) {
-   int lextoffset = epos->offset;
+   uint32_t lextoffset = epos->offset;
 
if (udf_next_aext(dir, epos, eloc, elen, 1) !=
(EXT_RECORDED_ALLOCATED >> 30))
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-07 16:46:37.694130197 -0500
+++ b/fs/udf/inode.c2017-10-11 12:49:03.972233635 -0500
@@ -480,7 +480,7 @@ static int udf_do_extend_file(struct ino
int count = 0, fake = !(last_ext->extLength & UDF_EXTENT_LENGTH_MASK);
struct super_block *sb = inode->i_sb;
struct kernel_lb_addr prealloc_loc = {};
-   int prealloc_len = 0;
+   uint32_t prealloc_len = 0;
struct udf_inode_info *iinfo;
int err;
 
@@ -1193,7 +1193,7 @@ int udf_setsize(struct inode *inode, lof
 {
int err;
struct udf_inode_info *iinfo;
-   int bsize = i_blocksize(inode);
+   unsigned int bsize = i_blocksize(inode);
 
if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
  S_ISLNK(inode->i_mode)))

[PATCH v2 2/3] udf: Fix signed/unsigned format specifiers

2017-10-12 Thread Steve Magnani

Fix problems noted in compilion with -Wformat=2 -Wformat-signedness.
In particular, a mismatch between the signedness of a value and the
signedness of its format specifier can result in unsigned values being
printed as negative numbers, e.g.:

  Partition (0 type 1511) starts at physical 460, block length -1779968542

...which occurs when mounting a large (> 1 TiB) UDF partition.

Changes since V1:
* Fixed additional issues noted in udf_bitmap_free_blocks(),
  udf_get_fileident(), udf_show_options()

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/balloc.c b/fs/udf/balloc.c
--- a/fs/udf/balloc.c   2017-10-11 15:18:22.039284808 -0500
+++ b/fs/udf/balloc.c   2017-10-11 15:18:45.259250760 -0500
@@ -58,7 +58,7 @@ static int __load_block_bitmap(struct su
int nr_groups = bitmap->s_nr_groups;
 
if (block_group >= nr_groups) {
-   udf_debug("block_group (%d) > nr_groups (%d)\n",
+   udf_debug("block_group (%u) > nr_groups (%d)\n",
  block_group, nr_groups);
}
 
@@ -122,7 +122,7 @@ static void udf_bitmap_free_blocks(struc
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -151,9 +151,9 @@ static void udf_bitmap_free_blocks(struc
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);
udf_debug("byte=%2x\n",
- ((char *)bh->b_data)[(bit + i) >> 3]);
+ ((__u8 *)bh->b_data)[(bit + i) >> 3]);
}
}
udf_add_free_space(sb, sbi->s_partition, count);

@@ -363,7 +363,7 @@ static void udf_table_free_blocks(struct
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -516,7 +516,7 @@ static int udf_table_prealloc_blocks(str
 
while (first_block != eloc.logicalBlockNum &&
   (etype = udf_next_aext(table, , , , 1)) != -1) {
-   udf_debug("eloc=%d, elen=%d, first_block=%d\n",
+   udf_debug("eloc=%u, elen=%u, first_block=%u\n",
  eloc.logicalBlockNum, elen, first_block);
; /* empty loop body */
}
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-11 15:03:12.300741772 -0500
+++ b/fs/udf/directory.c2017-10-11 15:18:45.255250766 -0500
@@ -176,7 +176,7 @@ struct fileIdentDesc *udf_get_fileident(
if (fi->descTag.tagIdent != cpu_to_le16(TAG_IDENT_FID)) {
udf_debug("0x%x != TAG_IDENT_FID\n",
  le16_to_cpu(fi->descTag.tagIdent));
-   udf_debug("offset: %u sizeof: %lu bufsize: %u\n",
+   udf_debug("offset: %d sizeof: %lu bufsize: %d\n",
  *offset, (unsigned long)sizeof(struct fileIdentDesc),
  bufsize);
return NULL;
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-11 15:18:22.039284808 -0500
+++ b/fs/udf/inode.c2017-10-11 15:18:45.251250771 -0500
@@ -1278,14 +1278,14 @@ static int udf_read_inode(struct inode *
 
 reread:
if (iloc->partitionReferenceNum >= sbi->s_partitions) {
-   udf_debug("partition reference: %d > logical volume partitions: 
%d\n",
+   udf_debug("partition reference: %u > logical volume partitions: 
%u\n",
  iloc->partitionReferenceNum, sbi->s_partitions);
return -EIO;
}
 
if (iloc->logicalBlockNum >=
sbi->s_partmaps[iloc->partitionReferenceNum].s_partition_len) {
-   udf_debug("block=%d, partition=%d out of range\n",
+   udf_debug("block=%u, partition=%u out of range\n",
  iloc->logicalBlockNum, iloc->partitionReferenceNum);
return -EIO;
}
@@ -1304,13 +1304,13 @@ reread:
 */
bh =

[PATCH v2 2/3] udf: Fix signed/unsigned format specifiers

2017-10-12 Thread Steve Magnani

Fix problems noted in compilion with -Wformat=2 -Wformat-signedness.
In particular, a mismatch between the signedness of a value and the
signedness of its format specifier can result in unsigned values being
printed as negative numbers, e.g.:

  Partition (0 type 1511) starts at physical 460, block length -1779968542

...which occurs when mounting a large (> 1 TiB) UDF partition.

Changes since V1:
* Fixed additional issues noted in udf_bitmap_free_blocks(),
  udf_get_fileident(), udf_show_options()

Signed-off-by: Steven J. Magnani 
---
diff -uprN a/fs/udf/balloc.c b/fs/udf/balloc.c
--- a/fs/udf/balloc.c   2017-10-11 15:18:22.039284808 -0500
+++ b/fs/udf/balloc.c   2017-10-11 15:18:45.259250760 -0500
@@ -58,7 +58,7 @@ static int __load_block_bitmap(struct su
int nr_groups = bitmap->s_nr_groups;
 
if (block_group >= nr_groups) {
-   udf_debug("block_group (%d) > nr_groups (%d)\n",
+   udf_debug("block_group (%u) > nr_groups (%d)\n",
  block_group, nr_groups);
}
 
@@ -122,7 +122,7 @@ static void udf_bitmap_free_blocks(struc
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -151,9 +151,9 @@ static void udf_bitmap_free_blocks(struc
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);
udf_debug("byte=%2x\n",
- ((char *)bh->b_data)[(bit + i) >> 3]);
+ ((__u8 *)bh->b_data)[(bit + i) >> 3]);
}
}
udf_add_free_space(sb, sbi->s_partition, count);

@@ -363,7 +363,7 @@ static void udf_table_free_blocks(struct
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -516,7 +516,7 @@ static int udf_table_prealloc_blocks(str
 
while (first_block != eloc.logicalBlockNum &&
   (etype = udf_next_aext(table, , , , 1)) != -1) {
-   udf_debug("eloc=%d, elen=%d, first_block=%d\n",
+   udf_debug("eloc=%u, elen=%u, first_block=%u\n",
  eloc.logicalBlockNum, elen, first_block);
; /* empty loop body */
}
diff -uprN a/fs/udf/directory.c b/fs/udf/directory.c
--- a/fs/udf/directory.c2017-10-11 15:03:12.300741772 -0500
+++ b/fs/udf/directory.c2017-10-11 15:18:45.255250766 -0500
@@ -176,7 +176,7 @@ struct fileIdentDesc *udf_get_fileident(
if (fi->descTag.tagIdent != cpu_to_le16(TAG_IDENT_FID)) {
udf_debug("0x%x != TAG_IDENT_FID\n",
  le16_to_cpu(fi->descTag.tagIdent));
-   udf_debug("offset: %u sizeof: %lu bufsize: %u\n",
+   udf_debug("offset: %d sizeof: %lu bufsize: %d\n",
  *offset, (unsigned long)sizeof(struct fileIdentDesc),
  bufsize);
return NULL;
diff -uprN a/fs/udf/inode.c b/fs/udf/inode.c
--- a/fs/udf/inode.c2017-10-11 15:18:22.039284808 -0500
+++ b/fs/udf/inode.c2017-10-11 15:18:45.251250771 -0500
@@ -1278,14 +1278,14 @@ static int udf_read_inode(struct inode *
 
 reread:
if (iloc->partitionReferenceNum >= sbi->s_partitions) {
-   udf_debug("partition reference: %d > logical volume partitions: 
%d\n",
+   udf_debug("partition reference: %u > logical volume partitions: 
%u\n",
  iloc->partitionReferenceNum, sbi->s_partitions);
return -EIO;
}
 
if (iloc->logicalBlockNum >=
sbi->s_partmaps[iloc->partitionReferenceNum].s_partition_len) {
-   udf_debug("block=%d, partition=%d out of range\n",
+   udf_debug("block=%u, partition=%u out of range\n",
  iloc->logicalBlockNum, iloc->partitionReferenceNum);
return -EIO;
}
@@ -1304,13 +1304,13 @@ reread:
 */
bh = udf_read_ptagged(inode->i_sb, iloc, 0, );

Re: [PATCH] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-10 Thread Steve Magnani


Jan -

On 10/10/2017 02:33 AM, Jan Kara wrote:

On Mon 09-10-17 10:04:52, Steve Magnani wrote:

...the patch seems to be mixing two changes into one which I'd prefer to be
  separate patches:

1) Changes so that physical block numbers are stored in uint32_t (and
accompanying format string changes). Also when doing this, could you please
create a dedicated type like

typedef uint32_t udf_pblk_t;

and use it instead of uint32_t? That way it would be cleaner what's going
on. Thanks!
I agree with this in principle and in fact do something like it in my 
application code for just that reason. But, doing a complete job of this 
in the driver would increase the scope far beyond what is needed to fix 
the bugs I see and beyond what I am able to support. Would it be 
acceptable to limit usage of this type to a subset of the places it 
could ultimately be used? (Example: use it in udf_readdir(), which has a 
bug requiring a type change, but not necessarily in udf_read_tagged(), 
which doesn't).



2) Changes fixing signedness in various format strings for various types -
put these in a separare patch please.
Sure - but would you be opposed to putting _all_ of the format string 
changes in that patch? There are some format string changes (i.e., in 
unicode.c) that obviously don't have anything to do with block numbers, 
but I think almost all of the rest do. It gets a little murky when block 
numbers or counts are used in calculations. The unifying idea behind all 
the format string changes is preventing sign extension from causing 
unsigned values to be printed as negative, so on that basis I think an 
argument can be made that they all "go" together.

--- a/fs/udf/balloc.c   (revision 26779)
+++ b/fs/udf/balloc.c   (working copy)

...

@@ -151,7 +151,7 @@
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);

This change looks wrong - bit and i are signed. However they are ints, not
longs, so that should indeed be fixed.
'bit' and 'i' are ints in the function _below_ this change, but unsigned 
long within this function. So I think this is correct.


Regards,


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-10 Thread Steve Magnani


Jan -

On 10/10/2017 02:33 AM, Jan Kara wrote:

On Mon 09-10-17 10:04:52, Steve Magnani wrote:

...the patch seems to be mixing two changes into one which I'd prefer to be
  separate patches:

1) Changes so that physical block numbers are stored in uint32_t (and
accompanying format string changes). Also when doing this, could you please
create a dedicated type like

typedef uint32_t udf_pblk_t;

and use it instead of uint32_t? That way it would be cleaner what's going
on. Thanks!
I agree with this in principle and in fact do something like it in my 
application code for just that reason. But, doing a complete job of this 
in the driver would increase the scope far beyond what is needed to fix 
the bugs I see and beyond what I am able to support. Would it be 
acceptable to limit usage of this type to a subset of the places it 
could ultimately be used? (Example: use it in udf_readdir(), which has a 
bug requiring a type change, but not necessarily in udf_read_tagged(), 
which doesn't).



2) Changes fixing signedness in various format strings for various types -
put these in a separare patch please.
Sure - but would you be opposed to putting _all_ of the format string 
changes in that patch? There are some format string changes (i.e., in 
unicode.c) that obviously don't have anything to do with block numbers, 
but I think almost all of the rest do. It gets a little murky when block 
numbers or counts are used in calculations. The unifying idea behind all 
the format string changes is preventing sign extension from causing 
unsigned values to be printed as negative, so on that basis I think an 
argument can be made that they all "go" together.

--- a/fs/udf/balloc.c   (revision 26779)
+++ b/fs/udf/balloc.c   (working copy)

...

@@ -151,7 +151,7 @@
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);

This change looks wrong - bit and i are signed. However they are ints, not
longs, so that should indeed be fixed.
'bit' and 'i' are ints in the function _below_ this change, but unsigned 
long within this function. So I think this is correct.


Regards,


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

[PATCH] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-09 Thread Steve Magnani

Large (> 1 TiB) UDF filesystems appear subject to several problems when
mounted on 64-bit systems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

* Unsigned values > 0x7FFF are output as negative numbers in some
  driver printks, e.g.:
  Partition (0 type 1511) starts at physical 460, block length -1779968542
 
Take care to use "%u" when printing unsigned values and to use unsigned
types to store UDF block addresses.

Signed-off-by: Steven J. Magnani 
---
Index: balloc.c
===
--- a/fs/udf/balloc.c   (revision 26779)
+++ b/fs/udf/balloc.c   (working copy)
@@ -58,7 +58,7 @@
int nr_groups = bitmap->s_nr_groups;
 
if (block_group >= nr_groups) {
-   udf_debug("block_group (%d) > nr_groups (%d)\n",
+   udf_debug("block_group (%u) > nr_groups (%d)\n",
  block_group, nr_groups);
}
 
@@ -122,7 +122,7 @@
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -151,7 +151,7 @@
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);
udf_debug("byte=%2x\n",
  ((char *)bh->b_data)[(bit + i) >> 3]);
}
@@ -218,16 +218,17 @@
return alloc_count;
 }
 
-static int udf_bitmap_new_block(struct super_block *sb,
+static uint32_t udf_bitmap_new_block(struct super_block *sb,
struct udf_bitmap *bitmap, uint16_t partition,
uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
-   int newbit, bit = 0, block, block_group, group_start;
+   int newbit, bit = 0;
+   uint32_t block, block_group, group_start;
int end_goal, nr_groups, bitmap_nr, i;
struct buffer_head *bh = NULL;
char *ptr;
-   int newblock = 0;
+   uint32_t newblock = 0;
 
*err = -ENOSPC;
mutex_lock(>s_alloc_mutex);
@@ -362,7 +363,7 @@
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -515,7 +516,7 @@
 
while (first_block != eloc.logicalBlockNum &&
   (etype = udf_next_aext(table, , , , 1)) != -1) {
-   udf_debug("eloc=%d, elen=%d, first_block=%d\n",
+   udf_debug("eloc=%u, elen=%u, first_block=%u\n",
  eloc.logicalBlockNum, elen, first_block);
; /* empty loop body */
}
@@ -700,12 +701,12 @@
return allocated;
 }
 
-inline int udf_new_block(struct super_block *sb,
+inline uint32_t udf_new_block(struct super_block *sb,
 struct inode *inode,
 uint16_t partition, uint32_t goal, int *err)
 {
struct udf_part_map *map = _SB(sb)->s_partmaps[partition];
-   int block;
+   uint32_t block;
 
if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP)
block = udf_bitmap_new_block(sb,
Index: dir.c
===
--- a/fs/udf/dir.c  (revision 26779)
+++ b/fs/udf/dir.c  (working copy)
@@ -43,7 +43,7 @@
struct udf_fileident_bh fibh = { .sbh = NULL, .ebh = NULL};
struct fileIdentDesc *fi = NULL;
struct fileIdentDesc cfi;
-   int block, iblock;
+   uint32_t block, iblock;
loff_t nf_pos;
int flen;
unsigned char *fname = NULL, *copy_name = NULL;
Index: directory.c
===
--- a/fs/udf/directory.c(revision 26779)
+++ b/fs/udf/directory.c(working copy)
@@ -26,7

[PATCH] udf: Fix 64-bit sign extension issues affecting blocks > 0x7FFFFFFF

2017-10-09 Thread Steve Magnani

Large (> 1 TiB) UDF filesystems appear subject to several problems when
mounted on 64-bit systems:

* readdir() can fail on a directory containing File Identifiers residing
  above 0x7FFF. This manifests as a 'ls' command failing with EIO.

* FIBMAP on a file block located above 0x7FFF can return a negative
  value. The low 32 bits are correct, but applications that don't mask the
  high 32 bits of the result can perform incorrectly.

* Unsigned values > 0x7FFF are output as negative numbers in some
  driver printks, e.g.:
  Partition (0 type 1511) starts at physical 460, block length -1779968542
 
Take care to use "%u" when printing unsigned values and to use unsigned
types to store UDF block addresses.

Signed-off-by: Steven J. Magnani 
---
Index: balloc.c
===
--- a/fs/udf/balloc.c   (revision 26779)
+++ b/fs/udf/balloc.c   (working copy)
@@ -58,7 +58,7 @@
int nr_groups = bitmap->s_nr_groups;
 
if (block_group >= nr_groups) {
-   udf_debug("block_group (%d) > nr_groups (%d)\n",
+   udf_debug("block_group (%u) > nr_groups (%d)\n",
  block_group, nr_groups);
}
 
@@ -122,7 +122,7 @@
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -151,7 +151,7 @@
bh = bitmap->s_block_bitmap[bitmap_nr];
for (i = 0; i < count; i++) {
if (udf_set_bit(bit + i, bh->b_data)) {
-   udf_debug("bit %ld already set\n", bit + i);
+   udf_debug("bit %lu already set\n", bit + i);
udf_debug("byte=%2x\n",
  ((char *)bh->b_data)[(bit + i) >> 3]);
}
@@ -218,16 +218,17 @@
return alloc_count;
 }
 
-static int udf_bitmap_new_block(struct super_block *sb,
+static uint32_t udf_bitmap_new_block(struct super_block *sb,
struct udf_bitmap *bitmap, uint16_t partition,
uint32_t goal, int *err)
 {
struct udf_sb_info *sbi = UDF_SB(sb);
-   int newbit, bit = 0, block, block_group, group_start;
+   int newbit, bit = 0;
+   uint32_t block, block_group, group_start;
int end_goal, nr_groups, bitmap_nr, i;
struct buffer_head *bh = NULL;
char *ptr;
-   int newblock = 0;
+   uint32_t newblock = 0;
 
*err = -ENOSPC;
mutex_lock(>s_alloc_mutex);
@@ -362,7 +363,7 @@
partmap = >s_partmaps[bloc->partitionReferenceNum];
if (bloc->logicalBlockNum + count < count ||
(bloc->logicalBlockNum + count) > partmap->s_partition_len) {
-   udf_debug("%d < %d || %d + %d > %d\n",
+   udf_debug("%u < %d || %u + %u > %u\n",
  bloc->logicalBlockNum, 0,
  bloc->logicalBlockNum, count,
  partmap->s_partition_len);
@@ -515,7 +516,7 @@
 
while (first_block != eloc.logicalBlockNum &&
   (etype = udf_next_aext(table, , , , 1)) != -1) {
-   udf_debug("eloc=%d, elen=%d, first_block=%d\n",
+   udf_debug("eloc=%u, elen=%u, first_block=%u\n",
  eloc.logicalBlockNum, elen, first_block);
; /* empty loop body */
}
@@ -700,12 +701,12 @@
return allocated;
 }
 
-inline int udf_new_block(struct super_block *sb,
+inline uint32_t udf_new_block(struct super_block *sb,
 struct inode *inode,
 uint16_t partition, uint32_t goal, int *err)
 {
struct udf_part_map *map = _SB(sb)->s_partmaps[partition];
-   int block;
+   uint32_t block;
 
if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP)
block = udf_bitmap_new_block(sb,
Index: dir.c
===
--- a/fs/udf/dir.c  (revision 26779)
+++ b/fs/udf/dir.c  (working copy)
@@ -43,7 +43,7 @@
struct udf_fileident_bh fibh = { .sbh = NULL, .ebh = NULL};
struct fileIdentDesc *fi = NULL;
struct fileIdentDesc cfi;
-   int block, iblock;
+   uint32_t block, iblock;
loff_t nf_pos;
int flen;
unsigned char *fname = NULL, *copy_name = NULL;
Index: directory.c
===
--- a/fs/udf/directory.c(revision 26779)
+++ b/fs/udf/directory.c(working copy)
@@ -26,7 +26,8 @@

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

2017-02-28 Thread Steve Magnani



On 02/27/2017 12:57 PM, Bart Van Assche wrote:

...
How about the (untested) patch below? The approach below avoids that the check 
is
duplicated and - at least in my opinion - results in code that is easier to 
read.
I find lba_too_large() a little dense, but functionally OK. The "shift 
>= 0" clause could be dropped.

I tested this on my "problem" system (READ CAPACITY(10)) without incident.


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..3533d1e46bde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2082,6 +2082,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
sdkp->capacity = 0; /* unknown mapped to zero - as usual */
  }
  
+/*

+ * Check whether or not logical_to_sectors(sdp, lba) will overflow.
+ */
+static bool lba_too_large(u64 lba, u32 logical_block_size)
+{
+   int shift = sizeof(sector_t) * 8 + 9 - ilog2(logical_block_size);
+
+   return shift >= 0 && shift < 64 && lba >= (1ULL << shift);
+}
+
  #define RC16_LEN 32
  #if RC16_LEN > SD_BUF_SIZE
  #error RC16_LEN must not be more than SD_BUF_SIZE
@@ -2154,7 +2164,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   if (lba_too_large(lba + 1, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2243,7 +2253,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   if (lba_too_large(lba + 1ULL, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

2017-02-28 Thread Steve Magnani



On 02/27/2017 12:57 PM, Bart Van Assche wrote:

...
How about the (untested) patch below? The approach below avoids that the check 
is
duplicated and - at least in my opinion - results in code that is easier to 
read.
I find lba_too_large() a little dense, but functionally OK. The "shift 
>= 0" clause could be dropped.

I tested this on my "problem" system (READ CAPACITY(10)) without incident.


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..3533d1e46bde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2082,6 +2082,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
sdkp->capacity = 0; /* unknown mapped to zero - as usual */
  }
  
+/*

+ * Check whether or not logical_to_sectors(sdp, lba) will overflow.
+ */
+static bool lba_too_large(u64 lba, u32 logical_block_size)
+{
+   int shift = sizeof(sector_t) * 8 + 9 - ilog2(logical_block_size);
+
+   return shift >= 0 && shift < 64 && lba >= (1ULL << shift);
+}
+
  #define RC16_LEN 32
  #if RC16_LEN > SD_BUF_SIZE
  #error RC16_LEN must not be more than SD_BUF_SIZE
@@ -2154,7 +2164,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   if (lba_too_large(lba + 1, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2243,7 +2253,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   if (lba_too_large(lba + 1ULL, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

2017-02-27 Thread Steve Magnani


Hi Bart -

Thanks for taking the time to look this over.

On 02/27/2017 10:13 AM, Bart Van Assche wrote:

On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:

@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = lba << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   ((lba >= 0xULL) || (lba_in_sectors >= 0xULL))) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
int the_result;
int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
sector_t lba;
+   unsigned long long lba_in_sectors;
unsigned sector_size;
  
  	do {

@@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   (lba_in_sectors >= 0xULL)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Why are the two checks slightly different? Could the same code be used for
both checks?
The checks are different because with READ CAPACITY(16) a _really_ huge 
device could report a max LBA so large that left-shifting it causes bits 
to drop off the end. That's not an issue with READ CAPACITY(10) because 
at most the 32-bit LBA reported by the device will become a 35-bit value 
(since the max supported block size is 4096 == (512 << 3)).



BTW, using the macro below would make the above checks less
verbose and easier to read:

/*
  * Test whether the result of a shift-left operation would be larger than
  * what fits in a variable with the type of @a.
  */
#define shift_left_overflows(a, b)  \
({  \
typeof(a) _minus_one = -1LL;\
typeof(a) _plus_one = 1;\
bool _a_is_signed = _minus_one < 0;  \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed);  \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})

Bart.
Perhaps but I am not a fan of putting braces in non-obvious places such 
as within array subscripts (which I've encountered recently) or 
conditional expressions, which is what this amounts to.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

2017-02-27 Thread Steve Magnani


Hi Bart -

Thanks for taking the time to look this over.

On 02/27/2017 10:13 AM, Bart Van Assche wrote:

On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:

@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = lba << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   ((lba >= 0xULL) || (lba_in_sectors >= 0xULL))) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
int the_result;
int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
sector_t lba;
+   unsigned long long lba_in_sectors;
unsigned sector_size;
  
  	do {

@@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   (lba_in_sectors >= 0xULL)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Why are the two checks slightly different? Could the same code be used for
both checks?
The checks are different because with READ CAPACITY(16) a _really_ huge 
device could report a max LBA so large that left-shifting it causes bits 
to drop off the end. That's not an issue with READ CAPACITY(10) because 
at most the 32-bit LBA reported by the device will become a 35-bit value 
(since the max supported block size is 4096 == (512 << 3)).



BTW, using the macro below would make the above checks less
verbose and easier to read:

/*
  * Test whether the result of a shift-left operation would be larger than
  * what fits in a variable with the type of @a.
  */
#define shift_left_overflows(a, b)  \
({  \
typeof(a) _minus_one = -1LL;\
typeof(a) _plus_one = 1;\
bool _a_is_signed = _minus_one < 0;  \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed);  \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})

Bart.
Perhaps but I am not a fan of putting braces in non-obvious places such 
as within array subscripts (which I've encountered recently) or 
conditional expressions, which is what this amounts to.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include

Re: [PATCH] udf: Don't corrupt unalloc spacetable when writing it

2015-07-09 Thread Steve Magnani




On 07/09/2015 10:16 AM, Jan Kara wrote:

On Tue 07-07-15 13:06:05, Steven J. Magnani wrote:

For a UDF filesystem configured with an Unallocated Space Table,
a filesystem operation that triggers an update to the table results
in on-disk corruption that prevents remounting:

   udf_read_tagged: tag version 0x != 0x0002 || 0x0003, block 274

For example:
   1. Create a filesystem
   $ mkudffs --media-type=hd --blocksize=512 --lvid=BUGTEST \
   --vid=BUGTEST --fsid=BUGTEST --space=unalloctable \
   /dev/mmcblk0

   2. Mount it
   # mount /dev/mmcblk0 /mnt

...

So the patch looks good to me. But what kind of mkudffs are you using?
Because when I use the command you wrote into the changelog, I cannot mount
the filesystem. The kernel complains about:

UDF-fs: error (device ubdb): udf_read_inode: (ino 274) failed ident=256
mount: /dev/ubdb: can't read superblock

Now ident 256 is TAG_IDENT_FSD (file set descriptor). So apparently my
mkudffs doesn't create proper partition table with your parameters...



It identifies itself as:
mkudffs 1.0.0b2 for UDF FS 1.0.0-cvs, 2002/02/09

...from Ubuntu udftools 1.0.0b3-14.2.


 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] udf: Don't corrupt unalloc spacetable when writing it

2015-07-09 Thread Steve Magnani




On 07/09/2015 10:16 AM, Jan Kara wrote:

On Tue 07-07-15 13:06:05, Steven J. Magnani wrote:

For a UDF filesystem configured with an Unallocated Space Table,
a filesystem operation that triggers an update to the table results
in on-disk corruption that prevents remounting:

   udf_read_tagged: tag version 0x != 0x0002 || 0x0003, block 274

For example:
   1. Create a filesystem
   $ mkudffs --media-type=hd --blocksize=512 --lvid=BUGTEST \
   --vid=BUGTEST --fsid=BUGTEST --space=unalloctable \
   /dev/mmcblk0

   2. Mount it
   # mount /dev/mmcblk0 /mnt

...

So the patch looks good to me. But what kind of mkudffs are you using?
Because when I use the command you wrote into the changelog, I cannot mount
the filesystem. The kernel complains about:

UDF-fs: error (device ubdb): udf_read_inode: (ino 274) failed ident=256
mount: /dev/ubdb: can't read superblock

Now ident 256 is TAG_IDENT_FSD (file set descriptor). So apparently my
mkudffs doesn't create proper partition table with your parameters...



It identifies itself as:
mkudffs 1.0.0b2 for UDF FS 1.0.0-cvs, 2002/02/09

...from Ubuntu udftools 1.0.0b3-14.2.


 Steven J. Magnani   I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!

 #include standard.disclaimer

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH, resend] sd: fix infinite kernel/udev loop on non-removable Medium Not Present

2013-04-12 Thread Steve Magnani

Commit eface65c336eff420d70beb0fb6787a732e05ffb (2.6.38) altered
set_media_not_present() in a way that prevents the sd driver from
remembering that a non-removable device has reported "Medium Not Present".
This condition can occur on hotplug of a (i.e.) USB Mass Storage device
whose medium is offline due to an unrecoverable controller error,
but which is otherwise capable of SCSI communication (to download new 
microcode, etc.).

Under these conditions, the changed code results in an infinite loop
between the kernel and udevd. When udevd attempts to open the device
in response to a change notification, a SCSI "Medium Not Present" error
occurs which causes the kernel to signal another change. The cycle
repeats until the device is unplugged, resulting in udevd consuming ever-
increasing amounts of CPU and virtual memory.

Resolve this by remembering "media not present" whether the device has
declared itself "removable" or not.

Signed-off-by: Steven J. Magnani 
---
--- a/drivers/scsi/sd.c 2013-04-12 14:16:12.252531097 -0500
+++ b/drivers/scsi/sd.c 2013-04-12 14:21:55.197216521 -0500
@@ -1298,10 +1298,8 @@ out:
 
 static void set_media_not_present(struct scsi_disk *sdkp)
 {
-   if (sdkp->media_present)
+   if (sdkp->media_present) {
sdkp->device->changed = 1;
-
-   if (sdkp->device->removable) {
sdkp->media_present = 0;
sdkp->capacity = 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sd: fix infinite kernel/udev loop on non-removable Medium Not Present

2013-04-12 Thread Steve Magnani

Commit eface65c336eff420d70beb0fb6787a732e05ffb (2.6.38) altered
set_media_not_present() in a way that prevents the sd driver from
remembering that a non-removable device has reported "Medium Not Present".
This condition can occur on hotplug of a (i.e.) USB Mass Storage device
whose medium is offline due to an unrecoverable controller error,
but which is otherwise capable of SCSI communication (to download new 
microcode, etc.).

Under these conditions, the changed code results in an infinite loop
between the kernel and udevd. When udevd attempts to open the device
in response to a change notification, a SCSI "Medium Not Present" error
occurs which causes the kernel to signal another change. The cycle
repeats until the device is unplugged, resulting in udevd consuming ever-
increasing amounts of CPU and virtual memory.

Resolve this by remembering "media not present" whether the device has
declared itself "removable" or not.

Signed-off-by: Steven J. Magnani 
---
--- a/drivers/scsi/sd.c 2013-04-12 14:16:12.252531097 -0500
+++ b/drivers/scsi/sd.c 2013-04-12 14:21:55.197216521 -0500
@@ -1298,10 +1298,8 @@ out:
 
 static void set_media_not_present(struct scsi_disk *sdkp)
 {
-   if (sdkp->media_present)
+   if (sdkp->media_present) {
sdkp->device->changed = 1;
-
-   if (sdkp->device->removable) {
sdkp->media_present = 0;
sdkp->capacity = 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] mmc: debugfs: Add debugfs ability to read CID and CSD

2013-04-12 Thread Steve Magnani

Adds 'cid' and 'csd' debugfs entries for SD/MMC devices that allow
userland to obtain the values of the corresponding device registers.

Signed-off-by: Steven J. Magnani 
---
--- a/drivers/mmc/core/debugfs.c2013-04-12 07:39:22.532586948 -0500
+++ b/drivers/mmc/core/debugfs.c2013-04-12 07:40:14.513331587 -0500
@@ -321,7 +321,7 @@ static ssize_t mmc_ext_csd_read(struct f
   buf, EXT_CSD_STR_LEN);
 }
 
-static int mmc_ext_csd_release(struct inode *inode, struct file *file)
+static int mmc_dbg_buf_release(struct inode *inode, struct file *file)
 {
kfree(file->private_data);
return 0;
@@ -330,7 +330,65 @@ static int mmc_ext_csd_release(struct in
 static const struct file_operations mmc_dbg_ext_csd_fops = {
.open   = mmc_ext_csd_open,
.read   = mmc_ext_csd_read,
-   .release= mmc_ext_csd_release,
+   .release= mmc_dbg_buf_release,
+   .llseek = default_llseek,
+};
+
+/* CSD and CID have the same length and so can be handled much the same */
+#define CXD_STR_LEN 33
+
+static int mmc_cxd_open(struct file *filp, u32 *raw_cxd)
+{
+   char *buf;
+   ssize_t n = 0;
+   int i;
+
+   buf = kmalloc(CXD_STR_LEN + 1, GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;
+
+   for (i = 0; i < 4; i++)
+   n += sprintf(buf + n, "%08x", raw_cxd[i]);
+   n += sprintf(buf + n, "\n");
+   BUG_ON(n != CXD_STR_LEN);
+
+   filp->private_data = buf;
+   return 0;
+}
+
+static ssize_t mmc_cxd_read(struct file *filp, char __user *ubuf,
+   size_t cnt, loff_t *ppos)
+{
+   char *buf = filp->private_data;
+
+   return simple_read_from_buffer(ubuf, cnt, ppos,
+  buf, CXD_STR_LEN);
+}
+
+static int mmc_csd_open(struct inode *inode, struct file *filp)
+{
+   struct mmc_card *card = inode->i_private;
+   return mmc_cxd_open(filp, card->raw_csd);
+}
+
+static const struct file_operations mmc_dbg_csd_fops = {
+   .open   = mmc_csd_open,
+   .read   = mmc_cxd_read,
+   .release= mmc_dbg_buf_release,
+   .llseek = default_llseek,
+};
+
+
+static int mmc_cid_open(struct inode *inode, struct file *filp)
+{
+   struct mmc_card *card = inode->i_private;
+   return mmc_cxd_open(filp, card->raw_cid);
+}
+
+static const struct file_operations mmc_dbg_cid_fops = {
+   .open   = mmc_cid_open,
+   .read   = mmc_cxd_read,
+   .release= mmc_dbg_buf_release,
.llseek = default_llseek,
 };
 
@@ -356,10 +414,17 @@ void mmc_add_card_debugfs(struct mmc_car
if (!debugfs_create_x32("state", S_IRUSR, root, >state))
goto err;
 
-   if (mmc_card_mmc(card) || mmc_card_sd(card))
+   if (mmc_card_mmc(card) || mmc_card_sd(card)) {
if (!debugfs_create_file("status", S_IRUSR, root, card,
_dbg_card_status_fops))
goto err;
+   if (!debugfs_create_file("csd", S_IRUSR, root, card,
+   _dbg_csd_fops))
+   goto err;
+   if (!debugfs_create_file("cid", S_IRUSR, root, card,
+   _dbg_cid_fops))
+   goto err;
+   }
 
if (mmc_card_mmc(card))
if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] mmc: debugfs: Add debugfs ability to read CID and CSD

2013-04-12 Thread Steve Magnani

Adds 'cid' and 'csd' debugfs entries for SD/MMC devices that allow
userland to obtain the values of the corresponding device registers.

Signed-off-by: Steven J. Magnani st...@digidescorp.com
---
--- a/drivers/mmc/core/debugfs.c2013-04-12 07:39:22.532586948 -0500
+++ b/drivers/mmc/core/debugfs.c2013-04-12 07:40:14.513331587 -0500
@@ -321,7 +321,7 @@ static ssize_t mmc_ext_csd_read(struct f
   buf, EXT_CSD_STR_LEN);
 }
 
-static int mmc_ext_csd_release(struct inode *inode, struct file *file)
+static int mmc_dbg_buf_release(struct inode *inode, struct file *file)
 {
kfree(file-private_data);
return 0;
@@ -330,7 +330,65 @@ static int mmc_ext_csd_release(struct in
 static const struct file_operations mmc_dbg_ext_csd_fops = {
.open   = mmc_ext_csd_open,
.read   = mmc_ext_csd_read,
-   .release= mmc_ext_csd_release,
+   .release= mmc_dbg_buf_release,
+   .llseek = default_llseek,
+};
+
+/* CSD and CID have the same length and so can be handled much the same */
+#define CXD_STR_LEN 33
+
+static int mmc_cxd_open(struct file *filp, u32 *raw_cxd)
+{
+   char *buf;
+   ssize_t n = 0;
+   int i;
+
+   buf = kmalloc(CXD_STR_LEN + 1, GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;
+
+   for (i = 0; i  4; i++)
+   n += sprintf(buf + n, %08x, raw_cxd[i]);
+   n += sprintf(buf + n, \n);
+   BUG_ON(n != CXD_STR_LEN);
+
+   filp-private_data = buf;
+   return 0;
+}
+
+static ssize_t mmc_cxd_read(struct file *filp, char __user *ubuf,
+   size_t cnt, loff_t *ppos)
+{
+   char *buf = filp-private_data;
+
+   return simple_read_from_buffer(ubuf, cnt, ppos,
+  buf, CXD_STR_LEN);
+}
+
+static int mmc_csd_open(struct inode *inode, struct file *filp)
+{
+   struct mmc_card *card = inode-i_private;
+   return mmc_cxd_open(filp, card-raw_csd);
+}
+
+static const struct file_operations mmc_dbg_csd_fops = {
+   .open   = mmc_csd_open,
+   .read   = mmc_cxd_read,
+   .release= mmc_dbg_buf_release,
+   .llseek = default_llseek,
+};
+
+
+static int mmc_cid_open(struct inode *inode, struct file *filp)
+{
+   struct mmc_card *card = inode-i_private;
+   return mmc_cxd_open(filp, card-raw_cid);
+}
+
+static const struct file_operations mmc_dbg_cid_fops = {
+   .open   = mmc_cid_open,
+   .read   = mmc_cxd_read,
+   .release= mmc_dbg_buf_release,
.llseek = default_llseek,
 };
 
@@ -356,10 +414,17 @@ void mmc_add_card_debugfs(struct mmc_car
if (!debugfs_create_x32(state, S_IRUSR, root, card-state))
goto err;
 
-   if (mmc_card_mmc(card) || mmc_card_sd(card))
+   if (mmc_card_mmc(card) || mmc_card_sd(card)) {
if (!debugfs_create_file(status, S_IRUSR, root, card,
mmc_dbg_card_status_fops))
goto err;
+   if (!debugfs_create_file(csd, S_IRUSR, root, card,
+   mmc_dbg_csd_fops))
+   goto err;
+   if (!debugfs_create_file(cid, S_IRUSR, root, card,
+   mmc_dbg_cid_fops))
+   goto err;
+   }
 
if (mmc_card_mmc(card))
if (!debugfs_create_file(ext_csd, S_IRUSR, root, card,

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sd: fix infinite kernel/udev loop on non-removable Medium Not Present

2013-04-12 Thread Steve Magnani

Commit eface65c336eff420d70beb0fb6787a732e05ffb (2.6.38) altered
set_media_not_present() in a way that prevents the sd driver from
remembering that a non-removable device has reported Medium Not Present.
This condition can occur on hotplug of a (i.e.) USB Mass Storage device
whose medium is offline due to an unrecoverable controller error,
but which is otherwise capable of SCSI communication (to download new 
microcode, etc.).

Under these conditions, the changed code results in an infinite loop
between the kernel and udevd. When udevd attempts to open the device
in response to a change notification, a SCSI Medium Not Present error
occurs which causes the kernel to signal another change. The cycle
repeats until the device is unplugged, resulting in udevd consuming ever-
increasing amounts of CPU and virtual memory.

Resolve this by remembering media not present whether the device has
declared itself removable or not.

Signed-off-by: Steven J. Magnani st...@digidescorp.com
---
--- a/drivers/scsi/sd.c 2013-04-12 14:16:12.252531097 -0500
+++ b/drivers/scsi/sd.c 2013-04-12 14:21:55.197216521 -0500
@@ -1298,10 +1298,8 @@ out:
 
 static void set_media_not_present(struct scsi_disk *sdkp)
 {
-   if (sdkp-media_present)
+   if (sdkp-media_present) {
sdkp-device-changed = 1;
-
-   if (sdkp-device-removable) {
sdkp-media_present = 0;
sdkp-capacity = 0;
}

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH, resend] sd: fix infinite kernel/udev loop on non-removable Medium Not Present

2013-04-12 Thread Steve Magnani

Commit eface65c336eff420d70beb0fb6787a732e05ffb (2.6.38) altered
set_media_not_present() in a way that prevents the sd driver from
remembering that a non-removable device has reported Medium Not Present.
This condition can occur on hotplug of a (i.e.) USB Mass Storage device
whose medium is offline due to an unrecoverable controller error,
but which is otherwise capable of SCSI communication (to download new 
microcode, etc.).

Under these conditions, the changed code results in an infinite loop
between the kernel and udevd. When udevd attempts to open the device
in response to a change notification, a SCSI Medium Not Present error
occurs which causes the kernel to signal another change. The cycle
repeats until the device is unplugged, resulting in udevd consuming ever-
increasing amounts of CPU and virtual memory.

Resolve this by remembering media not present whether the device has
declared itself removable or not.

Signed-off-by: Steven J. Magnani st...@digidescorp.com
---
--- a/drivers/scsi/sd.c 2013-04-12 14:16:12.252531097 -0500
+++ b/drivers/scsi/sd.c 2013-04-12 14:21:55.197216521 -0500
@@ -1298,10 +1298,8 @@ out:
 
 static void set_media_not_present(struct scsi_disk *sdkp)
 {
-   if (sdkp-media_present)
+   if (sdkp-media_present) {
sdkp-device-changed = 1;
-
-   if (sdkp-device-removable) {
sdkp-media_present = 0;
sdkp-capacity = 0;
}

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

61 matches

Mail list logo