[PATCH] nvme-rdma: Always signal fabrics private commands

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

Some RDMA adapters were observed to have some issues
with selective completion signaling which might cause
a use-after-free condition when the device accidentally
reports a completion when the caller context (wr_cqe)
was already freed.

The first time this was detected was for flush requests
that were not allocated from the tagset, now we see that
in the error path of fabrics connect (admin). The normal
I/O selective signaling is safe because we free the tagset
only when all the queue-pairs were drained.

Reported-by: Steve Wise <swise at opengridcomputing.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b939f89ad936..bf141cb4e671 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1453,7 +1453,8 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	ib_dma_sync_single_for_device(dev, sqe->dma,
 			sizeof(struct nvme_command), DMA_TO_DEVICE);
 
-	if (rq->cmd_type == REQ_TYPE_FS && req_op(rq) == REQ_OP_FLUSH)
+	if ((rq->cmd_type == REQ_TYPE_FS && req_op(rq) == REQ_OP_FLUSH) ||
+	    rq->cmd_type == REQ_TYPE_DRV_PRIV)
 		flush = true;
 	ret = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
 			req->need_inval ? &req->reg_wr.wr : NULL, flush);
-- 
1.9.1

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

This fixes the problem.  Thanks Sagi!

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

[PATCH] nvme-rdma: Always signal fabrics private commands

[Christoph Hellwig]

On Thu, Jun 23, 2016@07:08:24PM +0300, Sagi Grimberg wrote:
> Some RDMA adapters were observed to have some issues
> with selective completion signaling which might cause
> a use-after-free condition when the device accidentally
> reports a completion when the caller context (wr_cqe)
> was already freed.

I'd really love to fully root cause this issue and find a way
to fix it in the driver or core.  This isn't really something
a ULP should have to care about, and I'm trying to understand how
the existing ULPs get away without this.

I think we should apply this anyway for now unless we can come up
woth something better, but I'm not exactly happy about it.

> The first time this was detected was for flush requests
> that were not allocated from the tagset, now we see that
> in the error path of fabrics connect (admin). The normal
> I/O selective signaling is safe because we free the tagset
> only when all the queue-pairs were drained.

So for flush we needed this because the flush request is allocated
as part of the hctx, but pass through requests aren't really
special in terms of allocation.  What's the reason we need to
treat these special?

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> On Thu, Jun 23, 2016@07:08:24PM +0300, Sagi Grimberg wrote:
> > Some RDMA adapters were observed to have some issues
> > with selective completion signaling which might cause
> > a use-after-free condition when the device accidentally
> > reports a completion when the caller context (wr_cqe)
> > was already freed.
> 
> I'd really love to fully root cause this issue and find a way
> to fix it in the driver or core.  This isn't really something
> a ULP should have to care about, and I'm trying to understand how
> the existing ULPs get away without this.
>

Haven't we root caused it?  iw_cxgb4 cannot free up SQ slots containing
unsignaled WRs until a subsequent signaled WR is completed and polled by the
ULP.  If the QP is moved out of RTS before that happens thyen the unsignaled WRs
are completed as FLUSHED.  And NVMF is not ensuring that for all unsignaled WRs,
the wr_cqe remains around until the qp is flushed. 

>From a quick browse of the ULPS that support iw_cxgb4, it looks like NFSRDMA
server always signals, and NFSRDMA client always posts chains that end in a
signaled WR (not 100% sure on this).  iser does control its signaling, and it
perhaps suffers from the same problem.  But the target side has only now become
enabled for iwarp/cxgb4, so we'll see if we hit the same problems.  It appears
isert always signals.  

> I think we should apply this anyway for now unless we can come up
> woth something better, but I'm not exactly happy about it.
> 
> > The first time this was detected was for flush requests
> > that were not allocated from the tagset, now we see that
> > in the error path of fabrics connect (admin). The normal
> > I/O selective signaling is safe because we free the tagset
> > only when all the queue-pairs were drained.
> 
> So for flush we needed this because the flush request is allocated
> as part of the hctx, but pass through requests aren't really
> special in terms of allocation.  What's the reason we need to
> treat these special?

