nvme-rdma: Fix early queue flags settings

PATCH New Sagi Grimberg 3 participants 13 messages Sep 20, 2016 – Sep 21, 2016 NVMe

Share & tools

Sagi Grimberg

author maintainer

2016-09-20 18:57 UTC #0 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.

Fixes e89ca58f9c90 ("nvme-rdma: add DELETING queue flag")

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/nvme/host/rdma.c +1 -1

index c2c2c28e6eb5..3437f0e8866f 100644
@@ -561,7 +561,6 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 
 	queue = &ctrl->queues[idx];
 	queue->ctrl = ctrl;
-	queue->flags = 0;
 	init_completion(&queue->cm_done);
 
 	if (idx > 0)

@@ -595,6 +594,7 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 		goto out_destroy_cm_id;
 	}
 
+	queue->flags = 0;
 	set_bit(NVME_RDMA_Q_CONNECTED, &queue->flags);
 
 	return 0;

Signature · 1 line

1.9.1

Steve Wise

2016-09-20 19:01 UTC #1 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 7 lines

When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.
Fixes e89ca58f9c90 ("nvme-rdma: add DELETING queue flag")
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Tests good.

Thanks Sagi!

Tested-by Steve Wise <swise at opengridcomputing.com>

Sagi Grimberg

author maintainer

2016-09-20 19:05 UTC #2 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 6 lines

Quoted 6 lines
When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.

Quoted 8 lines

Quoted 2 lines
Fixes e89ca58f9c90 ("nvme-rdma: add DELETING queue flag")
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Tests good.
Thanks Sagi!
Tested-by Steve Wise <swise at opengridcomputing.com>

Thanks Steve,

Jens, can you grab this last minute fix for rc8?

You can pull it from
git://git.infradead.org/nvme-fabrics.git nvmf-4.8-rc

Thanks

Christoph Hellwig

maintainer

2016-09-20 20:08 UTC #3 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

In reply to Sagi Grimberg

Quoted 4 lines · Sagi Grimberg

When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.

Can you add a comment explaining this to the assignment? Looking at the patch it looks like black magic to be, and that's even after reading the above changelog..

Steve Wise

2016-09-20 20:14 UTC #4 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 10 lines · Sagi Grimberg

Quoted 4 lines
When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.
Can you add a comment explaining this to the assignment? Looking at the patch it looks like black magic to be, and that's even after reading the above changelog..

Maybe this changelog?

nvme-rdma: only clear queue flags after successful connect

Otherwise, nvme_rdma_stop_and_clear_queue() will incorrectly try to stop/free rdma qps/cm_ids that are already freed.

Sagi Grimberg

author maintainer

2016-09-21 03:38 UTC #5 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 11 lines

Quoted 10 lines · Sagi Grimberg
Quoted 4 lines
When we reconnect we can't really clear the queue flags (DELETING flag in specific) because we might end up trigerring a use-after-free condition if we fail to establish the rdma connection.
Can you add a comment explaining this to the assignment? Looking at the patch it looks like black magic to be, and that's even after reading the above changelog..

Quoted 6 lines

Maybe this changelog?
nvme-rdma: only clear queue flags after successful connect
Otherwise, nvme_rdma_stop_and_clear_queue() will incorrectly try to stop/free rdma qps/cm_ids that are already freed.

I can modify the change log, Christoph do you still want a comment in the code?

'Christoph Hellwig'

maintainer

2016-09-21 14:01 UTC #6 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 9 lines · Sagi Grimberg

Quoted 6 lines
Maybe this changelog?
nvme-rdma: only clear queue flags after successful connect
Otherwise, nvme_rdma_stop_and_clear_queue() will incorrectly try to stop/free rdma qps/cm_ids that are already freed.
I can modify the change log, Christoph do you still want a comment in the code?

Honestly there more I look into this the less I'm happy with the patch. queue->flags is an atomic, and as the patch shows we can get nvme_rdma_init_queue caled on a queue that still has visibility in other threads So I think we really should not even do that simple queue->flags = 0 assignment at all. We'll need to use clear_bit to atomically clear anything that might be set, and we need to be careful where we do that. I think this whole situation that we can get an *_init_* function called on something that already is live and visible to other threads need to be well documented at least because it's just waiting for sucker like me that don't expect that.

Steve Wise

2016-09-21 14:18 UTC #7 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 4 lines

Quoted 3 lines
I can modify the change log, Christoph do you still want a comment in the code?

Quoted 10 lines

Honestly there more I look into this the less I'm happy with the patch. queue->flags is an atomic, and as the patch shows we can get nvme_rdma_init_queue caled on a queue that still has visibility in other threads So I think we really should not even do that simple queue->flags = 0 assignment at all. We'll need to use clear_bit to atomically clear anything that might be set, and we need to be careful where we do that. I think this whole situation that we can get an *_init_* function called on something that already is live and visible to other threads need to be well documented at least because it's just waiting for sucker like me that don't expect that.

Sagi, you originally proposed this in a patch for debugging the crash where a request is accessing a queue with rdma resources freed:

@@ -542,11 +542,12 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue,

                 goto out_destroy_qp;
         }
         set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);

+ clear_bit(NVME_RDMA_Q_DELETING, &queue->flags);

         return 0;

Perhaps this is how we should proceed?

Steve Wise

2016-09-21 14:52 UTC #8 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 1 line

Sagi, you originally proposed this in a patch for debugging the crash

where

Quoted 12 lines

a request is accessing a queue with rdma resources freed:

(quoted diff) +1 -0

@@ -542,11 +542,12 @@ static int nvme_rdma_create_queue_ib(struct
 nvme_rdma_queue *queue,
                 goto out_destroy_qp;
         }
         set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);
+       clear_bit(NVME_RDMA_Q_DELETING, &queue->flags);
 
         return 0;

Quoted 1 line

Perhaps this is how we should proceed?

Just tested this, and it doesn't fix the problem. This makes sense, because we really only want to clear it if we successfully setup the connection... So never mind. :)

Steve Wise

2016-09-21 15:10 UTC #9 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 11 lines

Quoted 1 line
Sagi, you originally proposed this in a patch for debugging the crash
where
Quoted 9 lines
a request is accessing a queue with rdma resources freed:
(quoted diff) +1 -0
@@ -542,11 +542,12 @@ static int nvme_rdma_create_queue_ib(struct
 nvme_rdma_queue *queue,
                 goto out_destroy_qp;
         }
         set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);
+       clear_bit(NVME_RDMA_Q_DELETING, &queue->flags);
 

Quoted 6 lines

Quoted 4 lines
(quoted diff) +0 -0
         return 0;
 
 
Perhaps this is how we should proceed?
Just tested this, and it doesn't fix the problem. This makes sense,

because

Quoted 2 lines

we really only want to clear it if we successfully setup the connection... So never mind. :)

What I'm trying to say is that if you use clear_bit(), it needs to be exactly where the flags = 0 was.

Thanks,

Steve.

Sagi Grimberg

author maintainer

2016-09-21 14:36 UTC #10 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

In reply to 'Christoph Hellwig'

Quoted 11 lines

Quoted 9 lines
Quoted 6 lines
Maybe this changelog?
nvme-rdma: only clear queue flags after successful connect
Otherwise, nvme_rdma_stop_and_clear_queue() will incorrectly try to stop/free rdma qps/cm_ids that are already freed.
I can modify the change log, Christoph do you still want a comment in the code?
Honestly there more I look into this the less I'm happy with the patch.

I agree that we should really do a proper queue state machine.

Quoted 9 lines

