* [PATCH] kcov: properly check if we are in an interrupt @ 2016-09-23 14:51 Andrey Konovalov 2016-09-26 23:32 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Andrey Konovalov @ 2016-09-23 14:51 UTC (permalink / raw) To: Andrew Morton, Dmitry Vyukov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, linux-kernel, vegard.nossum, quentin.casasnovas, ryabinin.a.a Cc: Andrey Konovalov in_interrupt() returns a nonzero value when we are either in an interrupt or have bh disabled via local_bh_disable(). Since we are interested in only ignoring coverage from actual interrupts, do a proper check of whether we are really in an interrupt. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- It would look totally better to reuse in_irq(), in_serving_softirq() and in_nmi() instead of checking flags manually, but that leads to slower generated code (three separate tests for each of the flags). Would it be better to add another macro to preempt.h that would check if we're actually in interrupt and use it? kernel/kcov.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/kcov.c b/kernel/kcov.c index 8d44b3f..47c35a8 100644 --- a/kernel/kcov.c +++ b/kernel/kcov.c @@ -54,7 +54,8 @@ void notrace __sanitizer_cov_trace_pc(void) * We are interested in code coverage as a function of a syscall inputs, * so we ignore code executed in interrupts. */ - if (!t || in_interrupt()) + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET + | NMI_MASK))) return; mode = READ_ONCE(t->kcov_mode); if (mode == KCOV_MODE_TRACE) { -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-23 14:51 [PATCH] kcov: properly check if we are in an interrupt Andrey Konovalov @ 2016-09-26 23:32 ` Andrew Morton 2016-09-27 6:21 ` Dmitry Vyukov 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2016-09-26 23:32 UTC (permalink / raw) To: Andrey Konovalov Cc: Dmitry Vyukov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, linux-kernel, vegard.nossum, quentin.casasnovas, ryabinin.a.a, Thomas Gleixner, Ingo Molnar, Peter Zijlstra On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <andreyknvl@google.com> wrote: > in_interrupt() returns a nonzero value when we are either in an > interrupt or have bh disabled via local_bh_disable(). Since we are > interested in only ignoring coverage from actual interrupts, do a > proper check of whether we are really in an interrupt. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > It would look totally better to reuse in_irq(), in_serving_softirq() and > in_nmi() instead of checking flags manually, but that leads to slower > generated code (three separate tests for each of the flags). Would it be > better to add another macro to preempt.h that would check if we're actually > in interrupt and use it? Yes please. Is there anywhere else where such a macro can be used? > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -54,7 +54,8 @@ void notrace __sanitizer_cov_trace_pc(void) > * We are interested in code coverage as a function of a syscall inputs, > * so we ignore code executed in interrupts. > */ > - if (!t || in_interrupt()) > + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET > + | NMI_MASK))) Or include a prominent and very apologetic comment here explaining why it is open-coded. But I do agree that not open-coding it is better. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-26 23:32 ` Andrew Morton @ 2016-09-27 6:21 ` Dmitry Vyukov 2016-09-27 7:34 ` Peter Zijlstra 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Vyukov @ 2016-09-27 6:21 UTC (permalink / raw) To: Andrew Morton Cc: Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Vegard Nossum, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar, Peter Zijlstra On Tue, Sep 27, 2016 at 1:32 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <andreyknvl@google.com> wrote: > >> in_interrupt() returns a nonzero value when we are either in an >> interrupt or have bh disabled via local_bh_disable(). Since we are >> interested in only ignoring coverage from actual interrupts, do a >> proper check of whether we are really in an interrupt. >> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com> >> --- >> It would look totally better to reuse in_irq(), in_serving_softirq() and >> in_nmi() instead of checking flags manually, but that leads to slower >> generated code (three separate tests for each of the flags). Would it be >> better to add another macro to preempt.h that would check if we're actually >> in interrupt and use it? > > Yes please. Is there anywhere else where such a macro can be used? I suspect there is a bunch of places that use in_interrupt(), but mean the same as KCOV wants -- am I in interrupt? and not am I in interrupt context or in normal task context but inside local_bh_disable(). For example, why does fput handles closure asynchronously if the task called local_bh_disable? 264 void fput(struct file *file) 265 { 266 if (atomic_long_dec_and_test(&file->f_count)) { 267 struct task_struct *task = current; 268 269 if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { 270 init_task_work(&file->f_u.fu_rcuhead, ____fput); 271 if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) 272 return; 273 /* 274 * After this task has run exit_task_work(), 275 * task_work_add() will fail. Fall through to delayed 276 * fput to avoid leaking *file. 277 */ 278 } 279 280 if (llist_add(&file->f_u.fu_llist, &delayed_fput_list)) 281 schedule_delayed_work(&delayed_fput_work, 1); 282 } 283 } >> --- a/kernel/kcov.c >> +++ b/kernel/kcov.c >> @@ -54,7 +54,8 @@ void notrace __sanitizer_cov_trace_pc(void) >> * We are interested in code coverage as a function of a syscall inputs, >> * so we ignore code executed in interrupts. >> */ >> - if (!t || in_interrupt()) >> + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET >> + | NMI_MASK))) > > Or include a prominent and very apologetic comment here explaining why > it is open-coded. But I do agree that not open-coding it is better. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 6:21 ` Dmitry Vyukov @ 2016-09-27 7:34 ` Peter Zijlstra 2016-09-27 7:50 ` Dmitry Vyukov 0 siblings, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2016-09-27 7:34 UTC (permalink / raw) To: Dmitry Vyukov Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Vegard Nossum, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: > > I suspect there is a bunch of places that use in_interrupt(), but mean > the same as KCOV wants -- am I in interrupt? and not am I in interrupt > context or in normal task context but inside local_bh_disable(). For > example, why does fput handles closure asynchronously if the task > called local_bh_disable? Agreed, but it would mean auditing all in_interrupt()/irq_count() users. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 7:34 ` Peter Zijlstra @ 2016-09-27 7:50 ` Dmitry Vyukov 2016-09-27 10:59 ` Peter Zijlstra 2016-09-27 11:20 ` Vegard Nossum 0 siblings, 2 replies; 9+ messages in thread From: Dmitry Vyukov @ 2016-09-27 7:50 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Vegard Nossum, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: >> >> I suspect there is a bunch of places that use in_interrupt(), but mean >> the same as KCOV wants -- am I in interrupt? and not am I in interrupt >> context or in normal task context but inside local_bh_disable(). For >> example, why does fput handles closure asynchronously if the task >> called local_bh_disable? > > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. I don't think this means auditing all users. We are not making things worse by introduction of a new predicate. It would be nice to look at some uses in core code, but the only place with observed harm is KCOV. Any naming suggestions? Other than really_in_interrupt or in_interrupt_and_not_in_bh_disabled? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 7:50 ` Dmitry Vyukov @ 2016-09-27 10:59 ` Peter Zijlstra 2016-09-27 12:32 ` Dmitry Vyukov 2016-09-27 11:20 ` Vegard Nossum 1 sibling, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2016-09-27 10:59 UTC (permalink / raw) To: Dmitry Vyukov Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Vegard Nossum, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On Tue, Sep 27, 2016 at 09:50:41AM +0200, Dmitry Vyukov wrote: > On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: > >> > >> I suspect there is a bunch of places that use in_interrupt(), but mean > >> the same as KCOV wants -- am I in interrupt? and not am I in interrupt > >> context or in normal task context but inside local_bh_disable(). For > >> example, why does fput handles closure asynchronously if the task > >> called local_bh_disable? > > > > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. > > > I don't think this means auditing all users. We are not making things > worse by introduction of a new predicate. > It would be nice to look at some uses in core code, but the only place > with observed harm is KCOV. > > Any naming suggestions? Other than really_in_interrupt or > in_interrupt_and_not_in_bh_disabled? Hence the suggestion to audit and fix instead of making a bigger mess :/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 10:59 ` Peter Zijlstra @ 2016-09-27 12:32 ` Dmitry Vyukov 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Vyukov @ 2016-09-27 12:32 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Vegard Nossum, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On Tue, Sep 27, 2016 at 12:59 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Sep 27, 2016 at 09:50:41AM +0200, Dmitry Vyukov wrote: >> On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> > On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: >> >> >> >> I suspect there is a bunch of places that use in_interrupt(), but mean >> >> the same as KCOV wants -- am I in interrupt? and not am I in interrupt >> >> context or in normal task context but inside local_bh_disable(). For >> >> example, why does fput handles closure asynchronously if the task >> >> called local_bh_disable? >> > >> > Agreed, but it would mean auditing all in_interrupt()/irq_count() users. >> >> >> I don't think this means auditing all users. We are not making things >> worse by introduction of a new predicate. >> It would be nice to look at some uses in core code, but the only place >> with observed harm is KCOV. >> >> Any naming suggestions? Other than really_in_interrupt or >> in_interrupt_and_not_in_bh_disabled? > > Hence the suggestion to audit and fix instead of making a bigger mess :/ Ah, I see what you mean. I am not sure Andrey and me are well equipped to tackle it. We have very narrow kernel experience. Lots of uses are in drivers, which we don't even know how to invoke. Guessing re current state of the things, calling potentially expensive functions with bh disabled is probably bad, but we have no idea as to the threshold. Any ideas regarding a shorter term fix? We have a very real problem in KCOV. We could open code and comment the new check in KCOV. Or maybe do a global s/in_interrupt/in_interrupt_or_bh_disabled/ and then introduce new in_interrupt. I don't know what's the practice with such global changes. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 7:50 ` Dmitry Vyukov 2016-09-27 10:59 ` Peter Zijlstra @ 2016-09-27 11:20 ` Vegard Nossum 2016-09-27 11:22 ` Vegard Nossum 1 sibling, 1 reply; 9+ messages in thread From: Vegard Nossum @ 2016-09-27 11:20 UTC (permalink / raw) To: Dmitry Vyukov, Peter Zijlstra Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On 09/27/2016 09:50 AM, Dmitry Vyukov wrote: > On Tue, Sep 27, 2016 at 9:34 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> On Tue, Sep 27, 2016 at 08:21:32AM +0200, Dmitry Vyukov wrote: >>> >>> I suspect there is a bunch of places that use in_interrupt(), but mean >>> the same as KCOV wants -- am I in interrupt? and not am I in interrupt >>> context or in normal task context but inside local_bh_disable(). For >>> example, why does fput handles closure asynchronously if the task >>> called local_bh_disable? >> >> Agreed, but it would mean auditing all in_interrupt()/irq_count() users. > > > I don't think this means auditing all users. We are not making things > worse by introduction of a new predicate. > It would be nice to look at some uses in core code, but the only place > with observed harm is KCOV. > > Any naming suggestions? Other than really_in_interrupt or > in_interrupt_and_not_in_bh_disabled? > Your patch was: - if (!t || in_interrupt()) + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET + | NMI_MASK))) But look at the definitions: #define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \ | NMI_MASK)) #define in_interrupt() (irq_count()) So isn't the patch a no-op to start with? Vegard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kcov: properly check if we are in an interrupt 2016-09-27 11:20 ` Vegard Nossum @ 2016-09-27 11:22 ` Vegard Nossum 0 siblings, 0 replies; 9+ messages in thread From: Vegard Nossum @ 2016-09-27 11:22 UTC (permalink / raw) To: Dmitry Vyukov, Peter Zijlstra Cc: Andrew Morton, Andrey Konovalov, Nicolai Stange, Andrey Ryabinin, Kees Cook, James Morse, LKML, Quentin Casasnovas, Andrey Ryabinin, Thomas Gleixner, Ingo Molnar On 09/27/2016 01:20 PM, Vegard Nossum wrote: > Your patch was: > > - if (!t || in_interrupt()) > + if (!t || (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_OFFSET > + | NMI_MASK))) > > But look at the definitions: > > #define irq_count() (preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \ > | NMI_MASK)) > #define in_interrupt() (irq_count()) > > So isn't the patch a no-op to start with? Eh, SOFTIRQ_OFFSET vs SOFTIRQ_MASK. I'll just go stand in the corner. Vegard ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-27 12:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-23 14:51 [PATCH] kcov: properly check if we are in an interrupt Andrey Konovalov 2016-09-26 23:32 ` Andrew Morton 2016-09-27 6:21 ` Dmitry Vyukov 2016-09-27 7:34 ` Peter Zijlstra 2016-09-27 7:50 ` Dmitry Vyukov 2016-09-27 10:59 ` Peter Zijlstra 2016-09-27 12:32 ` Dmitry Vyukov 2016-09-27 11:20 ` Vegard Nossum 2016-09-27 11:22 ` Vegard Nossum
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.