Re: [PATCH] xen/spinlock: use correct pointer

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@xxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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).