Re: [PATCH v2] btrfs: fix infinite directory reads

[Qu Wenruo <quwenruo.btrfs@xxxxxxx>]

On 2024/1/25 20:32, Filipe Manana wrote:
> On Thu, Jan 25, 2024 at 9:51 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > [ Upstream commit 9b378f6ad48cfa195ed868db9123c09ee7ec5ea2 ]
> >
> > The readdir implementation currently processes always up to the last index
> > it finds. This however can result in an infinite loop if the directory has
> > a large number of entries such that they won't all fit in the given buffer
> > passed to the readdir callback, that is, dir_emit() returns a non-zero
> > value. Because in that case readdir() will be called again and if in the
> > meanwhile new directory entries were added and we still can't put all the
> > remaining entries in the buffer, we keep repeating this over and over.
> >
> > The following C program and test script reproduce the problem:
> >
> >    $ cat /mnt/readdir_prog.c
> >    #include <sys/types.h>
> >    #include <dirent.h>
> >    #include <stdio.h>
> >
> >    int main(int argc, char *argv[])
> >    {
> >      DIR *dir = opendir(".");
> >      struct dirent *dd;
> >
> >      while ((dd = readdir(dir))) {
> >        printf("%s\n", dd->d_name);
> >        rename(dd->d_name, "TEMPFILE");
> >        rename("TEMPFILE", dd->d_name);
> >      }
> >      closedir(dir);
> >    }
> >
> >    $ gcc -o /mnt/readdir_prog /mnt/readdir_prog.c
> >
> >    $ cat test.sh
> >    #!/bin/bash
> >
> >    DEV=/dev/sdi
> >    MNT=/mnt/sdi
> >
> >    mkfs.btrfs -f $DEV &> /dev/null
> >    #mkfs.xfs -f $DEV &> /dev/null
> >    #mkfs.ext4 -F $DEV &> /dev/null
> >
> >    mount $DEV $MNT
> >
> >    mkdir $MNT/testdir
> >    for ((i = 1; i <= 2000; i++)); do
> >        echo -n > $MNT/testdir/file_$i
> >    done
> >
> >    cd $MNT/testdir
> >    /mnt/readdir_prog
> >
> >    cd /mnt
> >
> >    umount $MNT
> >
> > This behaviour is surprising to applications and it's unlike ext4, xfs,
> > tmpfs, vfat and other filesystems, which always finish. In this case where
> > new entries were added due to renames, some file names may be reported
> > more than once, but this varies according to each filesystem - for example
> > ext4 never reported the same file more than once while xfs reports the
> > first 13 file names twice.
> >
> > So change our readdir implementation to track the last index number when
> > opendir() is called and then make readdir() never process beyond that
> > index number. This gives the same behaviour as ext4.
> >
> > Reported-by: Rob Landley <rob@xxxxxxxxxxx>
> > Link: https://lore.kernel.org/linux-btrfs/2c8c55ec-04c6-e0dc-9c5c-8c7924778c35@xxxxxxxxxxx/
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217681
> > CC: stable@xxxxxxxxxxxxxxx # 5.15
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> > [ Resolve a conflict due to member changes in 96d89923fa94 ]
> > Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> > ---
>
> Thanks for the backport, and running the corresponding test case from
> fstests to verify it's working.
>
> However when backporting a commit, one should also check if there are
> fixes for that commit, as they
> often introduce regressions or have some other bug - and that's the
> case here. We also need to backport
> the following 3 commits:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=357950361cbc6d54fb68ed878265c647384684ae
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e60aa5da14d01fed8411202dbe4adf6c44bd2a57
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8e7f82deb0c0386a03b62e30082574347f8b57d5
>
> One regression, the one regarding rewinddir(3), even has a test case
> in fstests too (generic/471) and would have been caught
> when running the "dir" group tests in fstests:

My bad, I get used to be informed by our internal building system about
missing fixes.

And obviously there is no such automatic systems checking missing fixes
here...
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?h=for-next&id=68b958f5dc4ab13cfd86f7fb82621f9f022b7626
>
> I'll work on making backports of those 3 other patches on top of your
> backport, and then send all of them in a series,
> including your patch, to make it easier to follow and apply all at once.

Thanks a lot, that's much appreciated.

