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

[Jan Beulich <jbeulich@xxxxxxxx>]

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