[PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[Andrey Ryabinin]

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
 
 		/*
 		 * There is some code that runs in interrupts but for which
-- 
2.13.5

[PATCH 2/3] kcov: remove pointless current != NULL check

[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

[PATCH 3/3] kcov: remove useless barrier()s

[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

Re: [PATCH 3/3] kcov: remove useless barrier()s

[Dmitry Vyukov]

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
>

Re: [PATCH 2/3] kcov: remove pointless current != NULL check

[Dmitry Vyukov]

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
>

Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[Dmitry Vyukov]

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
>

Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[Mark Rutland]

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.

Re: [PATCH 2/3] kcov: remove pointless current != NULL check

[Mark Rutland]

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
> 

Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[Dmitry Vyukov]

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.

Re: [PATCH 3/3] kcov: remove useless barrier()s

[Andrey Ryabinin]

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

Re: [PATCH 3/3] kcov: remove useless barrier()s

[Dmitry Vyukov]

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.

Re: [PATCH 3/3] kcov: remove useless barrier()s

[Andrey Ryabinin]

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

Re: [PATCH 3/3] kcov: remove useless barrier()s

[Dmitry Vyukov]

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.

Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[kbuild test robot]

[-- 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 --]

Re: [PATCH 1/3] kcov: remove #ifdef CONFIG_RANDOMIZE_BASE

[Andrey Ryabinin]

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.