Re: [PATCH] xnsynch_requeue_sleeper: Return early when !XNSYNCH_PRIO

On 12.04.26 09:34, Richard Weinberger wrote:
> If the synch object is not of type XNSYNCH_PRIO, this function needs to return
> before it manipulates the pend queue.
> Otherwise we face subtle follow up issues due to linked list corruptions.
> With CONFIG_XENO_OPT_DEBUG_COBALT enabled, the issue can get caught earlier.
> 

Fixes: c6232b1f2bd6 ("cobalt/synch: add support for priority ceiling
protocol")

> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> Here you can find a modified version of xddp-echo to reproduce the issue.:
> 
> https://gitlab.com/Xenomai/legacy/xenomai-hacker-space/-/tree/rw/xnsynch_pi
> 

Confirmed that this crashes nicely - we need this pattern as real test
case, not "only" as hack of an existing one. We may need more cases to
consider, though, see below.

> $ /usr/xenomai/demo/xddp-echo
> [   15.888383] ------------[ cut here ]------------
> [   15.889018] kernel BUG at kernel/xenomai/synch.c:966!
> [   15.889021] [Xenomai] switching xddp-echo to secondary mode after exception #6 in kernel-space at xnsynch_requeue_sleeper+0x18f/0x1a0 (pid 290)
> [   15.892919] Oops: invalid opcode: 0000 [#1] SMP PTI IRQ_PIPELINE
> [   15.892927] CPU: 3 UID: 0 PID: 290 Comm: xddp-echo Not tainted 6.19.0-g2fa4f147d5ae-dirty #115 PREEMPT(voluntary)
> [   15.892930] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
> [   15.892931] IRQ stage: Linux
> [   15.892934] RIP: 0010:xnsynch_requeue_sleeper+0x18f/0x1a0
> [   15.892940] Code: 49 28 e9 20 ff ff ff 49 8b 81 88 00 00 00 4c 89 50 08 48 89 79 10 4c 89 cf 48 89 41 08 4d 89 91 88 00 00 00 e9 02 fd ff ff 90 <0f> 0b 0f 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90
> [   15.892943] RSP: 0018:ffff976f00b17d78 EFLAGS: 00010046
> [   15.892956] RAX: ffff8f233bce6bc0 RBX: ffff976f006cf830 RCX: ffffffffbb83c290
> [   15.892958] RDX: ffff8f233bce6bc0 RSI: ffff976f006cee08 RDI: ffff976f006cee08
> [   15.892959] RBP: 000000000000000e R08: 00000003c88a9ddc R09: ffffffffba421a40
> [   15.892961] R10: ffff976f006cda08 R11: 0000000000000000 R12: ffff976f006cf808
> [   15.892963] R13: ffff976f00ba4e20 R14: 000000000000000c R15: ffff976f006cee08
> [   15.892968] FS:  00007f5f4f41e6c0(0000) GS:ffff8f2380525000(0000) knlGS:0000000000000000
> [   15.892970] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   15.892971] CR2: 00007f5f4ff28000 CR3: 0000000108832000 CR4: 00000000000006f0
> [   15.892973] Call Trace:
> [   15.893671]  <TASK>
> [   15.893674]  xnsynch_acquire+0x4b0/0x8e0
> [   15.893680]  __cobalt_mutex_acquire_unchecked+0x6f/0x80
> [   15.893686]  __cobalt_mutex_timedlock_break+0x392/0x3c0
> [   15.893690]  ? __pfx_CoBaLt_mutex_lock+0x10/0x10
> [   15.893694]  CoBaLt_mutex_lock+0xd/0x20
> [   15.893698]  handle_head_syscall+0x147/0x4a0
> [   15.893703]  pipeline_syscall+0x89/0xf0
> [   15.893707]  do_syscall_64+0x96/0x3f0
> [   15.894348]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [   15.894352] RIP: 0033:0x7f5f4ff02a8b
> [   15.894355] Code: 45 e8 8b 40 04 8d 50 01 48 8b 45 e8 89 50 04 b8 00 00 00 00 eb 6a 90 48 8b 45 e8 48 89 45 d8 48 8b 7d d8 b8 1d 00 00 10 0f 05 <48> 89 45 d0 48 8b 45 d0 89 45 f4 83 7d f4 00 75 0b 48 8b 45 e8 c7
> [   15.894358] RSP: 002b:00007f5f4f41dd40 EFLAGS: 00000206 ORIG_RAX: 000000001000001d
> [   15.894361] RAX: ffffffffffffffda RBX: 00007ffc37f4aa20 RCX: 00007f5f4ff02a8b
> [   15.894362] RDX: 00007f5f4fceee20 RSI: 000000000000000e RDI: 000055d8735f0260
> [   15.894364] RBP: 00007f5f4f41dd80 R08: 00007f5f4f41de08 R09: 00007f5f4f41e6c0
> [   15.894365] R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000020
> [   15.894367] R13: 0000000000000000 R14: 00007ffc37f4a4b0 R15: 00007f5f4f40e000
> [   15.894369]  </TASK>
> [   15.894370] Modules linked in:
> [   15.894374] ---[ end trace 0000000000000000 ]---
> [   15.894376] RIP: 0010:xnsynch_requeue_sleeper+0x18f/0x1a0
> [   15.894382] Code: 49 28 e9 20 ff ff ff 49 8b 81 88 00 00 00 4c 89 50 08 48 89 79 10 4c 89 cf 48 89 41 08 4d 89 91 88 00 00 00 e9 02 fd ff ff 90 <0f> 0b 0f 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90
> [   15.894384] RSP: 0018:ffff976f00b17d78 EFLAGS: 00010046
> [   15.894386] RAX: ffff8f233bce6bc0 RBX: ffff976f006cf830 RCX: ffffffffbb83c290
> [   15.894387] RDX: ffff8f233bce6bc0 RSI: ffff976f006cee08 RDI: ffff976f006cee08
> [   15.894389] RBP: 000000000000000e R08: 00000003c88a9ddc R09: ffffffffba421a40
> [   15.894390] R10: ffff976f006cda08 R11: 0000000000000000 R12: ffff976f006cf808
> [   15.894391] R13: ffff976f00ba4e20 R14: 000000000000000c R15: ffff976f006cee08
> [   15.894395] FS:  00007f5f4f41e6c0(0000) GS:ffff8f2380525000(0000) knlGS:0000000000000000
> [   15.894397] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   15.894398] CR2: 00007f5f4ff28000 CR3: 0000000108832000 CR4: 00000000000006f0
> [   15.894400] note: xddp-echo[290] exited with irqs disabled
> 
> ---
>  kernel/cobalt/synch.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/cobalt/synch.c b/kernel/cobalt/synch.c
> index 70c05c1f4..0a0b8e80f 100644
> --- a/kernel/cobalt/synch.c
> +++ b/kernel/cobalt/synch.c
> @@ -963,7 +963,9 @@ void xnsynch_requeue_sleeper(struct xnthread *thread)
>  	struct xnsynch *synch = thread->wchan;
>  	struct xnthread *owner;
>  
> -	XENO_BUG_ON(COBALT, !(synch->status & XNSYNCH_PRIO));
> +	/* Only PI-enabled objects are of interest here. */
> +	if ((synch->status & XNSYNCH_PI) == 0)
> +		return;

But XNSYNCH_PI != XNSYNCH_PRIO. PI implies PRIO, but if we enter under
prio-ceiling conditions, we won't bail out here, and we won't get the
BUG_ON anymore.

Please check if you use prio-ceiling mutex instead what the behavior is.
Add that to your test case as well.

>  
>  	/*
>  	 * Update the position in the pend queue of a thread waiting
> @@ -974,10 +976,6 @@ void xnsynch_requeue_sleeper(struct xnthread *thread)
>  	list_add_priff(thread, &synch->pendq, wprio, plink);
>  	owner = synch->owner;
>  
> -	/* Only PI-enabled objects are of interest here. */
> -	if ((synch->status & XNSYNCH_PI) == 0)
> -		return;
> -

And moving this block around may have other side effects that I do not
fully oversee yet. So far, the thread was requeued, and we only bailed
out here when no PI was in place. I bet that was to handle prio-ceiling.
Your change is likely breaking that, even beyond the crash scenario.

>  	synch->wprio = thread->wprio;
>  	if (synch->status & XNSYNCH_CLAIMED)
>  		list_del(&synch->next);

Thanks for catching this!

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center