Messages in this thread |  | | From | Dmitry Vyukov <> | | Date | Fri, 15 May 2026 09:10:44 +0200 | | Subject | Re: [PATCH v3] kcov: move kcov_remote_data to task_struct for RT and remove local_lock |
| |
On Sat, 9 May 2026 at 10:09, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > In CONFIG_PREEMPT_RT=y kernels, softirqs are executed in a per-CPU > ksoftirqd thread or in the context of the task that raised the softirq. > This means in_task() can return true even while serving softirqs. This > behavior causes KCOV to incorrectly identify the context, leading to state > corruption and various kcov-related warnings reported by syzbot. > > Furthermore, commit d5d2c51f1e5f ("kcov: replace local_irq_save() with a > local_lock_t") introduced local_lock to protect per-CPU kcov_percpu_data. > However, the need for this physical CPU-level locking is questionable > because: > > 1. kcov_remote_start/stop() already bail out early if called from > in_hardirq() or in_nmi(). > 2. The core tracing function check_kcov_mode() also skips hardirq/NMI > contexts, meaning no KCOV state is accessed during hardirqs even if > they interrupt a KCOV-enabled task/softirq. > 3. In non-RT kernels, softirqs do not nest, so no concurrent access to > per-CPU data occurs between softirqs. > 4. In RT kernels, while softirqs can be preempted, this patch moves the > KCOV state from per-CPU variables to task_struct (per-task), eliminating > the contention on shared per-CPU resources. > > By moving kcov_remote_data to task_struct for RT kernels and replacing > the in_task() check with !in_serving_softirq(), we ensure consistent > context detection. Since the data is now isolated per-task and not accessed > by hardirqs, the local_lock (and the original local_irq_save) is no longer > necessary and is removed to reduce overhead. > > Changes: > > * Move remote coverage state from kcov_percpu_data to task_struct under > CONFIG_PREEMPT_RT. > * Replace in_task() with !in_serving_softirq() in kcov_remote_start/stop() > for accurate context tracking. > * Remove local_lock and IRQ disabling from kcov_remote_start/stop() as the > state is now task-local and hardirqs are already excluded. > * Ensure CONFIG_PREEMPT_RT=y uses kcov_remote_area_get() (the > vmalloc-backed pool) instead of the single per-CPU irq_area. > > Analyzed-by: AI Mode in Google Search (no mail address) > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: 5ff3b30ab57d ("kcov: collect coverage from interrupts") > --- > Only compile tested. I don't have environment to measure how not
+Aleksandr Nogikh
I don't see this patch on syzbot CI: https://ci.syzbot.org/?name=kcov%3A+move+kcov_remote_data+to+task_struct+for+RT+and+remove+local_lock
How can we make it at least tested on syzbot CI? Otherwise it risks breaking syzbot when merged.
> preallocating CONFIG_KCOV_IRQ_AREA_SIZE bytes of buffers and > add to the global pool in kcov_init() impacts reliability / performance. > Please be sure to test this patch using CONFIG_PREEMPT_RT=y and > CONFIG_PREEMPT_RT=n kernels in local syzkaller environment before > sending upstream. > > This patch is expected to address various KCOV related reports. > But I don't add Closes: lines because we can't tell whether this patch > alone is sufficient for marking as Closes:. We will find the answer > some time after being sent to upstream. > > https://syzkaller.appspot.com/bug?extid=90984d3713722683112e > https://syzkaller.appspot.com/bug?extid=47cf95ca1f9dcca872c8 > https://syzkaller.appspot.com/bug?extid=8a173e13208949931dc7 > https://syzkaller.appspot.com/bug?extid=3f51ad7ac3ae57a6fdcc > https://syzkaller.appspot.com/bug?extid=e6686317bd9fe911591a > > include/linux/sched.h | 10 +++++ > kernel/kcov.c | 94 +++++++++++++++++++++++++------------------ > lib/Kconfig.debug | 1 + > 3 files changed, 65 insertions(+), 40 deletions(-) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 368c7b4d7cb5..2c963f4271d6 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1522,6 +1522,16 @@ struct task_struct { > > /* Collect coverage from softirq context: */ > unsigned int kcov_softirq; > + > +#ifdef CONFIG_PREEMPT_RT
Is it possible to unify the logic between RT and non-RT kernels (not have any ifdef's)? The logic is already rather complicated, having 2 work modes, and limited testing will be problematic in the future.
> + /* Temporary storage for preempting remote coverage collection: */ > + unsigned int kcov_saved_mode; > + unsigned int kcov_saved_size; > + void *kcov_saved_area; > + struct kcov *kcov_saved_kcov; > + int kcov_saved_sequence; > +#endif > + > #endif > > #ifdef CONFIG_MEMCG_V1 > diff --git a/kernel/kcov.c b/kernel/kcov.c > index 0b369e88c7c9..965c11a75b36 100644 > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -88,9 +88,9 @@ static DEFINE_SPINLOCK(kcov_remote_lock); > static DEFINE_HASHTABLE(kcov_remote_map, 4); > static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas); > > +#ifndef CONFIG_PREEMPT_RT > struct kcov_percpu_data { > void *irq_area; > - local_lock_t lock; > > unsigned int saved_mode; > unsigned int saved_size; > @@ -100,8 +100,8 @@ struct kcov_percpu_data { > }; > > static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = { > - .lock = INIT_LOCAL_LOCK(lock), > }; > +#endif > > /* Must be called with kcov_remote_lock locked. */ > static struct kcov_remote *kcov_remote_find(u64 handle) > @@ -823,8 +823,38 @@ static inline bool kcov_mode_enabled(unsigned int mode) > return (mode & ~KCOV_IN_CTXSW) != KCOV_MODE_DISABLED; > } > > +#ifdef CONFIG_PREEMPT_RT > +static void kcov_remote_softirq_start(struct task_struct *t) > +{ > + unsigned int mode; > + > + mode = READ_ONCE(t->kcov_mode); > + barrier(); > + if (kcov_mode_enabled(mode)) { > + t->kcov_saved_mode = mode; > + t->kcov_saved_size = t->kcov_size; > + t->kcov_saved_area = t->kcov_area; > + t->kcov_saved_sequence = t->kcov_sequence; > + t->kcov_saved_kcov = t->kcov; > + kcov_stop(t); > + } > +} > + > +static void kcov_remote_softirq_stop(struct task_struct *t) > +{ > + if (t->kcov_saved_kcov) { > + kcov_start(t, t->kcov_saved_kcov, t->kcov_saved_size, > + t->kcov_saved_area, t->kcov_saved_mode, > + t->kcov_saved_sequence); > + t->kcov_saved_mode = 0; > + t->kcov_saved_size = 0; > + t->kcov_saved_area = NULL; > + t->kcov_saved_sequence = 0; > + t->kcov_saved_kcov = NULL; > + } > +} > +#else > static void kcov_remote_softirq_start(struct task_struct *t) > - __must_hold(&kcov_percpu_data.lock) > { > struct kcov_percpu_data *data = this_cpu_ptr(&kcov_percpu_data); > unsigned int mode; > @@ -842,7 +872,6 @@ static void kcov_remote_softirq_start(struct task_struct *t) > } > > static void kcov_remote_softirq_stop(struct task_struct *t) > - __must_hold(&kcov_percpu_data.lock) > { > struct kcov_percpu_data *data = this_cpu_ptr(&kcov_percpu_data); > > @@ -857,6 +886,7 @@ static void kcov_remote_softirq_stop(struct task_struct *t) > data->saved_kcov = NULL; > } > } > +#endif > > void kcov_remote_start(u64 handle) > { > @@ -867,43 +897,35 @@ void kcov_remote_start(u64 handle) > void *area; > unsigned int size; > int sequence; > - unsigned long flags; > > - if (WARN_ON(!kcov_check_handle(handle, true, true, true))) > + /* Don't use in_task() in order to allow consistent checks in RT kernels. */ > + if (in_hardirq() || in_nmi()) > return; > - if (!in_task() && !in_softirq_really()) > + if (WARN_ON(!kcov_check_handle(handle, true, true, true))) > return; > > - local_lock_irqsave(&kcov_percpu_data.lock, flags); > - > /* > * Check that kcov_remote_start() is not called twice in background > * threads nor called by user tasks (with enabled kcov). > */ > - mode = READ_ONCE(t->kcov_mode); > - if (WARN_ON(in_task() && kcov_mode_enabled(mode))) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > + if (WARN_ON(!in_serving_softirq() && kcov_mode_enabled(READ_ONCE(t->kcov_mode)))) > return; > - } > /* > * Check that kcov_remote_start() is not called twice in softirqs. > * Note, that kcov_remote_start() can be called from a softirq that > * happened while collecting coverage from a background thread. > */ > - if (WARN_ON(in_serving_softirq() && t->kcov_softirq)) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > + if (WARN_ON(in_serving_softirq() && t->kcov_softirq)) > return; > - } > > spin_lock(&kcov_remote_lock); > remote = kcov_remote_find(handle); > if (!remote) { > spin_unlock(&kcov_remote_lock); > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > return; > } > kcov_debug("handle = %llx, context: %s\n", handle, > - in_task() ? "task" : "softirq"); > + !in_serving_softirq() ? "task" : "softirq"); > kcov = remote->kcov; > /* Put in kcov_remote_stop(). */ > kcov_get(kcov); > @@ -915,6 +937,10 @@ void kcov_remote_start(u64 handle) > */ > mode = context_unsafe(kcov->mode); > sequence = kcov->sequence; > +#ifdef CONFIG_PREEMPT_RT > + size = kcov->remote_size; > + area = kcov_remote_area_get(size); > +#else > if (in_task()) { > size = kcov->remote_size; > area = kcov_remote_area_get(size); > @@ -922,17 +948,16 @@ void kcov_remote_start(u64 handle) > size = CONFIG_KCOV_IRQ_AREA_SIZE; > area = this_cpu_ptr(&kcov_percpu_data)->irq_area; > } > +#endif > spin_unlock(&kcov_remote_lock); > > - /* Can only happen when in_task(). */ > + /* Can only happen when CONFIG_PREEMPT_RT=y or in_task(). */ > if (!area) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > area = vmalloc(size * sizeof(unsigned long)); > if (!area) { > kcov_put(kcov); > return; > } > - local_lock_irqsave(&kcov_percpu_data.lock, flags); > } > > /* Reset coverage size. */ > @@ -943,9 +968,6 @@ void kcov_remote_start(u64 handle) > t->kcov_softirq = 1; > } > kcov_start(t, kcov, size, area, mode, sequence); > - > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > - > } > EXPORT_SYMBOL(kcov_remote_start); > > @@ -1022,32 +1044,24 @@ void kcov_remote_stop(void) > void *area; > unsigned int size; > int sequence; > - unsigned long flags; > > - if (!in_task() && !in_softirq_really()) > + /* Don't use in_task() in order to allow consistent checks in RT kernels. */ > + if (in_hardirq() || in_nmi()) > return; > > - local_lock_irqsave(&kcov_percpu_data.lock, flags); > - > mode = READ_ONCE(t->kcov_mode); > barrier(); > - if (!kcov_mode_enabled(mode)) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > + if (!kcov_mode_enabled(mode)) > return; > - } > /* > * When in softirq, check if the corresponding kcov_remote_start() > * actually found the remote handle and started collecting coverage. > */ > - if (in_serving_softirq() && !t->kcov_softirq) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > + if (in_serving_softirq() && !t->kcov_softirq) > return; > - } > /* Make sure that kcov_softirq is only set when in softirq. */ > - if (WARN_ON(!in_serving_softirq() && t->kcov_softirq)) { > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > + if (WARN_ON(!in_serving_softirq() && t->kcov_softirq)) > return; > - } > > kcov = t->kcov; > area = t->kcov_area; > @@ -1069,14 +1083,12 @@ void kcov_remote_stop(void) > kcov_move_area(kcov->mode, kcov->area, kcov->size, area); > spin_unlock(&kcov->lock); > > - if (in_task()) { > + if (IS_ENABLED(CONFIG_PREEMPT_RT) || in_task()) { > spin_lock(&kcov_remote_lock); > kcov_remote_area_put(area, size); > spin_unlock(&kcov_remote_lock); > } > > - local_unlock_irqrestore(&kcov_percpu_data.lock, flags); > - > /* Get in kcov_remote_start(). */ > kcov_put(kcov); > } > @@ -1119,6 +1131,7 @@ static void __init selftest(void) > > static int __init kcov_init(void) > { > +#ifndef CONFIG_PREEMPT_RT > int cpu; > > for_each_possible_cpu(cpu) { > @@ -1128,6 +1141,7 @@ static int __init kcov_init(void) > return -ENOMEM; > per_cpu_ptr(&kcov_percpu_data, cpu)->irq_area = area; > } > +#endif > > /* > * The kcov debugfs file won't ever get removed and thus, > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 13f3297aa823..493be2c73c9d 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2247,6 +2247,7 @@ config KCOV_INSTRUMENT_ALL > config KCOV_IRQ_AREA_SIZE > hex "Size of interrupt coverage collection area in words" > depends on KCOV > + depends on !PREEMPT_RT > default 0x40000 > help > KCOV uses preallocated per-cpu areas to collect coverage from > -- > 2.47.3 > >
|  |