Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

From: David Woodhouse <dwmw2@infradead.org>
To: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Paul Durrant <paul@xen.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	 Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	"open list:X86 Xen CPUs" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner
Date: Sat, 11 Nov 2023 11:01:45 +0000	[thread overview]
Message-ID: <4481f0fe9eb282333fd967b7ece590ead78ccdba.camel@infradead.org> (raw)
In-Reply-To: <20231110204207.2927514-4-volodymyr_babchuk@epam.com>

[-- Attachment #1: Type: text/plain, Size: 3976 bytes --]

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> Add option to preserve owner when creating an entry in Xen Store. This
> may be needed in cases when Qemu is working as device model in a
> domain that is Domain-0, e.g. in driver domain.
> 
> "owner" parameter for qemu_xen_xs_create() function can have special
> value XS_PRESERVE_OWNER, which will make specific implementation to
> get original owner of an entry and pass it back to
> set_permissions() call.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

I like this, although I'd like it more if XenStore itself offered this
facility rather than making QEMU do it. Can we make it abundantly clear
that XS_PRESERVE_OWNER is a QEMU internal thing?

>  hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>  hw/xen/xen-operations.c          | 12 ++++++++++++
>  include/hw/xen/xen_backend_ops.h |  2 ++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..7b894a9884 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        GList *perms;
> +        char letter;
> +
> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
> +        if (err) {
> +            errno = err;
> +            return false;
> +        }

I guess we get away without a race here because it's all internal and
we're holding the QEMU iothread mutex? Perhaps assert that?

> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {

I'd be slightly happier if you used parse_perm() from xenstore_impl.c,
but it's static so I suppose that's fair enough.

> +            errno = EFAULT;
> +            g_list_free_full(perms, g_free);
> +            return false;
> +        }
> +        g_list_free_full(perms, g_free);
> +    }
> +
>      perms_list = g_list_append(perms_list,
>                                 xs_perm_as_string(XS_PERM_NONE, owner));
>      perms_list = g_list_append(perms_list,
> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
> index e00983ec44..1df59b3c08 100644
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        struct xs_permissions *tmp;
> +        unsigned int num;
> +
> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> +        if (tmp == NULL) {
> +            return false;
> +        }
> +        perms_list[0].id = tmp[0].id;
> +        free(tmp);
> +    }
> +

Don't see what saves you from someone else changing it at this point on
true Xen though. Which is why I'd prefer XenStore to do it natively.

>      return xs_set_permissions(h->xsh, t, path, perms_list,
>                                ARRAY_SIZE(perms_list));
>  }
> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
> index 90cca85f52..273e414559 100644
> --- a/include/hw/xen/xen_backend_ops.h
> +++ b/include/hw/xen/xen_backend_ops.h
> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>  #define XS_PERM_READ  0x01
>  #define XS_PERM_WRITE 0x02
>  
> +#define XS_PRESERVE_OWNER        0xFFFE
> +
>  struct xenstore_backend_ops {
>      struct qemu_xs_handle *(*open)(void);
>      void (*close)(struct qemu_xs_handle *h);

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]