Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[David Woodhouse]

On Tue, 2023-11-28 at 01:20 +0000, Volodymyr Babchuk wrote:
> Hi David,
> 
> Thank you for the review
> 
> David Woodhouse <dwmw2@infradead.org> writes:
> 
> > [[S/MIME Signed Part:Undecided]]
> > On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> > > Xen PV devices in QEMU can be created in two ways: either by QEMU
> > > itself, if they were passed via command line, or by Xen toolstack. In
> > > the latter case, QEMU scans XenStore entries and configures devices
> > > accordingly.
> > > 
> > > In the second case we don't want QEMU to write/delete front-end
> > > entries for two reasons: it might have no access to those entries if
> > > it is running in un-privileged domain and it is just incorrect to
> > > overwrite entries already provided by Xen toolstack, because
> > > toolstack
> > > manages those nodes. For example, it might read backend- or frontend-
> > > state to be sure that they are both disconnected and it is safe to
> > > destroy a domain.
> > > 
> > > This patch checks presence of xendev->backend to check if Xen PV
> > > device was configured by Xen toolstack to decide if it should touch
> > > frontend entries in XenStore. Also, when we need to remove XenStore
> > > entries during device teardown only if they weren't created by Xen
> > > toolstack. If they were created by toolstack, then it is toolstack's
> > > job to do proper clean-up.
> > > 
> > > Suggested-by: Paul Durrant <xadimgnik@gmail.com>
> > > Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
> > > Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > 
> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > ... albeit with a couple of suggestions... 
> > 
> > > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > > index bef8a3a621..b52ddddabf 100644
> > > --- a/hw/char/xen_console.c
> > > +++ b/hw/char/xen_console.c
> > > @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, 
> > > Error **errp)
> > > 
> > >      trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
> > > 
> > > -    if (CHARDEV_IS_PTY(cs)) {
> > > +    if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
> > >          /* Strip the leading 'pty:' */
> > >          xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 
> > > 4);
> > >      }
> > 
> > 
> > It's kind of weird that that one is a frontend node at all; surely it
> > should have been a backend node?
> 
> Yeah, AFAIK, console was the first PV driver, so it is a bit strange.
> As I see, this frontend entry is used by "xl console" tool to find PTY
> device to attach to. So yes, it should be in backend part of the
> xenstore. But I don't believe we can fix this right now.

Agreed.

> > But it is known only to QEMU once it
> > actually opens /dev/ptmx and creates a new pty. It can't be populated
> > by the toolstack in advance.
> > 
> > So shouldn't the toolstack have made it writable by the driver domain?
> 
> Maybe it can lead to a weird situation when user in Dom-0 tries to use
> "xl console" command to attach to a console that is absent in Dom-0,
> because "tty" entry points to PTY in the driver domain.

True, but that possibility exists the moment you have console provided
in a driver domain.

...

> > Perhaps here you should create the "mac" node if it doesn't exist (or
> > is that "if it doesn't match netdev->conf.macaddr"?) and just
> > gracefully accept failure too?
> > 
> 
> I am not sure that I got this right. conf.maccadr can be sent in two
> ways: via xen_net_device_create(), which will fail if toolstack didn't
> provided a MAC address, or via qemu_macaddr_default_if_unset(), which is
> the case for Xen emulation.

The latter happens with hotplug under Xen too, not just Xen emulation.

But the key point you make is that xen_net_device_create() will refuse
to drive a device which the toolstack created, if the "mac" node isn't
present. So creating it for ourselves based on 'if (!xendev->backend)'
as you did in your patch is reasonable enough. Thanks.

smime.p7s
Description: S/MIME cryptographic signature