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