[PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Ani Sinha]

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(-)

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

Re: [PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Bacchus]

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:

Re: [PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Michael S. Tsirkin]

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

Re: [PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Ani Sinha]

> 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
> 

Re: [PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Philippe Mathieu-Daudé]

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

Re: [PATCH] net/vhost-net: do not assert on null pointer return from tap_get_vhost_net()

[Ani Sinha]

> 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.