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

[Alexander Potapenko]

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

Fine, we can do that.

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

This is all assuming that kcov_remote_start()/kcov_remote_stop() are
placed correctly.
If for some reason a developer fails to call kcov_remote_stop() for a
certain module, we have stale pointers to the remote context, which
will leak to the child task upon fork.
If we reset them upon fork, the stale pointers are contained to the
original task.

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

It won't, but it is still a coding style we don't want to tolerate.
Please remove it.


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

Sashiko did not report the if (1) either, so I wouldn't bet money on
it (although I love Sashiko).

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

Removing the NULL check here is a regression compared to the previous
version of this code.
Restoring it is easier than adding a comment explaining its absence,
which would otherwise be necessary.