| Message ID | 20231121221023.419901-4-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it | expand |
On 21/11/2023 22:10, 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 *may* be needed? I don't undertstand why this patch is necessary and the commit comment isn't really helping me. > 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. > > Please note, that XenStore inherits permissions, so even if entry is > newly created by, it already has the owner set to match owner of entry > at previous level. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
On Wed, 22 Nov 2023, Paul Durrant wrote: > On 21/11/2023 22:10, 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 > > *may* be needed? > > I don't undertstand why this patch is necessary and the commit comment isn't > really helping me. > > > domain that is Domain-0, e.g. in driver domain. I think Volodymyr meant: domain that is *not* Domain-0 So I am guessing this patch is needed otherwise QEMU will run into permissions errors doing xenstore operations > > "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. > > > > Please note, that XenStore inherits permissions, so even if entry is > > newly created by, it already has the owner set to match owner of entry > > at previous level. > > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >
On Tue, 2023-11-21 at 22:10 +0000, Volodymyr Babchuk wrote: > > --- 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, t, path, &num); > + if (tmp == NULL) { > + return false; > + } > + perms_list[0].id = tmp[0].id; > + free(tmp); > + } > + > return xs_set_permissions(h->xsh, t, path, perms_list, > ARRAY_SIZE(perms_list)); > } If the existing transaction is XBT_NULL I think you want to create a new transaction for it, don't you?
Hi David, David Woodhouse <dwmw2@infradead.org> writes: > [[S/MIME Signed Part:Undecided]] > On Tue, 2023-11-21 at 22:10 +0000, Volodymyr Babchuk wrote: >> >> --- 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, t, path, &num); >> + if (tmp == NULL) { >> + return false; >> + } >> + perms_list[0].id = tmp[0].id; >> + free(tmp); >> + } >> + >> return xs_set_permissions(h->xsh, t, path, perms_list, >> ARRAY_SIZE(perms_list)); >> } > > If the existing transaction is XBT_NULL I think you want to create a > new transaction for it, don't you? As per Stefano's and Paul's comments I'll drop this patch completely. Thanks for review, thought.
Hi David, David Woodhouse <dwmw2@infradead.org> writes: > [[S/MIME Signed Part:Undecided]] > On Tue, 2023-11-21 at 22:10 +0000, Volodymyr Babchuk wrote: >> >> --- 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, t, path, &num); >> + if (tmp == NULL) { >> + return false; >> + } >> + perms_list[0].id = tmp[0].id; >> + free(tmp); >> + } >> + >> return xs_set_permissions(h->xsh, t, path, perms_list, >> ARRAY_SIZE(perms_list)); >> } > > If the existing transaction is XBT_NULL I think you want to create a > new transaction for it, don't you? I must say that your comment is valid even without my changes. xenstore_mkdir() calls qemu_xen_xs_create, providing just plain "0" (not even XBT_NULL) as a transaction, but actual xenstore interface implementation, like xs_be_create(), issue multiple calls to lower layer, passing "t" downwards. For example, xs_be_create() calls xs_impl_read, xs_impl_write and xs_impl_set_perms(). If called from xesntore_mkdir(), those three operations will be non-atomic. I don't know if this can lead to a problem, because apparently it was so for a long time...
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c index 6e651960b3..d0fd5d4681 100644 --- a/hw/i386/kvm/xen_xenstore.c +++ b/hw/i386/kvm/xen_xenstore.c @@ -1595,6 +1595,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t, return false; } + if (owner == XS_PRESERVE_OWNER) { + GList *prev_perms; + char letter; + + err = xs_impl_get_perms(h->impl, 0, t, path, &prev_perms); + if (err) { + errno = err; + return false; + } + + if (sscanf(prev_perms->data, "%c%u", &letter, &owner) != 2) { + errno = EFAULT; + g_list_free_full(prev_perms, g_free); + return false; + } + g_list_free_full(prev_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..ae8265635f 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, t, path, &num); + if (tmp == NULL) { + return false; + } + perms_list[0].id = tmp[0].id; + free(tmp); + } + 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..79021538a3 100644 --- a/include/hw/xen/xen_backend_ops.h +++ b/include/hw/xen/xen_backend_ops.h @@ -266,6 +266,13 @@ typedef uint32_t xs_transaction_t; #define XS_PERM_READ 0x01 #define XS_PERM_WRITE 0x02 +/* + * This is QEMU-specific special value used only by QEMU wrappers + * around XenStore. It can be passed to qemu_xen_xs_create() to + * inherit owner value from higher-level XS entry. + */ +#define XS_PRESERVE_OWNER 0xFFFE + struct xenstore_backend_ops { struct qemu_xs_handle *(*open)(void); void (*close)(struct qemu_xs_handle *h);
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. Please note, that XenStore inherits permissions, so even if entry is newly created by, it already has the owner set to match owner of entry at previous level. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> -- In v2: - Pass transaction to xs_get_permissions() in libxenstore_create() - Added comment before XS_PRESERVE_OWNER defintion - Extended the commit message --- hw/i386/kvm/xen_xenstore.c | 18 ++++++++++++++++++ hw/xen/xen-operations.c | 12 ++++++++++++ include/hw/xen/xen_backend_ops.h | 7 +++++++ 3 files changed, 37 insertions(+)