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@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <andreyknvl@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.