Skip to contentSkip to conversation

FAILED: patch "[PATCH] Btrfs: fix negative subv_writers counter and data space leak" failed to apply to 4.9-stable tree

PATCH [email protected] 2 participants 2 messages Dec 15, 2019

Subsystems (2)
Stable branch Btrfs file system
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From a0e248bb502d5165b3314ac3819e888fdcdf7d9f Mon Sep 17 00:00:00 2001
From: Filipe Manana <[email protected]>
Date: Fri, 11 Oct 2019 16:41:20 +0100
Subject: [PATCH] Btrfs: fix negative subv_writers counter and data space leak
after buffered write

When doing a buffered write it's possible to leave the subv_writers counter of the root, used for synchronization between buffered nocow writers and snapshotting. This happens in an exceptional case like the following:

1) We fail to allocate data space for the write, since there's not enough available data space nor enough unallocated space for allocating a new data block group;

2) Because of that failure, we try to go to NOCOW mode, which succeeds and therefore we set the local variable 'only_release_metadata' to true and set the root's sub_writers counter to 1 through the call to btrfs_start_write_no_snapshotting() made by check_can_nocow();

3) The call to btrfs_copy_from_user() returns zero, which is very unlikely to happen but not impossible;

4) No pages are copied because btrfs_copy_from_user() returned zero;

5) We call btrfs_end_write_no_snapshotting() which decrements the root's subv_writers counter to 0;

6) We don't set 'only_release_metadata' back to 'false' because we do it only if 'copied', the value returned by btrfs_copy_from_user(), is greater than zero;

7) On the next iteration of the while loop, which processes the same page range, we are now able to allocate data space for the write (we got enough data space released in the meanwhile);

8) After this if we fail at btrfs_delalloc_reserve_metadata(), because now there isn't enough free metadata space, or in some other place further below (prepare_pages(), lock_and_cleanup_extent_if_need(), btrfs_dirty_pages()), we break out of the while loop with 'only_release_metadata' having a value of 'true';

9) Because 'only_release_metadata' is 'true' we end up decrementing the root's subv_writers counter to -1 (through a call to btrfs_end_write_no_snapshotting()), and we also end up not releasing the data space previously reserved through btrfs_check_data_free_space(). As a consequence the mechanism for synchronizing NOCOW buffered writes with snapshotting gets broken.

Fix this by always setting 'only_release_metadata' to false at the start of each iteration.

Fixes 8257b2dc3c1a ("Btrfs: introduce btrfs_{start, end}_nocow_write() for each subvolume")
Fixes 7ee9e4405f26 ("Btrfs: check if we can nocow if we don't have data space")
CC [email protected] # 4.4+
Reviewed-by Josef Bacik <[email protected]>
Signed-off-by Filipe Manana <[email protected]>
Reviewed-by David Sterba <[email protected]>
Signed-off-by David Sterba <[email protected]>
fs/btrfs/file.c +1 -1
index f9434fa3e387..91c98a6d1408 100644
@@ -1636,6 +1636,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 			break;
 		}
 
+		only_release_metadata = false;
 		sector_offset = pos & (fs_info->sectorsize - 1);
 		reserve_bytes = round_up(write_bytes + sector_offset,
 				fs_info->sectorsize);

@@ -1791,7 +1792,6 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 			set_extent_bit(&BTRFS_I(inode)->io_tree, lockstart,
 				       lockend, EXTENT_NORESERVE, NULL,
 				       NULL, GFP_NOFS);
-			only_release_metadata = false;
 		}
 
 		btrfs_drop_pages(pages, num_pages);
maintainer
#1 lore raw
Quoted 64 lines · On Sun, Dec 15, 2019 at 12:10:23PM +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From a0e248bb502d5165b3314ac3819e888fdcdf7d9f Mon Sep 17 00:00:00 2001
From: Filipe Manana <[email protected]>
Date: Fri, 11 Oct 2019 16:41:20 +0100
Subject: [PATCH] Btrfs: fix negative subv_writers counter and data space leak after buffered write

When doing a buffered write it's possible to leave the subv_writers counter of the root, used for synchronization between buffered nocow writers and snapshotting. This happens in an exceptional case like the following:

1) We fail to allocate data space for the write, since there's not enough available data space nor enough unallocated space for allocating a new data block group;

2) Because of that failure, we try to go to NOCOW mode, which succeeds and therefore we set the local variable 'only_release_metadata' to true and set the root's sub_writers counter to 1 through the call to btrfs_start_write_no_snapshotting() made by check_can_nocow();

3) The call to btrfs_copy_from_user() returns zero, which is very unlikely to happen but not impossible;

4) No pages are copied because btrfs_copy_from_user() returned zero;

5) We call btrfs_end_write_no_snapshotting() which decrements the root's subv_writers counter to 0;

6) We don't set 'only_release_metadata' back to 'false' because we do it only if 'copied', the value returned by btrfs_copy_from_user(), is greater than zero;

7) On the next iteration of the while loop, which processes the same page range, we are now able to allocate data space for the write (we got enough data space released in the meanwhile);

8) After this if we fail at btrfs_delalloc_reserve_metadata(), because now there isn't enough free metadata space, or in some other place further below (prepare_pages(), lock_and_cleanup_extent_if_need(), btrfs_dirty_pages()), we break out of the while loop with 'only_release_metadata' having a value of 'true';

9) Because 'only_release_metadata' is 'true' we end up decrementing the root's subv_writers counter to -1 (through a call to btrfs_end_write_no_snapshotting()), and we also end up not releasing the data space previously reserved through btrfs_check_data_free_space(). As a consequence the mechanism for synchronizing NOCOW buffered writes with snapshotting gets broken.

Fix this by always setting 'only_release_metadata' to false at the start of each iteration.

Quoted 7 lines
Fixes 8257b2dc3c1a ("Btrfs: introduce btrfs_{start, end}_nocow_write() for each subvolume")
Fixes 7ee9e4405f26 ("Btrfs: check if we can nocow if we don't have data space")
CC [email protected] # 4.4+
Reviewed-by Josef Bacik <[email protected]>
Signed-off-by Filipe Manana <[email protected]>
Reviewed-by David Sterba <[email protected]>
Signed-off-by David Sterba <[email protected]>

Adjusted context for missing da17066c4047 ("btrfs: pull node/sector/stripe sizes out of root and into fs_info") and queued up for 4.9 and 4.4.

Signature · 2 lines
Thanks,
Sasha