Perhaps it is just avoiding the problem by nature of being a signaled WR causing
iw_cxgb4 to now know the unsignaled WRs are complete...

I'm happy to help with guidance.  I'm not very familiar with the NVMF code above
its use of RDMA though.  And my solutions to try and fix this have all been
considered incorrect. :)

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

>> Some RDMA adapters were observed to have some issues
>> with selective completion signaling which might cause
>> a use-after-free condition when the device accidentally
>> reports a completion when the caller context (wr_cqe)
>> was already freed.
>
> I'd really love to fully root cause this issue and find a way
> to fix it in the driver or core.
> This isn't really something a ULP should have to care about, and I'm trying to understand how
> the existing ULPs get away without this.

It's a cxgb4 specific work-around (the only device this was observed
by). Not sure how this can be addressed in the core. We could comment
that in the code.

> I think we should apply this anyway for now unless we can come up
> woth something better, but I'm not exactly happy about it.
>
>> The first time this was detected was for flush requests
>> that were not allocated from the tagset, now we see that
>> in the error path of fabrics connect (admin). The normal
>> I/O selective signaling is safe because we free the tagset
>> only when all the queue-pairs were drained.
>
> So for flush we needed this because the flush request is allocated
> as part of the hctx, but pass through requests aren't really
> special in terms of allocation.  What's the reason we need to
> treat these special?

OK heres what I think is going on. we allocate the rdma queue and
issue admin connect (unsignaled). connect fails, and we orderly
teardown the admin queue (free the tagset, put the device and free the
queue).

Due to the fact that the cxgb4 driver is responsible for flushing
pending work requests and has no way of telling what the HW processed
other than the head/tail indexes (which are probably updated at
completion time) it sees the admin connect in the send-queue, it
doesn't know if the HW did anything with it, so it flushes it anyway.

Our error path is freeing the tagset before we free the queue (draining
the qp) so we get to a use-after-free condition (->done() is a freed
tag memory).

Note that we must allocate the qp before we allocate the tagset because
we need the device when init_request callouts come. So we allocated
before, we free after. An alternative fix was to free the queue before
the tagset even though we allocated it before (as Steve suggested).

[PATCH] nvme-rdma: Always signal fabrics private commands

[Christoph Hellwig]

On Sun, Jun 26, 2016@07:41:39PM +0300, Sagi Grimberg wrote:
> Our error path is freeing the tagset before we free the queue (draining
> the qp) so we get to a use-after-free condition (->done() is a freed
> tag memory).
>
> Note that we must allocate the qp before we allocate the tagset because
> we need the device when init_request callouts come. So we allocated
> before, we free after. An alternative fix was to free the queue before
> the tagset even though we allocated it before (as Steve suggested).

Would draining, but not freeing the qp before freeing the tagset work?
That seems like the most sensible option here.

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> On Sun, Jun 26, 2016@07:41:39PM +0300, Sagi Grimberg wrote:
> > Our error path is freeing the tagset before we free the queue (draining
> > the qp) so we get to a use-after-free condition (->done() is a freed
> > tag memory).
> >
> > Note that we must allocate the qp before we allocate the tagset because
> > we need the device when init_request callouts come. So we allocated
> > before, we free after. An alternative fix was to free the queue before
> > the tagset even though we allocated it before (as Steve suggested).
> 
> Would draining, but not freeing the qp before freeing the tagset work?
> That seems like the most sensible option here.

