linux-kernel - RE: [EXT] Re: [PATCH] softirq: add irq off checking for __raise_softirq

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]

Date:   Fri, 14 Aug 2020 04:17:19 +0000
From:   Jiafei Pan <jiafei.pan@....com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "romain.perier@...il.com" <romain.perier@...il.com>,
        "will@...nel.org" <will@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rt-users@...r.kernel.org" <linux-rt-users@...r.kernel.org>,
        Leo Li <leoyang.li@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Jiafei Pan <jiafei.pan@....com>
Subject: RE: [EXT] Re: [PATCH] softirq: add irq off checking for
 __raise_softirq_irqoff

> From: Steven Rostedt <rostedt@...dmis.org>
> Sent: Thursday, August 13, 2020 10:57 PM
> 
> On Thu, 13 Aug 2020 03:03:46 +0000
> Jiafei Pan <jiafei.pan@....com> wrote:
> 
> > Any comments? Thanks.
> >
> > @Steven Rostedt, I thinks irq off checking is necessary especially
> 
> This is probably more for Thomas Gleixner.
> 
> > for Preempt-RT kernel, because some context may be changed from irq
> > off to irq on when enable Preempt RT, I once met a issue that hrtimer
> > soft irq is lost when enabled Preempt RT, finally I found
> > napi_schedule_irqoff is called in hardware interrupt handler, there
> > maybe no issue for non RT kernel, but for Preempt RT, interrupt is
> > threaded, so irq is on in interrupt handler, the result is
> > __raise_softirq_irqoff is called in irq on context, so that per-CPU
> > softirq masking is corrupted because of the process of updating of
> > soft irq masking is interrupted and not a atomic operation , and then
> > caused hrtimer soft irq is lost. So I think adding irq status checking
> > in __raise_softirq_irqoff can report such issue directly and help us
> > to find the root cause of such issue.
> >
> > I know that there may be performance impaction to add extra checking
> > here, if it is the case, how about to include it in some debug
> > configuration items? Such as CONFIG_DEBUG_PREEMPT or other debug
> > items?
> >
> 
> 
> > Best Regards,
> > Jiafei.
> >
> > -----Original Message-----
> > From: Jiafei Pan <Jiafei.Pan@....com>
> > Sent: Thursday, August 6, 2020 12:07 PM
> > To: peterz@...radead.org; mingo@...nel.org; tglx@...utronix.de;
> > rostedt@...dmis.org; romain.perier@...il.com; will@...nel.org
> > Cc: linux-kernel@...r.kernel.org; linux-rt-users@...r.kernel.org;
> > Jiafei Pan <jiafei.pan@....com>; Leo Li <leoyang.li@....com>; Vladimir
> > Oltean <vladimir.oltean@....com>; Jiafei Pan <jiafei.pan@....com>
> > Subject: [PATCH] softirq: add irq off checking for
> > __raise_softirq_irqoff
> >
> > __raise_softirq_irqoff will update per-CPU mask of pending softirqs, it need
> to be called in irq disabled context in order to keep it atomic operation,
> otherwise it will be interrupted by hardware interrupt, and per-CPU softirqs
> pending mask will be corrupted, the result is there will be unexpected issue,
> for example hrtimer soft irq will be losed and soft hrtimer will never be expire
> and handled.
> 
> Please wrap your change logs.
[Jiafei Pan] Thanks, will update it.
> 
> >
> > Adding irqs disabled checking here to provide warning in irqs enabled
> context.
> >
> > Signed-off-by: Jiafei Pan <Jiafei.Pan@....com>
> > ---
> >  kernel/softirq.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/softirq.c b/kernel/softirq.c index
> > bf88d7f62433..11f61e54a3ae 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -481,6 +481,11 @@ void raise_softirq(unsigned int nr)
> >
> >  void __raise_softirq_irqoff(unsigned int nr)  {
> > +     /* This function can only be called in irq disabled context,
> > +      * otherwise or_softirq_pending will be interrupted by hardware
> > +      * interrupt, so that there will be unexpected issue.
> > +      */
> > +     WARN_ON_ONCE(!irqs_disabled());
> 
> Perhaps: lockdep_assert_irqs_disabled() is more appropriate, and doesn't add
> extra overhead on production systems.
> 
> -- Steve
[Jiafei Pan] Thanks, will update it.
> 
> 
> >       trace_softirq_raise(nr);
> >       or_softirq_pending(1UL << nr);
> >  }
> > --
> > 2.17.1