Date: Fri, 28 Jun 2024 00:28:14 +0900
From: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Gwendal
 Grignou <gwendal@...omium.org>, dlunev@...omium.org
Subject: Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and
 .. entries

Thadeu Lima de Souza Cascardo <cascardo@...lia.com> writes:

>> First of all, I'm not thinking this is the fix, I'm thinking this as the
>> workaround of broken formatter (because the windows's fsck also think it
>> as broken). So very low priority to support.
>> 
>> As said, I also think low chance to break the userspace. However it
>> changes real offset to pseudo offset. So if userspace saved it to
>> persistent space, breaks userspace. Unlikely, but I think there is no
>> value to change the behavior for workaround.
>
> So I started doing some investigation and that lead me to the following
> code from fs/fat/inode.c:
>
> static void fat_evict_inode(struct inode *inode)
> {
> 	truncate_inode_pages_final(&inode->i_data);
> 	if (!inode->i_nlink) {
> 		inode->i_size = 0;
> 		fat_truncate_blocks(inode, 0);
> 	} else
> 		fat_free_eofblocks(inode);
> [...]
>
> That is, since the directory has no links, once it is evicted (which
> happens right after reading the number of subdirectories and failing
> verification), it is truncated. That means all clusters are marked as FREE.
> Then, later, if trying to fsck or mount this filesystem again, the
> directory entry is removed or further errors show up (as an EOF is
> expected, not a FREE cluster).
>
> And that is caused by attributing a number of 0 links. I looked it up on
> how other filesystems handle this situation and I found out that exfat adds
> 2 to the number of subdirectories, just as I am suggesting. When
> enumerating the directories (at its readdir), it also relies on
> dir_emit_dots for all cases.

Because exfat doesn't have "."/".." always, IIRC.

> As for programs persisting the offset, the manpage for telldir has on its
> NOTES section:
>
> """
> Application programs should treat this strictly as an opaque value, making
> no assumptions about its contents.
> """
>
> I know this doesn't refer to persisting or not that opaque value, but any
> other changes to the directory would change the offset of its current
> subdirectories and given those values are opaque, no assumptions should be
> made. And unless we find such programs in the wild, the same argunent could
> be made that there may be programs that expect . and .. to be at offset 0
> and 1, like every filesystem that uses dir_emit_dots does.
>
> I understand the cautiousness to prevent regressions, but I did the work
> here to test and understand the changes that are being proposed. I even
> looked into another way of preventing the further corruption, but that
> convinced me even more that the right fix is to assign a minimum number of
> links to directories and I found precedence to this.

I seriously recommend to change app that make this, or changing the fsck
to fix this. Because this looks like broken as FAT.

Honestly I'm not accepting willingly though, the way to add the
workaround for this would be, detect this breakage and warn it, then
mark the dir inode as broken. And add the workaround codes only for
broken dir inode, and make it work for all operations (just make
mountable and readable is not enough, at least write must not corrupt fs
or panic etc.), without changing the behavior of correct inodes.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>

Powered by blists - more mailing lists