Thanks,
Qu
> Thanks.
>
>
> >   fs/btrfs/ctree.h         |   1 +
> >   fs/btrfs/delayed-inode.c |   5 +-
> >   fs/btrfs/delayed-inode.h |   1 +
> >   fs/btrfs/inode.c         | 131 +++++++++++++++++++++++----------------
> >   4 files changed, 84 insertions(+), 54 deletions(-)
> > ---
> > Changelog:
> > v2:
> > - Fix the upstream commit hash
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 1467bf439cb4..7905f178efa3 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -1361,6 +1361,7 @@ struct btrfs_drop_extents_args {
> >
> >   struct btrfs_file_private {
> >          void *filldir_buf;
> > +       u64 last_index;
> >   };
> >
> >
> > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > index fd951aeaeac5..5a98c5da1225 100644
> > --- a/fs/btrfs/delayed-inode.c
> > +++ b/fs/btrfs/delayed-inode.c
> > @@ -1513,6 +1513,7 @@ int btrfs_inode_delayed_dir_index_count(struct btrfs_inode *inode)
> >   }
> >
> >   bool btrfs_readdir_get_delayed_items(struct inode *inode,
> > +                                    u64 last_index,
> >                                       struct list_head *ins_list,
> >                                       struct list_head *del_list)
> >   {
> > @@ -1532,14 +1533,14 @@ bool btrfs_readdir_get_delayed_items(struct inode *inode,
> >
> >          mutex_lock(&delayed_node->mutex);
> >          item = __btrfs_first_delayed_insertion_item(delayed_node);
> > -       while (item) {
> > +       while (item && item->key.offset <= last_index) {
> >                  refcount_inc(&item->refs);
> >                  list_add_tail(&item->readdir_list, ins_list);
> >                  item = __btrfs_next_delayed_item(item);
> >          }
> >
> >          item = __btrfs_first_delayed_deletion_item(delayed_node);
> > -       while (item) {
> > +       while (item && item->key.offset <= last_index) {
> >                  refcount_inc(&item->refs);
> >                  list_add_tail(&item->readdir_list, del_list);
> >                  item = __btrfs_next_delayed_item(item);
> > diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h
> > index b2412160c5bc..a9cfce856d2e 100644
> > --- a/fs/btrfs/delayed-inode.h
> > +++ b/fs/btrfs/delayed-inode.h
> > @@ -123,6 +123,7 @@ void btrfs_destroy_delayed_inodes(struct btrfs_fs_info *fs_info);
> >
> >   /* Used for readdir() */
> >   bool btrfs_readdir_get_delayed_items(struct inode *inode,
> > +                                    u64 last_index,
> >                                       struct list_head *ins_list,
> >                                       struct list_head *del_list);
> >   void btrfs_readdir_put_delayed_items(struct inode *inode,
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 95af29634e55..1df374ce829b 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -6121,6 +6121,74 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
> >          return d_splice_alias(inode, dentry);
> >   }
> >
> > +/*
> > + * Find the highest existing sequence number in a directory and then set the
> > + * in-memory index_cnt variable to the first free sequence number.
> > + */
> > +static int btrfs_set_inode_index_count(struct btrfs_inode *inode)
> > +{
> > +       struct btrfs_root *root = inode->root;
> > +       struct btrfs_key key, found_key;
> > +       struct btrfs_path *path;
> > +       struct extent_buffer *leaf;
> > +       int ret;
> > +
> > +       key.objectid = btrfs_ino(inode);
> > +       key.type = BTRFS_DIR_INDEX_KEY;
> > +       key.offset = (u64)-1;
> > +
> > +       path = btrfs_alloc_path();
> > +       if (!path)
> > +               return -ENOMEM;
> > +
> > +       ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > +       if (ret < 0)
> > +               goto out;
> > +       /* FIXME: we should be able to handle this */
> > +       if (ret == 0)
> > +               goto out;
> > +       ret = 0;
> > +
> > +       if (path->slots[0] == 0) {
> > +               inode->index_cnt = BTRFS_DIR_START_INDEX;
> > +               goto out;
> > +       }
> > +
> > +       path->slots[0]--;
> > +
> > +       leaf = path->nodes[0];
> > +       btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> > +
> > +       if (found_key.objectid != btrfs_ino(inode) ||
> > +           found_key.type != BTRFS_DIR_INDEX_KEY) {
> > +               inode->index_cnt = BTRFS_DIR_START_INDEX;
> > +               goto out;
> > +       }
> > +
> > +       inode->index_cnt = found_key.offset + 1;
> > +out:
> > +       btrfs_free_path(path);
> > +       return ret;
> > +}
> > +
> > +static int btrfs_get_dir_last_index(struct btrfs_inode *dir, u64 *index)
> > +{
> > +       if (dir->index_cnt == (u64)-1) {
> > +               int ret;
> > +
> > +               ret = btrfs_inode_delayed_dir_index_count(dir);
> > +               if (ret) {
> > +                       ret = btrfs_set_inode_index_count(dir);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       }
> > +
> > +       *index = dir->index_cnt;
> > +
> > +       return 0;
> > +}
> > +
> >   /*
> >    * All this infrastructure exists because dir_emit can fault, and we are holding
> >    * the tree lock when doing readdir.  For now just allocate a buffer and copy
> > @@ -6133,10 +6201,17 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
> >   static int btrfs_opendir(struct inode *inode, struct file *file)
> >   {
> >          struct btrfs_file_private *private;
> > +       u64 last_index;
> > +       int ret;
> > +
> > +       ret = btrfs_get_dir_last_index(BTRFS_I(inode), &last_index);
> > +       if (ret)
> > +               return ret;
> >
> >          private = kzalloc(sizeof(struct btrfs_file_private), GFP_KERNEL);
> >          if (!private)
> >                  return -ENOMEM;
> > +       private->last_index = last_index;
> >          private->filldir_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> >          if (!private->filldir_buf) {
> >                  kfree(private);
> > @@ -6205,7 +6280,8 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >
> >          INIT_LIST_HEAD(&ins_list);
> >          INIT_LIST_HEAD(&del_list);
> > -       put = btrfs_readdir_get_delayed_items(inode, &ins_list, &del_list);
> > +       put = btrfs_readdir_get_delayed_items(inode, private->last_index,
> > +                                             &ins_list, &del_list);
> >
> >   again:
> >          key.type = BTRFS_DIR_INDEX_KEY;
> > @@ -6238,6 +6314,8 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >                          break;
> >                  if (found_key.offset < ctx->pos)
> >                          goto next;
> > +               if (found_key.offset > private->last_index)
> > +                       break;
> >                  if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
> >                          goto next;
> >                  di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
> > @@ -6371,57 +6449,6 @@ static int btrfs_update_time(struct inode *inode, struct timespec64 *now,
> >          return dirty ? btrfs_dirty_inode(inode) : 0;
> >   }
> >
> > -/*
> > - * find the highest existing sequence number in a directory
> > - * and then set the in-memory index_cnt variable to reflect
> > - * free sequence numbers
> > - */
> > -static int btrfs_set_inode_index_count(struct btrfs_inode *inode)
> > -{
> > -       struct btrfs_root *root = inode->root;
> > -       struct btrfs_key key, found_key;
> > -       struct btrfs_path *path;
> > -       struct extent_buffer *leaf;
> > -       int ret;
> > -
> > -       key.objectid = btrfs_ino(inode);
> > -       key.type = BTRFS_DIR_INDEX_KEY;
> > -       key.offset = (u64)-1;
> > -
> > -       path = btrfs_alloc_path();
> > -       if (!path)
> > -               return -ENOMEM;
> > -
> > -       ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > -       if (ret < 0)
> > -               goto out;
> > -       /* FIXME: we should be able to handle this */
> > -       if (ret == 0)
> > -               goto out;
> > -       ret = 0;
> > -
> > -       if (path->slots[0] == 0) {
> > -               inode->index_cnt = BTRFS_DIR_START_INDEX;
> > -               goto out;
> > -       }
> > -
> > -       path->slots[0]--;
> > -
> > -       leaf = path->nodes[0];
> > -       btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> > -
> > -       if (found_key.objectid != btrfs_ino(inode) ||
> > -           found_key.type != BTRFS_DIR_INDEX_KEY) {
> > -               inode->index_cnt = BTRFS_DIR_START_INDEX;
> > -               goto out;
> > -       }
> > -
> > -       inode->index_cnt = found_key.offset + 1;
> > -out:
> > -       btrfs_free_path(path);
> > -       return ret;
> > -}
> > -
> >   /*
> >    * helper to find a free sequence number in a given directory.  This current
> >    * code is very simple, later versions will do smarter things in the btree
> > --
> > 2.43.0