disconnecting and draining, I think.

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> > On Sun, Jun 26, 2016@07:41:39PM +0300, Sagi Grimberg wrote:
> > > Our error path is freeing the tagset before we free the queue (draining
> > > the qp) so we get to a use-after-free condition (->done() is a freed
> > > tag memory).
> > >
> > > Note that we must allocate the qp before we allocate the tagset because
> > > we need the device when init_request callouts come. So we allocated
> > > before, we free after. An alternative fix was to free the queue before
> > > the tagset even though we allocated it before (as Steve suggested).
> >
> > Would draining, but not freeing the qp before freeing the tagset work?
> > That seems like the most sensible option here.
> 
> disconnecting and draining, I think.

How about this?

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index e1205c0..d3a0396 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -604,23 +604,32 @@ out_destroy_cm_id:
        return ret;
 }

-static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue)
+static void nvme_rdma_stop_queue(struct nvme_rdma_queue *queue)
 {
-       if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags))
-               return;
-
        rdma_disconnect(queue->cm_id);
        ib_drain_qp(queue->qp);
+}
+
+static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue)
+{
        nvme_rdma_destroy_queue_ib(queue);
        rdma_destroy_id(queue->cm_id);
 }

+static void nvme_rdma_stop_and_free_queue(struct nvme_rdma_queue *queue)
+{
+       if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags))
+               return;
+       nvme_rdma_stop_queue(queue);
+       nvme_rdma_free_queue(queue);
+}
+
 static void nvme_rdma_free_io_queues(struct nvme_rdma_ctrl *ctrl)
 {
        int i;

        for (i = 1; i < ctrl->queue_count; i++)
-               nvme_rdma_free_queue(&ctrl->queues[i]);
+               nvme_rdma_stop_and_free_queue(&ctrl->queues[i]);
 }

 static int nvme_rdma_connect_io_queues(struct nvme_rdma_ctrl *ctrl)
@@ -653,7 +662,7 @@ static int nvme_rdma_init_io_queues(struct nvme_rdma_ctrl
*ctrl)

 out_free_queues:
        for (; i >= 1; i--)
-               nvme_rdma_free_queue(&ctrl->queues[i]);
+               nvme_rdma_stop_and_free_queue(&ctrl->queues[i]);

        return ret;
 }
@@ -662,7 +671,7 @@ static void nvme_rdma_destroy_admin_queue(struct
nvme_rdma_ctrl *ctrl)
 {
        nvme_rdma_free_qe(ctrl->queues[0].device->dev, &ctrl->async_event_sqe,
                        sizeof(struct nvme_command), DMA_TO_DEVICE);
-       nvme_rdma_free_queue(&ctrl->queues[0]);
+       nvme_rdma_stop_and_free_queue(&ctrl->queues[0]);
        blk_cleanup_queue(ctrl->ctrl.admin_q);
        blk_mq_free_tag_set(&ctrl->admin_tag_set);
        nvme_rdma_dev_put(ctrl->device);
@@ -705,7 +714,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct
*work)
                        goto requeue;
        }

-       nvme_rdma_free_queue(&ctrl->queues[0]);
+       nvme_rdma_stop_and_free_queue(&ctrl->queues[0]);

        ret = blk_mq_reinit_tagset(&ctrl->admin_tag_set);
        if (ret)
@@ -1617,6 +1626,7 @@ static int nvme_rdma_configure_admin_queue(struct
nvme_rdma_ctrl *ctrl)
 out_cleanup_queue:
        blk_cleanup_queue(ctrl->ctrl.admin_q);
 out_free_tagset:
+       nvme_rdma_stop_queue(&ctrl->queues[0]);
        blk_mq_free_tag_set(&ctrl->admin_tag_set);
 out_put_dev:
        nvme_rdma_dev_put(ctrl->device);

[PATCH] nvme-rdma: Always signal fabrics private commands

['Christoph Hellwig']

On Wed, Jun 29, 2016@09:57:16AM -0500, Steve Wise wrote:
> > disconnecting and draining, I think.
> 
> How about this?

This looks reasonable, although it came throught totally whitespace
damaged..

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> >
> > How about this?
> 
> This looks reasonable, although it came throught totally whitespace
> damaged..
> 

Yes, I just cut/pasted.  I'll send out a patch today.  Sagi, did you have any
comments?

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> 
> > >
> > > How about this?
> >
> > This looks reasonable, although it came throught totally whitespace
> > damaged..
> >
> 
> Yes, I just cut/pasted.  I'll send out a patch today.  Sagi, did you have any
> comments?
> 

Hey Christoph,

I see you added it to nvmf-all.3.  Should I still send out a formal patch? 

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

>>> How about this?
>>
>> This looks reasonable, although it came throught totally whitespace
>> damaged..
>>
>
> Yes, I just cut/pasted.  I'll send out a patch today.  Sagi, did you have any
> comments?

Hi Steve,

Sorry for the late response, I was on vacation.

The patch looks fine,

Reviewed-by: Sagi Grimberg <sagi at imberg.me>

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

> Hi Steve,
>
> Sorry for the late response, I was on vacation.
>
> The patch looks fine,
>
> Reviewed-by: Sagi Grimberg <sagi at imberg.me>

Although, I think that we really should fix the
double completion for unsignaled wqes in cxgb4...
it might bite us elsewhere down the road.

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

 
> 
> > Hi Steve,
> >
> > Sorry for the late response, I was on vacation.
> >
> > The patch looks fine,
> >
> > Reviewed-by: Sagi Grimberg <sagi at imberg.me>
> 
> Although, I think that we really should fix the
> double completion for unsignaled wqes in cxgb4...
> it might bite us elsewhere down the road.

Double completion?  When the QP exits RTS with pending unsignaled SQ WRs, cxgb4
doesn't know if those were actually completed by hardware, so they are completed
with FLUSH_ERR status.  I _could_ change cxgb4 to just eat those, but I'm a
little worried about breaking the iWARP Verbs semantics.  Perhaps I shouldn't
be.  It does seem to be causing lots of pain...

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

> Double completion?  When the QP exits RTS with pending unsignaled SQ WRs, cxgb4
> doesn't know if those were actually completed by hardware, so they are completed
> with FLUSH_ERR status.  I _could_ change cxgb4 to just eat those, but I'm a
> little worried about breaking the iWARP Verbs semantics.  Perhaps I shouldn't
> be.  It does seem to be causing lots of pain...

What exactly breaks iWARP semantics here?

Think of a case where we posted unsignaled send, got a successful reply
from the peer, now we drain the qp, and the send which belongs to a
transaction that we already completed is flush with error. Does that
sound like a correct behavior?

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> > Double completion?  When the QP exits RTS with pending unsignaled SQ WRs,
> cxgb4
> > doesn't know if those were actually completed by hardware, so they are
> completed
> > with FLUSH_ERR status.  I _could_ change cxgb4 to just eat those, but I'm a
> > little worried about breaking the iWARP Verbs semantics.  Perhaps I
shouldn't
> > be.  It does seem to be causing lots of pain...
> 
> What exactly breaks iWARP semantics here?
> 
> Think of a case where we posted unsignaled send, got a successful reply
> from the peer, now we drain the qp, and the send which belongs to a
> transaction that we already completed is flush with error. Does that
> sound like a correct behavior?

Well, from the specification, yes.    From
https://tools.ietf.org/html/draft-hilland-rddp-verbs-00#section-8.1.3.1 :

----
An Unsignaled WR is defined as completed successfully when all of
   the following rules are met:


   *   A Work Completion is retrieved from the CQ associated with the
       SQ where the unsignaled Work Request was posted,

   *   that Work Completion corresponds to a subsequent Work Request on
       the same Send Queue as the unsignaled Work Request, and

   *   the subsequent Work Request is ordered after the unsignaled Work
       Request as per the ordering rules. Depending on the Work Request
       used, this may require using the Local Fence indicator in order
       to guarantee ordering.
