| Message ID | 20210621041650.5826-8-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-vDPA multiqueue | expand |
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. >+err_init: > return -1; > } > >-- >2.25.1 > >
在 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. Thanks > >> +err_init: >> return -1; >> } >> >> -- >> 2.25.1 >> >> >
在 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 :) So we are fine here. Thanks > > Thanks > > >> >>> +err_init: >>> return -1; >>> } >>> >>> -- >>> 2.25.1 >>> >>> >>
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`. Thanks, Stefano
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 >
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); +err_init: return -1; }
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(-)