[v4] libfs: getdents() should return 0 after reaching EOD

Commit Message

Chuck Lever Nov. 19, 2023, 11:56 p.m. UTC

From: Chuck Lever <[e-mail]>

The new directory offset helpers don't conform with the convention
of getdents() returning no more entries once a directory file
descriptor has reached the current end-of-directory.

To address this, copy the logic from dcache_readdir() to mark the
open directory file descriptor once EOD has been reached. Seeking
resets the mark.

Reported-by: Tavian Barnes <[e-mail]>
Closes: https://lore.kernel.org/linux-fsdevel/[e-mail]/
Fixes: 6faddda69f62 ("libfs: Add directory operations for stable offsets")
Signed-off-by: Chuck Lever <[e-mail]>
---
 fs/libfs.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

v4 of this patch passes Tavian's reproducer and fstests over NFS and
directly on a tmpfs mount.

Changes since v3:
- Ensure that llseek() resets the EOD mark too

Changes since v2:
- Go back to marking EOD in the file->private_data field

Changes since RFC:
- Keep file->private_data stable while directory descriptor remains open

Comments

Christian Brauner Nov. 20, 2023, 2:37 p.m. UTC | #1

On Sun, 19 Nov 2023 18:56:17 -0500, Chuck Lever wrote:
> The new directory offset helpers don't conform with the convention
> of getdents() returning no more entries once a directory file
> descriptor has reached the current end-of-directory.
> 
> To address this, copy the logic from dcache_readdir() to mark the
> open directory file descriptor once EOD has been reached. Seeking
> resets the mark.
> 
> [...]

Should fix the regression report I also received earlier today. Thanks for the
reviews with LPC and MS I couldn't really do any meaningful review.

---

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] libfs: getdents() should return 0 after reaching EOD
      https://git.kernel.org/vfs/vfs/c/796432efab1e

Chuck Lever Nov. 20, 2023, 2:39 p.m. UTC | #2

> On Nov 20, 2023, at 9:37 AM, Christian Brauner <[e-mail]> wrote:
> 
> On Sun, 19 Nov 2023 18:56:17 -0500, Chuck Lever wrote:
>> The new directory offset helpers don't conform with the convention
>> of getdents() returning no more entries once a directory file
>> descriptor has reached the current end-of-directory.
>> 
>> To address this, copy the logic from dcache_readdir() to mark the
>> open directory file descriptor once EOD has been reached. Seeking
>> resets the mark.
>> 
>> [...]
> 
> Should fix the regression report I also received earlier today.

You mean this one?

https://bugzilla.kernel.org/show_bug.cgi?id=218147

It feels like it is similar if not the same.


> Thanks for the
> reviews with LPC and MS I couldn't really do any meaningful review.
> 
> ---
> 
> Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> Patches in the vfs.fixes branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.fixes
> 
> [1/1] libfs: getdents() should return 0 after reaching EOD
>      https://git.kernel.org/vfs/vfs/c/796432efab1e

--
Chuck Lever

Christian Brauner Nov. 20, 2023, 2:48 p.m. UTC | #3

> You mean this one?

No, there had been another one about readdir specifically that was sent
to me.

Patch

diff --git a/fs/libfs.c b/fs/libfs.c
index e9440d55073c..c2aa6fd4795c 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -399,6 +399,8 @@  static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
 		return -EINVAL;
 	}
 
+	/* In this case, ->private_data is protected by f_pos_lock */
+	file->private_data = NULL;
 	return vfs_setpos(file, offset, U32_MAX);
 }
 
@@ -428,7 +430,7 @@  static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
 			  inode->i_ino, fs_umode_to_dtype(inode->i_mode));
 }
 
-static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
+static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
 {
 	struct offset_ctx *so_ctx = inode->i_op->get_offset_ctx(inode);
 	XA_STATE(xas, &so_ctx->xa, ctx->pos);
@@ -437,7 +439,7 @@  static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
 	while (true) {
 		dentry = offset_find_next(&xas);
 		if (!dentry)
-			break;
+			return ERR_PTR(-ENOENT);
 
 		if (!offset_dir_emit(ctx, dentry)) {
 			dput(dentry);
@@ -447,6 +449,7 @@  static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
 		dput(dentry);
 		ctx->pos = xas.xa_index + 1;
 	}
+	return NULL;
 }
 
 /**
@@ -479,7 +482,12 @@  static int offset_readdir(struct file *file, struct dir_context *ctx)
 	if (!dir_emit_dots(file, ctx))
 		return 0;
 
-	offset_iterate_dir(d_inode(dir), ctx);
+	/* In this case, ->private_data is protected by f_pos_lock */
+	if (ctx->pos == 2)
+		file->private_data = NULL;
+	else if (file->private_data == ERR_PTR(-ENOENT))
+		return 0;
+	file->private_data = offset_iterate_dir(d_inode(dir), ctx);
 	return 0;
 }