lkml.org 
[lkml]   [2021]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.12 067/127] nvme-tcp: fix possible use-after-completion
    Date
    From: Sagi Grimberg <sagi@grimberg.me>

    commit 825619b09ad351894d2c6fb6705f5b3711d145c7 upstream.

    Commit db5ad6b7f8cd ("nvme-tcp: try to send request in queue_rq
    context") added a second context that may perform a network send.
    This means that now RX and TX are not serialized in nvme_tcp_io_work
    and can run concurrently.

    While there is correct mutual exclusion in the TX path (where
    the send_mutex protect the queue socket send activity) RX activity,
    and more specifically request completion may run concurrently.

    This means we must guarantee that any mutation of the request state
    related to its lifetime, bytes sent must not be accessed when a completion
    may have possibly arrived back (and processed).

    The race may trigger when a request completion arrives, processed
    _and_ reused as a fresh new request, exactly in the (relatively short)
    window between the last data payload sent and before the request iov_iter
    is advanced.

    Consider the following race:
    1. 16K write request is queued
    2. The nvme command and the data is sent to the controller (in-capsule
    or solicited by r2t)
    3. After the last payload is sent but before the req.iter is advanced,
    the controller sends back a completion.
    4. The completion is processed, the request is completed, and reused
    to transfer a new request (write or read)
    5. The new request is queued, and the driver reset the request parameters
    (nvme_tcp_setup_cmd_pdu).
    6. Now context in (2) resumes execution and advances the req.iter

    ==> use-after-completion as this is already a new request.

    Fix this by making sure the request is not advanced after the last
    data payload send, knowing that a completion may have arrived already.

    An alternative solution would have been to delay the request completion
    or state change waiting for reference counting on the TX path, but besides
    adding atomic operations to the hot-path, it may present challenges in
    multi-stage R2T scenarios where a r2t handler needs to be deferred to
    an async execution.

    Reported-by: Narayan Ayalasomayajula <narayan.ayalasomayajula@wdc.com>
    Tested-by: Anil Mishra <anil.mishra@wdc.com>
    Reviewed-by: Keith Busch <kbusch@kernel.org>
    Cc: stable@vger.kernel.org # v5.8+
    Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    drivers/nvme/host/tcp.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

    --- a/drivers/nvme/host/tcp.c
    +++ b/drivers/nvme/host/tcp.c
    @@ -940,7 +940,6 @@ static int nvme_tcp_try_send_data(struct
    if (ret <= 0)
    return ret;

    - nvme_tcp_advance_req(req, ret);
    if (queue->data_digest)
    nvme_tcp_ddgst_update(queue->snd_hash, page,
    offset, ret);
    @@ -957,6 +956,7 @@ static int nvme_tcp_try_send_data(struct
    }
    return 1;
    }
    + nvme_tcp_advance_req(req, ret);
    }
    return -EAGAIN;
    }

    \
     
     \ /
      Last update: 2021-05-24 18:31    [W:4.209 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site