udf: Suspicious values in udf_statfs()

udf: Suspicious values in udf_statfs()

[Pali Rohár]

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

Hello,

I looked at udf_statfs() implementation and I see there two things which
are probably incorrect:

First one:

	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;

If sbi->s_partition points to Metadata partition then reported number
of blocks seems to be incorrect. Similar like in udf_count_free().

Second one:

	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
					  le32_to_cpu(lvidiu->numDirs)) : 0)
			+ buf->f_bfree;

What f_files entry should report? Because result of sum of free blocks
and number of files+directories does not make sense for me.

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

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

Re: udf: Suspicious values in udf_statfs()

[Jan Kara]

Hello,

On Sun 12-01-20 17:23:11, Pali Rohár wrote:
> I looked at udf_statfs() implementation and I see there two things which
> are probably incorrect:
> 
> First one:
> 
> 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> 
> If sbi->s_partition points to Metadata partition then reported number
> of blocks seems to be incorrect. Similar like in udf_count_free().

Oh, right. This needs similar treatment like udf_count_free(). I'll fix it.
Thanks for spotting.

> Second one:
> 
> 	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> 					  le32_to_cpu(lvidiu->numDirs)) : 0)
> 			+ buf->f_bfree;
> 
> What f_files entry should report? Because result of sum of free blocks
> and number of files+directories does not make sense for me.

This is related to the fact that we return 'f_bfree' as the number of 'free
file nodes' in 'f_ffree'. And tools generally display f_files-f_ffree as
number of used inodes. In other words we treat every free block also as a
free 'inode' and report it in total amount of 'inodes'. I know this is not
very obvious but IMHO it causes the least confusion to users reading df(1)
output.

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

Re: udf: Suspicious values in udf_statfs()

[Pali Rohár]

On Monday 13 January 2020 13:08:51 Jan Kara wrote:
> Hello,
> 
> On Sun 12-01-20 17:23:11, Pali Rohár wrote:
> > I looked at udf_statfs() implementation and I see there two things which
> > are probably incorrect:
> > 
> > First one:
> > 
> > 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > 
> > If sbi->s_partition points to Metadata partition then reported number
> > of blocks seems to be incorrect. Similar like in udf_count_free().
> 
> Oh, right. This needs similar treatment like udf_count_free(). I'll fix it.
> Thanks for spotting.

Ok.

> > Second one:
> > 
> > 	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > 					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > 			+ buf->f_bfree;
> > 
> > What f_files entry should report? Because result of sum of free blocks
> > and number of files+directories does not make sense for me.
> 
> This is related to the fact that we return 'f_bfree' as the number of 'free
> file nodes' in 'f_ffree'. And tools generally display f_files-f_ffree as
> number of used inodes. In other words we treat every free block also as a
> free 'inode' and report it in total amount of 'inodes'. I know this is not
> very obvious but IMHO it causes the least confusion to users reading df(1)
> output.

So current code which returns sum of free blocks and number of
files+directories is correct. Could be this information about statvfs
f_files somewhere documented? Because this is not really obvious nor for
userspace applications which use statvfs() nor for kernel filesystem
drivers.

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

Re: udf: Suspicious values in udf_statfs()

[Jan Kara]

On Thu 16-01-20 16:30:19, Pali Rohár wrote:
> On Monday 13 January 2020 13:08:51 Jan Kara wrote:
> > > Second one:
> > > 
> > > 	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > > 					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > > 			+ buf->f_bfree;
> > > 
> > > What f_files entry should report? Because result of sum of free blocks
> > > and number of files+directories does not make sense for me.
> > 
> > This is related to the fact that we return 'f_bfree' as the number of 'free
> > file nodes' in 'f_ffree'. And tools generally display f_files-f_ffree as
> > number of used inodes. In other words we treat every free block also as a
> > free 'inode' and report it in total amount of 'inodes'. I know this is not
> > very obvious but IMHO it causes the least confusion to users reading df(1)
> > output.
> 
> So current code which returns sum of free blocks and number of
> files+directories is correct. Could be this information about statvfs
> f_files somewhere documented? Because this is not really obvious nor for
> userspace applications which use statvfs() nor for kernel filesystem
> drivers.

Well, I can certainly add a comment to udf_statfs(). Documenting in some
manpage might be worth it but I'm not 100% sure where - maybe directly in
the statfs(2) to the NOTES section? Also note that this behavior is not
unique to UDF - e.g. XFS also does the same.

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

Re: udf: Suspicious values in udf_statfs()

[Pali Rohár]

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

