Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 07.07.23 10:04, Juergen Gross wrote:

Hello Juergen


> Re-reading the whole thread again ...
> 
> On 29.06.23 03:00, Stefano Stabellini wrote:
>> On Wed, 21 Jun 2023, Oleksandr Tyshchenko wrote:
>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>
>>>
>>> Hello Petr
>>>
>>>
>>>> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
>>>> devices (all x86_64), dom0 tries to establish a grant for itself which
>>>> eventually results in a hang during the boot.
>>>>
>>>> The backtrace looks as follows, the while loop in __send_control_msg()
>>>> makes no progress:
>>>>
>>>>     #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0xffff8880074a8400, 
>>>> len=len@entry=0xffffc90000413c94, ctx=ctx@entry=0x0 
>>>> <fixed_percpu_data>) at ../drivers/virtio/virtio_ring.c:2326
>>>>     #1  0xffffffff817086b7 in virtqueue_get_buf 
>>>> (_vq=_vq@entry=0xffff8880074a8400, len=len@entry=0xffffc90000413c94) 
>>>> at ../drivers/virtio/virtio_ring.c:2333
>>>>     #2  0xffffffff8175f6b2 in __send_control_msg (portdev=<optimized 
>>>> out>, port_id=0xffffffff, event=0x0, value=0x1) at 
>>>> ../drivers/char/virtio_console.c:562
>>>>     #3  0xffffffff8175f6ee in __send_control_msg (portdev=<optimized 
>>>> out>, port_id=<optimized out>, event=<optimized out>, 
>>>> value=<optimized out>) at ../drivers/char/virtio_console.c:569
>>>>     #4  0xffffffff817618b1 in virtcons_probe 
>>>> (vdev=0xffff88800585e800) at ../drivers/char/virtio_console.c:2098
>>>>     #5  0xffffffff81707117 in virtio_dev_probe 
>>>> (_d=0xffff88800585e810) at ../drivers/virtio/virtio.c:305
>>>>     #6  0xffffffff8198e348 in call_driver_probe 
>>>> (drv=0xffffffff82be40c0 <virtio_console>, drv=0xffffffff82be40c0 
>>>> <virtio_console>, dev=0xffff88800585e810) at ../drivers/base/dd.c:579
>>>>     #7  really_probe (dev=dev@entry=0xffff88800585e810, 
>>>> drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/dd.c:658
>>>>     #8  0xffffffff8198e58f in __driver_probe_device 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, 
>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>     #9  0xffffffff8198e65a in driver_probe_device 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>, 
>>>> dev=dev@entry=0xffff88800585e810) at ../drivers/base/dd.c:830
>>>>     #10 0xffffffff8198e832 in __driver_attach 
>>>> (dev=0xffff88800585e810, data=0xffffffff82be40c0 <virtio_console>) 
>>>> at ../drivers/base/dd.c:1216
>>>>     #11 0xffffffff8198bfb2 in bus_for_each_dev (bus=<optimized out>, 
>>>> start=start@entry=0x0 <fixed_percpu_data>, 
>>>> data=data@entry=0xffffffff82be40c0 <virtio_console>,
>>>>         fn=fn@entry=0xffffffff8198e7b0 <__driver_attach>) at 
>>>> ../drivers/base/bus.c:368
>>>>     #12 0xffffffff8198db65 in driver_attach 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/dd.c:1233
>>>>     #13 0xffffffff8198d207 in bus_add_driver 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/bus.c:673
>>>>     #14 0xffffffff8198f550 in driver_register 
>>>> (drv=drv@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/base/driver.c:246
>>>>     #15 0xffffffff81706b47 in register_virtio_driver 
>>>> (driver=driver@entry=0xffffffff82be40c0 <virtio_console>) at 
>>>> ../drivers/virtio/virtio.c:357
>>>>     #16 0xffffffff832cd34b in virtio_console_init () at 
>>>> ../drivers/char/virtio_console.c:2258
>>>>     #17 0xffffffff8100105c in do_one_initcall (fn=0xffffffff832cd2e0 
>>>> <virtio_console_init>) at ../init/main.c:1246
>>>>     #18 0xffffffff83277293 in do_initcall_level 
>>>> (command_line=0xffff888003e2f900 "root", level=0x6) at 
>>>> ../init/main.c:1319
>>>>     #19 do_initcalls () at ../init/main.c:1335
>>>>     #20 do_basic_setup () at ../init/main.c:1354
>>>>     #21 kernel_init_freeable () at ../init/main.c:1571
>>>>     #22 0xffffffff81f64be1 in kernel_init (unused=<optimized out>) 
>>>> at ../init/main.c:1462
>>>>     #23 0xffffffff81001f49 in ret_from_fork () at 
>>>> ../arch/x86/entry/entry_64.S:308
>>>>     #24 0x0000000000000000 in ?? ()
>>>>
>>>> Fix the problem by preventing xen_grant_init_backend_domid() from
>>>> setting dom0 as a backend when running in dom0.
>>>>
>>>> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of 
>>>> "xen-grant-dma" devices")
>>>
>>>
>>> I am not 100% sure whether the Fixes tag points to precise commit. If I
>>> am not mistaken, the said commit just moves the code in the context
>>> without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was
>>> introduced before.
>>>
>>>
>>>> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
>>>> ---
>>>>    drivers/xen/grant-dma-ops.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>> index 76f6f26265a3..29ed27ac450e 100644
>>>> --- a/drivers/xen/grant-dma-ops.c
>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct 
>>>> device *dev,
>>>>        if (np) {
>>>>            ret = xen_dt_grant_init_backend_domid(dev, np, 
>>>> backend_domid);
>>>>            of_node_put(np);
>>>> -    } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || 
>>>> xen_pv_domain()) {
>>>> +    } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
>>>> +            xen_pv_domain()) &&
>>>> +           !xen_initial_domain()) {
>>>
>>> The commit lgtm, just one note:
>>>
>>>
>>> I would even bail out early in xen_virtio_restricted_mem_acc() instead,
>>> as I assume the same issue could happen on Arm with DT (although there
>>> we don't guess the backend's domid, we read it from DT and quite
>>> unlikely we get Dom0 being in Dom0 with correct DT).
>>>
>>> Something like:
>>>
>>> @@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct
>>> virtio_device *dev)
>>>    {
>>>           domid_t backend_domid;
>>>
>>> +       /* Xen grant DMA ops are not used when running as initial 
>>> domain */
>>> +       if (xen_initial_domain())
>>> +               return false;
>>> +
>>>           if (!xen_grant_init_backend_domid(dev->dev.parent,
>>> &backend_domid)) {
>>>                   xen_grant_setup_dma_ops(dev->dev.parent, 
>>> backend_domid);
>>>                   return true;
>>> (END)
>>>
>>>
>>>
>>> If so, that commit subject would need to be updated accordingly.
>>>
>>> Let's see what other reviewers will say.
>>
>> This doesn't work in all cases. Imagine using PCI Passthrough to assign
>> a "physical" virtio device to a domU. The domU will run into the same
>> error, right?
>>
>> The problem is that we need a way for the virtio backend to advertise
>> its ability of handling grants. Right now we only have a way to do with
>> that with device tree on ARM. On x86, we only have
>> CONFIG_XEN_VIRTIO_FORCE_GRANT, and if we take
>> CONFIG_XEN_VIRTIO_FORCE_GRANT at face value, it also enables grants for
>> "physical" virtio devices. Note that in this case we are fixing a
>> nested-virtualization bug, but there are actually physical
>> virtio-compatible devices out there. CONFIG_XEN_VIRTIO_FORCE_GRANT will
>> break those too.
> 
> In case you want virtio device passthrough, you shouldn't use a kernel
> built with CONFIG_XEN_VIRTIO_FORCE_GRANT.
> 
> And supporting passing through virtio devices of the host to pv-domUs is
> a security risk anyway.
> 
> We _could_ drop the requirement of the backend needing to set
> VIRTIO_F_ACCESS_PLATFORM for PV guests and allow grant-less virtio
> handling for all guests. For this to work xen_virtio_restricted_mem_acc()
> would need to check for VIRTIO_F_ACCESS_PLATFORM and return true if set.
> Maybe we'd want to enable that possibility via a boot parameter?


Maybe, yes. I don't see at the moment why this won't work.

At the same time I wonder, could we just modify xen_pv_init_platform() 
to call virtio_no_restricted_mem_acc() if forcibly disabled by boot 
parameter irrespective of VIRTIO_F_ACCESS_PLATFORM presence?


> 
> 
> Juergen