diff mbox series

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

Message ID 0f344e692b14ffbec90cb9f32e0d177c30326c37.1627498953.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show
Series [v2] libbtrfsutil: fix race between subvolume iterator and deletion | expand

Commit Message

Omar Sandoval July 28, 2021, 7:04 p.m. UTC
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 included test cases.

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.

Reviewed-by: Neal Gompa <ngompa13@gmail.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Changes from v1 -> v2:

- Added Neal's reviewed-by.
- Added test cases.

Let me know if you'd prefer the test cases as a separate patch instead.

 libbtrfsutil/python/tests/__init__.py       | 11 +++-
 libbtrfsutil/python/tests/test_subvolume.py | 73 +++++++++++++++++++--
 libbtrfsutil/subvolume.c                    | 18 ++++-
 3 files changed, 91 insertions(+), 11 deletions(-)

Comments

Neal Gompa July 28, 2021, 9:14 p.m. UTC | #1
On Wed, Jul 28, 2021 at 3:05 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 included test cases.
>
> 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.
>
> Reviewed-by: Neal Gompa <ngompa13@gmail.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> Changes from v1 -> v2:
>
> - Added Neal's reviewed-by.
> - Added test cases.
>
> Let me know if you'd prefer the test cases as a separate patch instead.
>
>  libbtrfsutil/python/tests/__init__.py       | 11 +++-
>  libbtrfsutil/python/tests/test_subvolume.py | 73 +++++++++++++++++++--
>  libbtrfsutil/subvolume.c                    | 18 ++++-
>  3 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/libbtrfsutil/python/tests/__init__.py b/libbtrfsutil/python/tests/__init__.py
> index 9fd6f6de..a1ea740e 100644
> --- a/libbtrfsutil/python/tests/__init__.py
> +++ b/libbtrfsutil/python/tests/__init__.py
> @@ -77,7 +77,16 @@ class BtrfsTestCase(unittest.TestCase):
>              mkfs = 'mkfs.btrfs'
>          try:
>              subprocess.check_call([mkfs, '-q', image])
> -            subprocess.check_call(['mount', '-o', 'loop', '--', image, mountpoint])
> +            subprocess.check_call(
> +                [
> +                    'mount',
> +                    '-o',
> +                    'loop,user_subvol_rm_allowed',
> +                    '--',
> +                    image,
> +                    mountpoint,
> +                ]
> +            )
>          except Exception as e:
>              os.rmdir(mountpoint)
>              os.remove(image)
> diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
> index 61055f53..2620b5c5 100644
> --- a/libbtrfsutil/python/tests/test_subvolume.py
> +++ b/libbtrfsutil/python/tests/test_subvolume.py
> @@ -17,6 +17,7 @@
>
>  import fcntl
>  import errno
> +import multiprocessing
>  import os
>  import os.path
>  from pathlib import PurePath
> @@ -493,20 +494,78 @@ class TestSubvolume(BtrfsTestCase):
>          finally:
>              os.chdir(pwd)
>
> +    def _skip_unless_have_unprivileged_subvolume_iterator(self, path):
> +        with drop_privs():
> +            try:
> +                for _ in btrfsutil.SubvolumeIterator(path):
> +                    break
> +            except OSError as e:
> +                if e.errno == errno.ENOTTY:
> +                    self.skipTest('BTRFS_IOC_GET_SUBVOL_ROOTREF is not available')
> +                else:
> +                    raise
> +
>      @skipUnlessHaveNobody
>      def test_subvolume_iterator_unprivileged(self):
>          os.chown(self.mountpoint, NOBODY_UID, -1)
>          pwd = os.getcwd()
>          try:
>              os.chdir(self.mountpoint)
> +            self._skip_unless_have_unprivileged_subvolume_iterator('.')
>              with drop_privs():
> -                try:
> -                    list(btrfsutil.SubvolumeIterator('.'))
> -                except OSError as e:
> -                    if e.errno == errno.ENOTTY:
> -                        self.skipTest('BTRFS_IOC_GET_SUBVOL_ROOTREF is not available')
> -                    else:
> -                        raise
>                  self._test_subvolume_iterator()
>          finally:
>              os.chdir(pwd)
> +
> +    @staticmethod
> +    def _create_and_delete_subvolume(i):
> +        dir_name = f'dir{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 _test_subvolume_iterator_race(self):
> +        procs = []
> +        fd = os.open('.', os.O_RDONLY | os.O_DIRECTORY)
> +        try:
> +            for i in range(10):
> +                procs.append(
> +                    multiprocessing.Process(
> +                        target=self._create_and_delete_subvolume,
> +                        args=(i,),
> +                        daemon=True,
> +                    )
> +                )
> +            for proc in procs:
> +                proc.start()
> +            for i in range(1000):
> +                with btrfsutil.SubvolumeIterator(fd) as it:
> +                    for _ in it:
> +                        pass
> +        finally:
> +            for proc in procs:
> +                proc.terminate()
> +                proc.join()
> +            os.close(fd)
> +
> +    def test_subvolume_iterator_race(self):
> +        pwd = os.getcwd()
> +        try:
> +            os.chdir(self.mountpoint)
> +            self._test_subvolume_iterator_race()
> +        finally:
> +            os.chdir(pwd)
> +
> +    def test_subvolume_iterator_race_unprivileged(self):
> +        os.chown(self.mountpoint, NOBODY_UID, -1)
> +        pwd = os.getcwd()
> +        try:
> +            os.chdir(self.mountpoint)
> +            self._skip_unless_have_unprivileged_subvolume_iterator('.')
> +            with drop_privs():
> +                self._test_subvolume_iterator_race()
> +        finally:
> +            os.chdir(pwd)
> 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
>

