From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
the previous owner of the directory.

Note that for other than Dom0 domain (non toolstack domain) the
"driver_domain" property should be set in domain config file for the
toolstack to create required directories in advance.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/xen/xen_pvdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index c5ad71e8dc..42bdd4f6c8 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -60,7 +60,8 @@ void xen_config_cleanup(void)
 
 int xenstore_mkdir(char *path, int p)
 {
-    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
+    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
+                            xen_domid, p, path)) {
         xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
         return -1;
     }
-- 
2.42.0

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
> the previous owner of the directory.
> 

You're missing the words "... if it already exists" from that sentence.

If the directory *didn't* already exist, it gets created with dom0 as
the owner still, right? Assuming XenStore allows QEMU to do that.

Strictly, the node gets created (if permitted) and *then*
xs_set_permissions() attempts to set dom0 as the owner (if permitted).

> Note that for other than Dom0 domain (non toolstack domain) the
> "driver_domain" property should be set in domain config file for the
> toolstack to create required directories in advance.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  hw/xen/xen_pvdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> index c5ad71e8dc..42bdd4f6c8 100644
> --- a/hw/xen/xen_pvdev.c
> +++ b/hw/xen/xen_pvdev.c
> @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
>  
>  int xenstore_mkdir(char *path, int p)
>  {
> -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
> +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
> +                            xen_domid, p, path)) {
>          xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
>          return -1;
>      }

Why bother with xenstore_mkdir()? AFAICT it's *only* used from the
legacy XenLegacyDevice stuff, and can't we just finish the job of
moving from that to the XenDevice model? I've done console and net
recently; want to keep going?

And even then... the xenstore_mkdir() function is called twice from
xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend
and backend directories — which is what the rest of your patch series
is trying to eliminate because a driver domain doesn't have permissions
to do that anyway.

It's also called from xen_be_register() in hw/xen/xen_devconfig.c to
create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a
relative path, so in the driver domain's XenStore). That one presumably
*won't* exist already, and so XS_PRESERVE_OWNER won't even have any
effect?

What practical difference does this even make? Am I missing something?

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> 
>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
>> the previous owner of the directory.
>> 
>
> You're missing the words "... if it already exists" from that sentence.
>
> If the directory *didn't* already exist, it gets created with dom0 as
> the owner still, right? Assuming XenStore allows QEMU to do that.

If it didn't already exist, it is created and it inherits access rights
from the parent node.

> Strictly, the node gets created (if permitted) and *then*
> xs_set_permissions() attempts to set dom0 as the owner (if permitted).

Yes. I'll rephrase this as "Instead of forcing the owner to domid 0, use
 XS_PRESERVE_OWNER to save the inherited owner of the directory."

will it be okay?

>
>> Note that for other than Dom0 domain (non toolstack domain) the
>> "driver_domain" property should be set in domain config file for the
>> toolstack to create required directories in advance.
>> 
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  hw/xen/xen_pvdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
>> index c5ad71e8dc..42bdd4f6c8 100644
>> --- a/hw/xen/xen_pvdev.c
>> +++ b/hw/xen/xen_pvdev.c
>> @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
>>  
>>  int xenstore_mkdir(char *path, int p)
>>  {
>> -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
>> +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
>> +                            xen_domid, p, path)) {
>>          xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
>>          return -1;
>>      }
>
> Why bother with xenstore_mkdir()? AFAICT it's *only* used from the
> legacy XenLegacyDevice stuff, and can't we just finish the job of
> moving from that to the XenDevice model? I've done console and net
> recently; want to keep going?

Well, I am not so familiar with QEMU to accomplish this in a short
time. If you really need help, I can take alook at 9p driver, it looks
simplest of them all...

>
> And even then... the xenstore_mkdir() function is called twice from
> xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend
> and backend directories — which is what the rest of your patch series
> is trying to eliminate because a driver domain doesn't have permissions
> to do that anyway.
>
> It's also called from xen_be_register() in hw/xen/xen_devconfig.c to
> create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a
> relative path, so in the driver domain's XenStore). That one presumably
> *won't* exist already, and so XS_PRESERVE_OWNER won't even have any
> effect?

As I said, it will inherit driver's domain access rights. So yeah,
Oleksandr's patch covers this case, mostly.

I agree, it would be better to drop xenstore_mkdir() at all, but this is
tangent to my task of adding virtio-pci support for ARM guests. Those
Oleksandr's patches for drive domain, that you are seeing, come to life
only because our system happens to use a separate driver domain.

-- 
WBR, Volodymyr