Re: [PATCH v5] kcov: fix data corruption and race conditions on PREEMPT_RT by moving saved remote state to task_struct

[Tetsuo Handa]

On 2026/07/09 0:28, Alexander Potapenko wrote:
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 373bcc0598d1..7a53c15cecb5 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1543,6 +1543,14 @@ struct task_struct {
>>
>>         /* Collect coverage from softirq context: */
>>         unsigned int                    kcov_softirq;
>> +
>> +       /* 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;
> 
> I think we need to land "kcov: factor out struct kcov_state"
> (https://lore.kernel.org/all/20250731115139.3035888-4-glider@xxxxxxxxxx/#r),
> these stray members of task_struct are getting out of control.
> Let me send it separately from the rest of my changes.

Can we apply my patch first for smaller changes and easier backporting?
I sent my patch to linux-next-20260708, and so far

  https://syzkaller.appspot.com/bug?extid=3f51ad7ac3ae57a6fdcc
  https://syzkaller.appspot.com/bug?extid=47cf95ca1f9dcca872c8
  https://syzkaller.appspot.com/bug?extid=8a173e13208949931dc7
  https://syzkaller.appspot.com/bug?extid=90984d3713722683112e

are not occurring in linux-next for 22 hours.

> 
> Also note these new fields will need to be initialized in kcov_task_init().

I and Sashiko don't think so. You can check
**Analysis of `kcov` Initialization and Forking Behavior**
section in https://sashiko.dev/#/log/87568 . These new fields are used only during
KCOV collection is running. Since these fields are duplicated by dup_task_struct()
where KCOV collection is not running, there is no need to initialize.

> 
>>
>> -       if (in_task()) {
>> +       if (1) {
> 
> Why? Can you remove this if statement?
> (I am also curious why Sashiko did not report it, CC Roman)

Just to minimize lines changed for easier review. Since the compiler will
optimize, I don't think this "if (1)" will cause performance penalty.

> 
> 
>> +#ifdef CONFIG_PREEMPT_RT
>> +       /* Allocate some extra buffers in order to prepare for softirq preemption. */
>> +       cpu = cpu >= 4 ? cpu * 2 : cpu + 4;
>> +#endif
>> +       while (cpu--) {
>> +               void *area = vmalloc(CONFIG_KCOV_IRQ_AREA_SIZE * sizeof(unsigned long));
> 
> We need a NULL check here, kcov_remote_area_put() is going to dereference area.

Although Sashiko knows lack of NULL check here, Sashiko did not report this change in
the final report, for this function is executed as __init function of built-in module.

If GFP_KERNEL memory allocation by vmalloc() cannot find memory, the system won't be able
to survive even if we handled allocation failure. Also, since there is no userspace process
which the OOM killer can select as an OOM victim, the OOM killer will call
panic("System is deadlocked on memory\n") if size of GFP_KERNEL memory allocation within
vmalloc() request is up to 8 pages (i.e. 32KB in many architectures).

That is, skipping NULL check is acceptable for __init function of built-in module.