| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bd194479-3799-58cb-7e4a-8619818cd03a@suse.com>
Date: Sat, 8 Jul 2023 12:54:32 +0200
From: Juergen Gross <jgross@...e.com>
To: Stefano Stabellini <sstabellini@...nel.org>
Cc: Petr Pavlu <petr.pavlu@...e.com>,
Oleksandr Tyshchenko <Oleksandr_Tyshchenko@...m.com>,
"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0
On 07.07.23 23:02, Stefano Stabellini wrote:
> On Fri, 7 Jul 2023, Juergen Gross wrote:
>> On 26.06.23 15:17, Petr Pavlu wrote:
>>> On 6/21/23 19:58, Oleksandr Tyshchenko wrote:
>>>> On 21.06.23 16:12, Petr Pavlu wrote:
>>>>> 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@...ry=0xffff8880074a8400,
>>>>> len=len@...ry=0xffffc90000413c94, ctx=ctx@...ry=0x0 <fixed_percpu_data>)
>>>>> at ../drivers/virtio/virtio_ring.c:2326
>>>>> #1 0xffffffff817086b7 in virtqueue_get_buf
>>>>> (_vq=_vq@...ry=0xffff8880074a8400, len=len@...ry=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@...ry=0xffff88800585e810,
>>>>> drv=drv@...ry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:658
>>>>> #8 0xffffffff8198e58f in __driver_probe_device
>>>>> (drv=drv@...ry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@...ry=0xffff88800585e810) at ../drivers/base/dd.c:800
>>>>> #9 0xffffffff8198e65a in driver_probe_device
>>>>> (drv=drv@...ry=0xffffffff82be40c0 <virtio_console>,
>>>>> dev=dev@...ry=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@...ry=0x0 <fixed_percpu_data>,
>>>>> data=data@...ry=0xffffffff82be40c0 <virtio_console>,
>>>>> fn=fn@...ry=0xffffffff8198e7b0 <__driver_attach>) at
>>>>> ../drivers/base/bus.c:368
>>>>> #12 0xffffffff8198db65 in driver_attach
>>>>> (drv=drv@...ry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/dd.c:1233
>>>>> #13 0xffffffff8198d207 in bus_add_driver
>>>>> (drv=drv@...ry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/bus.c:673
>>>>> #14 0xffffffff8198f550 in driver_register
>>>>> (drv=drv@...ry=0xffffffff82be40c0 <virtio_console>) at
>>>>> ../drivers/base/driver.c:246
>>>>> #15 0xffffffff81706b47 in register_virtio_driver
>>>>> (driver=driver@...ry=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.
>>>
>>> I see, the tag should better point to 7228113d1fa0 ("xen/virtio: use
>>> dom0 as default backend for CONFIG_XEN_VIRTIO_FORCE_GRANT") which
>>> introduced the original logic to use dom0 as backend.
>>>
>>> Commit 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma"
>>> devices") is relevant in sense that it extended when this logic is
>>> active by adding an OR check for xen_pv_domain().
>>>
>>>>
>>>>
>>>>> Signed-off-by: Petr Pavlu <petr.pavlu@...e.com>
>>>>> ---
>>>>> 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.
>>>
>>> Ok, makes sense.
>>
>> I think this is okay for a fix of the current problem.
>>
>> Passing through virtio devices to a PV domU is not covered by this fix, but
>> this
>> should be a rather rare configuration, which doesn't work today either. So the
>> suggested patch would fix the current issue without introducing a regression.
>>
>> Anything else can be done later.
>
> Why do you say that passing through virtio devices to a PV domU doesn't
> work today anyway? Also, as you know many people use Xen outside of
> datacenter deployments (laptops, embedded etc.) where drivers domains
> and device assignment are very common. You could assign a virtio network
> card to a domU and use PV network to share the network with other
> guests. Physical virtio devices, especially virtio-net devices, exist. I
> could probably repro this problem today in a domU just installing
> QubesOS inside QEMU. QubesOS uses network driver domains and if QEMU
> provides a virtio-net network card, this would break even with this
> patch.
I might be wrong, but I don't think all virtio frontends will work in that
scenario. The main reason is the PFN/MFN difference: a frontend using guest
consecutive memory for doing large I/Os will fail miserably. This was the
main reason why I had to add the functionality of consecutive grants for
large I/O buffers. The same goes for multi-page virtio ring pages.
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists