Re: [PATCH 07/18] vhost-vdpa: tweak the error label in vhost_vdpa_add()

[Jason Wang]

On Tue, Jul 6, 2021 at 4:27 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Jul 06, 2021 at 04:10:22PM +0800, Jason Wang wrote:
> >
> >在 2021/7/6 下午4:03, Jason Wang 写道:
> >>
> >>在 2021/6/23 下午11:03, Stefano Garzarella 写道:
> >>>On Mon, Jun 21, 2021 at 12:16:39PM +0800, Jason Wang wrote:
> >>>>Introduce new error label to avoid the unnecessary checking of net
> >>>>pointer.
> >>>>
> >>>>Fixes: 1e0a84ea49b68 ("vhost-vdpa: introduce vhost-vdpa net client")
> >>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>---
> >>>>net/vhost-vdpa.c | 13 ++++++-------
> >>>>1 file changed, 6 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >>>>index 21f09c546f..0da7bc347a 100644
> >>>>--- a/net/vhost-vdpa.c
> >>>>+++ b/net/vhost-vdpa.c
> >>>>@@ -100,19 +100,18 @@ static int vhost_vdpa_add(NetClientState
> >>>>*ncs, void *be)
> >>>>    net = vhost_net_init(&options);
> >>>>    if (!net) {
> >>>>        error_report("failed to init vhost_net for queue");
> >>>>-        goto err;
> >>>>+        goto err_init;
> >>>>    }
> >>>>    s->vhost_net = net;
> >>>>    ret = vhost_vdpa_net_check_device_id(net);
> >>>>    if (ret) {
> >>>>-        goto err;
> >>>>+        goto err_check;
> >>>>    }
> >>>>    return 0;
> >>>>-err:
> >>>>-    if (net) {
> >>>>-        vhost_net_cleanup(net);
> >>>>-        g_free(net);
> >>>>-    }
> >>>>+err_check:
> >>>>+    vhost_net_cleanup(net);
> >>>>+    g_free(net);
> >>>
> >>>Should we set s->vhost_net to NULL to avoid use after free?
> >>>
> >>>Then we should also remove the `assert(s->vhost_net)` in
> >>>net_vhost_vdpa_init() since we can fail.
> >>
> >>
> >>Right, will do this in a separate patch.
> >
> >
> >I just forget the job has been done in the next patch :)
>
> I saw it later too ;-)
>
> >
> >So we are fine here.
>
> Yep for the assert(), but what about setting s->vhost_net to NULL?
> Or just move the s->vhost_net assignment just before the `return 0`.

We can do, I've posted V2. If it has comment, I will do that in V3.
Otherwise I will send a separate patch for this.

Thanks

>
> Thanks,
> Stefano
>