On Friday 17 January 2020 12:38:33 Jan Kara wrote:
> On Thu 16-01-20 16:30:19, Pali Rohár wrote:
> > On Monday 13 January 2020 13:08:51 Jan Kara wrote:
> > > > Second one:
> > > > 
> > > > 	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > > > 					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > > > 			+ buf->f_bfree;
> > > > 
> > > > What f_files entry should report? Because result of sum of free blocks
> > > > and number of files+directories does not make sense for me.
> > > 
> > > This is related to the fact that we return 'f_bfree' as the number of 'free
> > > file nodes' in 'f_ffree'. And tools generally display f_files-f_ffree as
> > > number of used inodes. In other words we treat every free block also as a
> > > free 'inode' and report it in total amount of 'inodes'. I know this is not
> > > very obvious but IMHO it causes the least confusion to users reading df(1)
> > > output.
> > 
> > So current code which returns sum of free blocks and number of
> > files+directories is correct. Could be this information about statvfs
> > f_files somewhere documented? Because this is not really obvious nor for
> > userspace applications which use statvfs() nor for kernel filesystem
> > drivers.
> 
> Well, I can certainly add a comment to udf_statfs().

Adding comment directly to udf_statfs() would really help.

> Documenting in some
> manpage might be worth it but I'm not 100% sure where - maybe directly in
> the statfs(2) to the NOTES section? Also note that this behavior is not
> unique to UDF - e.g. XFS also does the same.

I think it make sense to write these information into statfs(2) manpage.
And specially if this is implemented in more filesystems. Also this
structure is exported into statvfs(3) call too.

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

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

Re: udf: Suspicious values in udf_statfs()

[Jan Kara]

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

On Mon 13-01-20 13:08:51, Jan Kara wrote:
> Hello,
> 
> On Sun 12-01-20 17:23:11, Pali Rohár wrote:
> > I looked at udf_statfs() implementation and I see there two things which
> > are probably incorrect:
> > 
> > First one:
> > 
> > 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > 
> > If sbi->s_partition points to Metadata partition then reported number
> > of blocks seems to be incorrect. Similar like in udf_count_free().
> 
> Oh, right. This needs similar treatment like udf_count_free(). I'll fix it.
> Thanks for spotting.

Patch for this is attached.

								Honza

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

