linux-kernel - Re: [PATCH v3 1/2] irq: abstract irqaction handler invocation

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] [day] [month] [year] [list]

Message-ID: <20210715131044.GB4717@worktop.programming.kicks-ass.net>
Date:   Thu, 15 Jul 2021 15:10:44 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org, tglx@...utronix.de, maz@...nel.org,
        paulmck@...nel.org
Subject: Re: [PATCH v3 1/2] irq: abstract irqaction handler invocation

On Thu, Jul 15, 2021 at 12:15:31PM +0100, Mark Rutland wrote:
> On Thu, Jul 15, 2021 at 12:49:54PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 15, 2021 at 10:50:30AM +0100, Mark Rutland wrote:
> > > diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> > > index 54363527feea..70a4694cc891 100644
> > > --- a/kernel/irq/internals.h
> > > +++ b/kernel/irq/internals.h
> > > @@ -11,6 +11,8 @@
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/sched/clock.h>
> > >  
> > > +#include <trace/events/irq.h>
> > > +
> > >  #ifdef CONFIG_SPARSE_IRQ
> > >  # define IRQ_BITMAP_BITS	(NR_IRQS + 8196)
> > >  #else
> > > @@ -107,6 +109,32 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
> > >  irqreturn_t handle_irq_event_percpu(struct irq_desc *desc);
> > >  irqreturn_t handle_irq_event(struct irq_desc *desc);
> > >  
> > > +static inline irqreturn_t __handle_irqaction(unsigned int irq,
> > > +					     struct irqaction *action,
> > > +					     void *dev_id)
> > > +{
> > > +	irqreturn_t res;
> > > +
> > > +	trace_irq_handler_entry(irq, action);
> > > +	res = action->handler(irq, dev_id);
> > > +	trace_irq_handler_exit(irq, action, res);
> > > +
> > > +	return res;
> > > +}
> > > +
> > > +static inline irqreturn_t handle_irqaction(unsigned int irq,
> > > +					   struct irqaction *action)
> > > +{
> > > +	return __handle_irqaction(irq, action, action->dev_id);
> > > +}
> > > +
> > > +static inline irqreturn_t handle_irqaction_percpu_devid(unsigned int irq,
> > > +							struct irqaction *action)
> > > +{
> > > +	return __handle_irqaction(irq, action,
> > > +				  raw_cpu_ptr(action->percpu_dev_id));
> > > +}
> > 
> > So I like this patch, it's a nice cleanup.
> > 
> > However, you could implement the next patch as a module that hooks into
> > those two tracepoints. Quite possibly the existing IRQ latency tracer
> > would already work for what you need and also provide you a function
> > trace of WTH the CPU was doing.
> 
> The issue with the existing tracers is that they're logging for
> later/concurrent analysis, whereas what I need is a notification (e.g. a
> WARN) when the maximum expected latency has been breached. That way it
> gets caught by Syzkaller or whatever without needing to specially manage
> the tracer.
> 
> If there's a way to do that (e.g. with boot-time options), I'm happy to
> use that instead; I just couldn't see hwo to do that today, and was
> under the impression that the existing tracepoints don't give quite what
> I need (e.g. since the entry/exit hooks are separate, so I'd have to
> store some state somewhere else).
> 
> I'm happy to take another look if you think I'm wrong on that. :)

For this particular thing I think you can use a simple per-cpu variable;
we don't do nested interrupts.

DEFINE_PER_CPU(u64, my_timestamp);

static notrace void my_entry(unsigned int irq, struct irq_action *action)
{
	this_cpu_write(my_timestamp, sched_clock());
}

static notrace void my_exit(unsigned int irq, struct irq_action *action)
{
	u64 delta = sched_clock() - this_cpu_read(my_timestamp);
	WARN_ON_ONCE(delta > biggie);
}

__init int mod_init(void)
{
	register_trace_irq_handler_exit(my_exit, NULL);
	register_trace_irq_handler_entry(my_entry, NULL);
}

Should work, no?