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

[Jason Wang <jasowang@xxxxxxxxxx>]

在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@xxxxxxxxxx> wrote:
> > > -----Original Message-----
> > > From: Max Gurtovoy [mailto:mgurtovoy@xxxxxxxxxx]
> > > Sent: Sunday, May 23, 2021 15:25
> > > To: Guodeqing (A) <geffrey.guo@xxxxxxxxxx>; mst@xxxxxxxxxx
> > > Cc: jasowang@xxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> > > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxx>
> > > > ---
> > > >    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;
> > > >    }

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization