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 <[email protected]>
>
> Reviewed-by: Jan Beulich <[email protected]>
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).
