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

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

[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.

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

[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 <steve@digidescorp.com>
---
--- a/fs/udf/super.c	2019-02-08 10:30:00.397978609 -0600
+++ b/fs/udf/super.c	2019-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(&lvid->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(&lvid->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(&lvid->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(&lvid->descTag);
+	udf_finalize_lvid(lvid);
 	mark_buffer_dirty(bh);
 	sbi->s_lvid_dirty = 0;
 	mutex_unlock(&sbi->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(&lvid->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(&lvid->descTag);
+	udf_finalize_lvid(lvid);
 	mark_buffer_dirty(bh);
 	sbi->s_lvid_dirty = 0;
 	mutex_unlock(&sbi->s_alloc_mutex);

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

[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 <steve@digidescorp.com>
---
--- a/fs/udf/super.c	2019-02-08 10:39:48.351039434 -0600
+++ b/fs/udf/super.c	2019-02-08 10:42:26.017400020 -0600
@@ -1856,8 +1856,8 @@ u64 lvid_get_unique_id(struct super_bloc
 	if (!(++uniqueID & 0xFFFFFFFF))
 		uniqueID += 16;
 	lvhd->uniqueID = cpu_to_le64(uniqueID);
+	udf_updated_lvid(sb);
 	mutex_unlock(&sbi->s_alloc_mutex);
-	mark_buffer_dirty(bh);
 
 	return ret;
 }
@@ -2154,11 +2154,20 @@ static int udf_sync_fs(struct super_bloc
 
 	mutex_lock(&sbi->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;
 	}

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

[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 <standard.disclaimer>

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

[Jan Kara]

On Fri 08-02-19 22:06:59, Steve Magnani wrote:
> 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.

Thanks for the patches! I've added them to my tree. Just they seem to be
based on some relatively old kernel which required me to fixup some patch
conflicts. Please try to base your patches on current upstream kernel if
possible. Thanks!

> "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.

Ah, I'm sorry to hear that.

> 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.

Agreed. OSTA UDF standard requires LVID to be present so forcing read-only
mount is perfectly fine.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR