[v1,1/7] xen-block: Do not write frontend nodes

Commit Message

Volodymyr Babchuk Nov. 10, 2023, 8:42 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

The PV backend running in other than Dom0 domain (non toolstack domain)
is not allowed to write frontend nodes. The more, the backend does not
need to do that at all, this is purely toolstack/xl devd business.

I do not know for what reason the backend does that here, this is not really
needed, probably it is just a leftover and all xen_device_frontend_printf()
instances should go away completely.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
Signed-off-by: Volodymyr Babchuk <[e-mail]>
---
 hw/block/xen-block.c | 11 +++++++----
 hw/xen/xen-bus.c     |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

David Woodhouse Nov. 11, 2023, 10:55 a.m. UTC | #1

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> The PV backend running in other than Dom0 domain (non toolstack domain)
> is not allowed to write frontend nodes. The more, the backend does not
> need to do that at all, this is purely toolstack/xl devd business.
> 
> I do not know for what reason the backend does that here, this is not really
> needed, probably it is just a leftover and all xen_device_frontend_printf()
> instances should go away completely.

No, this is what allows qemu to create PV devices, as opposed to just
handle the ones which are created for it by the toolstack.

Perhaps we should only create the frontend nodes (and likewise, only
destroy those and the backend nodes on destruction) in the case where
the device was instantiated directly by the QEMU command line, and
refrain from doing so for the devices which were created by the
toolstack and merely 'discovered' by xen_block_device_create()?

(Note that we need to look at net and console devices too, now they've
finally been converted to the 'new' XenBus framework in QEMU 8.2.)

Andrew Cooper Nov. 11, 2023, 1:43 p.m. UTC | #2

On 11/11/2023 10:55 am, David Woodhouse wrote:
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> The PV backend running in other than Dom0 domain (non toolstack domain)
>> is not allowed to write frontend nodes. The more, the backend does not
>> need to do that at all, this is purely toolstack/xl devd business.
>>
>> I do not know for what reason the backend does that here, this is not really
>> needed, probably it is just a leftover and all xen_device_frontend_printf()
>> instances should go away completely.
> No, this is what allows qemu to create PV devices, as opposed to just
> handle the ones which are created for it by the toolstack.
>
> Perhaps we should only create the frontend nodes (and likewise, only
> destroy those and the backend nodes on destruction) in the case where
> the device was instantiated directly by the QEMU command line, and
> refrain from doing so for the devices which were created by the
> toolstack and merely 'discovered' by xen_block_device_create()?
>
> (Note that we need to look at net and console devices too, now they've
> finally been converted to the 'new' XenBus framework in QEMU 8.2.)
>
>

Furthermore, the control domain doesn't always have the domid of 0.

If qemu wants/needs to make changes like this, the control domain has to
arrange for qemu's domain to have appropriate permissions on the nodes.

~Andrew

David Woodhouse Nov. 11, 2023, 8:18 p.m. UTC | #3

On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>Furthermore, the control domain doesn't always have the domid of 0.
>
>If qemu wants/needs to make changes like this, the control domain has to
>arrange for qemu's domain to have appropriate permissions on the nodes.

Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack.

The criterion used in this patch series should be "did QEMU create this device, or discover it".

Andrew Cooper Nov. 11, 2023, 9:51 p.m. UTC | #4

On 11/11/2023 8:18 pm, David Woodhouse wrote:
> On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>> Furthermore, the control domain doesn't always have the domid of 0.
>>
>> If qemu wants/needs to make changes like this, the control domain has to
>> arrange for qemu's domain to have appropriate permissions on the nodes.
> Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack.
>
> The criterion used in this patch series should be "did QEMU create this device, or discover it".
>

Yeah, that sounds like the right approach.

~Andrew

David Woodhouse Nov. 11, 2023, 10:22 p.m. UTC | #5

On 11 November 2023 16:51:22 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>On 11/11/2023 8:18 pm, David Woodhouse wrote:
>> On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>>> Furthermore, the control domain doesn't always have the domid of 0.
>>>
>>> If qemu wants/needs to make changes like this, the control domain has to
>>> arrange for qemu's domain to have appropriate permissions on the nodes.
>> Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack.
>>
>> The criterion used in this patch series should be "did QEMU create this device, or discover it".
>>
>
>Yeah, that sounds like the right approach.

I think we want to kill the xen_backend_set_device() function and instead set the backend as a property of the XenDevice *before* realizing it.

Paul Durrant Nov. 12, 2023, 8:29 p.m. UTC | #6

On 10/11/2023 15:42, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> The PV backend running in other than Dom0 domain (non toolstack domain)
> is not allowed to write frontend nodes. The more, the backend does not
> need to do that at all, this is purely toolstack/xl devd business.
> 
> I do not know for what reason the backend does that here, this is not really
> needed, probably it is just a leftover and all xen_device_frontend_printf()
> instances should go away completely.
>

It is not a leftover and it is needed in the case that QEMU is 
instantiating the backend unilaterally... i.e. without the involvement 
of any Xen toolstack.
Agreed that, if QEMU, is running in a deprivileged context, that is not 
an option. The correct way to determined this though is whether the 
device is being created via the QEMU command line or whether is being 
created because XenStore nodes written by a toolstack were discovered.
In the latter case there should be a XenBackendInstance that corresponds 
to the XenDevice whereas in the former case there should not be. For 
example, see xen_backend_try_device_destroy()

   Paul


> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Signed-off-by: Volodymyr Babchuk <[e-mail]>
> ---
>   hw/block/xen-block.c | 11 +++++++----
>   hw/xen/xen-bus.c     |  2 +-
>   2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a07cd7eb5d..dc4d477c22 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -221,6 +221,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
>       XenBlockVdev *vdev = &blockdev->props.vdev;
>       BlockConf *conf = &blockdev->props.conf;
>       BlockBackend *blk = conf->blk;
> +    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>   
>       if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
>           error_setg(errp, "vdev property not set");
> @@ -280,10 +281,12 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
>   
>       xen_device_backend_printf(xendev, "info", "%u", blockdev->info);
>   
> -    xen_device_frontend_printf(xendev, "virtual-device", "%lu",
> -                               vdev->number);
> -    xen_device_frontend_printf(xendev, "device-type", "%s",
> -                               blockdev->device_type);
> +    if (xenbus->backend_id == 0) {
> +        xen_device_frontend_printf(xendev, "virtual-device", "%lu",
> +                                   vdev->number);
> +        xen_device_frontend_printf(xendev, "device-type", "%s",
> +                                   blockdev->device_type);
> +    }
>   
>       xen_device_backend_printf(xendev, "sector-size", "%u",
>                                 conf->logical_block_size);
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index ece8ec40cd..06d5192aca 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -1048,7 +1048,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>       xen_device_backend_set_online(xendev, true);
>       xen_device_backend_set_state(xendev, XenbusStateInitWait);
>   
> -    if (!xen_device_frontend_exists(xendev)) {
> +    if (!xen_device_frontend_exists(xendev) && xenbus->backend_id == 0) {
>           xen_device_frontend_printf(xendev, "backend", "%s",
>                                      xendev->backend_path);
>           xen_device_frontend_printf(xendev, "backend-id", "%u",

Volodymyr Babchuk Nov. 14, 2023, 9:32 p.m. UTC | #7

Hi David,

David Woodhouse <[e-mail]> writes:

> On 11 November 2023 16:51:22 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>>On 11/11/2023 8:18 pm, David Woodhouse wrote:
>>> On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <[e-mail]> wrote:
>>>> Furthermore, the control domain doesn't always have the domid of 0.
>>>>
>>>> If qemu wants/needs to make changes like this, the control domain has to
>>>> arrange for qemu's domain to have appropriate permissions on the nodes.
>>> Right. And that's simple enough: if you are running QEMU in a
>>> domain which doesn't have permission to create the backend
>>> directory and/or the frontend nodes, don't ask it to *create*
>>> devices. In that case it is only able to connect as the backend for
>>> devices which were created *for* it by the toolstack.
>>>
>>> The criterion used in this patch series should be "did QEMU create this device, or discover it".
>>>
>>
>>Yeah, that sounds like the right approach.
>
> I think we want to kill the xen_backend_set_device() function and
> instead set the backend as a property of the XenDevice *before*
> realizing it.

Not sure that I got this. Right now device is property of
XenBackendInstance. Are you proposing to make this other way around?

Right now I am looking for a place where to store the information of
XenDevice creator. My plan was to add "found_in_xenbus" property to
XenDevice and set it in xen_backend_device_create.

David Woodhouse Nov. 14, 2023, 9:54 p.m. UTC | #8

On Tue, 2023-11-14 at 21:32 +0000, Volodymyr Babchuk wrote:
> 
> > I think we want to kill the xen_backend_set_device() function and
> > instead set the backend as a property of the XenDevice *before*
> > realizing it.
> 
> Not sure that I got this. Right now device is property of
> XenBackendInstance. Are you proposing to make this other way around?
> 
> Right now I am looking for a place where to store the information of
> XenDevice creator. My plan was to add "found_in_xenbus" property to
> XenDevice and set it in xen_backend_device_create.

A bit like this?

https://lore.kernel.org/qemu-devel/[e-mail]

Patch

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..dc4d477c22 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -221,6 +221,7 @@  static void xen_block_realize(XenDevice *xendev, Error **errp)
     XenBlockVdev *vdev = &blockdev->props.vdev;
     BlockConf *conf = &blockdev->props.conf;
     BlockBackend *blk = conf->blk;
+    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
 
     if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
         error_setg(errp, "vdev property not set");
@@ -280,10 +281,12 @@  static void xen_block_realize(XenDevice *xendev, Error **errp)
 
     xen_device_backend_printf(xendev, "info", "%u", blockdev->info);
 
-    xen_device_frontend_printf(xendev, "virtual-device", "%lu",
-                               vdev->number);
-    xen_device_frontend_printf(xendev, "device-type", "%s",
-                               blockdev->device_type);
+    if (xenbus->backend_id == 0) {
+        xen_device_frontend_printf(xendev, "virtual-device", "%lu",
+                                   vdev->number);
+        xen_device_frontend_printf(xendev, "device-type", "%s",
+                                   blockdev->device_type);
+    }
 
     xen_device_backend_printf(xendev, "sector-size", "%u",
                               conf->logical_block_size);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..06d5192aca 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1048,7 +1048,7 @@  static void xen_device_realize(DeviceState *dev, Error **errp)
     xen_device_backend_set_online(xendev, true);
     xen_device_backend_set_state(xendev, XenbusStateInitWait);
 
-    if (!xen_device_frontend_exists(xendev)) {
+    if (!xen_device_frontend_exists(xendev) && xenbus->backend_id == 0) {
         xen_device_frontend_printf(xendev, "backend", "%s",
                                    xendev->backend_path);
         xen_device_frontend_printf(xendev, "backend-id", "%u",