Re: [PATCH] libfs: fix infinite directory reads for offset dir

[yangerkun <yangerkun@huaweicloud.com>]

Hi!

在 2024/7/31 19:51, Jan Kara 写道:
> On Wed 31-07-24 12:38:35, yangerkun wrote:
>> After we switch tmpfs dir operations from simple_dir_operations to
>> simple_offset_dir_operations, every rename happened will fill new dentry
>> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
>> key starting with octx->newx_offset, and then set newx_offset equals to
>> free key + 1. This will lead to infinite readdir combine with rename
>> happened at the same time, which fail generic/736 in xfstests(detail show
>> as below).
>>
>> 1. create 5000 files(1 2 3...) under one dir
>> 2. call readdir(man 3 readdir) once, and get one entry
>> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
>> 4. loop 2~3, until readdir return nothing or we loop too many
>>     times(tmpfs break test with the second condition)
>>
>> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
>> directory reads") to fix it, record the last_index when we open dir, and
>> do not emit the entry which index >= last_index. The file->private_data
>> now used in offset dir can use directly to do this, and we also update
>> the last_index when we llseek the dir file.
> 
> The patch looks good! Just I'm not sure about the llseek part. As far as I
> understand it was added due to this sentence in the standard:
> 
> "If a file is removed from or added to the directory after the most recent
> call to opendir() or rewinddir(), whether a subsequent call to readdir()
> returns an entry for that file is unspecified."
> 
> So if the offset used in offset_dir_llseek() is 0, then we should update
> last_index. But otherwise I'd leave it alone because IMHO it would do more
> harm than good.

IIUC, what you means is that we should only reset the private_data to
new last_index when we call rewinddir(which will call lseek to set
offset of dir file to 0)?

Yeah, I prefer the logic you describle! Besides, we may also change
btrfs that do the same(e60aa5da14d0 ("btrfs: refresh dir last index
during a rewinddir(3) call")). Filipe, how do you think?

Thanks,
Erkun.

> 								Honza
> 
>>
>> Fixes: a2e459555c5f ("shmem: stable directory offsets")
>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>> ---
>>   fs/libfs.c | 34 +++++++++++++++++++++++-----------
>>   1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/libfs.c b/fs/libfs.c
>> index 8aa34870449f..38b306738c00 100644
>> --- a/fs/libfs.c
>> +++ b/fs/libfs.c
>> @@ -450,6 +450,14 @@ 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
>> @@ -463,6 +471,9 @@ void simple_offset_destroy(struct offset_ctx *octx)
>>    */
>>   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;
>> @@ -476,7 +487,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 */
>> -	file->private_data = NULL;
>> +	file->private_data = (void *)ctx->next_offset;
>>   	return vfs_setpos(file, offset, LONG_MAX);
>>   }
>>   
>> @@ -507,7 +518,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, long last_index)
>>   {
>>   	struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode);
>>   	struct dentry *dentry;
>> @@ -515,17 +526,21 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
>>   	while (true) {
>>   		dentry = offset_find_next(octx, ctx->pos);
>>   		if (!dentry)
>> -			return ERR_PTR(-ENOENT);
>> +			return;
>> +
>> +		if (dentry2offset(dentry) >= last_index) {
>> +			dput(dentry);
>> +			return;
>> +		}
>>   
>>   		if (!offset_dir_emit(ctx, dentry)) {
>>   			dput(dentry);
>> -			break;
>> +			return;
>>   		}
>>   
>>   		ctx->pos = dentry2offset(dentry) + 1;
>>   		dput(dentry);
>>   	}
>> -	return NULL;
>>   }
>>   
>>   /**
>> @@ -552,22 +567,19 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
>>   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;
>>   
>> -	/* 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);
>> +	offset_iterate_dir(d_inode(dir), ctx, last_index);
>>   	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,
>> -- 
>> 2.39.2
>>