I like that test cases are part of the commit. It makes sense as part
of a logical change.

I know I've already done the review, but I'll reaffirm this version.

Reviewed-by: Neal Gompa <ngompa13@gmail.com>
David Sterba July 29, 2021, 11:11 a.m. UTC | #2
On Wed, Jul 28, 2021 at 12:04:45PM -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 included test cases.
> 
> 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.
> 
> Reviewed-by: Neal Gompa <ngompa13@gmail.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> Changes from v1 -> v2:
> 
> - Added Neal's reviewed-by.
> - Added test cases.

Replaced in devel, thanks.
diff mbox series

Patch

diff --git a/libbtrfsutil/python/tests/__init__.py b/libbtrfsutil/python/tests/__init__.py
index 9fd6f6de..a1ea740e 100644
--- a/libbtrfsutil/python/tests/__init__.py
+++ b/libbtrfsutil/python/tests/__init__.py
@@ -77,7 +77,16 @@  class BtrfsTestCase(unittest.TestCase):
             mkfs = 'mkfs.btrfs'
         try:
             subprocess.check_call([mkfs, '-q', image])
-            subprocess.check_call(['mount', '-o', 'loop', '--', image, mountpoint])
+            subprocess.check_call(
+                [
+                    'mount',
+                    '-o',
+                    'loop,user_subvol_rm_allowed',
+                    '--',
+                    image,
+                    mountpoint,
+                ]
+            )
         except Exception as e:
             os.rmdir(mountpoint)
             os.remove(image)
diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
index 61055f53..2620b5c5 100644
--- a/libbtrfsutil/python/tests/test_subvolume.py
+++ b/libbtrfsutil/python/tests/test_subvolume.py
@@ -17,6 +17,7 @@ 
 
 import fcntl
 import errno
+import multiprocessing
 import os
 import os.path
 from pathlib import PurePath
@@ -493,20 +494,78 @@  class TestSubvolume(BtrfsTestCase):
         finally:
             os.chdir(pwd)
 
+    def _skip_unless_have_unprivileged_subvolume_iterator(self, path):
+        with drop_privs():
+            try:
+                for _ in btrfsutil.SubvolumeIterator(path):
+                    break
+            except OSError as e:
+                if e.errno == errno.ENOTTY:
+                    self.skipTest('BTRFS_IOC_GET_SUBVOL_ROOTREF is not available')
+                else:
+                    raise
+
     @skipUnlessHaveNobody
     def test_subvolume_iterator_unprivileged(self):
         os.chown(self.mountpoint, NOBODY_UID, -1)
         pwd = os.getcwd()
         try:
             os.chdir(self.mountpoint)
+            self._skip_unless_have_unprivileged_subvolume_iterator('.')
             with drop_privs():
-                try:
-                    list(btrfsutil.SubvolumeIterator('.'))
-                except OSError as e:
-                    if e.errno == errno.ENOTTY:
-                        self.skipTest('BTRFS_IOC_GET_SUBVOL_ROOTREF is not available')
-                    else:
-                        raise
                 self._test_subvolume_iterator()
         finally:
             os.chdir(pwd)
+
+    @staticmethod
+    def _create_and_delete_subvolume(i):
+        dir_name = f'dir{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 _test_subvolume_iterator_race(self):
+        procs = []
+        fd = os.open('.', os.O_RDONLY | os.O_DIRECTORY)
+        try:
+            for i in range(10):
+                procs.append(
+                    multiprocessing.Process(
+                        target=self._create_and_delete_subvolume,
+                        args=(i,),
+                        daemon=True,
+                    )
+                )
+            for proc in procs:
+                proc.start()
+            for i in range(1000):
+                with btrfsutil.SubvolumeIterator(fd) as it:
+                    for _ in it:
+                        pass
+        finally:
+            for proc in procs:
+                proc.terminate()
+                proc.join()
+            os.close(fd)
+
+    def test_subvolume_iterator_race(self):
+        pwd = os.getcwd()
+        try:
+            os.chdir(self.mountpoint)
+            self._test_subvolume_iterator_race()
+        finally:
+            os.chdir(pwd)
+
+    def test_subvolume_iterator_race_unprivileged(self):
+        os.chown(self.mountpoint, NOBODY_UID, -1)
+        pwd = os.getcwd()
+        try:
+            os.chdir(self.mountpoint)
+            self._skip_unless_have_unprivileged_subvolume_iterator('.')
+            with drop_privs():
+                self._test_subvolume_iterator_race()
+        finally:
+            os.chdir(pwd)
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;
 		}