btrfs: Fix handling of -ENOENT from btrfs_uuid_iter_rem
diff mbox

Message ID 1473233938-21560-1-git-send-email-kernel@kyup.com
State New
Headers show

Commit Message

kernel@kyup.com Sept. 7, 2016, 7:38 a.m. UTC
btrfs_uuid_iter_rem is able to return -ENOENT, however this condition
is not handled in btrfs_uuid_tree_iterate which can lead to calling
btrfs_next_item with freed path argument, leading to a null pointer
dereference. Fix it by redoing the search but with an incremented
objectid so we don't loop over the same key.

Signed-off-by: Nikolay Borisov <kernel@kyup.com>
Suggested-by: Chris Mason <clm@fb.com>
Link: https://lkml.kernel.org/r/57A473B0.2040203@kyup.com
---
 fs/btrfs/uuid-tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Hello Chris, 

Since I keep getting those crashes I (hopefully correctly) implemented
your suggestion of redoing the search with an incremented key so we 
don't end up in a loop. Does that look correct?

Comments

David Sterba Sept. 19, 2016, 6:13 p.m. UTC | #1
On Wed, Sep 07, 2016 at 10:38:58AM +0300, Nikolay Borisov wrote:
> btrfs_uuid_iter_rem is able to return -ENOENT, however this condition
> is not handled in btrfs_uuid_tree_iterate which can lead to calling
> btrfs_next_item with freed path argument, leading to a null pointer
> dereference. Fix it by redoing the search but with an incremented
> objectid so we don't loop over the same key.
> 
> Signed-off-by: Nikolay Borisov <kernel@kyup.com>
> Suggested-by: Chris Mason <clm@fb.com>
> Link: https://lkml.kernel.org/r/57A473B0.2040203@kyup.com

I'll queue the patch for 4.9, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Mason Sept. 19, 2016, 6:49 p.m. UTC | #2
On 09/19/2016 02:13 PM, David Sterba wrote:
> On Wed, Sep 07, 2016 at 10:38:58AM +0300, Nikolay Borisov wrote:
>> btrfs_uuid_iter_rem is able to return -ENOENT, however this condition
>> is not handled in btrfs_uuid_tree_iterate which can lead to calling
>> btrfs_next_item with freed path argument, leading to a null pointer
>> dereference. Fix it by redoing the search but with an incremented
>> objectid so we don't loop over the same key.
>>
>> Signed-off-by: Nikolay Borisov <kernel@kyup.com>
>> Suggested-by: Chris Mason <clm@fb.com>
>> Link: https://lkml.kernel.org/r/57A473B0.2040203@kyup.com
>
> I'll queue the patch for 4.9, thanks.
>

Not having a good test for this kept me from trying the patch cold.  I 
think bumping the objectid will end up missing items.

We know its returning -ENOENT, so it should in theory be enough to just 
goto again_search_slot, assuming that we just raced with the deletion.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Sept. 19, 2016, 8:18 p.m. UTC | #3
On Mon, Sep 19, 2016 at 02:49:41PM -0400, Chris Mason wrote:
> On 09/19/2016 02:13 PM, David Sterba wrote:
> > On Wed, Sep 07, 2016 at 10:38:58AM +0300, Nikolay Borisov wrote:
> >> btrfs_uuid_iter_rem is able to return -ENOENT, however this condition
> >> is not handled in btrfs_uuid_tree_iterate which can lead to calling
> >> btrfs_next_item with freed path argument, leading to a null pointer
> >> dereference. Fix it by redoing the search but with an incremented
> >> objectid so we don't loop over the same key.
> >>
> >> Signed-off-by: Nikolay Borisov <kernel@kyup.com>
> >> Suggested-by: Chris Mason <clm@fb.com>
> >> Link: https://lkml.kernel.org/r/57A473B0.2040203@kyup.com
> >
> > I'll queue the patch for 4.9, thanks.
> >
> 
> Not having a good test for this kept me from trying the patch cold.  I 
> think bumping the objectid will end up missing items.

Ok, so I can keep it in the branches that are not for the upcoming
merges but still in for-next.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel@kyup.com Sept. 20, 2016, 7:36 a.m. UTC | #4
[Resend due to gmail mobile interface defaulting to html layout]
>>
>> We know its returning -ENOENT, so it should in theory be enough to just
>> goto again_search_slot, assuming that we just raced with the deletion.
>
>
> I will apply this on the machine which are exhibitting problems and will
> report whether it rectified the situation. i bump the objectid wince this is
> what you suggested. i can also try without it.
>>
>>
>> -chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/uuid-tree.c b/fs/btrfs/uuid-tree.c
index 778282944530..6e5b3866a65c 100644
--- a/fs/btrfs/uuid-tree.c
+++ b/fs/btrfs/uuid-tree.c
@@ -329,8 +329,12 @@  again_search_slot:
 					 * entry per UUID exists.
 					 */
 					goto again_search_slot;
-				}
-				if (ret < 0 && ret != -ENOENT)
+				} else if (ret == -ENOENT) {
+					key.type = 0;
+					key.offset = 0;
+					key.objectid++;
+					goto again_search_slot;
+				} else if (ret < 0)
 					goto out;
 			}
 			item_size -= sizeof(subid_le);