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

-----Original Message-----
From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
Sent: Monday, May 24, 2021 16:35
To: Jason Wang <redacted>; Xuan Zhuo
[off-list ref]; Guodeqing (A) [off-list ref]
Cc: davem@davemloft.net; kuba@kernel.org; virtualization@lists.linux-
foundation.org; netdev@vger.kernel.org; mst@redhat.com
Subject: Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem


On 5/24/2021 5:37 AM, Jason Wang wrote:

quoted

在 2021/5/24 上午10:06, Xuan Zhuo 写道:

quoted

On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A)
[off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
Sent: Sunday, May 23, 2021 15:25
To: Guodeqing (A) <redacted>; 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:

quoted

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.

quoted

quoted

quoted

quoted

quoted

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.

    Ok, I see.
    Thanks.

quoted

quoted

quoted

quoted

quoted

Signed-off-by: guodeqing <redacted>
---
   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)

quoted

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