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) [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

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")

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

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

在 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

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.

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

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


在 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

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

quoted

   be freed which  was not allocated, this may give people a

misunderstanding.

quoted

quoted

   Thanks.

I think it may be enough to add a comment, and the code does not need to

be

quoted

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

ok, I see. Is it necessary to set the vi->ctrl value to null,because the vi->ctrl value is set
to zero in the alloc_etherdev_mq function by kzalloc.

Thanks.

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")

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

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

On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:

在 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.

It is not assumption but standard practice that can be seen as side
effect of "7) Centralized exiting of functions" section of coding-style.rst.

Thanks

在 2021/6/2 下午1:50, Leon Romanovsky 写道:

On Mon, May 24, 2021 at 10:37:14AM +0800, 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.

It is not assumption but standard practice that can be seen as side
effect of "7) Centralized exiting of functions" section of coding-style.rst.

Thanks

I don't see the connection to the centralized exiting.

Something like:

if (foo)
     kfree(foo);

won't break the centralization.

Thanks

On Wed, Jun 02, 2021 at 03:19:46PM +0800, Jason Wang wrote:

在 2021/6/2 下午1:50, Leon Romanovsky 写道:

quoted

On Mon, May 24, 2021 at 10:37:14AM +0800, 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.

It is not assumption but standard practice that can be seen as side
effect of "7) Centralized exiting of functions" section of coding-style.rst.

Thanks

I don't see the connection to the centralized exiting.

Something like:

if (foo)
    kfree(foo);

won't break the centralization.

The key words are "side effect". Once you centralize everything, you
won't want to see "if (foo) kfree(foo)" spaghetti code.

Of course such construction doesn't break anything, but the idea is
to reduce useless code and not add it.

Thanks

Thanks

quoted