| Message ID | 20231110204207.2927514-4-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/7] xen-block: Do not write frontend nodes | expand |
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);
On 10/11/2023 15:42, 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. > If QEMU is running in a driver domain then it should know whether the domid of the domain it is running in and use that. Yes, it is hardcoded to 0 at the moment but surely a backend domid (which defaults to 0) could be passed on the command line? Paul > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > 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; > + } > + > + if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) { > + 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); > + } > + > 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);
On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse 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, 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. I suppose maybe you could do it in a transaction *if* the transaction_t you're passed in isn't already XBT_NULL? One might argue that the mkdir+set_perms in libxenstore_create() ought to have been within the same transaction *anyway*?
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: >> 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. 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]]
Hi David, David Woodhouse <dwmw2@infradead.org> writes: > [[S/MIME Signed Part:Undecided]] > On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse 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, 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. > > I suppose maybe you could do it in a transaction *if* the transaction_t > you're passed in isn't already XBT_NULL? Yes, I need to pass "t" to xs_get_permissions(), of course. > One might argue that the mkdir+set_perms in libxenstore_create() ought > to have been within the same transaction *anyway*? Yes, all operations should be performed inside one transaction.
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; + } + + if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) { + 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); + } + 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);
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> --- 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(+)