diff mbox series

fat: ignore .. subdir and always add a link to dirs

Message ID 20240222203013.2649457-1-cascardo@igalia.com
State New
Headers
Series fat: ignore .. subdir and always add a link to dirs |

Commit Message

Thadeu Lima de Souza Cascardo Feb. 22, 2024, 8:30 p.m. UTC 
  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 .. subdir and add one when
setting the initial link count for directories. This way, when .. is
present, it is still accounted for, and when neither . or .. are present, a
single link is still done, as it should, since this link would be the one
from the parent directory.

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

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   |  3 ++-
 fs/fat/inode.c | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

OGAWA Hirofumi Feb. 23, 2024, 1:52 a.m. UTC | #1 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:

> 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 .. subdir and add one when
> setting the initial link count for directories. This way, when .. is
> present, it is still accounted for, and when neither . or .. are present, a
> single link is still done, as it should, since this link would be the one
> from the parent directory.
>
> With this fix applied, we can mount an image with such empty directories,
> access them, create subdirectories and remove them.

This looks like the bug of those tools, isn't it?

Thanks.
Thadeu Lima de Souza Cascardo Feb. 23, 2024, 2:02 a.m. UTC | #2 
On Fri, Feb 23, 2024 at 10:52:12AM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> > 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 .. subdir and add one when
> > setting the initial link count for directories. This way, when .. is
> > present, it is still accounted for, and when neither . or .. are present, a
> > single link is still done, as it should, since this link would be the one
> > from the parent directory.
> >
> > With this fix applied, we can mount an image with such empty directories,
> > access them, create subdirectories and remove them.
> 
> This looks like the bug of those tools, isn't it?
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Which they refused to fix, arguing that there are already filesystems out there
in the world like that. Also, there is argument that this works on Windows,
though I haven't been able to test this.

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

Thanks.
Cascardo.
OGAWA Hirofumi Feb. 23, 2024, 8:27 a.m. UTC | #3 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:

> On Fri, Feb 23, 2024 at 10:52:12AM +0900, OGAWA Hirofumi wrote:
>> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
>> 
>> > 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 .. subdir and add one when
>> > setting the initial link count for directories. This way, when .. is
>> > present, it is still accounted for, and when neither . or .. are present, a
>> > single link is still done, as it should, since this link would be the one
>> > from the parent directory.
>> >
>> > With this fix applied, we can mount an image with such empty directories,
>> > access them, create subdirectories and remove them.
>> 
>> This looks like the bug of those tools, isn't it?
>> 
>> Thanks.
>> -- 
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> Which they refused to fix, arguing that there are already filesystems out there
> in the world like that. Also, there is argument that this works on Windows,
> though I haven't been able to test this.
>
> https://github.com/microsoft/pxt-ev3/issues/980
> https://github.com/microsoft/uf2-linux/issues/6

OK.

If you want to add the workaround for this, it must emulate the correct
format. I.e. sane link count even if without "."/"..". And furthermore
it works for any operations.

Thanks.
OGAWA Hirofumi Feb. 23, 2024, 8:32 a.m. UTC | #4 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> OK.
>
> If you want to add the workaround for this, it must emulate the correct
> format. I.e. sane link count even if without "."/"..". And furthermore
> it works for any operations.

Of course, it must not affect the correct format. And it should not
accept the other really corrupted format.
Thadeu Lima de Souza Cascardo Feb. 23, 2024, 9:58 a.m. UTC | #5 
On Fri, Feb 23, 2024 at 05:32:47PM +0900, OGAWA Hirofumi wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> 
> > OK.
> >
> > If you want to add the workaround for this, it must emulate the correct
> > format. I.e. sane link count even if without "."/"..". And furthermore
> > it works for any operations.
> 
> Of course, it must not affect the correct format. And it should not
> accept the other really corrupted format.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

So far, I have only seen expected correct behavior here: mkdir/rmdir inside the
"bogus" directory works. rmdir of the "bogus" directory works.

