Hi David,
David Woodhouse <dw...@infradead.org> writes:
> [[S/MIME Signed Part:Undecided]]
> 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_babc...@epam.com>
>
> I like this, although I'd like it more if XenStore itself offered this
> facility rather than making QEMU do it.
XenStore itself ensures that access rights are inherited. The problem
is with qemu_xen_xs_create() function that does two things at a time:
creates a new entry and then assigns permissions, overwriting any
permissions that existed before.
> Can we make it abundantly clear
> that XS_PRESERVE_OWNER is a QEMU internal thing?
It is defined in xen_backend_ops.h which is internal QEMU interface for
XenStore. Do you have any suggestions how to make it even clearer?
>> 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?
>
I am not quite familiar with QEMU internals, but why we do we need
assert here? xe_be_create() calls xs_impl* function before and after
this part. Is this piece of code special in some way?
>> + 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.
>
Yes, I wanted to use that function, but it is internal for
xenstore_impl.c
I can rename it to xs_impl_parse_perm() and make it public, if you
believe that this is a better approach.
>> + 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.
>
Oh, I missed the transaction parameter. My bad. Will fix in the next
version.
>> 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);
>
> [[End of S/MIME Signed Part]]
--
WBR, Volodymyr
