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