Re: [PATCH v2 5/7] xen: add new domctl get_changed_domain

[Jürgen Groß]

On 10.12.24 17:29, Jan Beulich wrote:

On 10.12.2024 16:52, Jürgen Groß wrote:

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);
   }

+                                  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;

+static void set_domain_state_info(struct xen_domctl_get_domain_state *info,

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".

Well, maybe have both DYING and DEAD, even if Xenstore right now needs only one?

Yes, might be interesting in the future.

@@ -866,6 +873,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
                   __HYPERVISOR_domctl, "h", u_domctl);
           break;

+        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;

+ case XEN_DOMCTL_get_domain_state:

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).

Which internal struct? The function is passed &op->... for both parameters.
And op is fully copied from guest space.

Sigh. I shouldn't have answered so quickly while being deep into other
topics. :-(

While I agree that the caller _should_ pass these fields zeroed, I'm still
not sure we should rely on it. Do you insist on removing the memset()? If
not I'd rather keep it.


Juergen

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature