[syzbot] [kernel?] WARNING in posixtimer_send_sigqueue (2)

[syzbot] [kernel?] WARNING in posixtimer_send_sigqueue (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    78d4f34e2115 Linux 6.13-rc3
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10010b44580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b86955f5c0c7be27
dashboard link: https://syzkaller.appspot.com/bug?extid=3c2e3cc60665d71de2f7
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10d4b4f8580000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=132b9730580000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7feb34a89c2a/non_bootable_disk-78d4f34e.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/e3d545fe5a74/vmlinux-78d4f34e.xz
kernel image: https://storage.googleapis.com/syzbot-assets/3c244cffe535/bzImage-78d4f34e.xz

The issue was bisected to:

commit df7a996b4dab03c889fa86d849447b716f07b069
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Tue Nov 5 08:14:54 2024 +0000

    signal: Queue ignored posixtimers on ignore list

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=110727e8580000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=130727e8580000
console output: https://syzkaller.appspot.com/x/log.txt?x=150727e8580000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+3c2e3cc60665d71de2f7@syzkaller.appspotmail.com
Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")

------------[ cut here ]------------
WARNING: CPU: 1 PID: 5933 at kernel/signal.c:2050 posixtimer_send_sigqueue+0xba8/0x1020 kernel/signal.c:2050
Modules linked in:
CPU: 1 UID: 0 PID: 5933 Comm: syz-executor261 Not tainted 6.13.0-rc3-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
RIP: 0010:posixtimer_send_sigqueue+0xba8/0x1020 kernel/signal.c:2050
Code: ff ff 4c 89 e7 e8 98 ff 9d 00 e9 7e f8 ff ff 41 bf 02 00 00 00 e9 87 f8 ff ff 48 89 54 24 10 48 89 44 24 08 e8 59 51 3b 00 90 <0f> 0b 90 48 8d 7b 10 48 8b 44 24 08 48 b9 00 00 00 00 00 fc ff df
RSP: 0018:ffffc900006b0d50 EFLAGS: 00010046
RAX: 0000000080010003 RBX: ffff888031d62000 RCX: 1ffff110063ac403
RDX: ffff888022920000 RSI: ffffffff815ec2e7 RDI: 0000000000000001
RBP: ffff888029fb2440 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000003 R12: ffff888031d620a4
R13: 1ffff920000d61af R14: ffff888031d620d8 R15: ffff888031d620c0
FS:  00007fb777ee36c0(0000) GS:ffff88806a700000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020040fe0 CR3: 0000000023c34000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <IRQ>
 posix_timer_fn+0x31/0x60 kernel/time/posix-timers.c:323
 __run_hrtimer kernel/time/hrtimer.c:1739 [inline]
 __hrtimer_run_queues+0x20a/0xae0 kernel/time/hrtimer.c:1803
 hrtimer_interrupt+0x392/0x8e0 kernel/time/hrtimer.c:1865
 local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1038 [inline]
 __sysvec_apic_timer_interrupt+0x10f/0x400 arch/x86/kernel/apic/apic.c:1055
 instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1049 [inline]
 sysvec_apic_timer_interrupt+0x9f/0xc0 arch/x86/kernel/apic/apic.c:1049
 </IRQ>
 <TASK>
 asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:702
RIP: 0010:__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
RIP: 0010:_raw_spin_unlock_irqrestore+0x31/0x80 kernel/locking/spinlock.c:194
Code: f5 53 48 8b 74 24 10 48 89 fb 48 83 c7 18 e8 86 04 4b f6 48 89 df e8 ce 83 4b f6 f7 c5 00 02 00 00 75 23 9c 58 f6 c4 02 75 37 <bf> 01 00 00 00 e8 c5 68 3c f6 65 8b 05 96 23 d8 74 85 c0 74 16 5b
RSP: 0018:ffffc900028f7d58 EFLAGS: 00000246
RAX: 0000000000000002 RBX: ffff888031d62030 RCX: 1ffffffff20bdd41
RDX: 0000000000000000 RSI: ffffffff8b6cd840 RDI: ffffffff8bd1e7e0
RBP: 0000000000000293 R08: 0000000000000001 R09: 0000000000000001
R10: ffffffff905f2c57 R11: 0000000000000002 R12: ffffc900028f7e50
R13: 0000000000000000 R14: 1ffff9200051efb1 R15: dffffc0000000000
 spin_unlock_irqrestore include/linux/spinlock.h:406 [inline]
 unlock_timer kernel/time/posix-timers.c:128 [inline]
 do_timer_settime+0x315/0x400 kernel/time/posix-timers.c:908
 __do_sys_timer_settime kernel/time/posix-timers.c:928 [inline]
 __se_sys_timer_settime kernel/time/posix-timers.c:914 [inline]
 __x64_sys_timer_settime+0x26a/0x2c0 kernel/time/posix-timers.c:914
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fb777f283a9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 18 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fb777ee3228 EFLAGS: 00000246 ORIG_RAX: 00000000000000df
RAX: ffffffffffffffda RBX: 000000000000001e RCX: 00007fb777f283a9
RDX: 0000000020040fe0 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 00007fb777fb2308 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb777fb2300
R13: 00007fb777fb230c R14: 00007fffae0f2ba0 R15: 00007fffae0f2c88
 </TASK>
----------------
Code disassembly (best guess):
   0:	f5                   	cmc
   1:	53                   	push   %rbx
   2:	48 8b 74 24 10       	mov    0x10(%rsp),%rsi
   7:	48 89 fb             	mov    %rdi,%rbx
   a:	48 83 c7 18          	add    $0x18,%rdi
   e:	e8 86 04 4b f6       	call   0xf64b0499
  13:	48 89 df             	mov    %rbx,%rdi
  16:	e8 ce 83 4b f6       	call   0xf64b83e9
  1b:	f7 c5 00 02 00 00    	test   $0x200,%ebp
  21:	75 23                	jne    0x46
  23:	9c                   	pushf
  24:	58                   	pop    %rax
  25:	f6 c4 02             	test   $0x2,%ah
  28:	75 37                	jne    0x61
* 2a:	bf 01 00 00 00       	mov    $0x1,%edi <-- trapping instruction
  2f:	e8 c5 68 3c f6       	call   0xf63c68f9
  34:	65 8b 05 96 23 d8 74 	mov    %gs:0x74d82396(%rip),%eax        # 0x74d823d1
  3b:	85 c0                	test   %eax,%eax
  3d:	74 16                	je     0x55
  3f:	5b                   	pop    %rbx


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

[Thomas Gleixner]

syzbot triggered the warning in posixtimer_send_sigqueue(), which warns
about a non-ignored signal being already queued on the ignored list:

  WARNING: ... at kernel/signal.c:2050 posixtimer_send_sigqueue

The warning is actually bogus, as the following valid sequence can
trigger it:

  signal($SIG, SIGIGN);
  timer_settime(...);			// arm periodic timer

    timer fires, signal is ignored and queued on ignored list

  sigprocmask(SIG_BLOCK, ...);        // block the signal
  timer_settime(...);			// re-arm periodic timer

    timer fires, signal is not ignored because it is blocked
      ---> Warning triggers as signal is on the ignored list

Ideally timer_settime() should remove the signal, but that's racy and
incomplete vs. other scenarios and requires a full re-evaluation of the
pending signal list.

Instead of adding more complexity, handle it gracefully by removing the
warning and requeueing the signal to the pending list. If the signal gets
unblocked and is still ignored, it's going back to the ignore list. If a
handler was installed before unblocking, it's going to be delivered.

There is a related scenario to trigger the complementary warning in the
signal ignored path, which does not expect the signal to be on the pending
list when it is ignored.

  WARNING: ... at kernel/signal.c:2014 posixtimer_send_sigqueue

That can be triggered even before the above change via:

  task1			task2

  signal($SIG, SIGIGN);
			sigprocmask(SIG_BLOCK, ...);

  timer_create();	// Signal target is task2
  timer_settime(...);	// arm periodic timer

    timer fires, signal is not ignored because it is blocked
    and queued on the pending list of task2

       	      	     	syscall()
			   // Sets the pending flag
			   sigprocmask(SIG_UNBLOCK, ...);

			-> preemption, task2 does not make it
                           back to exit to userspace and therefore
                           cannot dequeue the signal before:

  timer_settime(...);	// re-arm periodic timer

    timer fires, signal is ignored
      ---> Warning triggers as signal is on task2's pending list
           and the thread group is not exiting

Consequently, remove that warning too and just keep the signal on the
pending list. If the signal is dequeued by task2 and still ignored, it will
be moved to the ignored list. If a handler gets installed before the
dequeue, then it will be delivered in the same way as a signal, which is on
the ignored list when SIGIGN is lifted and therefore moved back to the
pending list.

Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")
Reported-by: syzbot+3c2e3cc60665d71de2f7@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Closes: https://lore.kernel.org/all/6761b16e.050a0220.29fcd0.006d.GAE@google.com
---
 kernel/signal.c |   38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2007,11 +2007,23 @@ void posixtimer_send_sigqueue(struct k_i
 
 		if (!list_empty(&q->list)) {
 			/*
-			 * If task group is exiting with the signal already pending,
-			 * wait for __exit_signal() to do its job. Otherwise if
-			 * ignored, it's not supposed to be queued. Try to survive.
+			 * The signal was ignored and blocked. The timer
+			 * expiry queued it because blocked signals are
+			 * queued independent of the ignored state.
+			 *
+			 * The unblocking set SIGPENDING, but the signal
+			 * was not yet dequeued from the pending list,
+			 * which would have put it back on the ignore list.
+			 * So prepare_signal() sees unblocked and ignored,
+			 * which ends up here. Leave it queued like a
+			 * regular signal.
+			 *
+			 * The same happens when the task group is exiting
+			 * and the signal is already queued.
+			 * prepare_signal() treats SIGNAL_GROUP_EXIT as
+			 * ignored independent of its queued state. This
+			 * gets cleaned up in __exit_signal().
 			 */
-			WARN_ON_ONCE(!(t->signal->flags & SIGNAL_GROUP_EXIT));
 			goto out;
 		}
 
@@ -2046,17 +2058,25 @@ void posixtimer_send_sigqueue(struct k_i
 		goto out;
 	}
 
-	/* This should never happen and leaks a reference count */
-	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
-		hlist_del_init(&tmr->ignored_list);
-
 	if (unlikely(!list_empty(&q->list))) {
 		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 
-	posixtimer_sigqueue_getref(q);
+	/*
+	 * If the signal is on the ignore list, it got blocked after it was
+	 * ignored earlier. But nothing lifted the ignore. Move it back to
+	 * the pending list to be consistent with the regular signal
+	 * handling. If it gets unblocked, it will be ignored again unless
+	 * a handler has been installed before unblocking. If it's not on
+	 * the ignore list acquire a reference count.
+	 */
+	if (likely(hlist_unhashed(&tmr->ignored_list)))
+		posixtimer_sigqueue_getref(q);
+	else
+		hlist_del_init(&tmr->ignored_list);
+
 	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out:

Re: [PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

[Frederic Weisbecker]

Le Thu, Dec 19, 2024 at 08:46:25PM +0100, Thomas Gleixner a écrit :
>  kernel/signal.c |   38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2007,11 +2007,23 @@ void posixtimer_send_sigqueue(struct k_i
>  
>  		if (!list_empty(&q->list)) {
>  			/*
> -			 * If task group is exiting with the signal already pending,
> -			 * wait for __exit_signal() to do its job. Otherwise if
> -			 * ignored, it's not supposed to be queued. Try to survive.
> +			 * The signal was ignored and blocked. The timer
> +			 * expiry queued it because blocked signals are
> +			 * queued independent of the ignored state.
> +			 *
> +			 * The unblocking set SIGPENDING, but the signal
> +			 * was not yet dequeued from the pending list,
> +			 * which would have put it back on the ignore list.

I must be missing something. I don't see dequeue_signal() checking if a signal
is ignored upon delivery.

> +			 * So prepare_signal() sees unblocked and ignored,
> +			 * which ends up here. Leave it queued like a
> +			 * regular signal.
> +			 *
> +			 * The same happens when the task group is exiting
> +			 * and the signal is already queued.
> +			 * prepare_signal() treats SIGNAL_GROUP_EXIT as
> +			 * ignored independent of its queued state. This
> +			 * gets cleaned up in __exit_signal().
>  			 */
> -			WARN_ON_ONCE(!(t->signal->flags & SIGNAL_GROUP_EXIT));
>  			goto out;
>  		}
>  
> @@ -2046,17 +2058,25 @@ void posixtimer_send_sigqueue(struct k_i
>  		goto out;
>  	}
>  
> -	/* This should never happen and leaks a reference count */
> -	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
> -		hlist_del_init(&tmr->ignored_list);
> -
>  	if (unlikely(!list_empty(&q->list))) {
>  		/* This holds a reference count already */
>  		result = TRACE_SIGNAL_ALREADY_PENDING;
>  		goto out;
>  	}
>  
> -	posixtimer_sigqueue_getref(q);
> +	/*
> +	 * If the signal is on the ignore list, it got blocked after it was
> +	 * ignored earlier. But nothing lifted the ignore. Move it back to
> +	 * the pending list to be consistent with the regular signal
> +	 * handling. If it gets unblocked, it will be ignored again unless

I'm not sure about that. If I follow the sigprocmask() path, set_task_blocked()
doesn't take care about that. And later on, dequeue_signal() doesn't seem to
check either...

Thanks.

> +	 * a handler has been installed before unblocking. If it's not on
> +	 * the ignore list acquire a reference count.
> +	 */
> +	if (likely(hlist_unhashed(&tmr->ignored_list)))
> +		posixtimer_sigqueue_getref(q);
> +	else
> +		hlist_del_init(&tmr->ignored_list);
> +
>  	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
>  	result = TRACE_SIGNAL_DELIVERED;
>  out:

Re: [PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

[Thomas Gleixner]

On Fri, Dec 20 2024 at 14:06, Frederic Weisbecker wrote:
> Le Thu, Dec 19, 2024 at 08:46:25PM +0100, Thomas Gleixner a écrit :
>>  		if (!list_empty(&q->list)) {
>>  			/*
>> -			 * If task group is exiting with the signal already pending,
>> -			 * wait for __exit_signal() to do its job. Otherwise if
>> -			 * ignored, it's not supposed to be queued. Try to survive.
>> +			 * The signal was ignored and blocked. The timer
>> +			 * expiry queued it because blocked signals are
>> +			 * queued independent of the ignored state.
>> +			 *
>> +			 * The unblocking set SIGPENDING, but the signal
>> +			 * was not yet dequeued from the pending list,
>> +			 * which would have put it back on the ignore list.
>
> I must be missing something. I don't see dequeue_signal() checking if a signal
> is ignored upon delivery.


Sorry, I meant get_signal() which is what the actual signal delivery
path on exit to user space invokes. dequeue itself does not care.

Re: [PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

[Frederic Weisbecker]

Le Fri, Dec 20, 2024 at 02:14:07PM +0100, Thomas Gleixner a écrit :
> On Fri, Dec 20 2024 at 14:06, Frederic Weisbecker wrote:
> > Le Thu, Dec 19, 2024 at 08:46:25PM +0100, Thomas Gleixner a écrit :
> >>  		if (!list_empty(&q->list)) {
> >>  			/*
> >> -			 * If task group is exiting with the signal already pending,
> >> -			 * wait for __exit_signal() to do its job. Otherwise if
> >> -			 * ignored, it's not supposed to be queued. Try to survive.
> >> +			 * The signal was ignored and blocked. The timer
> >> +			 * expiry queued it because blocked signals are
> >> +			 * queued independent of the ignored state.
> >> +			 *
> >> +			 * The unblocking set SIGPENDING, but the signal
> >> +			 * was not yet dequeued from the pending list,
> >> +			 * which would have put it back on the ignore list.
> >
> > I must be missing something. I don't see dequeue_signal() checking if a signal
> > is ignored upon delivery.
> 
> 
> Sorry, I meant get_signal() which is what the actual signal delivery
> path on exit to user space invokes. dequeue itself does not care.
> 

Hmm, ok it eventually ignores the signal delivery to the user but:

_ Dequeue signal has delivered it to posix timers
_ The signal isn't moved back to the ignored list (or I'm missing something else?)

Thanks.

Re: [PATCH] signal/posixtimers: Handle ignore/blocked sequences correctly

[Thomas Gleixner]

On Fri, Dec 20 2024 at 14:23, Frederic Weisbecker wrote:
> Le Fri, Dec 20, 2024 at 02:14:07PM +0100, Thomas Gleixner a écrit :
>> On Fri, Dec 20 2024 at 14:06, Frederic Weisbecker wrote:
>> > Le Thu, Dec 19, 2024 at 08:46:25PM +0100, Thomas Gleixner a écrit :
>> >>  		if (!list_empty(&q->list)) {
>> >>  			/*
>> >> -			 * If task group is exiting with the signal already pending,
>> >> -			 * wait for __exit_signal() to do its job. Otherwise if
>> >> -			 * ignored, it's not supposed to be queued. Try to survive.
>> >> +			 * The signal was ignored and blocked. The timer
>> >> +			 * expiry queued it because blocked signals are
>> >> +			 * queued independent of the ignored state.
>> >> +			 *
>> >> +			 * The unblocking set SIGPENDING, but the signal
>> >> +			 * was not yet dequeued from the pending list,
>> >> +			 * which would have put it back on the ignore list.
>> >
>> > I must be missing something. I don't see dequeue_signal() checking if a signal
>> > is ignored upon delivery.
>> 
>> 
>> Sorry, I meant get_signal() which is what the actual signal delivery
>> path on exit to user space invokes. dequeue itself does not care.
>> 
> Hmm, ok it eventually ignores the signal delivery to the user but:
>
> _ Dequeue signal has delivered it to posix timers
> _ The signal isn't moved back to the ignored list (or I'm missing something else?)

Hmm. Right it is not, but that's correct for blocked signals because
they can be also retrieved by sig[timed]wait().

For the case where the signal is ignored, the dequeue and posix timer
delivery is not harmful. If it gets rearmed, then the next timer expiry
will see ignored and move it to the ignored list.

Let me stare at it some more and rewrite the comments to match reality.

Thanks for pointing it out!

       tglx

[PATCH V2] signal/posixtimers: Handle ignore/blocked sequences correctly

[Thomas Gleixner]

syzbot triggered the warning in posixtimer_send_sigqueue(), which warns
about a non-ignored signal being already queued on the ignored list.

The warning is actually bogus, as the following sequence causes this:

    signal($SIG, SIGIGN);
    timer_settime(...);			// arm periodic timer

      timer fires, signal is ignored and queued on ignored list

    sigprocmask(SIG_BLOCK, ...);        // block the signal
    timer_settime(...);			// re-arm periodic timer

      timer fires, signal is not ignored because it is blocked
        ---> Warning triggers as signal is on the ignored list

Ideally timer_settime() could remove the signal, but that's racy and
incomplete vs. other scenarios and requires a full reevaluation of the
pending signal list.

Instead of adding more complexity, handle it gracefully by removing the
warning and requeueing the signal to the pending list. That's correct
versus:

  1) sig[timed]wait() as that does not check for SIGIGN and only relies on
     dequeue_signal() -> posixtimers_deliver_signal() to check whether the
     pending signal is still valid.

  2) Unblocking of the signal.

     - If the unblocking happens before SIGIGN is replaced by a signal
       handler, then the timer is rearmed in dequeue_signal(), but
       get_signal() will ignore it. The next timer expiry will move it back
       to the ignored list.

     - If SIGIGN was replaced before unblocking, then the signal will be
       delivered and a subsequent expiry will queue a signal on the pending
       list again.

There is a related scenario to trigger the complementary warning in the
signal ignored path, which does not expect the signal to be on the pending
list when it is ignored. That can be triggered even before the above change
via:

task1			task2

signal($SIG, SIGIGN);
			sigprocmask(SIG_BLOCK, ...);

timer_create();		// Signal target is task2
timer_settime(...);	// arm periodic timer

   timer fires, signal is not ignored because it is blocked
   and queued on the pending list of task2

       	      	     	syscall()
			   // Sets the pending flag
			   sigprocmask(SIG_UNBLOCK, ...);

			-> preemption, task2 cannot dequeue the signal

timer_settime(...);	// re-arm periodic timer

   timer fires, signal is ignored
        ---> Warning triggers as signal is on task2's pending list
	     and the thread group is not exiting

Consequently, remove that warning too and just keep the signal on the
pending list.

The following attempt to deliver the signal on return to user space of
task2 will ignore the signal and a subsequent expiry will bring it back to
the ignored list, if it did not get blocked or un-ignored before that.

Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")
Reported-by: syzbot+3c2e3cc60665d71de2f7@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V2: Improve change log and comments - Frederic
---
 kernel/signal.c |   37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2007,11 +2007,22 @@ void posixtimer_send_sigqueue(struct k_i
 
 		if (!list_empty(&q->list)) {
 			/*
-			 * If task group is exiting with the signal already pending,
-			 * wait for __exit_signal() to do its job. Otherwise if
-			 * ignored, it's not supposed to be queued. Try to survive.
+			 * The signal was ignored and blocked. The timer
+			 * expiry queued it because blocked signals are
+			 * queued independent of the ignored state.
+			 *
+			 * The unblocking set SIGPENDING, but the signal
+			 * was not yet dequeued from the pending list.
+			 * So prepare_signal() sees unblocked and ignored,
+			 * which ends up here. Leave it queued like a
+			 * regular signal.
+			 *
+			 * The same happens when the task group is exiting
+			 * and the signal is already queued.
+			 * prepare_signal() treats SIGNAL_GROUP_EXIT as
+			 * ignored independent of its queued state. This
+			 * gets cleaned up in __exit_signal().
 			 */
-			WARN_ON_ONCE(!(t->signal->flags & SIGNAL_GROUP_EXIT));
 			goto out;
 		}
 
@@ -2046,17 +2057,25 @@ void posixtimer_send_sigqueue(struct k_i
 		goto out;
 	}
 
-	/* This should never happen and leaks a reference count */
-	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
-		hlist_del_init(&tmr->ignored_list);
-
 	if (unlikely(!list_empty(&q->list))) {
 		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 
-	posixtimer_sigqueue_getref(q);
+	/*
+	 * If the signal is on the ignore list, it got blocked after it was
+	 * ignored earlier. But nothing lifted the ignore. Move it back to
+	 * the pending list to be consistent with the regular signal
+	 * handling. This already holds a reference count.
+	 *
+	 * If it's not on the ignore list acquire a reference count.
+	 */
+	if (likely(hlist_unhashed(&tmr->ignored_list)))
+		posixtimer_sigqueue_getref(q);
+	else
+		hlist_del_init(&tmr->ignored_list);
+
 	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out:

Re: [PATCH V2] signal/posixtimers: Handle ignore/blocked sequences correctly

[Frederic Weisbecker]

Le Tue, Jan 14, 2025 at 06:28:44PM +0100, Thomas Gleixner a écrit :
> syzbot triggered the warning in posixtimer_send_sigqueue(), which warns
> about a non-ignored signal being already queued on the ignored list.
> 
> The warning is actually bogus, as the following sequence causes this:
> 
>     signal($SIG, SIGIGN);
>     timer_settime(...);			// arm periodic timer
> 
>       timer fires, signal is ignored and queued on ignored list
> 
>     sigprocmask(SIG_BLOCK, ...);        // block the signal
>     timer_settime(...);			// re-arm periodic timer
> 
>       timer fires, signal is not ignored because it is blocked
>         ---> Warning triggers as signal is on the ignored list
> 
> Ideally timer_settime() could remove the signal, but that's racy and
> incomplete vs. other scenarios and requires a full reevaluation of the
> pending signal list.
> 
> Instead of adding more complexity, handle it gracefully by removing the
> warning and requeueing the signal to the pending list. That's correct
> versus:
> 
>   1) sig[timed]wait() as that does not check for SIGIGN and only relies on
>      dequeue_signal() -> posixtimers_deliver_signal() to check whether the
>      pending signal is still valid.
> 
>   2) Unblocking of the signal.
> 
>      - If the unblocking happens before SIGIGN is replaced by a signal
>        handler, then the timer is rearmed in dequeue_signal(), but
>        get_signal() will ignore it. The next timer expiry will move it back
>        to the ignored list.
> 
>      - If SIGIGN was replaced before unblocking, then the signal will be
>        delivered and a subsequent expiry will queue a signal on the pending
>        list again.
> 
> There is a related scenario to trigger the complementary warning in the
> signal ignored path, which does not expect the signal to be on the pending
> list when it is ignored. That can be triggered even before the above change
> via:
> 
> task1			task2
> 
> signal($SIG, SIGIGN);
> 			sigprocmask(SIG_BLOCK, ...);
> 
> timer_create();		// Signal target is task2
> timer_settime(...);	// arm periodic timer
> 
>    timer fires, signal is not ignored because it is blocked
>    and queued on the pending list of task2
> 
>        	      	     	syscall()
> 			   // Sets the pending flag
> 			   sigprocmask(SIG_UNBLOCK, ...);
> 
> 			-> preemption, task2 cannot dequeue the signal
> 
> timer_settime(...);	// re-arm periodic timer
> 
>    timer fires, signal is ignored
>         ---> Warning triggers as signal is on task2's pending list
> 	     and the thread group is not exiting
> 
> Consequently, remove that warning too and just keep the signal on the
> pending list.
> 
> The following attempt to deliver the signal on return to user space of
> task2 will ignore the signal and a subsequent expiry will bring it back to
> the ignored list, if it did not get blocked or un-ignored before that.
> 
> Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")
> Reported-by: syzbot+3c2e3cc60665d71de2f7@syzkaller.appspotmail.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

[tip: timers/urgent] signal/posixtimers: Handle ignore/blocked sequences correctly

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     8c4840277b6daffe09dea0338f3fce1eb4319a43
Gitweb:        https://git.kernel.org/tip/8c4840277b6daffe09dea0338f3fce1eb4319a43
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 14 Jan 2025 18:28:44 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 15 Jan 2025 18:08:01 +01:00

signal/posixtimers: Handle ignore/blocked sequences correctly

syzbot triggered the warning in posixtimer_send_sigqueue(), which warns
about a non-ignored signal being already queued on the ignored list.

The warning is actually bogus, as the following sequence causes this:

    signal($SIG, SIGIGN);
    timer_settime(...);			// arm periodic timer

      timer fires, signal is ignored and queued on ignored list

    sigprocmask(SIG_BLOCK, ...);        // block the signal
    timer_settime(...);			// re-arm periodic timer

      timer fires, signal is not ignored because it is blocked
        ---> Warning triggers as signal is on the ignored list

Ideally timer_settime() could remove the signal, but that's racy and
incomplete vs. other scenarios and requires a full reevaluation of the
pending signal list.

Instead of adding more complexity, handle it gracefully by removing the
warning and requeueing the signal to the pending list. That's correct
versus:

  1) sig[timed]wait() as that does not check for SIGIGN and only relies on
     dequeue_signal() -> posixtimers_deliver_signal() to check whether the
     pending signal is still valid.

  2) Unblocking of the signal.

     - If the unblocking happens before SIGIGN is replaced by a signal
       handler, then the timer is rearmed in dequeue_signal(), but
       get_signal() will ignore it. The next timer expiry will move it back
       to the ignored list.

     - If SIGIGN was replaced before unblocking, then the signal will be
       delivered and a subsequent expiry will queue a signal on the pending
       list again.

There is a related scenario to trigger the complementary warning in the
signal ignored path, which does not expect the signal to be on the pending
list when it is ignored. That can be triggered even before the above change
via:

task1			task2

signal($SIG, SIGIGN);
			sigprocmask(SIG_BLOCK, ...);

timer_create();		// Signal target is task2
timer_settime(...);	// arm periodic timer

   timer fires, signal is not ignored because it is blocked
   and queued on the pending list of task2

       	      	     	syscall()
			   // Sets the pending flag
			   sigprocmask(SIG_UNBLOCK, ...);

			-> preemption, task2 cannot dequeue the signal

timer_settime(...);	// re-arm periodic timer

   timer fires, signal is ignored
        ---> Warning triggers as signal is on task2's pending list
	     and the thread group is not exiting

Consequently, remove that warning too and just keep the signal on the
pending list.

The following attempt to deliver the signal on return to user space of
task2 will ignore the signal and a subsequent expiry will bring it back to
the ignored list, if it did not get blocked or un-ignored before that.

Fixes: df7a996b4dab ("signal: Queue ignored posixtimers on ignore list")
Reported-by: syzbot+3c2e3cc60665d71de2f7@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Link: https://lore.kernel.org/all/87ikqhcnjn.ffs@tglx

---
 kernel/signal.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 989b1cc..a2afd54 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2007,11 +2007,22 @@ void posixtimer_send_sigqueue(struct k_itimer *tmr)
 
 		if (!list_empty(&q->list)) {
 			/*
-			 * If task group is exiting with the signal already pending,
-			 * wait for __exit_signal() to do its job. Otherwise if
-			 * ignored, it's not supposed to be queued. Try to survive.
+			 * The signal was ignored and blocked. The timer
+			 * expiry queued it because blocked signals are
+			 * queued independent of the ignored state.
+			 *
+			 * The unblocking set SIGPENDING, but the signal
+			 * was not yet dequeued from the pending list.
+			 * So prepare_signal() sees unblocked and ignored,
+			 * which ends up here. Leave it queued like a
+			 * regular signal.
+			 *
+			 * The same happens when the task group is exiting
+			 * and the signal is already queued.
+			 * prepare_signal() treats SIGNAL_GROUP_EXIT as
+			 * ignored independent of its queued state. This
+			 * gets cleaned up in __exit_signal().
 			 */
-			WARN_ON_ONCE(!(t->signal->flags & SIGNAL_GROUP_EXIT));
 			goto out;
 		}
 
@@ -2046,17 +2057,25 @@ void posixtimer_send_sigqueue(struct k_itimer *tmr)
 		goto out;
 	}
 
-	/* This should never happen and leaks a reference count */
-	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
-		hlist_del_init(&tmr->ignored_list);
-
 	if (unlikely(!list_empty(&q->list))) {
 		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
 
-	posixtimer_sigqueue_getref(q);
+	/*
+	 * If the signal is on the ignore list, it got blocked after it was
+	 * ignored earlier. But nothing lifted the ignore. Move it back to
+	 * the pending list to be consistent with the regular signal
+	 * handling. This already holds a reference count.
+	 *
+	 * If it's not on the ignore list acquire a reference count.
+	 */
+	if (likely(hlist_unhashed(&tmr->ignored_list)))
+		posixtimer_sigqueue_getref(q);
+	else
+		hlist_del_init(&tmr->ignored_list);
+
 	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out: