[PATCH] libbtrfsutil: fix race between subvolume iterator and deletion

[PATCH] libbtrfsutil: fix race between subvolume iterator and deletion

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Subvolume iteration has a window between when we get a root ref (with
BTRFS_IOC_TREE_SEARCH or BTRFS_IOC_GET_SUBVOL_ROOTREF) and when we look
up the path of the parent directory (with BTRFS_IOC_INO_LOOKUP{,_USER}).
If the subvolume is moved or deleted and its old parent directory is
deleted during that window, then BTRFS_IOC_INO_LOOKUP{,_USER} will fail
with ENOENT. The iteration will then fail with ENOENT as well.

We originally encountered this bug with an application that called
`btrfs subvolume show` (which iterates subvolumes to find snapshots) in
parallel with other threads creating and deleting subvolumes. It can be
reproduced almost instantly with the following script:

  import multiprocessing
  import os

  import btrfsutil

  def create_and_delete_subvolume(i):
      dir_name = f"subvol_iter_race{i}"
      subvol_name = dir_name + "/subvol"
      while True:
          os.mkdir(dir_name)
          btrfsutil.create_subvolume(subvol_name)
          btrfsutil.delete_subvolume(subvol_name)
          os.rmdir(dir_name)

  def iterate_subvolumes():
      fd = os.open(".", os.O_RDONLY | os.O_DIRECTORY)
      while True:
          with btrfsutil.SubvolumeIterator(fd, 5) as it:
              for _ in it:
                  pass

  if __name__ == "__main__":
      for i in range(10):
          multiprocessing.Process(target=create_and_delete_subvolume, args=(i,), daemon=True).start()
      iterate_subvolumes()