[-- Attachment #2: 0001-udf-Handle-metadata-partition-better-for-statfs-2.patch --]
[-- Type: text/x-patch, Size: 1544 bytes --]

From 2e831a1fb4fa4a6843e154edb1d9e80a1c610803 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Fri, 17 Jan 2020 12:41:39 +0100
Subject: [PATCH] udf: Handle metadata partition better for statfs(2)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When the filesystem uses Metadata partition, we need to look at the
underlying partition to get real total number of blocks in the
filesystem. Do the necessary remapping in udf_statfs().

Reported-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index f747bf72edbe..3bb9793db3aa 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2395,11 +2395,17 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct udf_sb_info *sbi = UDF_SB(sb);
 	struct logicalVolIntegrityDescImpUse *lvidiu;
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
+	unsigned int part = sbi->s_partition;
+	int ptype = sbi->s_partmaps[part].s_partition_type;
 
+	if (ptype == UDF_METADATA_MAP25) {
+		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
+							s_phys_partition_ref;
+	}
 	lvidiu = udf_sb_lvidiu(sb);
 	buf->f_type = UDF_SUPER_MAGIC;
 	buf->f_bsize = sb->s_blocksize;
-	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
+	buf->f_blocks = sbi->s_partmaps[part].s_partition_len;
 	buf->f_bfree = udf_count_free(sb);
 	buf->f_bavail = buf->f_bfree;
 	/*
-- 
2.16.4

Re: udf: Suspicious values in udf_statfs()

[Pali Rohár]

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

On Friday 17 January 2020 13:05:11 Jan Kara wrote:
> On Mon 13-01-20 13:08:51, Jan Kara wrote:
> > Hello,
> > 
> > On Sun 12-01-20 17:23:11, Pali Rohár wrote:
> > > I looked at udf_statfs() implementation and I see there two things which
> > > are probably incorrect:
> > > 
> > > First one:
> > > 
> > > 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > > 
> > > If sbi->s_partition points to Metadata partition then reported number
> > > of blocks seems to be incorrect. Similar like in udf_count_free().
> > 
> > Oh, right. This needs similar treatment like udf_count_free(). I'll fix it.
> > Thanks for spotting.
> 
> Patch for this is attached.

I was wrong. After reading UDF specification and kernel implementation
again I realized that there is a complete mess what "partition" means.

Sometimes it is Partition Map, sometimes it is Partition Descriptor,
sometimes it is index of Partition Map, sometimes index of Partition
Descriptor, sometimes it refers to data referenced by Partition
Descriptor and sometimes it refers to data referenced by Partition Map.

And "length" means length of any of above structure (either of map
structure, either of data pointed by map structure, either of descriptor
or either of data pointed by descriptor).

So to make it clear, member "s_partition_len" refers to length of data
pointed by Partition Descriptor, therefore length of physical partition.

As kernel probably does not support UDF fs with more then one Partition
Descriptor, whatever Partition Map we choose (s_partmaps[i] member) we
would always get same value in "s_partition_len" as it does not refer to
data of Partition Map, but rather to data of Partition Descriptor.

As both Metadata Partition Map and Type 1 Partition Map (or Sparable
Partition Map) shares same Partition Descriptor, this patch does not
change value of "f_blocks" member and therefore is not needed at all.
So current code should be correct.

But please, double check that I'm correct as "partition" naming in
probably every one variable is misleading.

Just to note, free space is calculated from Partition Map index
(not from Partition Descriptor index), therefore previous patch for
udf_count_free() is really needed and should be correct.

> From 2e831a1fb4fa4a6843e154edb1d9e80a1c610803 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Fri, 17 Jan 2020 12:41:39 +0100
> Subject: [PATCH] udf: Handle metadata partition better for statfs(2)
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> When the filesystem uses Metadata partition, we need to look at the
> underlying partition to get real total number of blocks in the
> filesystem. Do the necessary remapping in udf_statfs().
> 
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/super.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index f747bf72edbe..3bb9793db3aa 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -2395,11 +2395,17 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	struct udf_sb_info *sbi = UDF_SB(sb);
>  	struct logicalVolIntegrityDescImpUse *lvidiu;
>  	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
> +	unsigned int part = sbi->s_partition;
> +	int ptype = sbi->s_partmaps[part].s_partition_type;
>  
> +	if (ptype == UDF_METADATA_MAP25) {
> +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> +							s_phys_partition_ref;
> +	}
>  	lvidiu = udf_sb_lvidiu(sb);
>  	buf->f_type = UDF_SUPER_MAGIC;
>  	buf->f_bsize = sb->s_blocksize;
> -	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> +	buf->f_blocks = sbi->s_partmaps[part].s_partition_len;
>  	buf->f_bfree = udf_count_free(sb);
>  	buf->f_bavail = buf->f_bfree;
>  	/*

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

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

Re: udf: Suspicious values in udf_statfs()

[Jan Kara]

On Sat 18-01-20 01:11:07, Pali Rohár wrote:
> On Friday 17 January 2020 13:05:11 Jan Kara wrote:
> > On Mon 13-01-20 13:08:51, Jan Kara wrote:
> > > Hello,
> > > 
> > > On Sun 12-01-20 17:23:11, Pali Rohár wrote:
> > > > I looked at udf_statfs() implementation and I see there two things which
> > > > are probably incorrect:
> > > > 
> > > > First one:
> > > > 
> > > > 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > > > 
> > > > If sbi->s_partition points to Metadata partition then reported number
> > > > of blocks seems to be incorrect. Similar like in udf_count_free().
> > > 
> > > Oh, right. This needs similar treatment like udf_count_free(). I'll fix it.
> > > Thanks for spotting.
> > 
> > Patch for this is attached.
> 
> I was wrong. After reading UDF specification and kernel implementation
> again I realized that there is a complete mess what "partition" means.
> 
> Sometimes it is Partition Map, sometimes it is Partition Descriptor,
> sometimes it is index of Partition Map, sometimes index of Partition
> Descriptor, sometimes it refers to data referenced by Partition
> Descriptor and sometimes it refers to data referenced by Partition Map.
> 
> And "length" means length of any of above structure (either of map
> structure, either of data pointed by map structure, either of descriptor
> or either of data pointed by descriptor).
> 
> So to make it clear, member "s_partition_len" refers to length of data
> pointed by Partition Descriptor, therefore length of physical partition.

Yes, now that you say it I remember :)

> As kernel probably does not support UDF fs with more then one Partition
> Descriptor, whatever Partition Map we choose (s_partmaps[i] member) we
> would always get same value in "s_partition_len" as it does not refer to
> data of Partition Map, but rather to data of Partition Descriptor.
> 
> As both Metadata Partition Map and Type 1 Partition Map (or Sparable
> Partition Map) shares same Partition Descriptor, this patch does not
> change value of "f_blocks" member and therefore is not needed at all.
> So current code should be correct.
> 
> But please, double check that I'm correct as "partition" naming in
> probably every one variable is misleading.
> 
> Just to note, free space is calculated from Partition Map index
> (not from Partition Descriptor index), therefore previous patch for
> udf_count_free() is really needed and should be correct.

Right, I've checked the code and I completely agree with your analysis so
I've dropped the patch from my tree. Thanks for checking!

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