The only idiosyncrasies I can think of is that if neither "." or ".." are
present, the directory will have a link of 1, instead of 2. And when listing
the directory, those entries will not show up.

Do you expect any of these to be corrected? It will require a more convoluted
change.

Right now, I think accepting the idiosyncratic behavior for the bogus
filesystems is fine, as long as the correct filesystems continue to behave as
before. Which seems to be the case here as far as my testing has shown.

Thank you.
Cascardo.
OGAWA Hirofumi Feb. 23, 2024, 12:29 p.m. UTC | #6 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:

> So far, I have only seen expected correct behavior here: mkdir/rmdir inside the
> "bogus" directory works. rmdir of the "bogus" directory works.
>
> The only idiosyncrasies I can think of is that if neither "." or ".." are
> present, the directory will have a link of 1, instead of 2. And when listing
> the directory, those entries will not show up.
>
> Do you expect any of these to be corrected? It will require a more convoluted
> change.
>
> Right now, I think accepting the idiosyncratic behavior for the bogus
> filesystems is fine, as long as the correct filesystems continue to behave as
> before. Which seems to be the case here as far as my testing has shown.

There are many corrupted images, and attacks. Allowing too wide is
danger for fs.

BTW, this image works and pass fsck on windows? When I quickly tested
ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
on qemu, it didn't seem recognized as FAT. I can wrongly tested though.

Thanks.
Thadeu Lima de Souza Cascardo Feb. 23, 2024, 1:16 p.m. UTC | #7 
On Fri, Feb 23, 2024 at 09:29:35PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> > So far, I have only seen expected correct behavior here: mkdir/rmdir inside the
> > "bogus" directory works. rmdir of the "bogus" directory works.
> >
> > The only idiosyncrasies I can think of is that if neither "." or ".." are
> > present, the directory will have a link of 1, instead of 2. And when listing
> > the directory, those entries will not show up.
> >
> > Do you expect any of these to be corrected? It will require a more convoluted
> > change.
> >
> > Right now, I think accepting the idiosyncratic behavior for the bogus
> > filesystems is fine, as long as the correct filesystems continue to behave as
> > before. Which seems to be the case here as far as my testing has shown.
> 
> There are many corrupted images, and attacks. Allowing too wide is
> danger for fs.
> 
> BTW, this image works and pass fsck on windows? When I quickly tested
> ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
> on qemu, it didn't seem recognized as FAT. I can wrongly tested though.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

That image was further corrupted by mounting it on linux, as is mentioned
in one of the github issues. Let me see if I can arrange the Windows
testing of images I was able to test with. I can later attach them too, as
they should compress very well.

Cascardo.
Thadeu Lima de Souza Cascardo Feb. 23, 2024, 1:33 p.m. UTC | #8 
On Fri, Feb 23, 2024 at 09:29:35PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> > So far, I have only seen expected correct behavior here: mkdir/rmdir inside the
> > "bogus" directory works. rmdir of the "bogus" directory works.
> >
> > The only idiosyncrasies I can think of is that if neither "." or ".." are
> > present, the directory will have a link of 1, instead of 2. And when listing
> > the directory, those entries will not show up.
> >
> > Do you expect any of these to be corrected? It will require a more convoluted
> > change.
> >
> > Right now, I think accepting the idiosyncratic behavior for the bogus
> > filesystems is fine, as long as the correct filesystems continue to behave as
> > before. Which seems to be the case here as far as my testing has shown.
> 
> There are many corrupted images, and attacks. Allowing too wide is
> danger for fs.
> 
> BTW, this image works and pass fsck on windows? When I quickly tested
> ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
> on qemu, it didn't seem recognized as FAT. I can wrongly tested though.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Testing on FreeDOS, it is able to access the directory and create
subdirectories there. "." and ".." are missing when using "DIR" command. And
dosfsck complains about the missing "." and ".." but states it is not able to
fix those yet. But accessing the directory works nonetheless.

