Re: [PATCH v6 3/5] Revert "libfs: fix infinite directory reads for offset dir"

[yangerkun <yangerkun@huaweicloud.com>]

LGTM

Reviewed-by: Yang Erkun <yangerkun@huawei.com>


在 2024/12/20 23:33, cel@kernel.org 写道:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The current directory offset allocator (based on mtree_alloc_cyclic)
> stores the next offset value to return in octx->next_offset. This
> mechanism typically returns values that increase monotonically over
> time. Eventually, though, the newly allocated offset value wraps
> back to a low number (say, 2) which is smaller than other already-
> allocated offset values.
> 
> Yu Kuai <yukuai3@huawei.com> reports that, after commit 64a7ce76fb90
> ("libfs: fix infinite directory reads for offset dir"), if a
> directory's offset allocator wraps, existing entries are no longer
> visible via readdir/getdents because offset_readdir() stops listing
> entries once an entry's offset is larger than octx->next_offset.
> These entries vanish persistently -- they can be looked up, but will
> never again appear in readdir(3) output.
> 
> The reason for this is that the commit treats directory offsets as
> monotonically increasing integer values rather than opaque cookies,
> and introduces this comparison:
> 
> 	if (dentry2offset(dentry) >= last_index) {
> 
> On 64-bit platforms, the directory offset value upper bound is
> 2^63 - 1. Directory offsets will monotonically increase for millions
> of years without wrapping.
> 
> On 32-bit platforms, however, LONG_MAX is 2^31 - 1. The allocator
> can wrap after only a few weeks (at worst).
> 
> Revert commit 64a7ce76fb90 ("libfs: fix infinite directory reads for
> offset dir") to prepare for a fix that can work properly on 32-bit
> systems and might apply to recent LTS kernels where shmem employs
> the simple_offset mechanism.
> 
> Reported-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>   fs/libfs.c | 35 +++++++++++------------------------
>   1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 8380d9314ebd..8c9364a0174c 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -422,14 +422,6 @@ void simple_offset_destroy(struct offset_ctx *octx)
>   	mtree_destroy(&octx->mt);
>   }
>   
> -static int offset_dir_open(struct inode *inode, struct file *file)
> -{
> -	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> -
> -	file->private_data = (void *)ctx->next_offset;
> -	return 0;
> -}
> -
>   /**
>    * offset_dir_llseek - Advance the read position of a directory descriptor
>    * @file: an open directory whose position is to be updated
> @@ -443,9 +435,6 @@ static int offset_dir_open(struct inode *inode, struct file *file)
>    */
>   static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
>   {
> -	struct inode *inode = file->f_inode;
> -	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> -
>   	switch (whence) {
>   	case SEEK_CUR:
>   		offset += file->f_pos;
> @@ -459,8 +448,7 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
>   	}
>   
>   	/* In this case, ->private_data is protected by f_pos_lock */
> -	if (!offset)
> -		file->private_data = (void *)ctx->next_offset;
> +	file->private_data = NULL;
>   	return vfs_setpos(file, offset, LONG_MAX);
>   }
>   
> @@ -491,7 +479,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, long last_index)
> +static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
>   {
>   	struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode);
>   	struct dentry *dentry;
> @@ -499,21 +487,17 @@ static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx, lon
>   	while (true) {
>   		dentry = offset_find_next(octx, ctx->pos);
>   		if (!dentry)
> -			return;
> -
> -		if (dentry2offset(dentry) >= last_index) {
> -			dput(dentry);
> -			return;
> -		}
> +			return ERR_PTR(-ENOENT);
>   
>   		if (!offset_dir_emit(ctx, dentry)) {
>   			dput(dentry);
> -			return;
> +			break;
>   		}
>   
>   		ctx->pos = dentry2offset(dentry) + 1;
>   		dput(dentry);
>   	}
> +	return NULL;
>   }
>   
>   /**
> @@ -540,19 +524,22 @@ static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx, lon
>   static int offset_readdir(struct file *file, struct dir_context *ctx)
>   {
>   	struct dentry *dir = file->f_path.dentry;
> -	long last_index = (long)file->private_data;
>   
>   	lockdep_assert_held(&d_inode(dir)->i_rwsem);
>   
>   	if (!dir_emit_dots(file, ctx))
>   		return 0;
>   
> -	offset_iterate_dir(d_inode(dir), ctx, last_index);
> +	/* In this case, ->private_data is protected by f_pos_lock */
> +	if (ctx->pos == DIR_OFFSET_MIN)
> +		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;
>   }
>   
>   const struct file_operations simple_offset_dir_operations = {
> -	.open		= offset_dir_open,
>   	.llseek		= offset_dir_llseek,
>   	.iterate_shared	= offset_readdir,
>   	.read		= generic_read_dir,