Re: [PATCH RFC 1/4] xen: add a domain unique id to each domain

[Juergen Gross <jgross@xxxxxxxx>]

On 22.11.21 15:37, Julien Grall wrote:
> On 22/11/2021 14:29, Juergen Gross wrote:
> > On 22.11.21 15:10, Julien Grall wrote:
> > > Hi,
> Hi,
>
> > > On 22/11/2021 12:48, Juergen Gross wrote:
> > > > On 22.11.21 12:42, Julien Grall wrote:
> > > > > Hi Juergen,
> > > > >
> > > > > On 14/09/2021 13:35, Juergen Gross wrote:
> > > > > > Xenstore is referencing domains by their domid, but reuse of a domid
> > > > > > can lead to the situation that Xenstore can't tell whether a domain
> > > > > > with that domid has been deleted and created again without Xenstore
> > > > > > noticing the domain is a new one now.
> > > > > >
> > > > > > Add a global domain creation unique id which is updated when creating
> > > > > > a new domain, and store that value in struct domain of the new 
> > > > > > domain.
> > > > > > The global unique id is initialized with the system time and updates
> > > > > > are done via the xorshift algorithm which is used for pseudo random
> > > > > > number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
> > > > > >
> > > > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > > > > ---
> > > > > >   xen/common/domain.c     | 16 ++++++++++++++++
> > > > > >   xen/include/xen/sched.h |  3 +++
> > > > > >   2 files changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > > > > index 6ee5d033b0..755349b93f 100644
> > > > > > --- a/xen/common/domain.c
> > > > > > +++ b/xen/common/domain.c
> > > > > > @@ -84,6 +84,9 @@ vcpu_info_t dummy_vcpu_info;
> > > > > >   bool __read_mostly vmtrace_available;
> > > > > > +/* Unique domain identifier, protected by domctl lock. */
> > > > > > +static uint64_t unique_id;
> > > > > > +
> > > > > >   static void __domain_finalise_shutdown(struct domain *d)
> > > > > >   {
> > > > > >       struct vcpu *v;
> > > > > > @@ -473,6 +476,18 @@ static void _domain_destroy(struct domain *d)
> > > > > >       free_domain_struct(d);
> > > > > >   }
> > > > > > +static uint64_t get_unique_id(void)
> > > > > The implementation is assuming that domain cannot be created 
> > > > > concurrently. The rest of domain_create() seems to be able to cope 
> > > > > with concurrent call (even if domctl prevents this situation today).
> > > > > So I think we would want to make this call safe as well. One 
> > > > > possibility would be to (ab)use the domlist_update_lock (I think 
> > > > > the uniq ID is only necessary for real domains).
> > > > In case this is thought to be needed, I'd rather use a cmpxchg 
> > > > operation
> > > > for updating unique_id.
> > > I would be OK with cmpxchg(). But I would like to avoid cmpxchg() 
> > > loop if possible.
> > The chances for that loop to be needed are zero today, as the domctl
> > lock is prohibiting concurrent use.
> Well for domain created by the toolstack yes... For domain created by 
> Xen no. Today, this is only happening at boot so you are safe (for now).
> That said, I don't really see a reason to prevent concurrent 
> domain_create() and as I wrote nothing in domain_create() seems to make 
> this assumption. So I am not happy to build on that in this new helper.
Fair enough.

> > Another possibility would be to use an:
> >
> > ASSERT(spin_is_locked(&domctl_lock));
> This ASSERT() would be incorrect as it could be hit for any domain 
> created by Xen.
Oh, right.

In the end it doesn't really hurt to add another lock. Its not as if
we'd expect much contention for it. :-)

> [...]
>
> > > Reading the commit message (and cover letter) again, I understand 
> > > that you want a unique ID but it is not clear to me why this needs to 
> > > be pseudo-randomly generated. IOW, given the expected use, can you 
> > > clarify what would be the concern to use an atomic_inc_return() 
> > > instead of xorshift?
> > Jan had a concern regarding potential misuse of the unique id in case it
> > were kind of predictable.
> Fair enough. Can this be written down in the commit message?
Yes, will do that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature