[PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[Sebastian Andrzej Siewior]

The last patch in series is follow-up to address the PREEMPT_RT issue
within in kcov reported by Clark [0].
Patches 1-3 are smaller things that I noticed while staring at it.
Patch 4 is small change which makes replacement in #5 simpler / more
obvious.
I tested this with the three examples in the documentation folder and I
didn't notice higher latency with kcov enabled. Debug or not, I don't
see a reason to make the lock a raw_spin_lock_t annd it would complicate
memory allocation as mentioned in #5.

One thing I noticed and have no idea if this is right or not:
The code seems to mix long and uint64_t for the reported instruction
pointer / position in the buffer. For instance
__sanitizer_cov_trace_pc() refers to a 64bit pointer (in the comment)
while the area pointer itself is (long *). The problematic part is that
a 32bit application on a 64bit pointer will expect a four byte pointer
while kernel uses an eight byte pointer.

[0] https://lkml.kernel.org/r/20210809155909.333073de@theseus.lan

Sebastian

[PATCH 1/5] Documentation/kcov: Include types.h in the example.

[Sebastian Andrzej Siewior]

The first example code has includes at the top, the following two
example share that part. The last example (remote coverage collection)
requires the linux/types.h header file due its __aligned_u64 usage.

Add the linux/types.h to the top most example and a comment that the
header files from above are required as it is done in the second
example.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 Documentation/dev-tools/kcov.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index d2c4c27e1702d..347f3b6de8d40 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -50,6 +50,7 @@ The following program demonstrates coverage collection from within a test
     #include <sys/mman.h>
     #include <unistd.h>
     #include <fcntl.h>
+    #include <linux/types.h>
 
     #define KCOV_INIT_TRACE			_IOR('c', 1, unsigned long)
     #define KCOV_ENABLE			_IO('c', 100)
@@ -251,6 +252,8 @@ selectively from different subsystems.
 
 .. code-block:: c
 
+    /* Same includes and defines as above. */
+
     struct kcov_remote_arg {
 	__u32		trace_mode;
 	__u32		area_size;
-- 
2.33.0

Re: [PATCH 1/5] Documentation/kcov: Include types.h in the example.

[Dmitry Vyukov]

On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The first example code has includes at the top, the following two
> example share that part. The last example (remote coverage collection)
> requires the linux/types.h header file due its __aligned_u64 usage.
>
> Add the linux/types.h to the top most example and a comment that the
> header files from above are required as it is done in the second
> example.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Vyukov <dvyukov@google.com>


> ---
>  Documentation/dev-tools/kcov.rst | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index d2c4c27e1702d..347f3b6de8d40 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -50,6 +50,7 @@ The following program demonstrates coverage collection from within a test
>      #include <sys/mman.h>
>      #include <unistd.h>
>      #include <fcntl.h>
> +    #include <linux/types.h>
>
>      #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
>      #define KCOV_ENABLE                        _IO('c', 100)
> @@ -251,6 +252,8 @@ selectively from different subsystems.
>
>  .. code-block:: c
>
> +    /* Same includes and defines as above. */
> +
>      struct kcov_remote_arg {
>         __u32           trace_mode;
>         __u32           area_size;
> --
> 2.33.0
>

[PATCH 2/5] Documentation/kcov: Define `ip' in the example.

[Sebastian Andrzej Siewior]

The example code uses the variable `ip' but never declares it.

Declare `ip' as a 64bit variable which is the same type as the array
from which it loads its value.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 Documentation/dev-tools/kcov.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index 347f3b6de8d40..d83c9ab494275 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -178,6 +178,8 @@ Comparison operands collection
 	/* Read number of comparisons collected. */
 	n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
 	for (i = 0; i < n; i++) {
+		uint64_t ip;
+
 		type = cover[i * KCOV_WORDS_PER_CMP + 1];
 		/* arg1 and arg2 - operands of the comparison. */
 		arg1 = cover[i * KCOV_WORDS_PER_CMP + 2];
-- 
2.33.0

Re: [PATCH 2/5] Documentation/kcov: Define `ip' in the example.

[Dmitry Vyukov]

On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The example code uses the variable `ip' but never declares it.
>
> Declare `ip' as a 64bit variable which is the same type as the array
> from which it loads its value.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
>  Documentation/dev-tools/kcov.rst | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index 347f3b6de8d40..d83c9ab494275 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -178,6 +178,8 @@ Comparison operands collection
>         /* Read number of comparisons collected. */
>         n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
>         for (i = 0; i < n; i++) {
> +               uint64_t ip;
> +
>                 type = cover[i * KCOV_WORDS_PER_CMP + 1];
>                 /* arg1 and arg2 - operands of the comparison. */
>                 arg1 = cover[i * KCOV_WORDS_PER_CMP + 2];
> --
> 2.33.0
>

[PATCH 3/5] kcov: Allocate per-CPU memory on the relevant node.

[Sebastian Andrzej Siewior]

During boot kcov allocates per-CPU memory which is used later if remote/
softirq processing is enabled.

Allocate the per-CPU memory on the CPU local node to avoid cross node
memory access.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kcov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 80bfe71bbe13e..4f910231d99a2 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -1034,8 +1034,8 @@ static int __init kcov_init(void)
 	int cpu;
 
 	for_each_possible_cpu(cpu) {
-		void *area = vmalloc(CONFIG_KCOV_IRQ_AREA_SIZE *
-				sizeof(unsigned long));
+		void *area = vmalloc_node(CONFIG_KCOV_IRQ_AREA_SIZE *
+				sizeof(unsigned long), cpu_to_node(cpu));
 		if (!area)
 			return -ENOMEM;
 		per_cpu_ptr(&kcov_percpu_data, cpu)->irq_area = area;
-- 
2.33.0

Re: [PATCH 3/5] kcov: Allocate per-CPU memory on the relevant node.

[Dmitry Vyukov]

On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> During boot kcov allocates per-CPU memory which is used later if remote/
> softirq processing is enabled.
>
> Allocate the per-CPU memory on the CPU local node to avoid cross node
> memory access.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
>  kernel/kcov.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 80bfe71bbe13e..4f910231d99a2 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -1034,8 +1034,8 @@ static int __init kcov_init(void)
>         int cpu;
>
>         for_each_possible_cpu(cpu) {
> -               void *area = vmalloc(CONFIG_KCOV_IRQ_AREA_SIZE *
> -                               sizeof(unsigned long));
> +               void *area = vmalloc_node(CONFIG_KCOV_IRQ_AREA_SIZE *
> +                               sizeof(unsigned long), cpu_to_node(cpu));
>                 if (!area)
>                         return -ENOMEM;
>                 per_cpu_ptr(&kcov_percpu_data, cpu)->irq_area = area;
> --
> 2.33.0
>

[PATCH 4/5] kcov: Avoid enable+disable interrupts if !in_task().

[Sebastian Andrzej Siewior]

kcov_remote_start() may need to allocate memory in the in_task() case
(otherwise per-CPU memory has been pre-allocated) and therefore requires
enabled interrupts.
The interrupts are enabled before checking if the allocation is required
so if no allocation is required then the interrupts are needlessly
enabled and disabled again.

Enable interrupts only if memory allocation is performed.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kcov.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 4f910231d99a2..620dc4ffeb685 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -869,19 +869,19 @@ void kcov_remote_start(u64 handle)
 		size = CONFIG_KCOV_IRQ_AREA_SIZE;
 		area = this_cpu_ptr(&kcov_percpu_data)->irq_area;
 	}
-	spin_unlock_irqrestore(&kcov_remote_lock, flags);
+	spin_unlock(&kcov_remote_lock);
 
 	/* Can only happen when in_task(). */
 	if (!area) {
+		local_irqrestore(flags);
 		area = vmalloc(size * sizeof(unsigned long));
 		if (!area) {
 			kcov_put(kcov);
 			return;
 		}
+		local_irq_save(flags);
 	}
 
-	local_irq_save(flags);
-
 	/* Reset coverage size. */
 	*(u64 *)area = 0;
 
-- 
2.33.0

Re: [PATCH 4/5] kcov: Avoid enable+disable interrupts if !in_task().

[Dmitry Vyukov]

On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> kcov_remote_start() may need to allocate memory in the in_task() case
> (otherwise per-CPU memory has been pre-allocated) and therefore requires
> enabled interrupts.
> The interrupts are enabled before checking if the allocation is required
> so if no allocation is required then the interrupts are needlessly
> enabled and disabled again.
>
> Enable interrupts only if memory allocation is performed.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
>  kernel/kcov.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 4f910231d99a2..620dc4ffeb685 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -869,19 +869,19 @@ void kcov_remote_start(u64 handle)
>                 size = CONFIG_KCOV_IRQ_AREA_SIZE;
>                 area = this_cpu_ptr(&kcov_percpu_data)->irq_area;
>         }
> -       spin_unlock_irqrestore(&kcov_remote_lock, flags);
> +       spin_unlock(&kcov_remote_lock);
>
>         /* Can only happen when in_task(). */
>         if (!area) {
> +               local_irqrestore(flags);
>                 area = vmalloc(size * sizeof(unsigned long));
>                 if (!area) {
>                         kcov_put(kcov);
>                         return;
>                 }
> +               local_irq_save(flags);
>         }
>
> -       local_irq_save(flags);
> -
>         /* Reset coverage size. */
>         *(u64 *)area = 0;
>
> --
> 2.33.0
>

[PATCH 5/5] kcov: Replace local_irq_save() with a local_lock_t.

[Sebastian Andrzej Siewior]

The kcov code mixes local_irq_save() and spin_lock() in
kcov_remote_{start|end}(). This creates a warning on PREEMPT_RT because
local_irq_save() disables interrupts and spin_lock_t is turned into a
sleeping lock which can not be acquired in a section with disabled
interrupts.

The kcov_remote_lock is used to synchronize the access to the hash-list
kcov_remote_map. The local_irq_save() block protects access to the
per-CPU data kcov_percpu_data.

There no compelling reason to change the lock type to raw_spin_lock_t to
make it work with local_irq_save(). Changing it would require to move
memory allocation (in kcov_remote_add()) and deallocation outside of the
locked section.
Adding an unlimited amount of entries to the hashlist will increase the
IRQ-off time during lookup. It could be argued that this is debug code
and the latency does not matter. There is however no need to do so and
it would allow to use this facility in an RT enabled build.

Using a local_lock_t instead of local_irq_save() has the befit of adding
a protection scope within the source which makes it obvious what is
protected. On a !PREEMPT_RT && !LOCKDEP build the local_lock_irqsave()
maps directly to local_irq_save() so there is overhead at runtime.

Replace the local_irq_save() section with a local_lock_t.

Reported-by: Clark Williams <williams@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kcov.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 620dc4ffeb685..36ca640c4f8e7 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -88,6 +88,7 @@ static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas);
 
 struct kcov_percpu_data {
 	void			*irq_area;
+	local_lock_t		lock;
 
 	unsigned int		saved_mode;
 	unsigned int		saved_size;
@@ -96,7 +97,9 @@ struct kcov_percpu_data {
 	int			saved_sequence;
 };
 
-static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data);
+static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {
+	.lock = INIT_LOCAL_LOCK(lock),
+};
 
 /* Must be called with kcov_remote_lock locked. */
 static struct kcov_remote *kcov_remote_find(u64 handle)
@@ -824,7 +827,7 @@ void kcov_remote_start(u64 handle)
 	if (!in_task() && !in_serving_softirq())
 		return;
 
-	local_irq_save(flags);
+	local_lock_irqsave(&kcov_percpu_data.lock, flags);
 
 	/*
 	 * Check that kcov_remote_start() is not called twice in background
@@ -832,7 +835,7 @@ void kcov_remote_start(u64 handle)
 	 */
 	mode = READ_ONCE(t->kcov_mode);
 	if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 	/*
@@ -841,14 +844,15 @@ void kcov_remote_start(u64 handle)
 	 * happened while collecting coverage from a background thread.
 	 */
 	if (WARN_ON(in_serving_softirq() && t->kcov_softirq)) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 
 	spin_lock(&kcov_remote_lock);
 	remote = kcov_remote_find(handle);
 	if (!remote) {
-		spin_unlock_irqrestore(&kcov_remote_lock, flags);
+		spin_unlock(&kcov_remote_lock);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 	kcov_debug("handle = %llx, context: %s\n", handle,
@@ -873,13 +877,13 @@ void kcov_remote_start(u64 handle)
 
 	/* Can only happen when in_task(). */
 	if (!area) {
-		local_irqrestore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		area = vmalloc(size * sizeof(unsigned long));
 		if (!area) {
 			kcov_put(kcov);
 			return;
 		}
-		local_irq_save(flags);
+		local_lock_irqsave(&kcov_percpu_data.lock, flags);
 	}
 
 	/* Reset coverage size. */
@@ -891,7 +895,7 @@ void kcov_remote_start(u64 handle)
 	}
 	kcov_start(t, kcov, size, area, mode, sequence);
 
-	local_irq_restore(flags);
+	local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 
 }
 EXPORT_SYMBOL(kcov_remote_start);
@@ -965,12 +969,12 @@ void kcov_remote_stop(void)
 	if (!in_task() && !in_serving_softirq())
 		return;
 
-	local_irq_save(flags);
+	local_lock_irqsave(&kcov_percpu_data.lock, flags);
 
 	mode = READ_ONCE(t->kcov_mode);
 	barrier();
 	if (!kcov_mode_enabled(mode)) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 	/*
@@ -978,12 +982,12 @@ void kcov_remote_stop(void)
 	 * actually found the remote handle and started collecting coverage.
 	 */
 	if (in_serving_softirq() && !t->kcov_softirq) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 	/* Make sure that kcov_softirq is only set when in softirq. */
 	if (WARN_ON(!in_serving_softirq() && t->kcov_softirq)) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 		return;
 	}
 
@@ -1013,7 +1017,7 @@ void kcov_remote_stop(void)
 		spin_unlock(&kcov_remote_lock);
 	}
 
-	local_irq_restore(flags);
+	local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
 
 	/* Get in kcov_remote_start(). */
 	kcov_put(kcov);
-- 
2.33.0

Re: [PATCH 5/5] kcov: Replace local_irq_save() with a local_lock_t.

[Dmitry Vyukov]

/On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The kcov code mixes local_irq_save() and spin_lock() in
> kcov_remote_{start|end}(). This creates a warning on PREEMPT_RT because
> local_irq_save() disables interrupts and spin_lock_t is turned into a
> sleeping lock which can not be acquired in a section with disabled
> interrupts.
>
> The kcov_remote_lock is used to synchronize the access to the hash-list
> kcov_remote_map. The local_irq_save() block protects access to the
> per-CPU data kcov_percpu_data.
>
> There no compelling reason to change the lock type to raw_spin_lock_t to
> make it work with local_irq_save(). Changing it would require to move
> memory allocation (in kcov_remote_add()) and deallocation outside of the
> locked section.
> Adding an unlimited amount of entries to the hashlist will increase the
> IRQ-off time during lookup. It could be argued that this is debug code
> and the latency does not matter. There is however no need to do so and
> it would allow to use this facility in an RT enabled build.
>
> Using a local_lock_t instead of local_irq_save() has the befit of adding
> a protection scope within the source which makes it obvious what is
> protected. On a !PREEMPT_RT && !LOCKDEP build the local_lock_irqsave()
> maps directly to local_irq_save() so there is overhead at runtime.

s/befit/benefit/
s/overhead/no overhead/

but otherwise

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> Replace the local_irq_save() section with a local_lock_t.
>
> Reported-by: Clark Williams <williams@redhat.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/kcov.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 620dc4ffeb685..36ca640c4f8e7 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -88,6 +88,7 @@ static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas);
>
>  struct kcov_percpu_data {
>         void                    *irq_area;
> +       local_lock_t            lock;
>
>         unsigned int            saved_mode;
>         unsigned int            saved_size;
> @@ -96,7 +97,9 @@ struct kcov_percpu_data {
>         int                     saved_sequence;
>  };
>
> -static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data);
> +static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {
> +       .lock = INIT_LOCAL_LOCK(lock),
> +};
>
>  /* Must be called with kcov_remote_lock locked. */
>  static struct kcov_remote *kcov_remote_find(u64 handle)
> @@ -824,7 +827,7 @@ void kcov_remote_start(u64 handle)
>         if (!in_task() && !in_serving_softirq())
>                 return;
>
> -       local_irq_save(flags);
> +       local_lock_irqsave(&kcov_percpu_data.lock, flags);
>
>         /*
>          * Check that kcov_remote_start() is not called twice in background
> @@ -832,7 +835,7 @@ void kcov_remote_start(u64 handle)
>          */
>         mode = READ_ONCE(t->kcov_mode);
>         if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
> -               local_irq_restore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>         /*
> @@ -841,14 +844,15 @@ void kcov_remote_start(u64 handle)
>          * happened while collecting coverage from a background thread.
>          */
>         if (WARN_ON(in_serving_softirq() && t->kcov_softirq)) {
> -               local_irq_restore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>
>         spin_lock(&kcov_remote_lock);
>         remote = kcov_remote_find(handle);
>         if (!remote) {
> -               spin_unlock_irqrestore(&kcov_remote_lock, flags);
> +               spin_unlock(&kcov_remote_lock);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>         kcov_debug("handle = %llx, context: %s\n", handle,
> @@ -873,13 +877,13 @@ void kcov_remote_start(u64 handle)
>
>         /* Can only happen when in_task(). */
>         if (!area) {
> -               local_irqrestore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 area = vmalloc(size * sizeof(unsigned long));
>                 if (!area) {
>                         kcov_put(kcov);
>                         return;
>                 }
> -               local_irq_save(flags);
> +               local_lock_irqsave(&kcov_percpu_data.lock, flags);
>         }
>
>         /* Reset coverage size. */
> @@ -891,7 +895,7 @@ void kcov_remote_start(u64 handle)
>         }
>         kcov_start(t, kcov, size, area, mode, sequence);
>
> -       local_irq_restore(flags);
> +       local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>
>  }
>  EXPORT_SYMBOL(kcov_remote_start);
> @@ -965,12 +969,12 @@ void kcov_remote_stop(void)
>         if (!in_task() && !in_serving_softirq())
>                 return;
>
> -       local_irq_save(flags);
> +       local_lock_irqsave(&kcov_percpu_data.lock, flags);
>
>         mode = READ_ONCE(t->kcov_mode);
>         barrier();
>         if (!kcov_mode_enabled(mode)) {
> -               local_irq_restore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>         /*
> @@ -978,12 +982,12 @@ void kcov_remote_stop(void)
>          * actually found the remote handle and started collecting coverage.
>          */
>         if (in_serving_softirq() && !t->kcov_softirq) {
> -               local_irq_restore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>         /* Make sure that kcov_softirq is only set when in softirq. */
>         if (WARN_ON(!in_serving_softirq() && t->kcov_softirq)) {
> -               local_irq_restore(flags);
> +               local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>                 return;
>         }
>
> @@ -1013,7 +1017,7 @@ void kcov_remote_stop(void)
>                 spin_unlock(&kcov_remote_lock);
>         }
>
> -       local_irq_restore(flags);
> +       local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
>
>         /* Get in kcov_remote_start(). */
>         kcov_put(kcov);
> --
> 2.33.0
>

Re: [PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[Marco Elver]

On Mon, 30 Aug 2021 at 19:26, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> The last patch in series is follow-up to address the PREEMPT_RT issue
> within in kcov reported by Clark [0].
> Patches 1-3 are smaller things that I noticed while staring at it.
> Patch 4 is small change which makes replacement in #5 simpler / more
> obvious.
> I tested this with the three examples in the documentation folder and I
> didn't notice higher latency with kcov enabled. Debug or not, I don't
> see a reason to make the lock a raw_spin_lock_t annd it would complicate
> memory allocation as mentioned in #5.

Thanks for sorting this out. Given syzkaller is exercising all of
KCOV's feature, I let syzkaller run for a few hours with PROVE_LOCKING
(and PROVE_RAW_LOCK_NESTING) on, and looks fine:

    Acked-by: Marco Elver <elver@google.com>
    Tested-by: Marco Elver <elver@google.com>

> One thing I noticed and have no idea if this is right or not:
> The code seems to mix long and uint64_t for the reported instruction
> pointer / position in the buffer. For instance
> __sanitizer_cov_trace_pc() refers to a 64bit pointer (in the comment)
> while the area pointer itself is (long *). The problematic part is that
> a 32bit application on a 64bit pointer will expect a four byte pointer
> while kernel uses an eight byte pointer.

I think the code is consistent in using 'unsigned long' for writing
regular pos/IP (except write_comp_data(), which has a comment about
it). The mentions of 64-bit in comments might be inaccurate though.
But I think it's working as expected:

- on 64-bit kernels, pos/IP can be up to 64-bit;
- on 32-bit kernels, pos/IP can only be up to 32-bit.

User space necessarily has to know about the bit-ness of its kernel,
because the coverage information is entirely dependent on the kernel
image. I think the examples in documentation weren't exhaustive in
this regard. At least that's my take -- Dmitry or Andrey would know
for sure (Dmitry is currently on vacation, but hopefully can clarify
next week).

Thanks,
-- Marco

Re: [PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[Sebastian Andrzej Siewior]

On 2021-09-06 18:13:11 [+0200], Marco Elver wrote:
> Thanks for sorting this out. Given syzkaller is exercising all of
> KCOV's feature, I let syzkaller run for a few hours with PROVE_LOCKING
> (and PROVE_RAW_LOCK_NESTING) on, and looks fine:
> 
>     Acked-by: Marco Elver <elver@google.com>
>     Tested-by: Marco Elver <elver@google.com>

awesome.

> > One thing I noticed and have no idea if this is right or not:
> > The code seems to mix long and uint64_t for the reported instruction
> > pointer / position in the buffer. For instance
> > __sanitizer_cov_trace_pc() refers to a 64bit pointer (in the comment)
> > while the area pointer itself is (long *). The problematic part is that
> > a 32bit application on a 64bit pointer will expect a four byte pointer
> > while kernel uses an eight byte pointer.
> 
> I think the code is consistent in using 'unsigned long' for writing
> regular pos/IP (except write_comp_data(), which has a comment about
> it). The mentions of 64-bit in comments might be inaccurate though.
> But I think it's working as expected:
> 
> - on 64-bit kernels, pos/IP can be up to 64-bit;
> - on 32-bit kernels, pos/IP can only be up to 32-bit.
> 
> User space necessarily has to know about the bit-ness of its kernel,
> because the coverage information is entirely dependent on the kernel
> image. I think the examples in documentation weren't exhaustive in
> this regard. At least that's my take -- Dmitry or Andrey would know
> for sure (Dmitry is currently on vacation, but hopefully can clarify
> next week).

okay.

> Thanks,
> -- Marco

Sebastian

Re: [PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[Marco Elver]

On Mon, 6 Sept 2021 at 18:28, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2021-09-06 18:13:11 [+0200], Marco Elver wrote:
> > Thanks for sorting this out. Given syzkaller is exercising all of
> > KCOV's feature, I let syzkaller run for a few hours with PROVE_LOCKING
> > (and PROVE_RAW_LOCK_NESTING) on, and looks fine:
> >
> >     Acked-by: Marco Elver <elver@google.com>
> >     Tested-by: Marco Elver <elver@google.com>
>
> awesome.
>
> > > One thing I noticed and have no idea if this is right or not:
> > > The code seems to mix long and uint64_t for the reported instruction
> > > pointer / position in the buffer. For instance
> > > __sanitizer_cov_trace_pc() refers to a 64bit pointer (in the comment)
> > > while the area pointer itself is (long *). The problematic part is that
> > > a 32bit application on a 64bit pointer will expect a four byte pointer
> > > while kernel uses an eight byte pointer.
> >
> > I think the code is consistent in using 'unsigned long' for writing
> > regular pos/IP (except write_comp_data(), which has a comment about
> > it). The mentions of 64-bit in comments might be inaccurate though.
> > But I think it's working as expected:
> >
> > - on 64-bit kernels, pos/IP can be up to 64-bit;
> > - on 32-bit kernels, pos/IP can only be up to 32-bit.
> >
> > User space necessarily has to know about the bit-ness of its kernel,
> > because the coverage information is entirely dependent on the kernel
> > image. I think the examples in documentation weren't exhaustive in
> > this regard. At least that's my take -- Dmitry or Andrey would know
> > for sure (Dmitry is currently on vacation, but hopefully can clarify
> > next week).

Just for reference, this is what syzkaller does which confirms the above:
https://github.com/google/syzkaller/blob/3d9c9a2ac29573a117cde8ace07d0749eeda991b/executor/executor_linux.h#L84

> okay.

I saw Dmitry responded with Acks/comment. Did you have a tree in mind
to take it through? Usually KCOV changes go through the -mm tree, in
which case please Cc Andrew in the rest of the series.

Thanks,
-- Marco

Re: [PATCH 0/5] kcov: PREEMPT_RT fixup + misc

[Sebastian Andrzej Siewior]

On 2021-09-20 11:26:38 [+0200], Marco Elver wrote:
> I saw Dmitry responded with Acks/comment. Did you have a tree in mind
> to take it through? Usually KCOV changes go through the -mm tree, in
> which case please Cc Andrew in the rest of the series.

Okay. In that case I'm going to repost it with all the tags and akpm in
Cc:.

> Thanks,
> -- Marco

Sebastian