[Drbd-dev] [PATCH] Fixed possible use after free in drbd_thread_setup

[Drbd-dev] [PATCH] Fixed possible use after free in drbd_thread_setup

[johannes]

From: Johannes Thoma <johannes@johannesthoma.com>

drbd_thread might already be freed when complete returns,
hence we shouldn't access the drbd_thread object (thi)
after calling complete().

I am not 100% sure if this creates any further races,
alternative would be to acquire the lock before freeing
the thread object (so that spin_unlock_irqrestore() has
exited already). Please let me know what you think.

Signed-off-by: Johannes Thoma <johannes@johannesthoma.com>
---
 drbd/drbd_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
index a2b5683..dbf2e41 100644
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -563,8 +563,8 @@ restart:
 	else
 		drbd_info(resource, "Terminating %s thread\n", thi->name);
 
-	complete(&thi->stop);
 	spin_unlock_irqrestore(&thi->t_lock, flags);
+	complete(&thi->stop);
 
 	if (connection)
 		kref_put(&connection->kref, drbd_destroy_connection);
-- 
2.8.0-rc4

Re: [Drbd-dev] [PATCH] Fixed possible use after free in drbd_thread_setup

[Lars Ellenberg]

On Thu, Dec 21, 2017 at 06:53:30PM +0100, johannes@johannesthoma.com wrote:
> drbd_thread might already be freed when complete returns,

The lifetime of our "struct drbd_tread" thingies, which are embeded in
our struct drbd_resource and struct drbd_connection,
is different from the "running" time of the threads.

So no, this won't happen.

> hence we shouldn't access the drbd_thread object (thi)
> after calling complete().
> 
> I am not 100% sure if this creates any further races,

Moving that complete out of the spinlock would introduce
potential races between drbd_thread_setup, drbd_thread_start,
and _drbd_thread_stop, yes.

	Lars