minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
UDF 1.02. Previous UDF revisions used that area for implementation specific
data. So in this case do not touch these members.
To check if LVIDIU contain revisions members, first read UDF revision from
LVD. If revision is at least 1.02 LVIDIU should contain revision members.
This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
not touch, read overwrite implementation specific area of LVIDIU.
Signed-off-by: Pali Rohár <[email protected]>
---
fs/udf/super.c | 37 ++++++++++++++++++++++++++-----------
fs/udf/udf_sb.h | 3 +++
2 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 2d0b90800..8df6e9962 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
}
static int udf_verify_domain_identifier(struct super_block *sb,
- struct regid *ident, char *dname)
+ struct regid *ident, char *dname, u16 *udf_rev)
{
struct domainIdentSuffix *suffix;
@@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
goto force_ro;
}
suffix = (struct domainIdentSuffix *)ident->identSuffix;
+ if (udf_rev)
+ *udf_rev = le16_to_cpu(suffix->UDFRevision);
if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
(suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
if (!sb_rdonly(sb)) {
@@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
{
int ret;
- ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
+ ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
if (ret < 0)
return ret;
@@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
}
ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
- "logical volume");
+ "logical volume", &sbi->s_lvd_udfrev);
if (ret)
goto out_bh;
ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
@@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
mutex_lock(&sbi->s_alloc_mutex);
lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
- 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))
- 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);
+
+ /* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
+ * introduced in UDF 1.02. Previous UDF revisions used that area for
+ * implementation specific data. So in this case do not touch it. */
+ if (sbi->s_lvd_udfrev >= 0x0102) {
+ 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))
+ 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);
+ }
+
if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
@@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
ret = -EINVAL;
goto error_out;
}
- minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
- minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
+
+ if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
+ minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
+ minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
+ } else {
+ minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
+ }
+
if (minUDFReadRev > UDF_MAX_READ_VERSION) {
udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
minUDFReadRev,
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 3d83be54c..6bd0d4430 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -137,6 +137,9 @@ struct udf_sb_info {
/* Fileset Info */
__u16 s_serial_number;
+ /* LVD UDF revision filled to media at format time */
+ __u16 s_lvd_udfrev;
+
/* highest UDF revision we have recorded to this media */
__u16 s_udfrev;
--
2.20.1
On Sun 12-01-20 18:59:30, Pali Roh?r wrote:
> minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> UDF 1.02. Previous UDF revisions used that area for implementation specific
> data. So in this case do not touch these members.
>
> To check if LVIDIU contain revisions members, first read UDF revision from
> LVD. If revision is at least 1.02 LVIDIU should contain revision members.
>
> This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> not touch, read overwrite implementation specific area of LVIDIU.
>
> Signed-off-by: Pali Roh?r <[email protected]>
Maybe we could store the fs revision in the superblock as well to avoid
passing the udf_rev parameter?
Also this patch contains several lines over 80 columns.
Honza
> ---
> fs/udf/super.c | 37 ++++++++++++++++++++++++++-----------
> fs/udf/udf_sb.h | 3 +++
> 2 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 2d0b90800..8df6e9962 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
> }
>
> static int udf_verify_domain_identifier(struct super_block *sb,
> - struct regid *ident, char *dname)
> + struct regid *ident, char *dname, u16 *udf_rev)
> {
> struct domainIdentSuffix *suffix;
>
> @@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
> goto force_ro;
> }
> suffix = (struct domainIdentSuffix *)ident->identSuffix;
> + if (udf_rev)
> + *udf_rev = le16_to_cpu(suffix->UDFRevision);
> if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
> (suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
> if (!sb_rdonly(sb)) {
> @@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
> {
> int ret;
>
> - ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
> + ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
> if (ret < 0)
> return ret;
>
> @@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
> }
>
> ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
> - "logical volume");
> + "logical volume", &sbi->s_lvd_udfrev);
> if (ret)
> goto out_bh;
> ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
> @@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
> mutex_lock(&sbi->s_alloc_mutex);
> lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
> lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
> - 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))
> - 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);
> +
> + /* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
> + * introduced in UDF 1.02. Previous UDF revisions used that area for
> + * implementation specific data. So in this case do not touch it. */
> + if (sbi->s_lvd_udfrev >= 0x0102) {
> + 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))
> + 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);
> + }
> +
> if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
> lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
>
> @@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
> ret = -EINVAL;
> goto error_out;
> }
> - minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> - minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> +
> + if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
> + minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> + minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> + } else {
> + minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
> + }
> +
> if (minUDFReadRev > UDF_MAX_READ_VERSION) {
> udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
> minUDFReadRev,
> diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> index 3d83be54c..6bd0d4430 100644
> --- a/fs/udf/udf_sb.h
> +++ b/fs/udf/udf_sb.h
> @@ -137,6 +137,9 @@ struct udf_sb_info {
> /* Fileset Info */
> __u16 s_serial_number;
>
> + /* LVD UDF revision filled to media at format time */
> + __u16 s_lvd_udfrev;
> +
> /* highest UDF revision we have recorded to this media */
> __u16 s_udfrev;
>
> --
> 2.20.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Monday 13 January 2020 13:00:49 Jan Kara wrote:
> On Sun 12-01-20 18:59:30, Pali Rohár wrote:
> > minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> > UDF 1.02. Previous UDF revisions used that area for implementation specific
> > data. So in this case do not touch these members.
> >
> > To check if LVIDIU contain revisions members, first read UDF revision from
> > LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> >
> > This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> > not touch, read overwrite implementation specific area of LVIDIU.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
>
> Maybe we could store the fs revision in the superblock as well to avoid
> passing the udf_rev parameter?
Unfortunately not. Function udf_verify_domain_identifier() is called
also when parsing FSD. FSD is stored on partition map and e.g. Metadata
partition map depends on UDF revision. So it is not a good idea to
overwrite UDF revision from FSD. This is reason why I decided to use
initial UDF revision number only from LVD.
But whole stuff around UDF revision is a mess. UDF revision is stored on
these locations:
main LVD
reserve LVD
main IUVD
reserve IUVD
FSD
And optionally (when specific UDF feature is used) also on:
sparable partition map 1.50+
virtual partition map 1.50+
all sparing tables 1.50+
VAT 1.50
Plus tuple minimal read, minimal write, maximal write UDF revision is
stored on:
LVIDIU 1.02+
VAT 2.00+
VAT in 2.00+ format overrides information stored on LVIDIU.
> Also this patch contains several lines over 80 columns.
Ok, this is easy to solve.
> Honza
>
> > ---
> > fs/udf/super.c | 37 ++++++++++++++++++++++++++-----------
> > fs/udf/udf_sb.h | 3 +++
> > 2 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > index 2d0b90800..8df6e9962 100644
> > --- a/fs/udf/super.c
> > +++ b/fs/udf/super.c
> > @@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
> > }
> >
> > static int udf_verify_domain_identifier(struct super_block *sb,
> > - struct regid *ident, char *dname)
> > + struct regid *ident, char *dname, u16 *udf_rev)
> > {
> > struct domainIdentSuffix *suffix;
> >
> > @@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
> > goto force_ro;
> > }
> > suffix = (struct domainIdentSuffix *)ident->identSuffix;
> > + if (udf_rev)
> > + *udf_rev = le16_to_cpu(suffix->UDFRevision);
> > if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
> > (suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
> > if (!sb_rdonly(sb)) {
> > @@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
> > {
> > int ret;
> >
> > - ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
> > + ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
> > if (ret < 0)
> > return ret;
> >
> > @@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
> > }
> >
> > ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
> > - "logical volume");
> > + "logical volume", &sbi->s_lvd_udfrev);
> > if (ret)
> > goto out_bh;
> > ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
> > @@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
> > mutex_lock(&sbi->s_alloc_mutex);
> > lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
> > lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
> > - 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))
> > - 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);
> > +
> > + /* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
> > + * introduced in UDF 1.02. Previous UDF revisions used that area for
> > + * implementation specific data. So in this case do not touch it. */
> > + if (sbi->s_lvd_udfrev >= 0x0102) {
> > + 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))
> > + 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);
> > + }
> > +
> > if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
> > lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
> >
> > @@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
> > ret = -EINVAL;
> > goto error_out;
> > }
> > - minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> > - minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> > +
> > + if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
> > + minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> > + minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> > + } else {
> > + minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
> > + }
> > +
> > if (minUDFReadRev > UDF_MAX_READ_VERSION) {
> > udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
> > minUDFReadRev,
> > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > index 3d83be54c..6bd0d4430 100644
> > --- a/fs/udf/udf_sb.h
> > +++ b/fs/udf/udf_sb.h
> > @@ -137,6 +137,9 @@ struct udf_sb_info {
> > /* Fileset Info */
> > __u16 s_serial_number;
> >
> > + /* LVD UDF revision filled to media at format time */
> > + __u16 s_lvd_udfrev;
> > +
> > /* highest UDF revision we have recorded to this media */
> > __u16 s_udfrev;
> >
> > --
> > 2.20.1
> >
--
Pali Rohár
[email protected]
On Mon 13-01-20 19:37:28, Pali Roh?r wrote:
> On Monday 13 January 2020 13:00:49 Jan Kara wrote:
> > On Sun 12-01-20 18:59:30, Pali Roh?r wrote:
> > > minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> > > UDF 1.02. Previous UDF revisions used that area for implementation specific
> > > data. So in this case do not touch these members.
> > >
> > > To check if LVIDIU contain revisions members, first read UDF revision from
> > > LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> > >
> > > This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> > > not touch, read overwrite implementation specific area of LVIDIU.
> > >
> > > Signed-off-by: Pali Roh?r <[email protected]>
> >
> > Maybe we could store the fs revision in the superblock as well to avoid
> > passing the udf_rev parameter?
>
> Unfortunately not. Function udf_verify_domain_identifier() is called
> also when parsing FSD. FSD is stored on partition map and e.g. Metadata
> partition map depends on UDF revision. So it is not a good idea to
> overwrite UDF revision from FSD. This is reason why I decided to use
> initial UDF revision number only from LVD.
>
> But whole stuff around UDF revision is a mess. UDF revision is stored on
> these locations:
>
> main LVD
> reserve LVD
> main IUVD
> reserve IUVD
> FSD
>
> And optionally (when specific UDF feature is used) also on:
>
> sparable partition map 1.50+
> virtual partition map 1.50+
> all sparing tables 1.50+
> VAT 1.50
>
> Plus tuple minimal read, minimal write, maximal write UDF revision is
> stored on:
>
> LVIDIU 1.02+
> VAT 2.00+
>
> VAT in 2.00+ format overrides information stored on LVIDIU.
Thanks for the summary. This is indeed a mess in the standard so let's not
overcomplicate it. I agree with just taking the revision from 'main LVD'
and storing it in the superblock like you do in this patch. I'd just
slightly change your code so that extracting a revision from 'struct regid'
is a separate function and not "hidden" inside
udf_verify_domain_identifier(). There's no strong reason for combining
these two.
WRT parsing of minUDFReadRev and friends, I'd handle them similarly to
numDirs and numFiles. I'd initialize them to the version we've got from
LVD, then possibly override them in udf_load_logicalvolint(), and finally
possibly override them in udf_load_vat().
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR