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

[Sergei Antonov <saproj@gmail.com>]

On 16 October 2014 15:20, Anton Altaparmakov <anton@tuxera.com> wrote:
> Hi Sergei,
>
> On 15 Oct 2014, at 20:19, Sergei Antonov <saproj@gmail.com> wrote:
>> Implement ntfs_bmap() function for the "bmap" address space operation.
>> Now user space programs can send FIBMAP ioctl to get files' placement on
>> NTFS the same way they can do it on a number of other linux filesystems.
>>
>> The result is returned in FIGETBSZ units, which is equal to sector size
>> in ntfs driver. An error value zero is returned for resident, compressed,
>> encrypted files, and for holes in sparse files.
>>
>> Tested on drives with different sector sizes and different cluster sizes.
>
> Thanks for your patch.  It skirts/looks over some limitations so instead of applying your patch I have open sourced the bmap implementation from Tuxera NTFS driver (with permission) and have pushed it to the open source NTFS tree at git://git.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs.git.

Cool. Thanks.
It is good you liberate the code.

> I have sent a pull request to Linus so he will hopefully merge it into mainline.
>
> You can compare it to yours but most important points are that you do not forbid non-data attributes and you completely ignore initialized size which leads to exposing stale on-disk data on read and to losing the written data on writes which is obviously bad.  You also don't verify that the value to be returned isn't too large to fit in the return value size so you can potentially return truncated values on 32-bit kernels with 32-bit sector_t on a large volume where the sector number would need 64-bits which would then lead to disk corruption as the user would read/write random sectors on disk instead of the correct ones.

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.

What is an example of a platform with 32-bit sector_t? Interesting to know.

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.