Subvolume iteration should be robust against concurrent modifications to
subvolumes. So, if a subvolume's parent directory no longer exists, just
skip the subvolume, as it must have been deleted or moved elsewhere.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/subvolume.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index e30956b1..32086b7f 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -1469,8 +1469,16 @@ static enum btrfs_util_error subvolume_iterator_next_tree_search(struct btrfs_ut
 		name = (const char *)(ref + 1);
 		err = build_subvol_path_privileged(iter, header, ref, name,
 						   &path_len);
-		if (err)
+		if (err) {
+			/*
+			 * If the subvolume's parent directory doesn't exist,
+			 * then the subvolume was either moved or deleted. Skip
+			 * it.
+			 */
+			if (errno == ENOENT)
+				continue;
 			return err;
+		}
 
 		err = append_to_search_stack(iter,
 				btrfs_search_header_offset(header), path_len);
@@ -1539,8 +1547,12 @@ static enum btrfs_util_error subvolume_iterator_next_unprivileged(struct btrfs_u
 		err = build_subvol_path_unprivileged(iter, treeid, dirid,
 						     &path_len);
 		if (err) {
-			/* Skip the subvolume if we can't access it. */
-			if (errno == EACCES)
+			/*
+			 * If the subvolume's parent directory doesn't exist,
+			 * then the subvolume was either moved or deleted. Skip
+			 * it. Also skip it if we can't access it.
+			 */
+			if (errno == ENOENT || errno == EACCES)
 				continue;
 			return err;
 		}
-- 
2.32.0

Re: [PATCH] libbtrfsutil: fix race between subvolume iterator and deletion

[Neal Gompa]

On Tue, Jul 27, 2021 at 7:54 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> Subvolume iteration has a window between when we get a root ref (with
> BTRFS_IOC_TREE_SEARCH or BTRFS_IOC_GET_SUBVOL_ROOTREF) and when we look
> up the path of the parent directory (with BTRFS_IOC_INO_LOOKUP{,_USER}).
> If the subvolume is moved or deleted and its old parent directory is
> deleted during that window, then BTRFS_IOC_INO_LOOKUP{,_USER} will fail
> with ENOENT. The iteration will then fail with ENOENT as well.
>
> We originally encountered this bug with an application that called
> `btrfs subvolume show` (which iterates subvolumes to find snapshots) in
> parallel with other threads creating and deleting subvolumes. It can be
> reproduced almost instantly with the following script:
>
>   import multiprocessing
>   import os
>
>   import btrfsutil
>
>   def create_and_delete_subvolume(i):
>       dir_name = f"subvol_iter_race{i}"
>       subvol_name = dir_name + "/subvol"
>       while True:
>           os.mkdir(dir_name)
>           btrfsutil.create_subvolume(subvol_name)
>           btrfsutil.delete_subvolume(subvol_name)
>           os.rmdir(dir_name)
>
>   def iterate_subvolumes():
>       fd = os.open(".", os.O_RDONLY | os.O_DIRECTORY)
>       while True:
>           with btrfsutil.SubvolumeIterator(fd, 5) as it:
>               for _ in it:
>                   pass
>
>   if __name__ == "__main__":
>       for i in range(10):
>           multiprocessing.Process(target=create_and_delete_subvolume, args=(i,), daemon=True).start()
>       iterate_subvolumes()
>
> Subvolume iteration should be robust against concurrent modifications to
> subvolumes. So, if a subvolume's parent directory no longer exists, just
> skip the subvolume, as it must have been deleted or moved elsewhere.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  libbtrfsutil/subvolume.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
> index e30956b1..32086b7f 100644
> --- a/libbtrfsutil/subvolume.c
> +++ b/libbtrfsutil/subvolume.c
> @@ -1469,8 +1469,16 @@ static enum btrfs_util_error subvolume_iterator_next_tree_search(struct btrfs_ut
>                 name = (const char *)(ref + 1);
>                 err = build_subvol_path_privileged(iter, header, ref, name,
>                                                    &path_len);
> -               if (err)
> +               if (err) {
> +                       /*
> +                        * If the subvolume's parent directory doesn't exist,
> +                        * then the subvolume was either moved or deleted. Skip
> +                        * it.
> +                        */
> +                       if (errno == ENOENT)
> +                               continue;
>                         return err;
> +               }
>
>                 err = append_to_search_stack(iter,
>                                 btrfs_search_header_offset(header), path_len);
> @@ -1539,8 +1547,12 @@ static enum btrfs_util_error subvolume_iterator_next_unprivileged(struct btrfs_u
>                 err = build_subvol_path_unprivileged(iter, treeid, dirid,
>                                                      &path_len);
>                 if (err) {
> -                       /* Skip the subvolume if we can't access it. */
> -                       if (errno == EACCES)
> +                       /*
> +                        * If the subvolume's parent directory doesn't exist,
> +                        * then the subvolume was either moved or deleted. Skip
> +                        * it. Also skip it if we can't access it.
> +                        */
> +                       if (errno == ENOENT || errno == EACCES)
>                                 continue;
>                         return err;
>                 }
> --
> 2.32.0
>

LGTM.

Reviewed-by: Neal Gompa <ngompa13@gmail.com>


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH] libbtrfsutil: fix race between subvolume iterator and deletion

[David Sterba]

On Tue, Jul 27, 2021 at 04:53:28PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Subvolume iteration has a window between when we get a root ref (with
> BTRFS_IOC_TREE_SEARCH or BTRFS_IOC_GET_SUBVOL_ROOTREF) and when we look
> up the path of the parent directory (with BTRFS_IOC_INO_LOOKUP{,_USER}).
> If the subvolume is moved or deleted and its old parent directory is
> deleted during that window, then BTRFS_IOC_INO_LOOKUP{,_USER} will fail
> with ENOENT. The iteration will then fail with ENOENT as well.
> 
> We originally encountered this bug with an application that called
> `btrfs subvolume show` (which iterates subvolumes to find snapshots) in
> parallel with other threads creating and deleting subvolumes. It can be
> reproduced almost instantly with the following script:
> 
>   import multiprocessing
>   import os
> 
>   import btrfsutil
> 
>   def create_and_delete_subvolume(i):
>       dir_name = f"subvol_iter_race{i}"
>       subvol_name = dir_name + "/subvol"
>       while True:
>           os.mkdir(dir_name)
>           btrfsutil.create_subvolume(subvol_name)
>           btrfsutil.delete_subvolume(subvol_name)
>           os.rmdir(dir_name)
> 
>   def iterate_subvolumes():
>       fd = os.open(".", os.O_RDONLY | os.O_DIRECTORY)
>       while True:
>           with btrfsutil.SubvolumeIterator(fd, 5) as it:
>               for _ in it:
>                   pass
> 
>   if __name__ == "__main__":
>       for i in range(10):
>           multiprocessing.Process(target=create_and_delete_subvolume, args=(i,), daemon=True).start()
>       iterate_subvolumes()

Can you please turn this into a test case?

> Subvolume iteration should be robust against concurrent modifications to
> subvolumes. So, if a subvolume's parent directory no longer exists, just
> skip the subvolume, as it must have been deleted or moved elsewhere.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Added to devel, thanks.