* [PATCH 2/3] kcov: remove pointless current != NULL check
2017-09-19 12:46 [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Andrey Ryabinin
@ 2017-09-19 12:46 ` Andrey Ryabinin
2017-09-19 12:57 ` Dmitry Vyukov
2017-09-19 13:32 ` Mark Rutland
2017-09-19 12:46 ` [PATCH 3/3] kcov: remove useless barrier()s Andrey Ryabinin
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Andrey Ryabinin @ 2017-09-19 12:46 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: akpm, Andrey Konovalov, tchibo, syzkaller, Mark Rutland,
linux-kernel, Andrey Ryabinin
__sanitizer_cov_trace_pc() is a hot code, so it's worth
to remove pointless '!current' check. Current is never NULL.
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
kernel/kcov.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 2f0e7a7c7afc..14cc8c1a7cad 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -61,7 +61,7 @@ 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_task())
+ if (!in_task())
return;
mode = READ_ONCE(t->kcov_mode);
if (mode == KCOV_MODE_TRACE) {
--
2.13.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] kcov: remove pointless current != NULL check
2017-09-19 12:46 ` [PATCH 2/3] kcov: remove pointless current != NULL check Andrey Ryabinin
@ 2017-09-19 12:57 ` Dmitry Vyukov
2017-09-19 13:32 ` Mark Rutland
1 sibling, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 12:57 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> __sanitizer_cov_trace_pc() is a hot code, so it's worth
> to remove pointless '!current' check. Current is never NULL.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> kernel/kcov.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 2f0e7a7c7afc..14cc8c1a7cad 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -61,7 +61,7 @@ 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_task())
> + if (!in_task())
> return;
> mode = READ_ONCE(t->kcov_mode);
> if (mode == KCOV_MODE_TRACE) {
> --
> 2.13.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] kcov: remove pointless current != NULL check
2017-09-19 12:46 ` [PATCH 2/3] kcov: remove pointless current != NULL check Andrey Ryabinin
2017-09-19 12:57 ` Dmitry Vyukov
@ 2017-09-19 13:32 ` Mark Rutland
1 sibling, 0 replies; 15+ messages in thread
From: Mark Rutland @ 2017-09-19 13:32 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Dmitry Vyukov, akpm, Andrey Konovalov, tchibo, syzkaller, linux-kernel
On Tue, Sep 19, 2017 at 03:46:47PM +0300, Andrey Ryabinin wrote:
> __sanitizer_cov_trace_pc() is a hot code, so it's worth
> to remove pointless '!current' check. Current is never NULL.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
> ---
> kernel/kcov.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 2f0e7a7c7afc..14cc8c1a7cad 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -61,7 +61,7 @@ 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_task())
> + if (!in_task())
> return;
> mode = READ_ONCE(t->kcov_mode);
> if (mode == KCOV_MODE_TRACE) {
> --
> 2.13.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 12:46 [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Andrey Ryabinin
2017-09-19 12:46 ` [PATCH 2/3] kcov: remove pointless current != NULL check Andrey Ryabinin
@ 2017-09-19 12:46 ` Andrey Ryabinin
2017-09-19 12:57 ` Dmitry Vyukov
2017-09-19 13:03 ` [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Dmitry Vyukov
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Andrey Ryabinin @ 2017-09-19 12:46 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: akpm, Andrey Konovalov, tchibo, syzkaller, Mark Rutland,
linux-kernel, Andrey Ryabinin
As comment says barriers needed for preempt_schedule_irq() case
where in_interrupt() returns false. But we don't use in_interrupt()
since b274c0bb394c ("kcov: properly check if we are in an interrupt").
Now we use in_task() which handles preempt_schedule_irq() case properly,
thus no barrier required.
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
kernel/kcov.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 14cc8c1a7cad..b7fbcbef88c1 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
ip -= kaslr_offset();
- /*
- * There is some code that runs in interrupts but for which
- * in_interrupt() returns false (e.g. preempt_schedule_irq()).
- * READ_ONCE()/barrier() effectively provides load-acquire wrt
- * interrupts, there are paired barrier()/WRITE_ONCE() in
- * kcov_ioctl_locked().
- */
- barrier();
area = t->kcov_area;
/* The first word is number of subsequent PCs. */
pos = READ_ONCE(area[0]) + 1;
@@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
/* Cache in task struct for performance. */
t->kcov_size = kcov->size;
t->kcov_area = kcov->area;
- /* See comment in __sanitizer_cov_trace_pc(). */
- barrier();
WRITE_ONCE(t->kcov_mode, kcov->mode);
t->kcov = kcov;
kcov->t = t;
--
2.13.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 12:46 ` [PATCH 3/3] kcov: remove useless barrier()s Andrey Ryabinin
@ 2017-09-19 12:57 ` Dmitry Vyukov
2017-09-19 13:52 ` Andrey Ryabinin
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 12:57 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> As comment says barriers needed for preempt_schedule_irq() case
> where in_interrupt() returns false. But we don't use in_interrupt()
> since b274c0bb394c ("kcov: properly check if we are in an interrupt").
>
> Now we use in_task() which handles preempt_schedule_irq() case properly,
> thus no barrier required.
Are you sure in_task() handles preempt_schedule_irq() correctly?
They seem to differ only by SOFTIRQ_MASK vs SOFTIRQ_OFFSET, and that
only differs in local_bh_disable sections. But preempt_schedule_irq()
does not seem to have anything to do softirq/local_bh_disable. It's
called from real interrupts, right? So I would expect that in_task()
returns true in preempt_schedule_irq().
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> kernel/kcov.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 14cc8c1a7cad..b7fbcbef88c1 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
>
> ip -= kaslr_offset();
>
> - /*
> - * There is some code that runs in interrupts but for which
> - * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> - * READ_ONCE()/barrier() effectively provides load-acquire wrt
> - * interrupts, there are paired barrier()/WRITE_ONCE() in
> - * kcov_ioctl_locked().
> - */
> - barrier();
> area = t->kcov_area;
> /* The first word is number of subsequent PCs. */
> pos = READ_ONCE(area[0]) + 1;
> @@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> /* Cache in task struct for performance. */
> t->kcov_size = kcov->size;
> t->kcov_area = kcov->area;
> - /* See comment in __sanitizer_cov_trace_pc(). */
> - barrier();
> WRITE_ONCE(t->kcov_mode, kcov->mode);
> t->kcov = kcov;
> kcov->t = t;
> --
> 2.13.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 12:57 ` Dmitry Vyukov
@ 2017-09-19 13:52 ` Andrey Ryabinin
2017-09-19 13:54 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Andrey Ryabinin @ 2017-09-19 13:52 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On 09/19/2017 03:57 PM, Dmitry Vyukov wrote:
> On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> As comment says barriers needed for preempt_schedule_irq() case
>> where in_interrupt() returns false. But we don't use in_interrupt()
>> since b274c0bb394c ("kcov: properly check if we are in an interrupt").
>>
>> Now we use in_task() which handles preempt_schedule_irq() case properly,
>> thus no barrier required.
>
>
> Are you sure in_task() handles preempt_schedule_irq() correctly?
> They seem to differ only by SOFTIRQ_MASK vs SOFTIRQ_OFFSET, and that
> only differs in local_bh_disable sections. But preempt_schedule_irq()
> does not seem to have anything to do softirq/local_bh_disable. It's
> called from real interrupts, right? So I would expect that in_task()
> returns true in preempt_schedule_irq().
Indeed, you're right. I checked this only on !PREEMPT kernel, where this worked.
Still, I think that barrier() in __sanitizer_cov_trace_pc() is not needed. AFAIU it needed
to make sure that load of t->kcov_area isn't moved before load of t->kcov_mode, but I don't
think that compiler is allowed to make such reorder. That would be a bug in the compiler.
>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>> kernel/kcov.c | 10 ----------
>> 1 file changed, 10 deletions(-)
>>
>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>> index 14cc8c1a7cad..b7fbcbef88c1 100644
>> --- a/kernel/kcov.c
>> +++ b/kernel/kcov.c
>> @@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
>>
>> ip -= kaslr_offset();
>>
>> - /*
>> - * There is some code that runs in interrupts but for which
>> - * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> - * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> - * interrupts, there are paired barrier()/WRITE_ONCE() in
>> - * kcov_ioctl_locked().
>> - */
>> - barrier();
>> area = t->kcov_area;
>> /* The first word is number of subsequent PCs. */
>> pos = READ_ONCE(area[0]) + 1;
>> @@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>> /* Cache in task struct for performance. */
>> t->kcov_size = kcov->size;
>> t->kcov_area = kcov->area;
>> - /* See comment in __sanitizer_cov_trace_pc(). */
>> - barrier();
>> WRITE_ONCE(t->kcov_mode, kcov->mode);
>> t->kcov = kcov;
>> kcov->t = t;
>> --
>> 2.13.5
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 13:52 ` Andrey Ryabinin
@ 2017-09-19 13:54 ` Dmitry Vyukov
2017-09-19 14:29 ` Andrey Ryabinin
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 13:54 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On Tue, Sep 19, 2017 at 3:52 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 09/19/2017 03:57 PM, Dmitry Vyukov wrote:
>> On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>> As comment says barriers needed for preempt_schedule_irq() case
>>> where in_interrupt() returns false. But we don't use in_interrupt()
>>> since b274c0bb394c ("kcov: properly check if we are in an interrupt").
>>>
>>> Now we use in_task() which handles preempt_schedule_irq() case properly,
>>> thus no barrier required.
>>
>>
>> Are you sure in_task() handles preempt_schedule_irq() correctly?
>> They seem to differ only by SOFTIRQ_MASK vs SOFTIRQ_OFFSET, and that
>> only differs in local_bh_disable sections. But preempt_schedule_irq()
>> does not seem to have anything to do softirq/local_bh_disable. It's
>> called from real interrupts, right? So I would expect that in_task()
>> returns true in preempt_schedule_irq().
>
> Indeed, you're right. I checked this only on !PREEMPT kernel, where this worked.
>
> Still, I think that barrier() in __sanitizer_cov_trace_pc() is not needed. AFAIU it needed
> to make sure that load of t->kcov_area isn't moved before load of t->kcov_mode, but I don't
> think that compiler is allowed to make such reorder. That would be a bug in the compiler.
Why? C compiler is allowed to fuse/reorder loads from the same base
object. Also stores can be reordered.
>>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>> ---
>>> kernel/kcov.c | 10 ----------
>>> 1 file changed, 10 deletions(-)
>>>
>>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>>> index 14cc8c1a7cad..b7fbcbef88c1 100644
>>> --- a/kernel/kcov.c
>>> +++ b/kernel/kcov.c
>>> @@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
>>>
>>> ip -= kaslr_offset();
>>>
>>> - /*
>>> - * There is some code that runs in interrupts but for which
>>> - * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>>> - * READ_ONCE()/barrier() effectively provides load-acquire wrt
>>> - * interrupts, there are paired barrier()/WRITE_ONCE() in
>>> - * kcov_ioctl_locked().
>>> - */
>>> - barrier();
>>> area = t->kcov_area;
>>> /* The first word is number of subsequent PCs. */
>>> pos = READ_ONCE(area[0]) + 1;
>>> @@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>>> /* Cache in task struct for performance. */
>>> t->kcov_size = kcov->size;
>>> t->kcov_area = kcov->area;
>>> - /* See comment in __sanitizer_cov_trace_pc(). */
>>> - barrier();
>>> WRITE_ONCE(t->kcov_mode, kcov->mode);
>>> t->kcov = kcov;
>>> kcov->t = t;
>>> --
>>> 2.13.5
>>>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 13:54 ` Dmitry Vyukov
@ 2017-09-19 14:29 ` Andrey Ryabinin
2017-09-19 14:42 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Andrey Ryabinin @ 2017-09-19 14:29 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On 09/19/2017 04:54 PM, Dmitry Vyukov wrote:
> On Tue, Sep 19, 2017 at 3:52 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 09/19/2017 03:57 PM, Dmitry Vyukov wrote:
>>> On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>>>> As comment says barriers needed for preempt_schedule_irq() case
>>>> where in_interrupt() returns false. But we don't use in_interrupt()
>>>> since b274c0bb394c ("kcov: properly check if we are in an interrupt").
>>>>
>>>> Now we use in_task() which handles preempt_schedule_irq() case properly,
>>>> thus no barrier required.
>>>
>>>
>>> Are you sure in_task() handles preempt_schedule_irq() correctly?
>>> They seem to differ only by SOFTIRQ_MASK vs SOFTIRQ_OFFSET, and that
>>> only differs in local_bh_disable sections. But preempt_schedule_irq()
>>> does not seem to have anything to do softirq/local_bh_disable. It's
>>> called from real interrupts, right? So I would expect that in_task()
>>> returns true in preempt_schedule_irq().
>>
>> Indeed, you're right. I checked this only on !PREEMPT kernel, where this worked.
>>
>> Still, I think that barrier() in __sanitizer_cov_trace_pc() is not needed. AFAIU it needed
>> to make sure that load of t->kcov_area isn't moved before load of t->kcov_mode, but I don't
>> think that compiler is allowed to make such reorder. That would be a bug in the compiler.
>
Ugh, it should have saied READ_ONCE(area[0]) instead of t->kcov_area.
>
> Why? C compiler is allowed to fuse/reorder loads from the same base
> object. Also stores can be reordered.
>
Ok, right. t->kcov_area can be loaded before t->kcov_mode, and it's fine. But deference of the kcov_area
(READ_ONCE(area[0])) can't be moved before kcov_mode check. And this barrier intended to prevent such move, right?
>
>>>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>>> ---
>>>> kernel/kcov.c | 10 ----------
>>>> 1 file changed, 10 deletions(-)
>>>>
>>>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>>>> index 14cc8c1a7cad..b7fbcbef88c1 100644
>>>> --- a/kernel/kcov.c
>>>> +++ b/kernel/kcov.c
>>>> @@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
>>>>
>>>> ip -= kaslr_offset();
>>>>
>>>> - /*
>>>> - * There is some code that runs in interrupts but for which
>>>> - * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>>>> - * READ_ONCE()/barrier() effectively provides load-acquire wrt
>>>> - * interrupts, there are paired barrier()/WRITE_ONCE() in
>>>> - * kcov_ioctl_locked().
>>>> - */
>>>> - barrier();
>>>> area = t->kcov_area;
>>>> /* The first word is number of subsequent PCs. */
>>>> pos = READ_ONCE(area[0]) + 1;
>>>> @@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>>>> /* Cache in task struct for performance. */
>>>> t->kcov_size = kcov->size;
>>>> t->kcov_area = kcov->area;
>>>> - /* See comment in __sanitizer_cov_trace_pc(). */
>>>> - barrier();
>>>> WRITE_ONCE(t->kcov_mode, kcov->mode);
>>>> t->kcov = kcov;
>>>> kcov->t = t;
>>>> --
>>>> 2.13.5
>>>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] kcov: remove useless barrier()s
2017-09-19 14:29 ` Andrey Ryabinin
@ 2017-09-19 14:42 ` Dmitry Vyukov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 14:42 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Andrey Konovalov, syzkaller, Mark Rutland, LKML
On Tue, Sep 19, 2017 at 4:29 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> On 09/19/2017 04:54 PM, Dmitry Vyukov wrote:
>> On Tue, Sep 19, 2017 at 3:52 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>>
>>> On 09/19/2017 03:57 PM, Dmitry Vyukov wrote:
>>>> On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
>>>> <aryabinin@virtuozzo.com> wrote:
>>>>> As comment says barriers needed for preempt_schedule_irq() case
>>>>> where in_interrupt() returns false. But we don't use in_interrupt()
>>>>> since b274c0bb394c ("kcov: properly check if we are in an interrupt").
>>>>>
>>>>> Now we use in_task() which handles preempt_schedule_irq() case properly,
>>>>> thus no barrier required.
>>>>
>>>>
>>>> Are you sure in_task() handles preempt_schedule_irq() correctly?
>>>> They seem to differ only by SOFTIRQ_MASK vs SOFTIRQ_OFFSET, and that
>>>> only differs in local_bh_disable sections. But preempt_schedule_irq()
>>>> does not seem to have anything to do softirq/local_bh_disable. It's
>>>> called from real interrupts, right? So I would expect that in_task()
>>>> returns true in preempt_schedule_irq().
>>>
>>> Indeed, you're right. I checked this only on !PREEMPT kernel, where this worked.
>>>
>>> Still, I think that barrier() in __sanitizer_cov_trace_pc() is not needed. AFAIU it needed
>>> to make sure that load of t->kcov_area isn't moved before load of t->kcov_mode, but I don't
>>> think that compiler is allowed to make such reorder. That would be a bug in the compiler.
>>
>
> Ugh, it should have saied READ_ONCE(area[0]) instead of t->kcov_area.
>
>>
>> Why? C compiler is allowed to fuse/reorder loads from the same base
>> object. Also stores can be reordered.
>>
>
> Ok, right. t->kcov_area can be loaded before t->kcov_mode, and it's fine.
> But deference of the kcov_area
> (READ_ONCE(area[0])) can't be moved before kcov_mode check. And this barrier intended to prevent such move, right?
We want to prevent reordering of accesses to t->kcov_mode and
t->kcov_area to prevent situation when an interrupt reads
t->kcov_mode=KCOV_MODE_TRACE and t->kcov_area=NULL and crashes trying
to dereference area.
If we want to play on the edge we could remove _one_ of the barriers
-- the one in __sanitizer_cov_trace_pc. I wanted to model the usual
release/acquire synchronization pair, which is more common and easier
to understand. But interrupts are more restricted than normal threads.
Namely, an interrupt can't be intermixed with host thread execution,
it can execute only all at once at an arbitrary point in host thread.
So the following situation is _not_ possible: an interrupt loads
t->kcov_area=NULL, then host thread executes ioctl and sets both
fields, then interrupt resumes and loads t->kcov_mode=KCOV_MODE_TRACE.
So, it's only important for ioctl to set them in the correct order.
>>>>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>>>> ---
>>>>> kernel/kcov.c | 10 ----------
>>>>> 1 file changed, 10 deletions(-)
>>>>>
>>>>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>>>>> index 14cc8c1a7cad..b7fbcbef88c1 100644
>>>>> --- a/kernel/kcov.c
>>>>> +++ b/kernel/kcov.c
>>>>> @@ -71,14 +71,6 @@ void notrace __sanitizer_cov_trace_pc(void)
>>>>>
>>>>> ip -= kaslr_offset();
>>>>>
>>>>> - /*
>>>>> - * There is some code that runs in interrupts but for which
>>>>> - * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>>>>> - * READ_ONCE()/barrier() effectively provides load-acquire wrt
>>>>> - * interrupts, there are paired barrier()/WRITE_ONCE() in
>>>>> - * kcov_ioctl_locked().
>>>>> - */
>>>>> - barrier();
>>>>> area = t->kcov_area;
>>>>> /* The first word is number of subsequent PCs. */
>>>>> pos = READ_ONCE(area[0]) + 1;
>>>>> @@ -228,8 +220,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>>>>> /* Cache in task struct for performance. */
>>>>> t->kcov_size = kcov->size;
>>>>> t->kcov_area = kcov->area;
>>>>> - /* See comment in __sanitizer_cov_trace_pc(). */
>>>>> - barrier();
>>>>> WRITE_ONCE(t->kcov_mode, kcov->mode);
>>>>> t->kcov = kcov;
>>>>> kcov->t = t;
>>>>> --
>>>>> 2.13.5
>>>>>
>>>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE
2017-09-19 12:46 [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Andrey Ryabinin
2017-09-19 12:46 ` [PATCH 2/3] kcov: remove pointless current != NULL check Andrey Ryabinin
2017-09-19 12:46 ` [PATCH 3/3] kcov: remove useless barrier()s Andrey Ryabinin
@ 2017-09-19 13:03 ` Dmitry Vyukov
2017-09-19 13:30 ` Mark Rutland
2017-09-21 22:16 ` kbuild test robot
4 siblings, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 13:03 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Andrey Konovalov, Victor Chibotaru, syzkaller,
Mark Rutland, LKML
On Tue, Sep 19, 2017 at 2:46 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> There is no need to surround kaslr_offset() with CONFIG_RANDOMIZE_BASE ifdef.
> kaslr_offset() will just return 0 if CONFIG_RANDOMIZE_BASE isn't set.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
As you said this is a very hot function, but this changes code from:
2e: 48 8b 75 08 mov 0x8(%rbp),%rsi
to:
2b: 48 c7 c2 00 00 00 81 mov $0xffffffff81000000,%rdx
32: 48 81 ea 00 00 00 00 sub $0x0,%rdx
3c: 48 03 55 08 add 0x8(%rbp),%rdx
This also changes semantics, as returned PCs won't match nm/objdump output.
> ---
> kernel/kcov.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 3f693a0f6f3e..2f0e7a7c7afc 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -69,9 +69,7 @@ void notrace __sanitizer_cov_trace_pc(void)
> unsigned long pos;
> unsigned long ip = _RET_IP_;
>
> -#ifdef CONFIG_RANDOMIZE_BASE
> ip -= kaslr_offset();
> -#endif
>
> /*
> * There is some code that runs in interrupts but for which
> --
> 2.13.5
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE
2017-09-19 12:46 [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Andrey Ryabinin
` (2 preceding siblings ...)
2017-09-19 13:03 ` [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Dmitry Vyukov
@ 2017-09-19 13:30 ` Mark Rutland
2017-09-19 13:47 ` Dmitry Vyukov
2017-09-21 22:16 ` kbuild test robot
4 siblings, 1 reply; 15+ messages in thread
From: Mark Rutland @ 2017-09-19 13:30 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Dmitry Vyukov, akpm, Andrey Konovalov, tchibo, syzkaller, linux-kernel
Hi,
On Tue, Sep 19, 2017 at 03:46:46PM +0300, Andrey Ryabinin wrote:
> There is no need to surround kaslr_offset() with CONFIG_RANDOMIZE_BASE ifdef.
> kaslr_offset() will just return 0 if CONFIG_RANDOMIZE_BASE isn't set.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> kernel/kcov.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 3f693a0f6f3e..2f0e7a7c7afc 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -69,9 +69,7 @@ void notrace __sanitizer_cov_trace_pc(void)
> unsigned long pos;
> unsigned long ip = _RET_IP_;
>
> -#ifdef CONFIG_RANDOMIZE_BASE
> ip -= kaslr_offset();
> -#endif
I think this is sound, but as Dmitry points out it'll mean we do some
pointless work. For example on arm64 we have:
static inline unsigned long kaslr_offset(void)
{
return kimage_vaddr - KIMAGE_VADDR;
}
... where kimage_vaddr is a global variable, and KIMAGE_VADDR is a
constant (and should be identical for !CONFIG_RANDOMIZE_BASE kernels).
I think it would be reasonable to make that:
static inline unsigned long kaslr_offset(void)
{
if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
return 0;
return kimage_vaddr - KIMAGE_VADDR;
}
... and simplify callers as above.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE
2017-09-19 13:30 ` Mark Rutland
@ 2017-09-19 13:47 ` Dmitry Vyukov
2017-09-29 16:20 ` Andrey Ryabinin
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2017-09-19 13:47 UTC (permalink / raw)
To: Mark Rutland
Cc: Andrey Ryabinin, Andrew Morton, Andrey Konovalov,
Victor Chibotaru, syzkaller, LKML
On Tue, Sep 19, 2017 at 3:30 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Tue, Sep 19, 2017 at 03:46:46PM +0300, Andrey Ryabinin wrote:
>> There is no need to surround kaslr_offset() with CONFIG_RANDOMIZE_BASE ifdef.
>> kaslr_offset() will just return 0 if CONFIG_RANDOMIZE_BASE isn't set.
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>> kernel/kcov.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>> index 3f693a0f6f3e..2f0e7a7c7afc 100644
>> --- a/kernel/kcov.c
>> +++ b/kernel/kcov.c
>> @@ -69,9 +69,7 @@ void notrace __sanitizer_cov_trace_pc(void)
>> unsigned long pos;
>> unsigned long ip = _RET_IP_;
>>
>> -#ifdef CONFIG_RANDOMIZE_BASE
>> ip -= kaslr_offset();
>> -#endif
>
> I think this is sound, but as Dmitry points out it'll mean we do some
> pointless work. For example on arm64 we have:
>
> static inline unsigned long kaslr_offset(void)
> {
> return kimage_vaddr - KIMAGE_VADDR;
> }
>
> ... where kimage_vaddr is a global variable, and KIMAGE_VADDR is a
> constant (and should be identical for !CONFIG_RANDOMIZE_BASE kernels).
>
> I think it would be reasonable to make that:
>
> static inline unsigned long kaslr_offset(void)
> {
> if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> return 0;
>
> return kimage_vaddr - KIMAGE_VADDR;
> }
>
> ... and simplify callers as above.
Sounds reasonable to me.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE
2017-09-19 13:47 ` Dmitry Vyukov
@ 2017-09-29 16:20 ` Andrey Ryabinin
0 siblings, 0 replies; 15+ messages in thread
From: Andrey Ryabinin @ 2017-09-29 16:20 UTC (permalink / raw)
To: Dmitry Vyukov, Mark Rutland
Cc: Andrew Morton, Andrey Konovalov, syzkaller, LKML
On 09/19/2017 04:47 PM, Dmitry Vyukov wrote:
> On Tue, Sep 19, 2017 at 3:30 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi,
>>
>> On Tue, Sep 19, 2017 at 03:46:46PM +0300, Andrey Ryabinin wrote:
>>> There is no need to surround kaslr_offset() with CONFIG_RANDOMIZE_BASE ifdef.
>>> kaslr_offset() will just return 0 if CONFIG_RANDOMIZE_BASE isn't set.
>>>
>>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>> ---
>>> kernel/kcov.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>>> index 3f693a0f6f3e..2f0e7a7c7afc 100644
>>> --- a/kernel/kcov.c
>>> +++ b/kernel/kcov.c
>>> @@ -69,9 +69,7 @@ void notrace __sanitizer_cov_trace_pc(void)
>>> unsigned long pos;
>>> unsigned long ip = _RET_IP_;
>>>
>>> -#ifdef CONFIG_RANDOMIZE_BASE
>>> ip -= kaslr_offset();
>>> -#endif
>>
>> I think this is sound, but as Dmitry points out it'll mean we do some
>> pointless work. For example on arm64 we have:
>>
>> static inline unsigned long kaslr_offset(void)
>> {
>> return kimage_vaddr - KIMAGE_VADDR;
>> }
>>
>> ... where kimage_vaddr is a global variable, and KIMAGE_VADDR is a
>> constant (and should be identical for !CONFIG_RANDOMIZE_BASE kernels).
>>
>> I think it would be reasonable to make that:
>>
>> static inline unsigned long kaslr_offset(void)
>> {
>> if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
>> return 0;
>>
>> return kimage_vaddr - KIMAGE_VADDR;
>> }
>>
>> ... and simplify callers as above.
>
> Sounds reasonable to me.
>
As kbuilt robot pointed out, we would also need to introduce generic kaslr_offset() for arches
that don't have it. I seem like too much trouble for very little gain, so I'll keep current code as is.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE
2017-09-19 12:46 [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE Andrey Ryabinin
` (3 preceding siblings ...)
2017-09-19 13:30 ` Mark Rutland
@ 2017-09-21 22:16 ` kbuild test robot
4 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2017-09-21 22:16 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: kbuild-all, Dmitry Vyukov, akpm, Andrey Konovalov, tchibo,
syzkaller, Mark Rutland, linux-kernel, Andrey Ryabinin
[-- Attachment #1: Type: text/plain, Size: 4326 bytes --]
Hi Andrey,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc1 next-20170921]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andrey-Ryabinin/kcov-remove-ifdef-CONFIG_RANDOMIZE_BASE/20170922-035755
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
kernel/kcov.c: In function '__sanitizer_cov_trace_pc':
>> kernel/kcov.c:72:9: error: implicit declaration of function 'kaslr_offset' [-Werror=implicit-function-declaration]
ip -= kaslr_offset();
^~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/kaslr_offset +72 kernel/kcov.c
5c9a8750 Dmitry Vyukov 2016-03-22 49
5c9a8750 Dmitry Vyukov 2016-03-22 50 /*
5c9a8750 Dmitry Vyukov 2016-03-22 51 * Entry point from instrumented code.
5c9a8750 Dmitry Vyukov 2016-03-22 52 * This is called once per basic-block/edge.
5c9a8750 Dmitry Vyukov 2016-03-22 53 */
bdab42df James Morse 2016-04-28 54 void notrace __sanitizer_cov_trace_pc(void)
5c9a8750 Dmitry Vyukov 2016-03-22 55 {
5c9a8750 Dmitry Vyukov 2016-03-22 56 struct task_struct *t;
5c9a8750 Dmitry Vyukov 2016-03-22 57 enum kcov_mode mode;
5c9a8750 Dmitry Vyukov 2016-03-22 58
5c9a8750 Dmitry Vyukov 2016-03-22 59 t = current;
5c9a8750 Dmitry Vyukov 2016-03-22 60 /*
5c9a8750 Dmitry Vyukov 2016-03-22 61 * We are interested in code coverage as a function of a syscall inputs,
5c9a8750 Dmitry Vyukov 2016-03-22 62 * so we ignore code executed in interrupts.
5c9a8750 Dmitry Vyukov 2016-03-22 63 */
f61e869d Dmitry Vyukov 2017-05-08 64 if (!t || !in_task())
5c9a8750 Dmitry Vyukov 2016-03-22 65 return;
5c9a8750 Dmitry Vyukov 2016-03-22 66 mode = READ_ONCE(t->kcov_mode);
5c9a8750 Dmitry Vyukov 2016-03-22 67 if (mode == KCOV_MODE_TRACE) {
5c9a8750 Dmitry Vyukov 2016-03-22 68 unsigned long *area;
5c9a8750 Dmitry Vyukov 2016-03-22 69 unsigned long pos;
4983f0ab Alexander Popov 2016-12-19 70 unsigned long ip = _RET_IP_;
4983f0ab Alexander Popov 2016-12-19 71
4983f0ab Alexander Popov 2016-12-19 @72 ip -= kaslr_offset();
5c9a8750 Dmitry Vyukov 2016-03-22 73
5c9a8750 Dmitry Vyukov 2016-03-22 74 /*
5c9a8750 Dmitry Vyukov 2016-03-22 75 * There is some code that runs in interrupts but for which
5c9a8750 Dmitry Vyukov 2016-03-22 76 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
5c9a8750 Dmitry Vyukov 2016-03-22 77 * READ_ONCE()/barrier() effectively provides load-acquire wrt
5c9a8750 Dmitry Vyukov 2016-03-22 78 * interrupts, there are paired barrier()/WRITE_ONCE() in
5c9a8750 Dmitry Vyukov 2016-03-22 79 * kcov_ioctl_locked().
5c9a8750 Dmitry Vyukov 2016-03-22 80 */
5c9a8750 Dmitry Vyukov 2016-03-22 81 barrier();
5c9a8750 Dmitry Vyukov 2016-03-22 82 area = t->kcov_area;
5c9a8750 Dmitry Vyukov 2016-03-22 83 /* The first word is number of subsequent PCs. */
5c9a8750 Dmitry Vyukov 2016-03-22 84 pos = READ_ONCE(area[0]) + 1;
5c9a8750 Dmitry Vyukov 2016-03-22 85 if (likely(pos < t->kcov_size)) {
4983f0ab Alexander Popov 2016-12-19 86 area[pos] = ip;
5c9a8750 Dmitry Vyukov 2016-03-22 87 WRITE_ONCE(area[0], pos);
5c9a8750 Dmitry Vyukov 2016-03-22 88 }
5c9a8750 Dmitry Vyukov 2016-03-22 89 }
5c9a8750 Dmitry Vyukov 2016-03-22 90 }
5c9a8750 Dmitry Vyukov 2016-03-22 91 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
5c9a8750 Dmitry Vyukov 2016-03-22 92
:::::: The code at line 72 was first introduced by commit
:::::: 4983f0ab7ffaad1e534b21975367429736475205 kcov: make kcov work properly with KASLR enabled
:::::: TO: Alexander Popov <alex.popov@linux.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47659 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread