Page MenuHomeFreeBSD

msdosfs: Fix handling of eofflag in VOP_READDIR
ClosedPublic

Authored by markj on Jul 13 2025, 3:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 10, 5:03 PM
Unknown Object (File)
Fri, Oct 10, 5:03 PM
Unknown Object (File)
Fri, Oct 10, 5:03 PM
Unknown Object (File)
Fri, Oct 10, 5:03 PM
Unknown Object (File)
Fri, Oct 10, 11:42 AM
Unknown Object (File)
Fri, Oct 10, 12:22 AM
Unknown Object (File)
Fri, Oct 3, 3:25 AM
Unknown Object (File)
Sat, Sep 27, 4:55 PM
Subscribers

Details

Summary

We need to set it when an end-of-directory marker is reached.

Reported by: vishwin

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Jul 13 2025, 3:11 PM

Unconditionally initialize eofflag = 0.

Commenting here with the content of my lastest mail:

I've just read msdosfs_readdir(), and the problem could be there:

if (dep->de_FileSize - (offset - bias) <= 0)

because dep->de_FileSize is an unsigned long, and the test only passes with offset - bias exactly equal to de_FileSize. The assignment to diff in the loop above would be wrong as well.

Commenting here with the content of my lastest mail:

I've just read msdosfs_readdir(), and the problem could be there:

if (dep->de_FileSize - (offset - bias) <= 0)

because dep->de_FileSize is an unsigned long, and the test only passes with offset - bias exactly equal to de_FileSize. The assignment to diff in the loop above would be wrong as well.

I reproduced the problem locally, and I'm quite sure the problem is with the dentp->deName[0] == SLOT_EMPTY case. I think this patch fixes the bug you point out as well, since it assigns the difference to a signed int.

I reproduced the problem locally, and I'm quite sure the problem is with the dentp->deName[0] == SLOT_EMPTY case. I think this patch fixes the bug you point out as well, since it assigns the difference to a signed int.

By code reading, I agree that this is most likely what is happening. diff is a signed int, which should be more than enough (these varying types everywhere, besides complicating analysis, are ugly).

Can you point out how these changes make a difference? As I understand, all recent eofflag fixes were triggered by the vn_dir_next_dirent() usage in vfs_inotify.c, and all pathes I see correctly check for len == 0.

In D51290#1171280, @kib wrote:

Can you point out how these changes make a difference? As I understand, all recent eofflag fixes were triggered by the vn_dir_next_dirent() usage in vfs_inotify.c, and all pathes I see correctly check for len == 0.

vn_dir_next_dirent() asserts that if VOP_READDIR returns 0 bytes and error == 0, then it must also have set eofflag = 1. msdosfs_readdir() was not setting eofflag = 1 upon reaching the end-of-directory marker.

This revision is now accepted and ready to land.Jul 13 2025, 4:35 PM
In D51290#1171280, @kib wrote:

Can you point out how these changes make a difference? As I understand, all recent eofflag fixes were triggered by the vn_dir_next_dirent() usage in vfs_inotify.c, and all pathes I see correctly check for len == 0.

vn_dir_next_dirent() asserts that if VOP_READDIR returns 0 bytes and error == 0, then it must also have set eofflag = 1. msdosfs_readdir() was not setting eofflag = 1 upon reaching the end-of-directory marker.

So may be this check should be moved to vop_readdir_post().

In D51290#1171293, @kib wrote:
In D51290#1171280, @kib wrote:

Can you point out how these changes make a difference? As I understand, all recent eofflag fixes were triggered by the vn_dir_next_dirent() usage in vfs_inotify.c, and all pathes I see correctly check for len == 0.

vn_dir_next_dirent() asserts that if VOP_READDIR returns 0 bytes and error == 0, then it must also have set eofflag = 1. msdosfs_readdir() was not setting eofflag = 1 upon reaching the end-of-directory marker.

So may be this check should be moved to vop_readdir_post().

Hmm, but to check whether len == 0 we need to save uio_resid before and after the VOP_READDIR call, so to implement the suggestion, vop_readdir_pre() needs to save a value and then somehow pass it to vop_readdir_post(). Do we have some established pattern for this?

In D51290#1171293, @kib wrote:
In D51290#1171280, @kib wrote:

Can you point out how these changes make a difference? As I understand, all recent eofflag fixes were triggered by the vn_dir_next_dirent() usage in vfs_inotify.c, and all pathes I see correctly check for len == 0.

vn_dir_next_dirent() asserts that if VOP_READDIR returns 0 bytes and error == 0, then it must also have set eofflag = 1. msdosfs_readdir() was not setting eofflag = 1 upon reaching the end-of-directory marker.

So may be this check should be moved to vop_readdir_post().

Hmm, but to check whether len == 0 we need to save uio_resid before and after the VOP_READDIR call, so to implement the suggestion, vop_readdir_pre() needs to save a value and then somehow pass it to vop_readdir_post(). Do we have some established pattern for this?

Note that vop_XXX_pre/post can be macros, and sometimes they are. I suspect it should be enough to provide the place to save uio_resid before, and check it after.