Re: [PATCH] x86/domain: Fix domlist_insert() updating the domain hash

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 31 Jul 2024, Andrew Cooper wrote:
> A last minute review request was to dedup the expression calculating the
> domain hash bucket.
> 
> While the code reads correctly, it is buggy because rcu_assign_pointer() is a
> criminally stupid API assigning by name not value, and - contrary to it's name
> - does not hide an indirection.
> 
> Therefore, rcu_assign_pointer(bucket, d); updates the local bucket variable on
> the stack, not domain_hash[], causing all subsequent domid lookups to fail.
> 
> Rework the logic to use pd in the same way that domlist_remove() does.
> 
> Fixes: 19995bc70cc6 ("xen/domain: Factor domlist_{insert,remove}() out of 
> domain_{create,destroy}()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>


Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1395978459
> ---
>  xen/common/domain.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 8d8f40ccb245..92263a4fbdc5 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -70,7 +70,7 @@ struct domain *domain_list;
>   */
>  static void domlist_insert(struct domain *d)
>  {
> -    struct domain **pd, *bucket;
> +    struct domain **pd;
>  
>      spin_lock(&domlist_update_lock);
>  
> @@ -79,12 +79,12 @@ static void domlist_insert(struct domain *d)
>          if ( (*pd)->domain_id > d->domain_id )
>              break;
>  
> -    bucket = domain_hash[DOMAIN_HASH(d->domain_id)];
> -
>      d->next_in_list = *pd;
> -    d->next_in_hashbucket = bucket;
>      rcu_assign_pointer(*pd, d);
> -    rcu_assign_pointer(bucket, d);
> +
> +    pd = &domain_hash[DOMAIN_HASH(d->domain_id)];
> +    d->next_in_hashbucket = *pd;
> +    rcu_assign_pointer(*pd, d);
>  
>      spin_unlock(&domlist_update_lock);
>  }
> -- 
> 2.39.2
>