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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi David,

Thank you for the review

David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Suggested-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
>> Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
>
> ... 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.

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

> I think we should attempt to write this and just gracefully handle the
> failure if we can't. (In fact, xen_device_frontend_printf() will just
> use error_report_err() which is probably OK unless you feel strongly
> about silencing it).

Nope, I am fine with this approach. I'll remove this hunk in the next
version.

>
>> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
>> index afa10c96e8..27442bef38 100644
>> --- a/hw/net/xen_nic.c
>> +++ b/hw/net/xen_nic.c
>> @@ -315,14 +315,16 @@ static void xen_netdev_realize(XenDevice *xendev, 
>> Error **errp)
>> 
>>      qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
>> 
>> -    xen_device_frontend_printf(xendev, "mac", 
>> "%02x:%02x:%02x:%02x:%02x:%02x",
>> -                               netdev->conf.macaddr.a[0],
>> -                               netdev->conf.macaddr.a[1],
>> -                               netdev->conf.macaddr.a[2],
>> -                               netdev->conf.macaddr.a[3],
>> -                               netdev->conf.macaddr.a[4],
>> -                               netdev->conf.macaddr.a[5]);
>> -
>> +    if (!xendev->backend) {
>> +        xen_device_frontend_printf(xendev, "mac",
>> +                                   "%02x:%02x:%02x:%02x:%02x:%02x",
>> +                                   netdev->conf.macaddr.a[0],
>> +                                   netdev->conf.macaddr.a[1],
>> +                                   netdev->conf.macaddr.a[2],
>> +                                   netdev->conf.macaddr.a[3],
>> +                                   netdev->conf.macaddr.a[4],
>> +                                   netdev->conf.macaddr.a[5]);
>> +    }
>>      netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
>>                                 object_get_typename(OBJECT(xendev)),
>>                                 DEVICE(xendev)->id, netdev);
>
>
> 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.

Xen toolstack is written in a such way, that it always provides "mac"
XenStore entry and frontend driver expects that this entry is always
present. So, if by any chance toolstack has not provided the MAC, then
there is something wrong and it is better to fail, what
xen_net_device_create() does. I am not sure that it is a good idea to
try to silently fix any toolstack issues.

-- 
WBR, Volodymyr