btrfs: fix race on ENOMEM in alloc_extent_buffer

osandov · masoncl · commit 5ca64f45e92d · 2015-04-26T06:27:00.000-07:00
Consider the following interleaving of overlapping calls to
alloc_extent_buffer:

Call 1:

- Successfully allocates a few pages with find_or_create_page
- find_or_create_page fails, goto free_eb
- Unlocks the allocated pages

Call 2:
- Calls find_or_create_page and gets a page in call 1's extent_buffer
- Finds that the page is already associated with an extent_buffer
- Grabs a reference to the half-written extent_buffer and calls
  mark_extent_buffer_accessed on it

mark_extent_buffer_accessed will then try to call mark_page_accessed on
a null page and panic.

The fix is to decrement the reference count on the half-written
extent_buffer before unlocking the pages so call 2 won't use it. We
should also set exists = NULL in the case that we don't use exists to
avoid accidentally returning a freed extent_buffer in an error case.

Signed-off-by: Omar Sandoval &lt;osandov@osandov.com&gt;
Reviewed-by: David Sterba &lt;dsterba@suse.cz&gt;
Reviewed-by: Liu Bo &lt;bo.li.liu@oracle.com&gt;
Signed-off-by: Chris Mason &lt;clm@fb.com&gt;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
@@ -4870,6 +4870,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 				mark_extent_buffer_accessed(exists, p);
 				goto free_eb;
 			}
+			exists = NULL;
 
 			/*
 			 * Do this so attach doesn't complain and we need to
@@ -4933,12 +4934,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	return eb;
 
 free_eb:
+	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	for (i = 0; i < num_pages; i++) {
 		if (eb->pages[i])
 			unlock_page(eb->pages[i]);
 	}
 
-	WARN_ON(!atomic_dec_and_test(&eb->refs));
 	btrfs_release_extent_buffer(eb);
 	return exists;
 }

Original file line number	Diff line number	Diff line change
`@@ -4870,6 +4870,7 @@ struct extent_buffer alloc_extent_buffer(struct btrfs_fs_info fs_info,`
`4870`	`4870`	`mark_extent_buffer_accessed(exists, p);`
`4871`	`4871`	`goto free_eb;`
`4872`	`4872`	`}`
	`4873`	`+ exists = NULL;`
`4873`	`4874`
`4874`	`4875`	`/*`
`4875`	`4876`	`* Do this so attach doesn't complain and we need to`
`@@ -4933,12 +4934,12 @@ struct extent_buffer alloc_extent_buffer(struct btrfs_fs_info fs_info,`
`4933`	`4934`	`return eb;`
`4934`	`4935`
`4935`	`4936`	`free_eb:`
	`4937`	`+ WARN_ON(!atomic_dec_and_test(&eb->refs));`
`4936`	`4938`	`for (i = 0; i < num_pages; i++) {`
`4937`	`4939`	`if (eb->pages[i])`
`4938`	`4940`	`unlock_page(eb->pages[i]);`
`4939`	`4941`	`}`
`4940`	`4942`
`4941`		`- WARN_ON(!atomic_dec_and_test(&eb->refs));`
`4942`	`4943`	`btrfs_release_extent_buffer(eb);`
`4943`	`4944`	`return exists;`
`4944`	`4945`	`}`