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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <372caa22030e4d5d90e2bada303d8f77@huawei.com>
Date:   Tue, 25 May 2021 01:12:47 +0000
From:   "Guodeqing (A)" <geffrey.guo@...wei.com>
To:     Max Gurtovoy <mgurtovoy@...dia.com>,
        Jason Wang <jasowang@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "mst@...hat.com" <mst@...hat.com>
Subject: RE: [PATCH] virtio-net: fix the kzalloc/kfree mismatch problem



> -----Original Message-----
> From: Max Gurtovoy [mailto:mgurtovoy@...dia.com]
> Sent: Monday, May 24, 2021 16:35
> To: Jason Wang <jasowang@...hat.com>; Xuan Zhuo
> <xuanzhuo@...ux.alibaba.com>; Guodeqing (A) <geffrey.guo@...wei.com>
> Cc: davem@...emloft.net; kuba@...nel.org; virtualization@...ts.linux-
> foundation.org; netdev@...r.kernel.org; mst@...hat.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@...wei.com> wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Max Gurtovoy [mailto:mgurtovoy@...dia.com]
> >>>> Sent: Sunday, May 23, 2021 15:25
> >>>> To: Guodeqing (A) <geffrey.guo@...wei.com>; mst@...hat.com
> >>>> Cc: jasowang@...hat.com; davem@...emloft.net; kuba@...nel.org;
> >>>> virtualization@...ts.linux-foundation.org; netdev@...r.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@...wei.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;
> >>>>>    }
> >>>>>
> >