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

[PATCH] fat: ignore .. subdir and always add a link 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 .. 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(-)

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)
-- 
2.34.1

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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 <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

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.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

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.

[-- Attachment #2: vfat2.img_1 --]
[-- Type: application/octet-stream, Size: 4096 bytes --]

[-- Attachment #3: vfat2.img_2 --]
[-- Type: application/octet-stream, Size: 512 bytes --]

[-- Attachment #4: vfat2.img_3 --]
[-- Type: application/octet-stream, Size: 512 bytes --]

[-- Attachment #5: vfat2.img_4 --]
[-- Type: application/octet-stream, Size: 512 bytes --]

[-- Attachment #6: vfat.sh --]
[-- Type: application/x-sh, Size: 196 bytes --]

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

On Wed, Feb 28, 2024 at 06:10:29AM -0300, Thadeu Lima de Souza Cascardo wrote:
> 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.

Hi, OGAWA Hirofumi.

What are your thoughts here? Should we make it possible to read such
filesystems? Is the proposed approach acceptable?

Thanks.
Cascardo.

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

> On Wed, Feb 28, 2024 at 06:10:29AM -0300, Thadeu Lima de Souza Cascardo wrote:
>> On Wed, Feb 28, 2024 at 12:38:43PM +0900, OGAWA Hirofumi wrote:
>> > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
>> > 
>> 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.
>
> Hi, OGAWA Hirofumi.
>
> What are your thoughts here? Should we make it possible to read such
> filesystems? Is the proposed approach acceptable?

Sorry. I was busy recently, so I didn't have time to check this
yet. When I could make time to check, I will.

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

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

>> Can you share the image somehow? And fsck (chkdsk, etc.) works without
>> any complain?
>
> 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.

I tested a your image with some tweaks. Windows's chkdsk complains about
"BADDIR" directory, and it was fixed by converting it to normal
file. Probably, chkdsk thought that "BADDIR" got ATTR_DIR bit by
corruption.  IOW, Windows FATFS driver may accept this image, but
Windows also think this image as corrupt, like chkdsk says.

I think the app that make this should be fixed. Windows accepts more
than linux though, it looks also think as corrupt.

If we really want to accept this image, we have to change the fat driver
without affecting good image.  And your patch affects to good image,
because that patch doesn't count directory correctly, so bad link count.

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

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

On Sun, Mar 10, 2024 at 02:52:26PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> Can you share the image somehow? And fsck (chkdsk, etc.) works without
> >> any complain?
> >
> > 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.
> 
> I tested a your image with some tweaks. Windows's chkdsk complains about
> "BADDIR" directory, and it was fixed by converting it to normal
> file. Probably, chkdsk thought that "BADDIR" got ATTR_DIR bit by
> corruption.  IOW, Windows FATFS driver may accept this image, but
> Windows also think this image as corrupt, like chkdsk says.
> 
> I think the app that make this should be fixed. Windows accepts more
> than linux though, it looks also think as corrupt.
> 
> If we really want to accept this image, we have to change the fat driver
> without affecting good image.  And your patch affects to good image,
> because that patch doesn't count directory correctly, so bad link count.
> 

Well, it does behave the same on a correct image. It ignores the existence of
".." when counting subdirs, but always adds an extra link count.

So, images that have both "." and ".." subdirs, will have the 2 links, both
with the patch and without the patch.

Images with neither dirs will be rejected before the patch and have a link
count of 1 after the patch. Still, creating and removing subdirs will work.
Removing the bad dir itself also works.

Images with only "." or only ".." would have a link count of 1 and be rejected
without the patch.

With the patch, directories with only ".." should behave the same as if they
had neither subdirs. That is, link count of 1. And directories with only "."
will have a link count of 2.

Cascardo.

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

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

>> If we really want to accept this image, we have to change the fat driver
>> without affecting good image.  And your patch affects to good image,
>> because that patch doesn't count directory correctly, so bad link count.
>> 
>
> Well, it does behave the same on a correct image. It ignores the existence of
> ".." when counting subdirs, but always adds an extra link count.
>
> So, images that have both "." and ".." subdirs, will have the 2 links, both
> with the patch and without the patch.

You are forgetting to count about normal dirs other than "." and ".."?

Thanks.

> Images with neither dirs will be rejected before the patch and have a link
> count of 1 after the patch. Still, creating and removing subdirs will work.
> Removing the bad dir itself also works.
>
> Images with only "." or only ".." would have a link count of 1 and be rejected
> without the patch.
>
> With the patch, directories with only ".." should behave the same as if they
> had neither subdirs. That is, link count of 1. And directories with only "."
> will have a link count of 2.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

On Sun, Mar 10, 2024 at 11:59:34PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> If we really want to accept this image, we have to change the fat driver
> >> without affecting good image.  And your patch affects to good image,
> >> because that patch doesn't count directory correctly, so bad link count.
> >> 
> >
> > Well, it does behave the same on a correct image. It ignores the existence of
> > ".." when counting subdirs, but always adds an extra link count.
> >
> > So, images that have both "." and ".." subdirs, will have the 2 links, both
> > with the patch and without the patch.
> 
> You are forgetting to count about normal dirs other than "." and ".."?
> 

Yes, I was not counting those. The patch simply ignores ".." when counting dirs
(which is used only for determining the number of links), and always adds one
link. Then, when validating the inode, it also only requires that at least one
link exists instead of two.

There is only one other instance of fat_subdirs being called and that's when
the root dir link count is determined. I left that one unchanged, as usually
"." and ".." does not exist there and we always add two links there.

Cascardo.

> Thanks.
> 
> > Images with neither dirs will be rejected before the patch and have a link
> > count of 1 after the patch. Still, creating and removing subdirs will work.
> > Removing the bad dir itself also works.
> >
> > Images with only "." or only ".." would have a link count of 1 and be rejected
> > without the patch.
> >
> > With the patch, directories with only ".." should behave the same as if they
> > had neither subdirs. That is, link count of 1. And directories with only "."
> > will have a link count of 2.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

>> You are forgetting to count about normal dirs other than "." and ".."?
>> 
>
> Yes, I was not counting those. The patch simply ignores ".." when counting dirs
> (which is used only for determining the number of links), and always adds one
> link. Then, when validating the inode, it also only requires that at least one
> link exists instead of two.

So you break the mkdir/rmdir link counting, isn't it?

Thanks.

> There is only one other instance of fat_subdirs being called and that's when
> the root dir link count is determined. I left that one unchanged, as usually
> "." and ".." does not exist there and we always add two links there.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

On Wed, Mar 13, 2024 at 05:05:41PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> You are forgetting to count about normal dirs other than "." and ".."?
> >> 
> >
> > Yes, I was not counting those. The patch simply ignores ".." when counting dirs
> > (which is used only for determining the number of links), and always adds one
> > link. Then, when validating the inode, it also only requires that at least one
> > link exists instead of two.
> 
> So you break the mkdir/rmdir link counting, isn't it?
> 

It is off by one on those images with directories without ".." subdir.
Otherwise, everything else works fine. mkdir/rmdir inside such directories work
without any issues as rmdir that same directory.

If, on the other hand, we left everything as is and only skipped the
validation, such directories would be created with a link count of 0. Then,
doing a mkdir inside them would crash the kernel with a BUG as we cannot
increment the link count of an inode with 0 links.

So the idea of the fix here is that, independently of the existence of "..",
the link count will always be at least 1.

Cascardo.

> Thanks.
> 
> > There is only one other instance of fat_subdirs being called and that's when
> > the root dir link count is determined. I left that one unchanged, as usually
> > "." and ".." does not exist there and we always add two links there.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

>> So you break the mkdir/rmdir link counting, isn't it?
>> 
>
> It is off by one on those images with directories without ".." subdir.
> Otherwise, everything else works fine. mkdir/rmdir inside such directories work
> without any issues as rmdir that same directory.

mkdir() increase link count, rmdir decrease link count. Your change set
a dir link count always 2? So if there are 3 normal subdirs, and rmdir
all those normal dirs, link count underflow.

Thanks.

> If, on the other hand, we left everything as is and only skipped the
> validation, such directories would be created with a link count of 0. Then,
> doing a mkdir inside them would crash the kernel with a BUG as we cannot
> increment the link count of an inode with 0 links.
>
> So the idea of the fix here is that, independently of the existence of "..",
> the link count will always be at least 1.

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

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[Thadeu Lima de Souza Cascardo]

On Wed, Mar 13, 2024 at 08:06:41PM +0900, OGAWA Hirofumi wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
> 
> >> So you break the mkdir/rmdir link counting, isn't it?
> >> 
> >
> > It is off by one on those images with directories without ".." subdir.
> > Otherwise, everything else works fine. mkdir/rmdir inside such directories work
> > without any issues as rmdir that same directory.
> 
> mkdir() increase link count, rmdir decrease link count. Your change set
> a dir link count always 2? So if there are 3 normal subdirs, and rmdir
> all those normal dirs, link count underflow.
> 
> Thanks.
> 

No. The main change is as follows:

int fat_subdirs(struct inode *dir)
{
[...]
	int count = 0;
[...]
-		if (de->attr & ATTR_DIR)
+		if (de->attr & ATTR_DIR &&
+		    strncmp(de->name, MSDOS_DOTDOT, MSDOS_NAME))
 			count++;
[...]
	return count;
}

int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
{
[...]
	if ((de->attr & ATTR_DIR) && !IS_FREE(de->name)) {
[...]
-		set_nlink(inode, fat_subdirs(inode));
+		set_nlink(inode, fat_subdirs(inode) + 1);
[...]
}

That is, when first instatiating a directory inode, its link count was set to
the number of subdirs it had, including "." and "..". Now it is set to 1 + the
number of subdirs it has ignoring "..".

mkdir and rmdir still increment and decrement the parent directory link count.

Cascardo.


> > If, on the other hand, we left everything as is and only skipped the
> > validation, such directories would be created with a link count of 0. Then,
> > doing a mkdir inside them would crash the kernel with a BUG as we cannot
> > increment the link count of an inode with 0 links.
> >
> > So the idea of the fix here is that, independently of the existence of "..",
> > the link count will always be at least 1.
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: ignore .. subdir and always add a link to dirs

[OGAWA Hirofumi]

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

> On Wed, Mar 13, 2024 at 08:06:41PM +0900, OGAWA Hirofumi wrote:
>> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:
>> 
>> >> So you break the mkdir/rmdir link counting, isn't it?
>> >> 
>> >
>> > It is off by one on those images with directories without ".." subdir.
>> > Otherwise, everything else works fine. mkdir/rmdir inside such directories work
>> > without any issues as rmdir that same directory.
>> 
>> mkdir() increase link count, rmdir decrease link count. Your change set
>> a dir link count always 2? So if there are 3 normal subdirs, and rmdir
>> all those normal dirs, link count underflow.
>> 
>> Thanks.
>> 
>
> No. The main change is as follows:
>
> int fat_subdirs(struct inode *dir)
> {
> [...]
> 	int count = 0;
> [...]
> -		if (de->attr & ATTR_DIR)
> +		if (de->attr & ATTR_DIR &&
> +		    strncmp(de->name, MSDOS_DOTDOT, MSDOS_NAME))
>  			count++;
> [...]
> 	return count;
> }
>
> int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
> {
> [...]
> 	if ((de->attr & ATTR_DIR) && !IS_FREE(de->name)) {
> [...]
> -		set_nlink(inode, fat_subdirs(inode));
> +		set_nlink(inode, fat_subdirs(inode) + 1);
> [...]
> }
>
> That is, when first instatiating a directory inode, its link count was set to
> the number of subdirs it had, including "." and "..". Now it is set to 1 + the
> number of subdirs it has ignoring "..".
>
> mkdir and rmdir still increment and decrement the parent directory link count.

Ah, sorry, I misread.  So next, it should create "." and ".." on initial
create/mkdir or such, like mkdir does.

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