Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490213 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] kcov: properly check if we are in an interrupt Andrey Konovalov <andreyknvl@google.com> - 2016-09-23 17:00 +0200
    Re: [PATCH] kcov: properly check if we are in an interrupt Andrew Morton <akpm@linux-foundation.org> - 2016-09-27 01:40 +0200
      Re: [PATCH] kcov: properly check if we are in an interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-09-27 08:30 +0200
        Re: [PATCH] kcov: properly check if we are in an interrupt Peter Zijlstra <peterz@infradead.org> - 2016-09-27 09:40 +0200
          Re: [PATCH] kcov: properly check if we are in an interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-09-27 10:00 +0200
            Re: [PATCH] kcov: properly check if we are in an interrupt Peter Zijlstra <peterz@infradead.org> - 2016-09-27 13:00 +0200
              Re: [PATCH] kcov: properly check if we are in an interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-09-27 14:40 +0200
            Re: [PATCH] kcov: properly check if we are in an interrupt Vegard Nossum <vegard.nossum@oracle.com> - 2016-09-27 13:30 +0200
              Re: [PATCH] kcov: properly check if we are in an interrupt Vegard Nossum <vegard.nossum@oracle.com> - 2016-09-27 13:30 +0200

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

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

[toc] | [next] | [standalone]

#1491603

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.

[toc] | [prev] | [next] | [standalone]

#1491703

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.

[toc] | [prev] | [next] | [standalone]

#1491718

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.

[toc] | [prev] | [next] | [standalone]

#1491741

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?

[toc] | [prev] | [next] | [standalone]

#1491815

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

[toc] | [prev] | [next] | [standalone]

#1491884

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.

[toc] | [prev] | [next] | [standalone]

#1491824

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

[toc] | [prev] | [next] | [standalone]

#1491828

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web