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

* [PATCH v2] libbtrfsutil: fix race between subvolume iterator and deletion
@ 2021-07-28 19:04 Omar Sandoval
  2021-07-28 21:14 ` Neal Gompa
  2021-07-29 11:11 ` David Sterba
  0 siblings, 2 replies; 3+ messages in thread
From: Omar Sandoval @ 2021-07-28 19:04 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread