[PATCH v2 0/2] fat: add support for directories without . and .. entries

[PATCH v2 0/2] fat: add support for directories without . and .. entries

[Thadeu Lima de Souza Cascardo]

Some FAT filesystems do not have . and .. entries in some directories.
Currently, such filesystems are not mounted because such directories will
have no links. They are also corrupted as inodes are evicted and that leads
to such directories clusters being marked as freed. Later mounts will then
error out when finding such clusters.

These two commits allow those filesystems to be mounted and . and .. to
still appear when listing such directories.

v2:
- Also ignore the absence of . directory and always have at least two links.
- Add a second commit to always emit . and .. at readdir.

Thadeu Lima de Souza Cascardo (2):
  fat: ignore . and .. subdirs and always add links to dirs
  fat: always use dir_emit_dots and ignore . and .. entries

 fs/fat/dir.c   | 28 ++++++++++++----------------
 fs/fat/inode.c |  2 +-
 2 files changed, 13 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] fat: ignore . and .. subdirs and always add links to dirs

[Thadeu Lima de Souza Cascardo]

The tools used for creating images for the Lego Mindstrom EV3 are not
adding '.' and '..' entry in the 'Projects' directory.

Without this fix, the kernel can not fill the inode structure for
'Projects' directory.

See https://github.com/microsoft/pxt-ev3/issues/980
And https://github.com/microsoft/uf2-linux/issues/6

When counting the number of subdirs, ignore . and .. subdirs and add two
when setting the initial link count for directories. This way, the number
of links is always correctly accounted for.

With this fix applied, we can mount an image with such empty directories,
access them, create subdirectories and remove them.

This also prevents corrupting such filesystems as when the inodes would be
put, since no links were accounted for, all of its clusters would be marked
as free.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Link: https://lore.kernel.org/all/20220204062232.3410036-1-gwendal@chromium.org/
Cc: dlunev@chromium.org
---
 fs/fat/dir.c   | 4 +++-
 fs/fat/inode.c | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index acbec5bdd521..4e4a359a1ea3 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -949,7 +949,9 @@ int fat_subdirs(struct inode *dir)
 	bh = NULL;
 	cpos = 0;
 	while (fat_get_short_entry(dir, &cpos, &bh, &de) >= 0) {
-		if (de->attr & ATTR_DIR)
+		if (de->attr & ATTR_DIR &&
+		    strncmp(de->name, MSDOS_DOT   , MSDOS_NAME) &&
+		    strncmp(de->name, MSDOS_DOTDOT, MSDOS_NAME))
 			count++;
 	}
 	brelse(bh);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index d9e6fbb6f246..234c244d1252 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -534,7 +534,7 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 			return error;
 		MSDOS_I(inode)->mmu_private = inode->i_size;
 
-		set_nlink(inode, fat_subdirs(inode));
+		set_nlink(inode, fat_subdirs(inode)+2);
 
 		error = fat_validate_dir(inode);
 		if (error < 0)
-- 
2.34.1

[PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[Thadeu Lima de Souza Cascardo]

Instead of only using dir_emit_dots for the root inode and explictily
requiring the . and .. entries to emit them, use dir_emit_dots for all
directories.

That allows filesystems with directories without the . or .. entries to
still show them.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 fs/fat/dir.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 4e4a359a1ea3..e70781569de5 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -583,15 +583,14 @@ static int __fat_readdir(struct inode *inode, struct file *file,
 	mutex_lock(&sbi->s_lock);
 
 	cpos = ctx->pos;
-	/* Fake . and .. for the root directory. */
-	if (inode->i_ino == MSDOS_ROOT_INO) {
-		if (!dir_emit_dots(file, ctx))
-			goto out;
-		if (ctx->pos == 2) {
-			fake_offset = 1;
-			cpos = 0;
-		}
+
+	if (!dir_emit_dots(file, ctx))
+		goto out;
+	if (ctx->pos == 2) {
+		fake_offset = 1;
+		cpos = 0;
 	}
+
 	if (cpos & (sizeof(struct msdos_dir_entry) - 1)) {
 		ret = -ENOENT;
 		goto out;
@@ -671,13 +670,8 @@ static int __fat_readdir(struct inode *inode, struct file *file,
 	if (fake_offset && ctx->pos < 2)
 		ctx->pos = 2;
 
-	if (!memcmp(de->name, MSDOS_DOT, MSDOS_NAME)) {
-		if (!dir_emit_dot(file, ctx))
-			goto fill_failed;
-	} else if (!memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
-		if (!dir_emit_dotdot(file, ctx))
-			goto fill_failed;
-	} else {
+	if (memcmp(de->name, MSDOS_DOT, MSDOS_NAME) &&
+	    memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
 		unsigned long inum;
 		loff_t i_pos = fat_make_i_pos(sb, bh, de);
 		struct inode *tmp = fat_iget(sb, i_pos);
-- 
2.34.1

Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[OGAWA Hirofumi]

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

> Instead of only using dir_emit_dots for the root inode and explictily
> requiring the . and .. entries to emit them, use dir_emit_dots for all
> directories.
>
> That allows filesystems with directories without the . or .. entries to
> still show them.

Unacceptable to change the correct behavior to broken format. And
unlikely break the userspace, however this still has the user visible
change of seek pos.

Thanks.

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  fs/fat/dir.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 4e4a359a1ea3..e70781569de5 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -583,15 +583,14 @@ static int __fat_readdir(struct inode *inode, struct file *file,
>  	mutex_lock(&sbi->s_lock);
>  
>  	cpos = ctx->pos;
> -	/* Fake . and .. for the root directory. */
> -	if (inode->i_ino == MSDOS_ROOT_INO) {
> -		if (!dir_emit_dots(file, ctx))
> -			goto out;
> -		if (ctx->pos == 2) {
> -			fake_offset = 1;
> -			cpos = 0;
> -		}
> +
> +	if (!dir_emit_dots(file, ctx))
> +		goto out;
> +	if (ctx->pos == 2) {
> +		fake_offset = 1;
> +		cpos = 0;
>  	}
> +
>  	if (cpos & (sizeof(struct msdos_dir_entry) - 1)) {
>  		ret = -ENOENT;
>  		goto out;
> @@ -671,13 +670,8 @@ static int __fat_readdir(struct inode *inode, struct file *file,
>  	if (fake_offset && ctx->pos < 2)
>  		ctx->pos = 2;
>  
> -	if (!memcmp(de->name, MSDOS_DOT, MSDOS_NAME)) {
> -		if (!dir_emit_dot(file, ctx))
> -			goto fill_failed;
> -	} else if (!memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
> -		if (!dir_emit_dotdot(file, ctx))
> -			goto fill_failed;
> -	} else {
> +	if (memcmp(de->name, MSDOS_DOT, MSDOS_NAME) &&
> +	    memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
>  		unsigned long inum;
>  		loff_t i_pos = fat_make_i_pos(sb, bh, de);
>  		struct inode *tmp = fat_iget(sb, i_pos);

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[Thadeu Lima de Souza Cascardo]

On Wed, Jun 26, 2024 at 06:47:15AM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> > Instead of only using dir_emit_dots for the root inode and explictily
> > requiring the . and .. entries to emit them, use dir_emit_dots for all
> > directories.
> >
> > That allows filesystems with directories without the . or .. entries to
> > still show them.
> 
> Unacceptable to change the correct behavior to broken format. And
> unlikely break the userspace, however this still has the user visible
> change of seek pos.
> 
> Thanks.
> 

I agree that if this breaks userspace with a good filesystem or regresses
in a way that real applications would break, that this needs to be redone.

However, I spent a few hours doing some extra testing (I had already run
some xfstests that include directory testing) and I failed to find any
issues with this fix.

If this would break, it would have broken the root directory. In the case
of a directory including the . and .. entries, the d_off for the .. entry
will be set for the first non-dot-or-dotdot entry. For ., it will be set as
1, which, if used by telldir (or llseek), will emit the .. entry, as
expected.

For the case where both . and .. are absent, the first real entry will have
d_off as 2, and it will just work.

So everything seems to work as expected. Do you see any user visible change
that would break any applications?

Thanks.
Cascardo.

> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > ---
> >  fs/fat/dir.c | 24 +++++++++---------------
> >  1 file changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> > index 4e4a359a1ea3..e70781569de5 100644
> > --- a/fs/fat/dir.c
> > +++ b/fs/fat/dir.c
> > @@ -583,15 +583,14 @@ static int __fat_readdir(struct inode *inode, struct file *file,
> >  	mutex_lock(&sbi->s_lock);
> >  
> >  	cpos = ctx->pos;
> > -	/* Fake . and .. for the root directory. */
> > -	if (inode->i_ino == MSDOS_ROOT_INO) {
> > -		if (!dir_emit_dots(file, ctx))
> > -			goto out;
> > -		if (ctx->pos == 2) {
> > -			fake_offset = 1;
> > -			cpos = 0;
> > -		}
> > +
> > +	if (!dir_emit_dots(file, ctx))
> > +		goto out;
> > +	if (ctx->pos == 2) {
> > +		fake_offset = 1;
> > +		cpos = 0;
> >  	}
> > +
> >  	if (cpos & (sizeof(struct msdos_dir_entry) - 1)) {
> >  		ret = -ENOENT;
> >  		goto out;
> > @@ -671,13 +670,8 @@ static int __fat_readdir(struct inode *inode, struct file *file,
> >  	if (fake_offset && ctx->pos < 2)
> >  		ctx->pos = 2;
> >  
> > -	if (!memcmp(de->name, MSDOS_DOT, MSDOS_NAME)) {
> > -		if (!dir_emit_dot(file, ctx))
> > -			goto fill_failed;
> > -	} else if (!memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
> > -		if (!dir_emit_dotdot(file, ctx))
> > -			goto fill_failed;
> > -	} else {
> > +	if (memcmp(de->name, MSDOS_DOT, MSDOS_NAME) &&
> > +	    memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
> >  		unsigned long inum;
> >  		loff_t i_pos = fat_make_i_pos(sb, bh, de);
> >  		struct inode *tmp = fat_iget(sb, i_pos);
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[OGAWA Hirofumi]

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

>> Unacceptable to change the correct behavior to broken format. And
>> unlikely break the userspace, however this still has the user visible
>> change of seek pos.
>> 
>> Thanks.
>> 
>
> I agree that if this breaks userspace with a good filesystem or regresses
> in a way that real applications would break, that this needs to be redone.
>
> However, I spent a few hours doing some extra testing (I had already run
> some xfstests that include directory testing) and I failed to find any
> issues with this fix.
>
> If this would break, it would have broken the root directory. In the case
> of a directory including the . and .. entries, the d_off for the .. entry
> will be set for the first non-dot-or-dotdot entry. For ., it will be set as
> 1, which, if used by telldir (or llseek), will emit the .. entry, as
> expected.
>
> For the case where both . and .. are absent, the first real entry will have
> d_off as 2, and it will just work.
>
> So everything seems to work as expected. Do you see any user visible change
> that would break any applications?

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.

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

Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[Thadeu Lima de Souza Cascardo]

On Thu, Jun 27, 2024 at 05:10:44AM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> Unacceptable to change the correct behavior to broken format. And
> >> unlikely break the userspace, however this still has the user visible
> >> change of seek pos.
> >> 
> >> Thanks.
> >> 
> >
> > I agree that if this breaks userspace with a good filesystem or regresses
> > in a way that real applications would break, that this needs to be redone.
> >
> > However, I spent a few hours doing some extra testing (I had already run
> > some xfstests that include directory testing) and I failed to find any
> > issues with this fix.
> >
> > If this would break, it would have broken the root directory. In the case
> > of a directory including the . and .. entries, the d_off for the .. entry
> > will be set for the first non-dot-or-dotdot entry. For ., it will be set as
> > 1, which, if used by telldir (or llseek), will emit the .. entry, as
> > expected.
> >
> > For the case where both . and .. are absent, the first real entry will have
> > d_off as 2, and it will just work.
> >
> > So everything seems to work as expected. Do you see any user visible change
> > that would break any applications?
> 
> 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.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

I looked at that perspective, but still wanted to allow users to use such
filesystems, even if they needed to fsck it first.

But there is the issue that when such filesystems are mounted, they are
further corrupted, preventing such fsck from correctly fixing and allowing
access to the data.

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.

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.

Thanks.
Cascardo.

Re: [PATCH v2 2/2] fat: always use dir_emit_dots and ignore . and .. entries

[OGAWA Hirofumi]

Thadeu Lima de Souza Cascardo <cascardo@igalia.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@mail.parknet.co.jp>