Cascardo.
Thadeu Lima de Souza Cascardo Feb. 28, 2024, 1:42 a.m. UTC | #9 
On Fri, Feb 23, 2024 at 09:29:35PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> > So far, I have only seen expected correct behavior here: mkdir/rmdir inside the
> > "bogus" directory works. rmdir of the "bogus" directory works.
> >
> > The only idiosyncrasies I can think of is that if neither "." or ".." are
> > present, the directory will have a link of 1, instead of 2. And when listing
> > the directory, those entries will not show up.
> >
> > Do you expect any of these to be corrected? It will require a more convoluted
> > change.
> >
> > Right now, I think accepting the idiosyncratic behavior for the bogus
> > filesystems is fine, as long as the correct filesystems continue to behave as
> > before. Which seems to be the case here as far as my testing has shown.
> 
> There are many corrupted images, and attacks. Allowing too wide is
> danger for fs.
> 
> BTW, this image works and pass fsck on windows? When I quickly tested
> ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
> on qemu, it didn't seem recognized as FAT. I can wrongly tested though.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

The test image I managed to create mounts just fine on Windows. New
subdirectories can be created there just as well.

Regards.
Cascardo.
OGAWA Hirofumi Feb. 28, 2024, 3:38 a.m. UTC | #10 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:

>> There are many corrupted images, and attacks. Allowing too wide is
>> danger for fs.
>> 
>> BTW, this image works and pass fsck on windows? When I quickly tested
>> ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
>> on qemu, it didn't seem recognized as FAT. I can wrongly tested though.
>> 
>> Thanks.
>> -- 
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> The test image I managed to create mounts just fine on Windows. New
> subdirectories can be created there just as well.

Can you share the image somehow? And fsck (chkdsk, etc.) works without
any complain?

Thanks.
Thadeu Lima de Souza Cascardo Feb. 28, 2024, 9:10 a.m. UTC | #11 
On Wed, Feb 28, 2024 at 12:38:43PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> There are many corrupted images, and attacks. Allowing too wide is
> >> danger for fs.
> >> 
> >> BTW, this image works and pass fsck on windows? When I quickly tested
> >> ev3fs.zip (https://github.com/microsoft/pxt-ev3/issues/980) on windows
> >> on qemu, it didn't seem recognized as FAT. I can wrongly tested though.
> >> 
> >> Thanks.
> >> -- 
> >> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> >
> > The test image I managed to create mounts just fine on Windows. New
> > subdirectories can be created there just as well.
> 
> Can you share the image somehow? And fsck (chkdsk, etc.) works without
> any complain?
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Checking the filesystem on Windows runs without any complains, but it turns the
directory into an useless lump of data. Without checking the filesystem,
creating and reading files from that directory works just fine.

I tried to use gzip or xz to compress the very sparse filesystem image that I
got, but they made it larger on disk than it really was. So here is a script
and pieces of the filesystem that will create a sparse 8GB image.

Thank you for looking into this.
Cascardo.
diff mbox series

Patch

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 00235b8a1823..fcdb652efc53 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -937,7 +937,8 @@  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_DOTDOT, MSDOS_NAME))
 			count++;
 	}
 	brelse(bh);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 1fac3dabf130..9a3bd38a4494 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -494,8 +494,14 @@  static int fat_validate_dir(struct inode *dir)
 {
 	struct super_block *sb = dir->i_sb;
 
-	if (dir->i_nlink < 2) {
-		/* Directory should have "."/".." entries at least. */
+	if (dir->i_nlink < 1) {
+		/*
+		 * Though it is expected that directories have at least
+		 * "."/".." entries, there are filesystems in the field that
+		 * don't have either. Even in those cases, at least one link
+		 * is necessary, as otherwise, when trying to increment it,
+		 * VFS would BUG.
+		 */
 		fat_fs_error(sb, "corrupted directory (invalid entries)");
 		return -EIO;
 	}
@@ -534,7 +540,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) + 1);
 
 		error = fat_validate_dir(inode);
 		if (error < 0)