xen/spinlock: use correct pointer

[PATCH] xen/spinlock: use correct pointer

Posted by Stewart Hildebrand 1 week, 2 days ago

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

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,

base-commit: 23cd1207e7f6ee3e51fb42e11dba8d7cdb28e1e5
-- 
2.43.2

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

Posted by Jan Beulich 1 week, 2 days ago

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

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

Posted by Stewart Hildebrand 1 week, 1 day ago

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

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

Posted by Jan Beulich 6 days, 3 hours ago

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