Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 5/24/2021 5:37 AM, Jason Wang wrote:
>
> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
>> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) 
>> <geffrey.guo@huawei.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
>>>> Sent: Sunday, May 23, 2021 15:25
>>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; mst@redhat.com
>>>> Cc: jasowang@redhat.com; davem@davemloft.net; kuba@kernel.org;
>>>> virtualization@lists.linux-foundation.org; netdev@vger.kernel.org
>>>> Subject: Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch 
>>>> problem
>>>>
>>>>
>>>> On 5/22/2021 11:02 AM, guodeqing wrote:
>>>>> If the virtio_net device does not suppurt the ctrl queue feature, the
>>>>> vi->ctrl was not allocated, so there is no need to free it.
>>>> you don't need this check.
>>>>
>>>> from kfree doc:
>>>>
>>>> "If @objp is NULL, no operation is performed."
>>>>
>>>> This is not a bug. I've set vi->ctrl to be NULL in case !vi->has_cvq.
>>>>
>>>>
>>>    yes,  this is not a bug, the patch is just a optimization, 
>>> because the vi->ctrl maybe
>>>    be freed which  was not allocated, this may give people a 
>>> misunderstanding.
>>>    Thanks.
>>
>> I think it may be enough to add a comment, and the code does not need 
>> to be
>> modified.
>>
>> Thanks.
>
>
> Or even just leave the current code as is. A lot of kernel codes was 
> wrote under the assumption that kfree() should deal with NULL.
>
> Thanks
>
>
exactly.


>>
>>>>> Here I adjust the initialization sequence and the check of 
>>>>> vi->has_cvq
>>>>> to slove this problem.
>>>>>
>>>>> Fixes:     122b84a1267a ("virtio-net: don't allocate control_buf 
>>>>> if not
>>>> supported")


"Fixes" line should be added only if you fix some bug.


>>>>> Signed-off-by: guodeqing <geffrey.guo@huawei.com>
>>>>> ---
>>>>>    drivers/net/virtio_net.c | 20 ++++++++++----------
>>>>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c 
>>>>> index
>>>>> 9b6a4a875c55..894f894d3a29 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -2691,7 +2691,8 @@ static void virtnet_free_queues(struct
>>>>> virtnet_info *vi)
>>>>>
>>>>>        kfree(vi->rq);
>>>>>        kfree(vi->sq);
>>>>> -    kfree(vi->ctrl);
>>>>> +    if (vi->has_cvq)
>>>>> +        kfree(vi->ctrl);
>>>>>    }
>>>>>
>>>>>    static void _free_receive_bufs(struct virtnet_info *vi) @@ 
>>>>> -2870,13
>>>>> +2871,6 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
>>>>>    {
>>>>>        int i;
>>>>>
>>>>> -    if (vi->has_cvq) {
>>>>> -        vi->ctrl = kzalloc(sizeof(*vi->ctrl), GFP_KERNEL);
>>>>> -        if (!vi->ctrl)
>>>>> -            goto err_ctrl;
>>>>> -    } else {
>>>>> -        vi->ctrl = NULL;
>>>>> -    }
>>>>>        vi->sq = kcalloc(vi->max_queue_pairs, sizeof(*vi->sq), 
>>>>> GFP_KERNEL);
>>>>>        if (!vi->sq)
>>>>>            goto err_sq;
>>>>> @@ -2884,6 +2878,12 @@ static int virtnet_alloc_queues(struct
>>>> virtnet_info *vi)
>>>>>        if (!vi->rq)
>>>>>            goto err_rq;
>>>>>
>>>>> +    if (vi->has_cvq) {
>>>>> +        vi->ctrl = kzalloc(sizeof(*vi->ctrl), GFP_KERNEL);
>>>>> +        if (!vi->ctrl)
>>>>> +            goto err_ctrl;
>>>>> +    }
>>>>> +
>>>>>        INIT_DELAYED_WORK(&vi->refill, refill_work);
>>>>>        for (i = 0; i < vi->max_queue_pairs; i++) {
>>>>>            vi->rq[i].pages = NULL;
>>>>> @@ -2902,11 +2902,11 @@ static int virtnet_alloc_queues(struct
>>>>> virtnet_info *vi)
>>>>>
>>>>>        return 0;
>>>>>
>>>>> +err_ctrl:
>>>>> +    kfree(vi->rq);
>>>>>    err_rq:
>>>>>        kfree(vi->sq);
>>>>>    err_sq:
>>>>> -    kfree(vi->ctrl);
>>>>> -err_ctrl:
>>>>>        return -ENOMEM;
>>>>>    }
>>>>>
>