Re: [PATCH v2] kcsan: Don't enter an nbcon emergency section for verbose reports

[Petr Mladek]

On Wed 2026-11-04 15:48:06, John Ogness wrote:
> On 2026-11-04, Breno Leitao <leitao@xxxxxxxxxx> wrote:
> >> Wouldn't the below diff be the proper fix?
> >> 
> >> John
> >> 
> >> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> >> index c3dc84a7cef26..71f1b4b8f06d5 100644
> >> --- a/kernel/locking/lockdep.c
> >> +++ b/kernel/locking/lockdep.c
> >> @@ -4245,8 +4245,6 @@ void print_irqtrace_events(struct task_struct *curr)
> >>  {
> >>  	const struct irqtrace_events *trace = &curr->irqtrace;
> >>  
> >> -	nbcon_cpu_emergency_enter();
> >> -
> >>  	printk("irq event stamp: %u\n", trace->irq_events);
> >>  	printk("hardirqs last  enabled at (%u): [<%px>] %pS\n",
> >>  		trace->hardirq_enable_event, (void *)trace->hardirq_enable_ip,
> >> @@ -4260,8 +4258,6 @@ void print_irqtrace_events(struct task_struct *curr)
> >>  	printk("softirqs last disabled at (%u): [<%px>] %pS\n",
> >>  		trace->softirq_disable_event, (void *)trace->softirq_disable_ip,
> >>  		(void *)trace->softirq_disable_ip);
> >> -
> >> -	nbcon_cpu_emergency_exit();
> >>  }
> >>  
> >>  static int HARDIRQ_verbose(struct lock_class *class)
> >> @@ -5797,8 +5793,11 @@ static noinstr void check_flags(unsigned long flags)
> >>  	}
> >>  #endif
> >>  
> >> -	if (!debug_locks)
> >> +	if (!debug_locks) {
> >> +		nbcon_cpu_emergency_enter();
> >>  		print_irqtrace_events(current);
> >> +		nbcon_cpu_emergency_exit();
> >> +	}
> >>  
> >>  	instrumentation_end();
> >>  #endif
> >
> > Yes, but it was unclear to me what were the invocations of
> > print_irqtrace_events() that needed the emergency turned on.
> >
> > how did you decided that only check_flags() needed it?
> 
> The emergency enter/exit's are intended for lockdep splats. AFAICT
> check_flags() was the only call to print_irqtrace_events() that was not
> already in emergency state for lockdep splats.
>
> KCSAN is not the only non-lockdep caller of print_irqtrace_events(). So
> my suggested fix applies to the other non-lockdep users as well. Commit
> 59cd94ef8009 ("lockdep: Mark emergency sections in lockdep splats") was
> only intended for lockdep splats.

I agree with this reasoning. Well, more messages might be printed
in check_flags(). IMHO, we should print them all inside
nbcon_cpu_emergency_enter()/exit(). I mean:

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5769,6 +5769,7 @@ static noinstr void check_flags(unsigned long flags)
 
 	/* Get the warning out..  */
 	instrumentation_begin();
+	nbcon_cpu_emergency_enter();
 
 	if (irqs_disabled_flags(flags)) {
 		if (DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())) {
@@ -5800,6 +5801,7 @@ static noinstr void check_flags(unsigned long flags)
 	if (!debug_locks)
 		print_irqtrace_events(current);
 
+	nbcon_cpu_emergency_exit();
 	instrumentation_end();
 #endif
 }

And there seems to be one more lockdep-caller of
print_irqtrace_events():

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4649,6 +4649,8 @@ void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
 	if (unlikely(!debug_locks))
 		return;
 
+	nbcon_cpu_emergency_enter();
+
 	if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
 		pr_warn("CPU %u left hardirqs enabled!", cpu);
 		if (idle)
@@ -4656,6 +4658,8 @@ void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
 		/* Clean it up for when the CPU comes online again. */
 		per_cpu(hardirqs_enabled, cpu) = 0;
 	}
+
+	nbcon_cpu_emergency_enter();
 }
 
 static int

What do you think, please?

Best Regards,
Petr