msdosfs: Fix handling of eofflag in VOP_READDIR

[markj]

Summary

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

Reported by: vishwin

[markj]

Unconditionally initialize eofflag = 0.

[olce]

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.

[markj]

In D51290#1171210, @olce wrote:

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.

[olce]

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

[kib]

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.

[markj]

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.

[kib]

In D51290#1171290, @markj 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().

[markj]

In D51290#1171293, @kib wrote:

In D51290#1171290, @markj 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?

[kib]

In D51290#1171328, @markj wrote:

In D51290#1171293, @kib wrote:

In D51290#1171290, @markj 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.