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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+ZdxBwExjgxWqF8EwYR9AsmcVo-z7TypjiG9MMHedPSsw@mail.gmail.com>
Date:   Tue, 27 Sep 2016 08:21:32 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Andrey Konovalov <andreyknvl@...gle.com>,
        Nicolai Stange <nicstange@...il.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Kees Cook <keescook@...omium.org>,
        James Morse <james.morse@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Quentin Casasnovas <quentin.casasnovas@...cle.com>,
        Andrey Ryabinin <ryabinin.a.a@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...e.hu>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH] kcov: properly check if we are in an interrupt

On Tue, Sep 27, 2016 at 1:32 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Fri, 23 Sep 2016 16:51:13 +0200 Andrey Konovalov <andreyknvl@...gle.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@...gle.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.