[PATCH] kcov: properly check if we are in an interrupt

[PATCH] kcov: properly check if we are in an interrupt

[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

Re: [PATCH] kcov: properly check if we are in an interrupt

[Andrew Morton]

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.

Re: [PATCH] kcov: properly check if we are in an interrupt

[Dmitry Vyukov]

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.

Re: [PATCH] kcov: properly check if we are in an interrupt

[Peter Zijlstra]

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.

Re: [PATCH] kcov: properly check if we are in an interrupt

[Dmitry Vyukov]

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?

Re: [PATCH] kcov: properly check if we are in an interrupt

[Peter Zijlstra]

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 :/

Re: [PATCH] kcov: properly check if we are in an interrupt

[Vegard Nossum]

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

Re: [PATCH] kcov: properly check if we are in an interrupt

[Vegard Nossum]

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

Re: [PATCH] kcov: properly check if we are in an interrupt

[Dmitry Vyukov]

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.