Re: [PATCH] ntfs: add code to make FIBMAP ioctl work

[Sergei Antonov <saproj@gmail.com>]

On 16 October 2014 17:36, Anton Altaparmakov <anton@tuxera.com> wrote:
> Hi Sergei,
>
> On 16 Oct 2014, at 16:07, Sergei Antonov <saproj@gmail.com> wrote:
>> I looked at your code and do not quite understand the handling of
>> initialized size. You read it into a local copy while holding the
>> lock, but then after the lock is released it may change and, I'm
>> afraid, cause the same problems you mentioned.
>
> Correct.  That is why FIBMAP is a stupid interface!  However initialized_size can only ever go up and not down so in the worst case you will get result zero, i.e. hole instead of the now real sector number you could access.
>
> The only time it can shrink is when the file size shrinks and I am afraid if anyone is using FIBMAP against a file that can be truncated they will corrupt the file system!

I've tried to merge all your arguments in my head :) and concluded
that a practical importance of the check against initialized_size is
exaggerated :).

It is not a reliable protection (API safety issues) implemented in a
way that it may not always work (limited locking scope). Can be an
indicator of programmer's diligence, though - "you see, I care about
it!". Sorry for this ramble. Your patch will do the job.

>> What is an example of a platform with 32-bit sector_t? Interesting to know.
>
> Any 32-bit system that does not configure the kernel for 64-bit sector_t.  Sorry can't remember the .config option name at the moment.  Would need to look it up.

Thanks. Found it. It is CONFIG_LBDAF.

>> Is it really possible that ntfs_bmap() is called for non-data
>> attributes? In other words, what other attributes are accessible as
>> files? Just curious.
>
> I admit in current kernel driver it cannot.  It might in future - reparse points for example are very odd in many ways and though they are often indicated as directories rather than files they could not always be and what attributes a reparse point has is anyones guess - depends on the reparse point.
>
> Then there are system files that do not have an unnamed $DATA attribute at all so in a way the check should probably be extended to ensure the attribute is $DATA _and_ unnamed but again anyone using FIBMAP on a system file is asking to cause problems...

Aren't those files handled by 'ntfs_mst_aops' (the one without a bmap
function) rather than 'ntfs_normal_aops'?