UDF & open integrity type

UDF & open integrity type

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi! After doing more experiments with UDF Integrity Type stored in UDF
Logical Volume Integrity Descriptor, it looks like for me that kernel
udf fs driver does not implement handling of open integrity correctly.

Open Integrity Type (except on VAT disk) mean that filesystem was last
time improperly unmounted, e.g. removable media was disconnected when
some write operation was in progress or when umount on Linux was not
issued.

Filesystem with Open Integrity type should be checked for error and
ideally also repaired prior next write (or maybe also read) operations.
Probably Open Integrity type can be treated on Linux as Dirty bit in FAT
filesystem.

But it looks like that kernel udf driver automatically change Open
Integrity Type to Closed when doing unmount, even when Type was Open
prior to UDF mount. For me it does not seem to be correct as kernel
itself does not implement any repair/recovery for UDF filesystem and
automatically lost any information that chkdsk/fsck should be run.

As there is planned fsck.udf, it is really important to have correct
information of successful umount command stored in filesystem itself.
This can help fsck to know last state of filesystem, like Dirty bit in
FAT or clean state of journal for ext*.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: UDF & open integrity type

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi,

On Mon 26-02-18 20:32:25, Pali Roh�r wrote:
> Hi! After doing more experiments with UDF Integrity Type stored in UDF
> Logical Volume Integrity Descriptor, it looks like for me that kernel
> udf fs driver does not implement handling of open integrity correctly.
> 
> Open Integrity Type (except on VAT disk) mean that filesystem was last
> time improperly unmounted, e.g. removable media was disconnected when
> some write operation was in progress or when umount on Linux was not
> issued.
> 
> Filesystem with Open Integrity type should be checked for error and
> ideally also repaired prior next write (or maybe also read) operations.
> Probably Open Integrity type can be treated on Linux as Dirty bit in FAT
> filesystem.
> 
> But it looks like that kernel udf driver automatically change Open
> Integrity Type to Closed when doing unmount, even when Type was Open
> prior to UDF mount. For me it does not seem to be correct as kernel
> itself does not implement any repair/recovery for UDF filesystem and
> automatically lost any information that chkdsk/fsck should be run.
> 
> As there is planned fsck.udf, it is really important to have correct
> information of successful umount command stored in filesystem itself.
> This can help fsck to know last state of filesystem, like Dirty bit in
> FAT or clean state of journal for ext*.

Yeah, it makes sense to keep LVID in open state if it was like that when we
first saw it. Attached patch should do what you ask for. I'll just probably
silence the warning until fsck.udf actually works...

								Honza

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

[-- Attachment #2: 0001-udf-Do-not-mark-possibly-inconsistent-filesystems-as.patch --]
[-- Type: text/x-patch, Size: 2403 bytes --]

>From 8aece18ef57588e275be5e2fdcdc00473ee386f3 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 27 Feb 2018 18:55:31 +0100
Subject: [PATCH] udf: Do not mark possibly inconsistent filesystems as closed

If logical volume integrity descriptor contains non-closed integrity
type when mounting the volume, there are high chances that the volume is
not consistent (device was detached before the filesystem was
unmounted). Warn when mounting such volume and don't touch integrity
type of the volume so that fsck can recognize it and check such
filesystem.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c  | 11 +++++++++--
 fs/udf/udf_sb.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 2d4929fa884d..16861ee3b5d5 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1988,7 +1988,13 @@ static void udf_open_lvid(struct super_block *sb)
 	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
 	ktime_get_real_ts(&ts);
 	udf_time_to_disk_stamp(&lvid->recordingDateAndTime, ts);
-	lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
+	if (le32_to_cpu(lvid->integrityType) == LVID_INTEGRITY_TYPE_CLOSE) {
+		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
+	} else {
+		UDF_SET_FLAG(sb, UDF_FLAG_NEEDCHECK);
+		udf_warn(sb, "volume need not be in consistent state. Running "
+				"fsck is recommended.\n");
+	}
 
 	lvid->descTag.descCRC = cpu_to_le16(
 		crc_itu_t(0, (char *)lvid + sizeof(struct tag),
@@ -2028,7 +2034,8 @@ static void udf_close_lvid(struct super_block *sb)
 		lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
 	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
 		lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
-	lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
+	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_NEEDCHECK))
+		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
 
 	lvid->descTag.descCRC = cpu_to_le16(
 			crc_itu_t(0, (char *)lvid + sizeof(struct tag),
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 9dcb475fc74e..02e3aa95d491 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -29,6 +29,7 @@
 #define UDF_FLAG_SESSION_SET	15
 #define UDF_FLAG_LASTBLOCK_SET	16
 #define UDF_FLAG_BLOCKSIZE_SET	17
+#define UDF_FLAG_NEEDCHECK	18
 
 #define UDF_PART_FLAG_UNALLOC_BITMAP	0x0001
 #define UDF_PART_FLAG_UNALLOC_TABLE	0x0002
-- 
2.13.6

Re: UDF & open integrity type

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Tuesday 27 February 2018 19:01:14 Jan Kara wrote:
> Yeah, it makes sense to keep LVID in open state if it was like that when we
> first saw it. Attached patch should do what you ask for. I'll just probably
> silence the warning until fsck.udf actually works...

> @@ -1988,7 +1988,13 @@ static void udf_open_lvid(struct super_block *sb)
>  	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
>  	ktime_get_real_ts(&ts);
>  	udf_time_to_disk_stamp(&lvid->recordingDateAndTime, ts);
> -	lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
> +	if (le32_to_cpu(lvid->integrityType) == LVID_INTEGRITY_TYPE_CLOSE) {
> +		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
> +	} else {
> +		UDF_SET_FLAG(sb, UDF_FLAG_NEEDCHECK);
> +		udf_warn(sb, "volume need not be in consistent state. Running "
> +				"fsck is recommended.\n");

Maybe just?

		udf_warn(sb, "volume need not be in consistent state.\n");

And instead of UDF_FLAG_NEEDCHECK probably UDF_FLAG_INCONSISTENT?

But patch looks good.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: UDF & open integrity type

[Jan Kara]

On Tue 27-02-18 20:40:05, Pali Roh�r wrote:
> On Tuesday 27 February 2018 19:01:14 Jan Kara wrote:
> > Yeah, it makes sense to keep LVID in open state if it was like that when we
> > first saw it. Attached patch should do what you ask for. I'll just probably
> > silence the warning until fsck.udf actually works...
> 
> > @@ -1988,7 +1988,13 @@ static void udf_open_lvid(struct super_block *sb)
> >  	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
> >  	ktime_get_real_ts(&ts);
> >  	udf_time_to_disk_stamp(&lvid->recordingDateAndTime, ts);
> > -	lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
> > +	if (le32_to_cpu(lvid->integrityType) == LVID_INTEGRITY_TYPE_CLOSE) {
> > +		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN);
> > +	} else {
> > +		UDF_SET_FLAG(sb, UDF_FLAG_NEEDCHECK);
> > +		udf_warn(sb, "volume need not be in consistent state. Running "
> > +				"fsck is recommended.\n");
> 
> Maybe just?
> 
> 		udf_warn(sb, "volume need not be in consistent state.\n");

Well, until there's a tool to fix the warning, I don't want to emit any as
it has higher chances of confusing users than doing any good to them :)
After all so far we've got away with just overwriting the integrity type
and nobody complained ;).

> And instead of UDF_FLAG_NEEDCHECK probably UDF_FLAG_INCONSISTENT?

Yeah, that's a better name.

> But patch looks good.

Thanks.

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