| Message ID | 20240425204547.658536-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/spinlock: use correct pointer | expand |
On 25.04.2024 22:45, Stewart Hildebrand wrote: > The ->profile member is at different offsets in struct rspinlock and > struct spinlock. When initializing the profiling bits of an rspinlock, > an unrelated member in struct rspinlock was being overwritten, leading > to mild havoc. Use the correct pointer. > > Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t aware") > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void) > { > (*q)->next = lock_profile_glb_q.elem_q; > lock_profile_glb_q.elem_q = *q; > - (*q)->ptr.lock->profile = *q; > + > + if ( (*q)->is_rlock ) > + (*q)->ptr.rlock->profile = *q; > + else > + (*q)->ptr.lock->profile = *q; > } > > _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL, Just to mention it: Strictly speaking spinlock_profile_print_elem()'s printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval); isn't quite right either (and I would be surprised if Misra didn't have to say something about it). Jan
On 4/26/24 02:31, Jan Beulich wrote: > On 25.04.2024 22:45, Stewart Hildebrand wrote: >> The ->profile member is at different offsets in struct rspinlock and >> struct spinlock. When initializing the profiling bits of an rspinlock, >> an unrelated member in struct rspinlock was being overwritten, leading >> to mild havoc. Use the correct pointer. >> >> Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t aware") >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks! > >> --- a/xen/common/spinlock.c >> +++ b/xen/common/spinlock.c >> @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void) >> { >> (*q)->next = lock_profile_glb_q.elem_q; >> lock_profile_glb_q.elem_q = *q; >> - (*q)->ptr.lock->profile = *q; >> + >> + if ( (*q)->is_rlock ) >> + (*q)->ptr.rlock->profile = *q; >> + else >> + (*q)->ptr.lock->profile = *q; >> } >> >> _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL, > > Just to mention it: Strictly speaking spinlock_profile_print_elem()'s > > printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval); > > isn't quite right either (and I would be surprised if Misra didn't have > to say something about it). > > Jan I'd be happy to send a patch for that instance, too. Would you like a Reported-by: tag? That patch would look something like: --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -637,22 +637,25 @@ static void cf_check spinlock_profile_print_elem(struct lock_profile *data, { unsigned int cpu; unsigned int lockval; + void *lockaddr; if ( data->is_rlock ) { cpu = data->ptr.rlock->debug.cpu; lockval = data->ptr.rlock->tickets.head_tail; + lockaddr = data->ptr.rlock; } else { cpu = data->ptr.lock->debug.cpu; lockval = data->ptr.lock->tickets.head_tail; + lockaddr = data->ptr.lock; } printk("%s ", lock_profile_ancs[type].name); if ( type != LOCKPROF_TYPE_GLOBAL ) printk("%d ", idx); - printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval); + printk("%s: addr=%p, lockval=%08x, ", data->name, lockaddr, lockval); if ( cpu == SPINLOCK_NO_CPU ) printk("not locked\n"); else That case is benign since the pointer is not dereferenced. So the rationale would primarily be for consistency (and possibly satisfying Misra).
On 26.04.2024 16:33, Stewart Hildebrand wrote: > On 4/26/24 02:31, Jan Beulich wrote: >> On 25.04.2024 22:45, Stewart Hildebrand wrote: >>> The ->profile member is at different offsets in struct rspinlock and >>> struct spinlock. When initializing the profiling bits of an rspinlock, >>> an unrelated member in struct rspinlock was being overwritten, leading >>> to mild havoc. Use the correct pointer. >>> >>> Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t aware") >>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Thanks! > >> >>> --- a/xen/common/spinlock.c >>> +++ b/xen/common/spinlock.c >>> @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void) >>> { >>> (*q)->next = lock_profile_glb_q.elem_q; >>> lock_profile_glb_q.elem_q = *q; >>> - (*q)->ptr.lock->profile = *q; >>> + >>> + if ( (*q)->is_rlock ) >>> + (*q)->ptr.rlock->profile = *q; >>> + else >>> + (*q)->ptr.lock->profile = *q; >>> } >>> >>> _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL, >> >> Just to mention it: Strictly speaking spinlock_profile_print_elem()'s >> >> printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval); >> >> isn't quite right either (and I would be surprised if Misra didn't have >> to say something about it). > > I'd be happy to send a patch for that instance, too. Would you like a > Reported-by: tag? I'm inclined to say no, not worth it, but it's really up to you. In fact I'm not sure we need to change that; it all depends on whether ... > That patch would look something like: > > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -637,22 +637,25 @@ static void cf_check spinlock_profile_print_elem(struct lock_profile *data, > { > unsigned int cpu; > unsigned int lockval; > + void *lockaddr; > > if ( data->is_rlock ) > { > cpu = data->ptr.rlock->debug.cpu; > lockval = data->ptr.rlock->tickets.head_tail; > + lockaddr = data->ptr.rlock; > } > else > { > cpu = data->ptr.lock->debug.cpu; > lockval = data->ptr.lock->tickets.head_tail; > + lockaddr = data->ptr.lock; > } > > printk("%s ", lock_profile_ancs[type].name); > if ( type != LOCKPROF_TYPE_GLOBAL ) > printk("%d ", idx); > - printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval); > + printk("%s: addr=%p, lockval=%08x, ", data->name, lockaddr, lockval); > if ( cpu == SPINLOCK_NO_CPU ) > printk("not locked\n"); > else > > > That case is benign since the pointer is not dereferenced. So the > rationale would primarily be for consistency (and possibly satisfying > Misra). ... Misra takes issue with the "wrong" member of a union being used, which iirc is UB, but which I'm afraid elsewhere we do all the time. Jan
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 558ea7ac3518..28c6e9d3ac60 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void) { (*q)->next = lock_profile_glb_q.elem_q; lock_profile_glb_q.elem_q = *q; - (*q)->ptr.lock->profile = *q; + + if ( (*q)->is_rlock ) + (*q)->ptr.rlock->profile = *q; + else + (*q)->ptr.lock->profile = *q; } _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL,
The ->profile member is at different offsets in struct rspinlock and struct spinlock. When initializing the profiling bits of an rspinlock, an unrelated member in struct rspinlock was being overwritten, leading to mild havoc. Use the correct pointer. Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t aware") Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/common/spinlock.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) base-commit: 23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5