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

[John Meneghini <jmeneghi@redhat.com>]

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

On 10/21/21 10:56 AM, John Meneghini wrote:
> 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)
>>
>