linux-kernel - Re: 'perf test sigtrap' failing on PREEMPT_RT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZZcyzV8logh6BY0I@kernel.org>
Date: Thu, 4 Jan 2024 19:35:57 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Mike Galbraith <efault@....de>, Peter Zijlstra <peterz@...radead.org>,
	Marco Elver <elver@...gle.com>, linux-rt-users@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Clark Williams <williams@...hat.com>,
	Alessandro Carminati <acarmina@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: 'perf test sigtrap' failing on PREEMPT_RT_FULL

Em Fri, Jul 28, 2023 at 05:07:18PM +0200, Sebastian Andrzej Siewior escreveu:
> On 2023-07-26 08:10:45 [+0200], Mike Galbraith wrote:
> > > [   52.848925] BUG: scheduling while atomic: perf/6549/0x00000002

> > Had bf9ad37dc8a not been reverted due to insufficient beauty, you could
> > trivially make the sigtrap test a happy camper (wart tested in tip-rt).
 
> Thank you for the pointer Mike.
 
> I guess we need this preempt_disable_notrace() in perf_pending_task()
> due to context accounting in get_recursion_context(). Would a
> migrate_disable() be sufficient or could we send the signal outside of
> the preempt-disabled block?

I got back to this, need to go again over all the callers of
perf_swevent_get_recursion_context(), from the first quick glance there
are other places with preempt_disable()/enable(), but doing just the
switch to migrate disable/enable on perf_pending_task() makes this
specific test to work:

[acme@...e linux]$ git log --oneline -5
086dab66d504 (HEAD -> linux-rt-devel/linux-6.7.y-rt/send_sig_perf.fix, tag: v6.7-rc5-rt5, linux-rt-devel/linux-6.7.y-rt) v6.7-rc5-rt5
29e0d951f39b printk: Update the printk series.
2308ecc8ce88 (tag: v6.7-rc5-rt4) v6.7-rc5-rt4
10d5f3551216 Merge tag 'v6.7-rc5' into linux-6.7.y-rt
a39b6ac3781d (tag: v6.7-rc5, linux-rt-devel/master, linux-rt-devel/linux-6.7.y) Linux 6.7-rc5
[acme@...e linux]$ git diff
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c9d123e13b57..a9b9ef60f6b3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6801,7 +6801,7 @@ static void perf_pending_task(struct callback_head *head)
         * If we 'fail' here, that's OK, it means recursion is already disabled
         * and we won't recurse 'further'.
         */
-       preempt_disable_notrace();
+       migrate_disable();
        rctx = perf_swevent_get_recursion_context();
 
        if (event->pending_work) {
@@ -6812,7 +6812,7 @@ static void perf_pending_task(struct callback_head *head)
 
        if (rctx >= 0)
                perf_swevent_put_recursion_context(rctx);
-       preempt_enable_notrace();
+       migrate_enable();
 
        put_event(event);
 }
[acme@...e linux]$ uname -a
Linux nine 6.7.0-rc5-rt5.sigtrap-fix-dirty #2 SMP PREEMPT_RT Thu Jan  4 18:11:44 -03 2024 x86_64 x86_64 x86_64 GNU/Linux
[acme@...e linux]$ sudo su -
[sudo] password for acme: 
[root@...e ~]# 
[root@...e ~]# perf test sigtrap
 68: Sigtrap                                                         : Ok
[root@...e ~]# 
[root@...e ~]# perf probe -L perf_pending_task
<perf_pending_task@...me/acme/git/linux/kernel/events/core.c:0>
      0  static void perf_pending_task(struct callback_head *head)
         {
      2         struct perf_event *event = container_of(head, struct perf_event, pending_task);
      3         int rctx;
         
                /*
                 * If we 'fail' here, that's OK, it means recursion is already disabled
                 * and we won't recurse 'further'.
                 */
                migrate_disable();
     10         rctx = perf_swevent_get_recursion_context();
         
     12         if (event->pending_work) {
     13                 event->pending_work = 0;
     14                 perf_sigtrap(event);
     15                 local_dec(&event->ctx->nr_pending);
                }
         
     18         if (rctx >= 0)
     19                 perf_swevent_put_recursion_context(rctx);
     20         migrate_enable();
         
     22         put_event(event);
         }
         
         #ifdef CONFIG_GUEST_PERF_EVENTS

[root@...e ~]# perf probe perf_pending_task
Added new event:
  probe:perf_pending_task (on perf_pending_task)

You can now use it in all perf tools, such as:

	perf record -e probe:perf_pending_task -aR sleep 1

[root@...e ~]# perf trace --max-events=1 -e probe:perf_pending_task/max-stack=6/ perf test sigtrap 
 68: Sigtrap                                                         : Ok
     0.000 :9608/9608 probe:perf_pending_task(__probe_ip: -2064408784)
                                       perf_pending_task ([kernel.kallsyms])
                                       task_work_run ([kernel.kallsyms])
                                       exit_to_user_mode_loop ([kernel.kallsyms])
                                       exit_to_user_mode_prepare ([kernel.kallsyms])
                                       irqentry_exit_to_user_mode ([kernel.kallsyms])
                                       asm_sysvec_irq_work ([kernel.kallsyms])
[root@...e ~]#

[root@...e ~]# head -5 /etc/os-release
NAME="Red Hat Enterprise Linux"
VERSION="9.2 (Plow)"
ID="rhel"
ID_LIKE="fedora"
VERSION_ID="9.2"
[root@...e ~]#

I did the test without the above patch and the original problem is
reproduced.
 
> This is also used in perf_pending_irq() and on PREEMPT_RT this is
> invoked from softirq context which is preemptible.

Right.

- Arnaldo