On 09.12.24 18:04, Jan Beulich wrote:
On 06.12.2024 14:02, Juergen Gross wrote:--- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -192,6 +192,54 @@ static void domain_changed_state(const struct domain *d) spin_unlock(&dom_state_changed_lock); }+static void set_domain_state_info(struct xen_domctl_get_domain_state *info,+ const struct domain *d) +{ + info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST; + if ( d->is_shut_down ) + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN; + if ( d->is_dying == DOMDYING_dead ) + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;The public constant saying "dying" isn't quite in line with the internal constant saying "dead". It may well be that Xenstore only cares about the "dead" state, but then it would better be nemaed this way also in the public interface, I think.
Okay, I'll rename it to "XEN_DOMCTL_GETDOMSTATE_STATE_DEAD".
+ info->unique_id = d->unique_id; +} + +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, + domid_t *domid) +{ + unsigned int dom; + + memset(info, 0, sizeof(*info)); + + if ( d ) + { + set_domain_state_info(info, d); + + return 0; + } + + while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <I can't spot any check that dom_state_changed was actually allocated. Also, unlike the public header comments says, the new sub-op looks to be usable by _anyone_ (eligible privilege-wise) as long as _someone_ did set up the vIRQ. There looks to even be a race possible when a "wrong" caller tries to call this at jut the "right" time.
Right, I'll add a check that the calling domain is registered for the vIRQ. Additionally I'll put in an "ASSERT(dom_state_changed);".
--- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -278,6 +278,11 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo, return ERR_PTR(ret); }+static bool is_stable_domctl(uint32_t cmd)+{ + return cmd == XEN_DOMCTL_get_domain_state; +}Likely better as switch() from the very beginning.@@ -866,6 +873,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) __HYPERVISOR_domctl, "h", u_domctl); break;+ case XEN_DOMCTL_get_domain_state:+ ret = xsm_get_domain_state(XSM_XS_PRIV, d); + if ( ret ) + break; + + copyback = 1; + ret = get_domain_state(&op->u.get_domain_state, d, &op->domain); + break;Especially with this being a stable interface, surely the two padding fields want checking to be zero on input (to possibly allow their future use for something input-ish). Then even the memset() in the function may not really be needed.
I'll add the check. Removing the memset() is a little bit doubtful, as this might result in leaking hypervisor data e.g. in case a domain isn't existing (this will copy the internal struct to the user even in the -ENOENT case). Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
