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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6f7b729d-38df-9bf8-f023-bc1986da9a9e@nvidia.com>
Date:   Sun, 23 May 2021 10:24:34 +0300
From:   Max Gurtovoy <mgurtovoy@...dia.com>
To:     guodeqing <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.


>
> 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;
>   }
>