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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Nov 1, 2024 at 7:06 AM GMT, Jürgen Groß wrote:
> On 31.10.24 12:58, Alejandro Vallejo wrote:
> > On Wed Oct 23, 2024 at 3:27 PM BST, Juergen Gross wrote:
> >> On 23.10.24 16:08, Alejandro Vallejo wrote:
> >>> On Wed Oct 23, 2024 at 2:10 PM BST, 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>
> >>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> ---
> >>>> V1:
> >>>> - make unique_id local to function (Jan Beulich)
> >>>> - add lock (Julien Grall)
> >>>> - add comment (Julien Grall)
> >>>> ---
> >>>>    xen/common/domain.c     | 20 ++++++++++++++++++++
> >>>>    xen/include/xen/sched.h |  3 +++
> >>>>    2 files changed, 23 insertions(+)
> >>>>
> >>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >>>> index 92263a4fbd..3948640fb0 100644
> >>>> --- a/xen/common/domain.c
> >>>> +++ b/xen/common/domain.c
> >>>> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
> >>>>        free_domain_struct(d);
> >>>>    }
> >>>>    
> >>>> +static uint64_t get_unique_id(void)
> >>>> +{
> >>>> +    static uint64_t unique_id;
> >>>> +    static DEFINE_SPINLOCK(lock);
> >>>> +    uint64_t x = unique_id ? : NOW();
> >>>> +
> >>>> +    spin_lock(&lock);
> >>>> +
> >>>> +    /* Pseudo-randomize id in order to avoid consumers relying on 
> >>>> sequence. */
> >>>> +    x ^= x << 13;
> >>>> +    x ^= x >> 7;
> >>>> +    x ^= x << 17;
> >>>> +    unique_id = x;
> > 
> > How "unique" are they? With those shifts it's far less obvious to know how 
> > many
> > times we can call get_unique_id() and get an ID that hasn't been seen since
> > reset. With sequential numbers it's pretty obvious that it'd be a
> > non-overflowable monotonic counter. Here's it's far less clear, particularly
> > when it's randomly seeded.
>
> If you'd look into the Wikipedia article mentioned in the commit message
> you'd know that the period is 2^64 - 1.
>

Bah. I did, but skimmed too fast looking for keywords. Thanks for bearing with
me :). Ok, with that I'm perfectly happy.

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>

> > I don't quite see why sequential IDs are problematic. What is this
> > (pseudo)randomization specifically trying to prevent? If it's just breaking 
> > the
> > assumption that numbers go in strict sequence you could just flip the high 
> > and
> > low nibbles (or any other deterministic swapping of counter nibbles)
>
> That was a request from the RFC series of this patch.
>
> > Plus, with the counter going in sequence we could get rid of the lock 
> > because
> > an atomic fetch_add() would do.
>
> Its not as if this would be a hot path. So the lock is no real issue IMO.
>
> > 
> >>>> +
> >>>> +    spin_unlock(&lock);
> >>>> +
> >>>> +    return x;
> >>>> +}
> >>>> +
> >>>>    static int sanitise_domain_config(struct xen_domctl_createdomain 
> >>>> *config)
> >>>>    {
> >>>>        bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> >>>> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
> >>>>    
> >>>>        /* Sort out our idea of is_system_domain(). */
> >>>>        d->domain_id = domid;
> >>>> +    d->unique_id = get_unique_id();
> >>>>    
> >>>>        /* Holding CDF_* internal flags. */
> >>>>        d->cdf = flags;
> >>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> >>>> index 90666576c2..1dd8a425f9 100644
> >>>> --- a/xen/include/xen/sched.h
> >>>> +++ b/xen/include/xen/sched.h
> >>>> @@ -370,6 +370,9 @@ struct domain
> >>>>        domid_t          domain_id;
> >>>>    
> >>>>        unsigned int     max_vcpus;
> >>>> +
> >>>> +    uint64_t         unique_id;       /* Unique domain identifier */
> >>>> +
> >>>
> >>> Why not xen_domain_handle_t handle, defined later on? That's meant to be a
> >>> UUID, so this feels like a duplicate field.
> >>
> >> It is an input value for create domain. So there is absolutely no
> >> guarantee that it is unique.
> >>
> >> It can especially be specified in the xl config file, so you could have
> >> a host running multiple guests all with the same UUID (even if this might
> >> be rejected by libxl, destroying a guest and then recreating it with the
> >> same UUID is possible, but Xenstore would need to see different unique Ids
> >> for those 2 guests).
> > 
> > Fair points. With that into account, I wouldn't mind seeing a wider comment 
> > on
> > top of unique_id explaining how these IDs are meant to be non-repeatable
> > between resets and meant to have the same lifetime as their corresponding
> > domain_id.
>
> Okay, I can add such a comment.
>
>
> Juergen

Cheers,
Alejandro