| Message ID | 20240222203013.2649457-1-cascardo@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fat: ignore .. subdir and always add a link to dirs | expand |
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.
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.
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> 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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>
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.
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>
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.
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>
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.
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>
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.
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)
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(-)