| Message ID | 20240731203355.3652182-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/domain: Fix domlist_insert() updating the domain hash | expand |
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@citrix.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > > 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 >
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); }
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1395978459 --- xen/common/domain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)