| Message ID | 0cb0b314-e4f6-40a2-9628-0fe7d905a676@paragon-software.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bugfix and refactoring | expand |
On Wed, Apr 17, 2024 at 04:10:59PM +0300, Konstantin Komarov wrote: > When counting and checking hard links in an ntfs file record, > > struct MFT_REC { > struct NTFS_RECORD_HEADER rhdr; // 'FILE' > __le16 seq; // 0x10: Sequence number for this record. > >> __le16 hard_links; // 0x12: The number of hard links to record. > __le16 attr_off; // 0x14: Offset to attributes. > ... > > the ntfs3 driver ignored short names (DOS names), causing the link count > to be reduced by 1 and messages to be output to dmesg. I also reported seeing link counts being reduced by 2: [ 78.307412] ntfs3: nvme0n1p3: ino=34e6, Correct links count -> 1 (3). [ 78.307843] ntfs3: nvme0n1p3: ino=5bb23, Correct links count -> 1 (2). [ 78.308509] ntfs3: nvme0n1p3: ino=5c722, Correct links count -> 1 (2). [ 78.310018] ntfs3: nvme0n1p3: ino=5d761, Correct links count -> 1 (2). [ 78.310717] ntfs3: nvme0n1p3: ino=33d18, Correct links count -> 1 (3). [ 78.311179] ntfs3: nvme0n1p3: ino=5d75b, Correct links count -> 1 (3). [ 78.311605] ntfs3: nvme0n1p3: ino=5c708, Correct links count -> 1 (3). - https://lore.kernel.org/all/Zhz_axTjkJ6Aqeys@hovoldconsulting.com/ Are you sure there are not further issues with this code? > For Windows, such a situation is a minor error, meaning chkdsk does not > report > errors on such a volume, and in the case of using the /f switch, it silently > corrects them, reporting that no errors were found. This does not affect > the consistency of the file system. > > Nevertheless, the behavior in the ntfs3 driver is incorrect and > changes the content of the file system. This patch should fix that. This patch is white space damaged and does not apply. > PS: most likely, there has been a confusion of concepts > MFT_REC::hard_links and inode::__i_nlink. I'd also expect a Fixes and CC stable tag here. And as this patch does not seem to depend on the rest of the series it should go first (along with any other bug fixes). > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> Johan
On 18.04.2024 09:31, Johan Hovold wrote: > On Wed, Apr 17, 2024 at 04:10:59PM +0300, Konstantin Komarov wrote: >> When counting and checking hard links in an ntfs file record, >> >> struct MFT_REC { >> struct NTFS_RECORD_HEADER rhdr; // 'FILE' >> __le16 seq; // 0x10: Sequence number for this record. >> >> __le16 hard_links; // 0x12: The number of hard links to record. >> __le16 attr_off; // 0x14: Offset to attributes. >> ... >> >> the ntfs3 driver ignored short names (DOS names), causing the link count >> to be reduced by 1 and messages to be output to dmesg. > I also reported seeing link counts being reduced by 2: > > [ 78.307412] ntfs3: nvme0n1p3: ino=34e6, Correct links count -> 1 (3). > [ 78.307843] ntfs3: nvme0n1p3: ino=5bb23, Correct links count -> 1 (2). > [ 78.308509] ntfs3: nvme0n1p3: ino=5c722, Correct links count -> 1 (2). > [ 78.310018] ntfs3: nvme0n1p3: ino=5d761, Correct links count -> 1 (2). > [ 78.310717] ntfs3: nvme0n1p3: ino=33d18, Correct links count -> 1 (3). > [ 78.311179] ntfs3: nvme0n1p3: ino=5d75b, Correct links count -> 1 (3). > [ 78.311605] ntfs3: nvme0n1p3: ino=5c708, Correct links count -> 1 (3). > > - https://lore.kernel.org/all/Zhz_axTjkJ6Aqeys@hovoldconsulting.com/ > > Are you sure there are not further issues with this code? > >> For Windows, such a situation is a minor error, meaning chkdsk does not >> report >> errors on such a volume, and in the case of using the /f switch, it silently >> corrects them, reporting that no errors were found. This does not affect >> the consistency of the file system. >> >> Nevertheless, the behavior in the ntfs3 driver is incorrect and >> changes the content of the file system. This patch should fix that. > This patch is white space damaged and does not apply. > >> PS: most likely, there has been a confusion of concepts >> MFT_REC::hard_links and inode::__i_nlink. > I'd also expect a Fixes and CC stable tag here. > > And as this patch does not seem to depend on the rest of the series it > should go first (along with any other bug fixes). > >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > Johan Hi Johan, We are in the process of extending the tests for link counting and related scenarios. If I find bugs, I'll reply ASAP. Thanks for highlighting the bug.
diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c index ae4465bf099f..f98200b1a4d2 100644 --- a/fs/ntfs3/inode.c +++ b/fs/ntfs3/inode.c @@ -37,7 +37,7 @@ static struct inode *ntfs_read_mft(struct inode *inode, bool is_dir; unsigned long ino = inode->i_ino; u32 rp_fa = 0, asize, t32; - u16 roff, rsize, names = 0; + u16 roff, rsize, names = 0, links = 0; const struct ATTR_FILE_NAME *fname = NULL; const struct INDEX_ROOT *root; struct REPARSE_DATA_BUFFER rp; // 0x18 bytes @@ -200,11 +200,12 @@ static struct inode *ntfs_read_mft(struct inode *inode, rsize < SIZEOF_ATTRIBUTE_FILENAME) goto out; + names += 1; fname = Add2Ptr(attr, roff); if (fname->type == FILE_NAME_DOS) goto next_attr; - names += 1; + links += 1; if (name && name->len == fname->name_len && !ntfs_cmp_names_cpu(name, (struct le_str *)&fname->name_len, NULL, false)) @@ -429,7 +430,7 @@ static struct inode *ntfs_read_mft(struct inode *inode, ni->mi.dirty = true; } - set_nlink(inode, names); + set_nlink(inode, links); if (S_ISDIR(mode)) { ni->std_fa |= FILE_ATTRIBUTE_DIRECTORY; diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c index 6aa3a9d44df1..6c76503edc20 100644 --- a/fs/ntfs3/record.c +++ b/fs/ntfs3/record.c @@ -534,16 +534,9 @@ bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, if (aoff + asize > used) return false; - if (ni && is_attr_indexed(attr)) { + if (ni && is_attr_indexed(attr) && attr->type == ATTR_NAME) { u16 links = le16_to_cpu(ni->mi.mrec->hard_links); - struct ATTR_FILE_NAME *fname = - attr->type != ATTR_NAME ? - NULL : - resident_data_ex(attr, - SIZEOF_ATTRIBUTE_FILENAME); - if (fname && fname->type == FILE_NAME_DOS) { - /* Do not decrease links count deleting DOS name. */ - } else if (!links) { + if (!links) { /* minor error. Not critical. */ } else {
When counting and checking hard links in an ntfs file record, struct MFT_REC { struct NTFS_RECORD_HEADER rhdr; // 'FILE' __le16 seq; // 0x10: Sequence number for this record. >> __le16 hard_links; // 0x12: The number of hard links to record. __le16 attr_off; // 0x14: Offset to attributes. ... the ntfs3 driver ignored short names (DOS names), causing the link count to be reduced by 1 and messages to be output to dmesg. For Windows, such a situation is a minor error, meaning chkdsk does not report errors on such a volume, and in the case of using the /f switch, it silently corrects them, reporting that no errors were found. This does not affect the consistency of the file system. Nevertheless, the behavior in the ntfs3 driver is incorrect and changes the content of the file system. This patch should fix that. PS: most likely, there has been a confusion of concepts MFT_REC::hard_links and inode::__i_nlink. Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> --- fs/ntfs3/inode.c | 7 ++++--- fs/ntfs3/record.c | 11 ++--------- 2 files changed, 6 insertions(+), 12 deletions(-) ni->mi.mrec->hard_links = cpu_to_le16(links - 1);