ntfs: RO->RW remount is broken - two bugs in the quota-marking path

[Razvan Dinculescu]

Hi Namjae,

Thanks for the new in-kernel ntfs driver. read-only -> read-write remount
fails on any clean NTFS volume. I tracked it to TWO bugs in the quota-marking
path and verified the analysis by building a patched module against 7.1.1 and
confirming that fixing both makes remount,rw succeed.

(Resending in plain text - my earlier copy was rejected by the lists for
containing an HTML part.)

Reproducer
----------
Any clean NTFS volume (reproduced on two different disks):

mount -t ntfs -o ro /dev/sdXN /mnt/x
mount -o remount,rw /mnt/x # fails: -EROFS (exit 32)
umount /mnt/x; mount -t ntfs -o rw /dev/sdXN /mnt/x # works

ntfs_fill_super() never calls ntfs_mark_quotas_out_of_date(), so a fresh -o rw
mount is unaffected; only the RO->RW ntfs_reconfigure() path runs it.

dmesg on the failing remount:
ntfs: (device sdXN): ntfs_ir_lookup(): Failed to lookup $INDEX_ROOT
ntfs: (device sdXN): ntfs_mark_quotas_out_of_date(): Lookup of quota
defaults entry failed.
ntfs: (device sdXN): ntfs_reconfigure(): Failed to mark quotas out of
date. Cannot remount read-write.

Environment: kernel 7.1.1, x86_64, CONFIG_NTFS_FS=m. fs/ntfs code quoted from
torvalds/linux v7.1. (ntfs-3g/ntfsprogs not installed; in-kernel driver used.)

Bug 1 - wrong index name in ntfs_mark_quotas_out_of_date() (fs/ntfs/quota.c)
---------------------------------------------------------------------------
ictx = ntfs_index_ctx_get(NTFS_I(vol->quota_q_ino), I30, 4);

vol->quota_q_ino is the $Quota:$Q index (opened via
ntfs_index_iget(vol->quota_ino, Q, 2) at super.c:1235), but the lookup uses the
$I30 *directory* index name. ntfs_index_ctx_get() stores the name,
ntfs_index_lookup() forwards it to ntfs_ir_lookup() ->
ntfs_attr_lookup(AT_INDEX_ROOT, "$I30", ...), which finds no $I30 $INDEX_ROOT on
the $Q index -> ntfs_index_lookup() returns -EIO -> "Lookup of quota defaults
entry failed."

Note there is no global "$Q" name symbol (only I30 is global, defined in
dir.c), so the fix needs a $Q name added in quota.c, not just an argument swap.

Bug 2 - ntfs_index_lookup() returns the KEY, not the VALUE, for view indexes
---------------------------------------------------------------------------
With bug 1 fixed, remount then fails with "Quota defaults entry size is
invalid." In ntfs_index_lookup() the done: label (fs/ntfs/index.c:838) sets:

icx->entry = ie;
icx->data = (u8 *)ie + offsetof(struct index_entry, key);
icx->data_len = le16_to_cpu(ie->key_length);

i.e. the entry KEY and key length. For the $Q view index the key is the 4-byte
owner_id, so quota.c sees ictx->data_len == 4, which is < offsetof(
struct quota_control_entry, sid) (== 48), and bails. quota.c - and the
function's own kerneldoc at index.c:712 ("@icx->data and @icx->data_len are the
index entry data") - expect the entry's DATA (value), which for a view index
lives at ie->data.vi.data_offset / ie->data.vi.data_length.

This is correct for *directory* ($I30) lookups, where the key IS the data (e.g.
inode.c:2609 reads icx->data as a file_name_attr) - which is why it has gone
unnoticed - but wrong for view indexes whose consumers read icx->data as a
value; quota.c is the one that does.

Verification
------------
I built fs/ntfs as an out-of-tree module against 7.1.1 (clang, vermagic-matched)
and swapped it on a live kernel against an unpatched rebuild as control:

- unpatched rebuild : reproduces "Lookup of quota defaults entry failed"
- bug-1 fix only : advances to "Quota defaults entry size is invalid"
- both bugs fixed : `mount -o remount,rw` SUCCEEDS (exit 0); the volume
mounts read-write; directory reads still work.

Suggested fixes
---------------
1) quota.c: give it the "$Q" index name, e.g.
static __le16 Q[3] = { cpu_to_le16('$'), cpu_to_le16('Q'), 0 };
and pass (Q, 2) to ntfs_index_ctx_get().

2) For view indexes, expose the entry VALUE rather than the key. Either make
ntfs_index_lookup()'s done: label set icx->data/icx->data_len from
ie->data.vi.data_offset / ie->data.vi.data_length for view (non-directory)
indexes, or have the view consumer read the value from icx->entry. A blanket
change would break directory consumers like inode.c:2609 that rely on
data == key, so it must be type-aware. For testing I did the latter, locally
in quota.c, to avoid touching the directory path.

I haven't prepared a formal cross-index-type patch - flagging the two sites and
the mechanism. Happy to test any patch.

Thanks,
Razvan Dinculescu