---

So in your example, even though the application knows the SEND made it because
the peer replied and genereated an RQ completion, the iwarp provider does not
know the SEND made it...

[PATCH] nvme-rdma: Always signal fabrics private commands

[Sagi Grimberg]

>> Think of a case where we posted unsignaled send, got a successful reply
>> from the peer, now we drain the qp, and the send which belongs to a
>> transaction that we already completed is flush with error. Does that
>> sound like a correct behavior?
>
> Well, from the specification, yes.    From
> https://tools.ietf.org/html/draft-hilland-rddp-verbs-00#section-8.1.3.1 :
>
> ----
> An Unsignaled WR is defined as completed successfully when all of
>     the following rules are met:
>
>
>     *   A Work Completion is retrieved from the CQ associated with the
>         SQ where the unsignaled Work Request was posted,
>
>     *   that Work Completion corresponds to a subsequent Work Request on
>         the same Send Queue as the unsignaled Work Request, and
>
>     *   the subsequent Work Request is ordered after the unsignaled Work
>         Request as per the ordering rules. Depending on the Work Request
>         used, this may require using the Local Fence indicator in order
>         to guarantee ordering.
> ---

OK, thanks for educating me :)

> So in your example, even though the application knows the SEND made it because
> the peer replied and genereated an RQ completion, the iwarp provider does not
> know the SEND made it...

So we have two options here:

1. always make sure not to free anything related to SQEs until we
destroy the QP (hopefully won't bite us again, which is not a good
bet given that the sequence is not trivial).

2. always signal sends for iWARP (yukk...)

I pick poison 1 (for now...)

[PATCH] nvme-rdma: Always signal fabrics private commands

[Steve Wise]

> 
> 
> >> Think of a case where we posted unsignaled send, got a successful reply
> >> from the peer, now we drain the qp, and the send which belongs to a
> >> transaction that we already completed is flush with error. Does that
> >> sound like a correct behavior?
> >
> > Well, from the specification, yes.    From
> > https://tools.ietf.org/html/draft-hilland-rddp-verbs-00#section-8.1.3.1 :
> >
> > ----
> > An Unsignaled WR is defined as completed successfully when all of
> >     the following rules are met:
> >
> >
> >     *   A Work Completion is retrieved from the CQ associated with the
> >         SQ where the unsignaled Work Request was posted,
> >
> >     *   that Work Completion corresponds to a subsequent Work Request on
> >         the same Send Queue as the unsignaled Work Request, and
> >
> >     *   the subsequent Work Request is ordered after the unsignaled Work
> >         Request as per the ordering rules. Depending on the Work Request
> >         used, this may require using the Local Fence indicator in order
> >         to guarantee ordering.
> > ---
> 
> OK, thanks for educating me :)
>

No problem. :)  By the way, IB Verbs has the same rules.  From 1.3 of the IBTA
spec:
----
10.8.6 UNSIGNALED COMPLETIONS

An unsignaled Work Request that completed successfully is confirmed
when all of the following rules are met:

. A Work Completion is retrieved from the same CQ that is associated
with the Send Queue to which the unsignaled Work Request
was submitted.

. That Work Completion corresponds to a subsequent Work Request
on the same Send Queue as the unsignaled Work Request.

C10-108: The CI shall not access buffers associated with an Unsignaled
Work Request once a Work Completion has been retrieved that corresponds
to a subsequent Work Request on the same Send Queue.
----

 
> > So in your example, even though the application knows the SEND made it
because
> > the peer replied and genereated an RQ completion, the iwarp provider does
not
> > know the SEND made it...
> 
> So we have two options here:
> 
> 1. always make sure not to free anything related to SQEs until we
> destroy the QP (hopefully won't bite us again, which is not a good
> bet given that the sequence is not trivial).
> 
> 2. always signal sends for iWARP (yukk...)
> 
> I pick poison 1 (for now...)
>

I agree.