queue->flags is an atomic, and as the patch shows we can get nvme_rdma_init_queue caled on a queue that still has visibility in other threads So I think we really should not even do that simple queue->flags = 0 assignment at all. We'll need to use clear_bit to atomically clear anything that might be set, and we need to be careful where we do that. I think this whole situation that we can get an *_init_* function called on something that already is live and visible to other threads need to be well documented at least because it's just waiting for sucker like me that don't expect that.

I don't think its a multithreading issue. We aren't expected to get here concurrently from different contexts (I think we're screwed if we do). The issue was that we are not clearing the DELETING flag on reconnects causing possible leaks, then I "fixed" it by resetting the queue flags at init_queue start, the problem was that if we failed to init the queue we lost the DELETING flag and got to a use-after-free condition (nothing prevented stop_and_free_queue to make forward progress...

I can move it to clear_bit if you want...

Steve Wise

2016-09-21 15:50 UTC #11 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 34 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Maybe this changelog?
nvme-rdma: only clear queue flags after successful connect
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Otherwise, nvme_rdma_stop_and_clear_queue() will incorrectly try to stop/free rdma qps/cm_ids that are already freed.
I can modify the change log, Christoph do you still want a comment in the code?
Honestly there more I look into this the less I'm happy with the patch.
I agree that we should really do a proper queue state machine.
Quoted 9 lines
queue->flags is an atomic, and as the patch shows we can get nvme_rdma_init_queue caled on a queue that still has visibility in other threads So I think we really should not even do that simple queue->flags = 0 assignment at all. We'll need to use clear_bit to atomically clear anything that might be set, and we need to be careful where we do that. I think this whole situation that we can get an *_init_* function called on something that already is live and visible to other threads need to be well documented at least because it's just waiting for sucker like me that don't expect that.
I don't think its a multithreading issue. We aren't expected to get here concurrently from different contexts (I think we're screwed if we do). The issue was that we are not clearing the DELETING flag on reconnects causing possible leaks, then I "fixed" it by resetting the queue flags at init_queue start, the problem was that if we failed to init the queue we lost the DELETING flag and got to a use-after-free condition (nothing prevented stop_and_free_queue to make forward progress...

Quoted 2 lines

I can move it to clear_bit if you want...

I'd like this in 4.8 to avoid the crash during reconnect. (the clear_bit() is fine and tests ok).

I'll sign up to work with Sagi on a proper state machine fix for 4.9. How's that sound?

Steve.

Sagi Grimberg

author maintainer

2016-09-21 19:57 UTC #12 lore raw

PATCH 1/1 nvme-rdma: Fix early queue flags settings

Quoted 24 lines

Quoted 24 lines
Quoted 2 lines
Honestly there more I look into this the less I'm happy with the patch.
I agree that we should really do a proper queue state machine.
Quoted 9 lines
queue->flags is an atomic, and as the patch shows we can get nvme_rdma_init_queue caled on a queue that still has visibility in other threads So I think we really should not even do that simple queue->flags = 0 assignment at all. We'll need to use clear_bit to atomically clear anything that might be set, and we need to be careful where we do that. I think this whole situation that we can get an *_init_* function called on something that already is live and visible to other threads need to be well documented at least because it's just waiting for sucker like me that don't expect that.
I don't think its a multithreading issue. We aren't expected to get here concurrently from different contexts (I think we're screwed if we do). The issue was that we are not clearing the DELETING flag on reconnects causing possible leaks, then I "fixed" it by resetting the queue flags at init_queue start, the problem was that if we failed to init the queue we lost the DELETING flag and got to a use-after-free condition (nothing prevented stop_and_free_queue to make forward progress...

Quoted 8 lines

Quoted 2 lines
I can move it to clear_bit if you want...
I'd like this in 4.8 to avoid the crash during reconnect. (the clear_bit() is fine and tests ok).
I'll sign up to work with Sagi on a proper state machine fix for 4.9. How's that sound?

Sounds good to me, I'll send out the clear_bit() version.