Xuan Zhuo
2021-May-24 02:06 UTC
[PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
On Mon, 24 May 2021 01:48:53 +0000, Guodeqing (A) <geffrey.guo at huawei.com> wrote:> > > > -----Original Message----- > > From: Max Gurtovoy [mailto:mgurtovoy at nvidia.com] > > Sent: Sunday, May 23, 2021 15:25 > > To: Guodeqing (A) <geffrey.guo at huawei.com>; mst at redhat.com > > Cc: jasowang at redhat.com; davem at davemloft.net; kuba at kernel.org; > > virtualization at lists.linux-foundation.org; netdev at 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.> > > > > > 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 at 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; > > > } > > > >
Jason Wang
2021-May-24 02:37 UTC
[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 at huawei.com> wrote: >> >>> -----Original Message----- >>> From: Max Gurtovoy [mailto:mgurtovoy at nvidia.com] >>> Sent: Sunday, May 23, 2021 15:25 >>> To: Guodeqing (A) <geffrey.guo at huawei.com>; mst at redhat.com >>> Cc: jasowang at redhat.com; davem at davemloft.net; kuba at kernel.org; >>> virtualization at lists.linux-foundation.org; netdev at 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 at 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; >>>> } >>>>
Leon Romanovsky
2021-Jun-02 05:50 UTC
[PATCH] virtio-net: fix the kzalloc/kfree mismatch problem
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 at huawei.com> wrote: > > > > > > > -----Original Message----- > > > > From: Max Gurtovoy [mailto:mgurtovoy at nvidia.com] > > > > Sent: Sunday, May 23, 2021 15:25 > > > > To: Guodeqing (A) <geffrey.guo at huawei.com>; mst at redhat.com > > > > Cc: jasowang at redhat.com; davem at davemloft.net; kuba at kernel.org; > > > > virtualization at lists.linux-foundation.org; netdev at 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