* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
@ 2021-05-24 2:37 ` Jason Wang
0 siblings, 0 replies; 15+ messages in thread
From: Jason Wang @ 2021-05-24 2:37 UTC (permalink / raw)
To: Xuan Zhuo, Guodeqing (A)
Cc: Max Gurtovoy, mst, netdev, virtualization, kuba, davem
在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
>>
>>> -----Original Message-----
>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
>>> Sent: Sunday, May 23, 2021 15:25
>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
>>>> 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@huawei.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;
>>>> }
>>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-05-24 2:37 ` Jason Wang
(?)
@ 2021-05-24 8:34 ` Max Gurtovoy
2021-05-25 1:12 ` Guodeqing (A)
-1 siblings, 1 reply; 15+ messages in thread
From: Max Gurtovoy @ 2021-05-24 8:34 UTC (permalink / raw)
To: Jason Wang, Xuan Zhuo, Guodeqing (A)
Cc: davem, kuba, virtualization, netdev, mst
On 5/24/2021 5:37 AM, Jason Wang wrote:
>
> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
>> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A)
>> <geffrey.guo@huawei.com> wrote:
>>>
>>>> -----Original Message-----
>>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
>>>> Sent: Sunday, May 23, 2021 15:25
>>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
>>>>> 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.
>>
>>>>> 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.
>>>>> Signed-off-by: guodeqing <geffrey.guo@huawei.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;
>>>>> }
>>>>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-05-24 8:34 ` Max Gurtovoy
@ 2021-05-25 1:12 ` Guodeqing (A)
0 siblings, 0 replies; 15+ messages in thread
From: Guodeqing (A) @ 2021-05-25 1:12 UTC (permalink / raw)
To: Max Gurtovoy, Jason Wang, Xuan Zhuo
Cc: davem, kuba, virtualization, netdev, mst
> -----Original Message-----
> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> Sent: Monday, May 24, 2021 16:35
> To: Jason Wang <jasowang@redhat.com>; Xuan Zhuo
> <xuanzhuo@linux.alibaba.com>; Guodeqing (A) <geffrey.guo@huawei.com>
> 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:
> >
> > 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> >> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A)
> >> <geffrey.guo@huawei.com> wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> >>>> Sent: Sunday, May 23, 2021 15:25
> >>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> >>>>> 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.
>
>
> >>
> >>>>> 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.
>
> >>>>> Signed-off-by: guodeqing <geffrey.guo@huawei.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;
> >>>>> }
> >>>>>
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-05-24 2:37 ` Jason Wang
(?)
(?)
@ 2021-05-25 1:11 ` Guodeqing (A)
-1 siblings, 0 replies; 15+ messages in thread
From: Guodeqing (A) @ 2021-05-25 1:11 UTC (permalink / raw)
To: Jason Wang, Xuan Zhuo
Cc: davem, kuba, virtualization, netdev, Max Gurtovoy, mst
> -----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)
> <geffrey.guo@huawei.com>
> Cc: davem@davemloft.net; kuba@kernel.org; virtualization@lists.linux-
> foundation.org; netdev@vger.kernel.org; Max Gurtovoy
> <mgurtovoy@nvidia.com>; mst@redhat.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@huawei.com> wrote:
> >>
> >>> -----Original Message-----
> >>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> >>> Sent: Sunday, May 23, 2021 15:25
> >>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> >>>> 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
>
>
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.
> >
> >>>> 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@huawei.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;
> >>>> }
> >>>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-05-24 2:37 ` Jason Wang
@ 2021-06-02 5:50 ` Leon Romanovsky
-1 siblings, 0 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-06-02 5:50 UTC (permalink / raw)
To: Jason Wang
Cc: Xuan Zhuo, Guodeqing (A),
davem, kuba, virtualization, netdev, Max Gurtovoy, mst
On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
>
> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> > On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> > > > Sent: Sunday, May 23, 2021 15:25
> > > > To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> > > > > 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
@ 2021-06-02 5:50 ` Leon Romanovsky
0 siblings, 0 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-06-02 5:50 UTC (permalink / raw)
To: Jason Wang
Cc: Max Gurtovoy, Guodeqing (A), netdev, mst, virtualization, kuba, davem
On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
>
> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> > On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> > > > Sent: Sunday, May 23, 2021 15:25
> > > > To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> > > > > 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-06-02 5:50 ` Leon Romanovsky
@ 2021-06-02 7:19 ` Jason Wang
-1 siblings, 0 replies; 15+ messages in thread
From: Jason Wang @ 2021-06-02 7:19 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Xuan Zhuo, Guodeqing (A),
davem, kuba, virtualization, netdev, Max Gurtovoy, mst
在 2021/6/2 下午1:50, Leon Romanovsky 写道:
> On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
>> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
>>> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
>>>>> Sent: Sunday, May 23, 2021 15:25
>>>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
>>>>>> 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
@ 2021-06-02 7:19 ` Jason Wang
0 siblings, 0 replies; 15+ messages in thread
From: Jason Wang @ 2021-06-02 7:19 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Max Gurtovoy, Guodeqing (A), netdev, mst, virtualization, kuba, davem
在 2021/6/2 下午1:50, Leon Romanovsky 写道:
> On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
>> 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
>>> On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
>>>>> Sent: Sunday, May 23, 2021 15:25
>>>>> To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
>>>>>> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
2021-06-02 7:19 ` Jason Wang
@ 2021-06-02 12:59 ` Leon Romanovsky
-1 siblings, 0 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-06-02 12:59 UTC (permalink / raw)
To: Jason Wang
Cc: Xuan Zhuo, Guodeqing (A),
davem, kuba, virtualization, netdev, Max Gurtovoy, mst
On Wed, Jun 02, 2021 at 03:19:46PM +0800, Jason Wang wrote:
>
> 在 2021/6/2 下午1:50, Leon Romanovsky 写道:
> > On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
> > > 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> > > > On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> > > > > > Sent: Sunday, May 23, 2021 15:25
> > > > > > To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> > > > > > > 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
>
>
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
@ 2021-06-02 12:59 ` Leon Romanovsky
0 siblings, 0 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-06-02 12:59 UTC (permalink / raw)
To: Jason Wang
Cc: Max Gurtovoy, Guodeqing (A), netdev, mst, virtualization, kuba, davem
On Wed, Jun 02, 2021 at 03:19:46PM +0800, Jason Wang wrote:
>
> 在 2021/6/2 下午1:50, Leon Romanovsky 写道:
> > On Mon, May 24, 2021 at 10:37:14AM +0800, Jason Wang wrote:
> > > 在 2021/5/24 上午10:06, Xuan Zhuo 写道:
> > > > On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo@huawei.com> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Max Gurtovoy [mailto:mgurtovoy@nvidia.com]
> > > > > > Sent: Sunday, May 23, 2021 15:25
> > > > > > To: Guodeqing (A) <geffrey.guo@huawei.com>; 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:
> > > > > > > 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
>
>
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 15+ messages in thread