Re: udf: Suspicious values in udf_statfs()

[Jan Kara <jack@suse.cz>]

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