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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 May 2021 10:37:14 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        "Guodeqing (A)" <geffrey.guo@...wei.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        "mst@...hat.com" <mst@...hat.com>
Subject: Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem


在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@...wei.com> wrote:
>>
>>> -----Original Message-----
>>> From: Max Gurtovoy [mailto:mgurtovoy@...dia.com]
>>> Sent: Sunday, May 23, 2021 15:25
>>> To: Guodeqing (A) <geffrey.guo@...wei.com>; mst@...hat.com
>>> Cc: jasowang@...hat.com; davem@...emloft.net; kuba@...nel.org;
>>> virtualization@...ts.linux-foundation.org; netdev@...r.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


>
>>>> 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")
>>>> Signed-off-by: guodeqing <geffrey.guo@...wei.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;
>>>>    }
>>>>