Re: [PATCH 1/2] nvmet: add an helper to free the iovec

John Meneghini
2021-10-21T14:58:31Z

Looks good Maurizio.

You should also mention that you have a simple test you can run which reproduces this bug and results in a kernel panic.

We might want to add that test to the blktests tool.

Reviewed-by: John Meneghini <jmeneghi.redhat.com>

On 10/21/21 4:41 AM, Maurizio Lombardi wrote:
> Makes the code easier to read and to debug.Reviewed-by: John Meneghini <jmeneghi at redhat.com>
>
> Sets the freed pointers to NULL, it will be useful
> when destroying the queues to understand if the
> iovecs have been released already or not.
>
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
> drivers/nvme/target/tcp.c | 28 +++++++++++++++++++---------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index c33a0464346f..2f03a94725ae 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -166,6 +166,8 @@ static struct workqueue_struct *nvmet_tcp_wq;
> static const struct nvmet_fabrics_ops nvmet_tcp_ops;
> static void nvmet_tcp_free_cmd(struct nvmet_tcp_cmd *c);
> static void nvmet_tcp_finish_cmd(struct nvmet_tcp_cmd *cmd);
> +static void nvmet_tcp_free_iovec(struct nvmet_tcp_cmd *cmd);
> +static void nvmet_tcp_unmap_pdu_iovec(struct nvmet_tcp_cmd *cmd);
>
> static inline u16 nvmet_tcp_cmd_tag(struct nvmet_tcp_queue *queue,
> struct nvmet_tcp_cmd *cmd)
> @@ -297,6 +299,16 @@ static int nvmet_tcp_check_ddgst(struct nvmet_tcp_queue *queue, void *pdu)
> return 0;
> }
>
> +static void nvmet_tcp_free_iovec(struct nvmet_tcp_cmd *cmd)
> +{
> + WARN_ON(unlikely(cmd->nr_mapped > 0));
> +
> + kfree(cmd->iov);
> + sgl_free(cmd->req.sg);
> + cmd->iov = NULL;
> + cmd->req.sg = NULL;
> +}
> +
> static void nvmet_tcp_unmap_pdu_iovec(struct nvmet_tcp_cmd *cmd)
> {
> struct scatterlist *sg;
> @@ -306,6 +318,8 @@ static void nvmet_tcp_unmap_pdu_iovec(struct nvmet_tcp_cmd *cmd)
>
> for (i = 0; i < cmd->nr_mapped; i++)
> kunmap(sg_page(&sg[i]));
> +
> + cmd->nr_mapped = 0;
> }
>
> static void nvmet_tcp_map_pdu_iovec(struct nvmet_tcp_cmd *cmd)
> @@ -387,7 +401,7 @@ static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd)
>
> return 0;
> err:
> - sgl_free(cmd->req.sg);
> + nvmet_tcp_free_iovec(cmd);
> return NVME_SC_INTERNAL;
> }
>
> @@ -632,10 +646,8 @@ static int nvmet_try_send_data(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
> }
> }
>
> - if (queue->nvme_sq.sqhd_disabled) {
> - kfree(cmd->iov);
> - sgl_free(cmd->req.sg);
> - }
> + if (queue->nvme_sq.sqhd_disabled)
> + nvmet_tcp_free_iovec(cmd);
>
> return 1;
>
> @@ -664,8 +676,7 @@ static int nvmet_try_send_response(struct nvmet_tcp_cmd *cmd,
> if (left)
> return -EAGAIN;
>
> - kfree(cmd->iov);
> - sgl_free(cmd->req.sg);
> + nvmet_tcp_free_iovec(cmd);
> cmd->queue->snd_cmd = NULL;
> nvmet_tcp_put_cmd(cmd);
> return 1;
> @@ -1406,8 +1417,7 @@ static void nvmet_tcp_finish_cmd(struct nvmet_tcp_cmd *cmd)
> {
> nvmet_req_uninit(&cmd->req);
> nvmet_tcp_unmap_pdu_iovec(cmd);
> - kfree(cmd->iov);
> - sgl_free(cmd->req.sg);
> + nvmet_tcp_free_iovec(cmd);
> }
>
> static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
>