| Message ID | 20230628112804.36676-1-anisinha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net/vhost-net: do not assert on null pointer return from tap_get_vhost_net() | expand |
On 6/28/23 13:28, Ani Sinha wrote: > When 'vhost=off' or no vhost specific options at all are passed for the tap > net-device backend, tap_get_vhost_net() can return NULL. The function > net_init_tap_one() does not call vhost_net_init() on such cases and therefore > vhost_net pointer within the tap device state structure remains NULL. Hence, > assertion here on a NULL pointer return from tap_get_vhost_net() would not be > correct. Remove it and fix the crash generated by qemu upon initialization in > the following call chain : > > qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> > virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() > > fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") > Signed-off-by: Ani Sinha <anisinha@redhat.com> Reviewed-by: Cédric Le Goater <clg@redhat.com> Tested-by: Cédric Le Goater <clg@redhat.com> Thanks ! C. > --- > hw/net/vhost_net.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 6db23ca323..6b958d6363 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -507,7 +507,12 @@ VHostNetState *get_vhost_net(NetClientState *nc) > switch (nc->info->type) { > case NET_CLIENT_DRIVER_TAP: > vhost_net = tap_get_vhost_net(nc); > - assert(vhost_net); > + /* > + * tap_get_vhost_net() can return NULL if a tap net-device backend is > + * created with 'vhost=off' option, 'vhostforce=off' or no vhost or > + * vhostforce or vhostfd options at all. Please see net_init_tap_one(). > + * Hence, we omit the assertion here. > + */ > break; > #ifdef CONFIG_VHOST_NET_USER > case NET_CLIENT_DRIVER_VHOST_USER:
On Wed, Jun 28, 2023 at 04:58:04PM +0530, Ani Sinha wrote: > When 'vhost=off' or no vhost specific options at all are passed for the tap > net-device backend, tap_get_vhost_net() can return NULL. The function > net_init_tap_one() does not call vhost_net_init() on such cases and therefore > vhost_net pointer within the tap device state structure remains NULL. Hence, > assertion here on a NULL pointer return from tap_get_vhost_net() would not be > correct. Remove it and fix the crash generated by qemu upon initialization in > the following call chain : > > qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> > virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() > > fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") > Signed-off-by: Ani Sinha <anisinha@redhat.com> I added a bunch of tags and sent it upstream. Take a look at the pull request so you can do it yourself going forward, pls. > --- > hw/net/vhost_net.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 6db23ca323..6b958d6363 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -507,7 +507,12 @@ VHostNetState *get_vhost_net(NetClientState *nc) > switch (nc->info->type) { > case NET_CLIENT_DRIVER_TAP: > vhost_net = tap_get_vhost_net(nc); > - assert(vhost_net); > + /* > + * tap_get_vhost_net() can return NULL if a tap net-device backend is > + * created with 'vhost=off' option, 'vhostforce=off' or no vhost or > + * vhostforce or vhostfd options at all. Please see net_init_tap_one(). > + * Hence, we omit the assertion here. > + */ > break; > #ifdef CONFIG_VHOST_NET_USER > case NET_CLIENT_DRIVER_VHOST_USER: > -- > 2.39.1
> On 28-Jun-2023, at 5:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 28, 2023 at 04:58:04PM +0530, Ani Sinha wrote: >> When 'vhost=off' or no vhost specific options at all are passed for the tap >> net-device backend, tap_get_vhost_net() can return NULL. The function >> net_init_tap_one() does not call vhost_net_init() on such cases and therefore >> vhost_net pointer within the tap device state structure remains NULL. Hence, >> assertion here on a NULL pointer return from tap_get_vhost_net() would not be >> correct. Remove it and fix the crash generated by qemu upon initialization in >> the following call chain : >> >> qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> >> virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() >> >> fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") >> Signed-off-by: Ani Sinha <anisinha@redhat.com> > > I added a bunch of tags and sent it upstream. Take a look > at the pull request so you can do it yourself going > forward, pls. I thought only maintainers sends PR? Do you have any handy scripts? > >> --- >> hw/net/vhost_net.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >> index 6db23ca323..6b958d6363 100644 >> --- a/hw/net/vhost_net.c >> +++ b/hw/net/vhost_net.c >> @@ -507,7 +507,12 @@ VHostNetState *get_vhost_net(NetClientState *nc) >> switch (nc->info->type) { >> case NET_CLIENT_DRIVER_TAP: >> vhost_net = tap_get_vhost_net(nc); >> - assert(vhost_net); >> + /* >> + * tap_get_vhost_net() can return NULL if a tap net-device backend is >> + * created with 'vhost=off' option, 'vhostforce=off' or no vhost or >> + * vhostforce or vhostfd options at all. Please see net_init_tap_one(). >> + * Hence, we omit the assertion here. >> + */ >> break; >> #ifdef CONFIG_VHOST_NET_USER >> case NET_CLIENT_DRIVER_VHOST_USER: >> -- >> 2.39.1 >
On 28/6/23 13:44, Ani Sinha wrote: > > >> On 28-Jun-2023, at 5:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Wed, Jun 28, 2023 at 04:58:04PM +0530, Ani Sinha wrote: >>> When 'vhost=off' or no vhost specific options at all are passed for the tap >>> net-device backend, tap_get_vhost_net() can return NULL. The function >>> net_init_tap_one() does not call vhost_net_init() on such cases and therefore >>> vhost_net pointer within the tap device state structure remains NULL. Hence, >>> assertion here on a NULL pointer return from tap_get_vhost_net() would not be >>> correct. Remove it and fix the crash generated by qemu upon initialization in >>> the following call chain : >>> >>> qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> >>> virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() >>> >>> fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") >>> Signed-off-by: Ani Sinha <anisinha@redhat.com> >> >> I added a bunch of tags and sent it upstream. Take a look >> at the pull request so you can do it yourself going >> forward, pls. > > I thought only maintainers sends PR? Do you have any handy scripts? https://github.com/stefanha/git-publish/blob/master/git-publish.pod#sending-pull-requests
> On 28-Jun-2023, at 5:20 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 28/6/23 13:44, Ani Sinha wrote: >>> On 28-Jun-2023, at 5:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Wed, Jun 28, 2023 at 04:58:04PM +0530, Ani Sinha wrote: >>>> When 'vhost=off' or no vhost specific options at all are passed for the tap >>>> net-device backend, tap_get_vhost_net() can return NULL. The function >>>> net_init_tap_one() does not call vhost_net_init() on such cases and therefore >>>> vhost_net pointer within the tap device state structure remains NULL. Hence, >>>> assertion here on a NULL pointer return from tap_get_vhost_net() would not be >>>> correct. Remove it and fix the crash generated by qemu upon initialization in >>>> the following call chain : >>>> >>>> qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> >>>> virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() >>>> >>>> fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") >>>> Signed-off-by: Ani Sinha <anisinha@redhat.com> >>> >>> I added a bunch of tags and sent it upstream. Take a look >>> at the pull request so you can do it yourself going >>> forward, pls. >> I thought only maintainers sends PR? Do you have any handy scripts? > > https://github.com/stefanha/git-publish/blob/master/git-publish.pod#sending-pull-requests Cool! Thanks Phil! Will check this and talk to Stefan.
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 6db23ca323..6b958d6363 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -507,7 +507,12 @@ VHostNetState *get_vhost_net(NetClientState *nc) switch (nc->info->type) { case NET_CLIENT_DRIVER_TAP: vhost_net = tap_get_vhost_net(nc); - assert(vhost_net); + /* + * tap_get_vhost_net() can return NULL if a tap net-device backend is + * created with 'vhost=off' option, 'vhostforce=off' or no vhost or + * vhostforce or vhostfd options at all. Please see net_init_tap_one(). + * Hence, we omit the assertion here. + */ break; #ifdef CONFIG_VHOST_NET_USER case NET_CLIENT_DRIVER_VHOST_USER:
When 'vhost=off' or no vhost specific options at all are passed for the tap net-device backend, tap_get_vhost_net() can return NULL. The function net_init_tap_one() does not call vhost_net_init() on such cases and therefore vhost_net pointer within the tap device state structure remains NULL. Hence, assertion here on a NULL pointer return from tap_get_vhost_net() would not be correct. Remove it and fix the crash generated by qemu upon initialization in the following call chain : qdev_realize() -> pci_qdev_realize() -> virtio_device_realize() -> virtio_bus_device_plugged() -> virtio_net_get_features() -> get_vhost_net() fixes: 0e994668d00c9c ("vhost_net: add an assertion for TAP client backends") Signed-off-by: Ani Sinha <anisinha@redhat.com> --- hw/net/vhost_net.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)