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

[yangerkun <yangerkun@huaweicloud.com>]

Hi!

在 2024/7/31 21:10, Filipe Manana 写道:
> On Wed, Jul 31, 2024 at 1:51 PM yangerkun <yangerkun@huaweicloud.com> wrote:
>>
>> 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?
> 
> What problem does it solve?
> The standard doesn't forbid it, and I can't see anything wrong with it.


Yeah, I didn't find any obvious bugs for this behavior. It's a choose
does this seems better, and I think it's also ok to keep btrfs not
change now. Thanks for your reply!

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