Re: [PATCH v3 1/3] selftests/damon: prevent cross-context state pollution in DamonCtx

[SJ Park <sj@kernel.org>]

On Mon, 29 Jun 2026 14:55:52 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch introduces a split-brain state corruption in `DamonCtx` by implementing an incomplete fix for Python mutable default arguments. While it replaces the defaults in `DamonCtx.__init__` with `None` sentinels and instantiates them at runtime (e.g., `monitoring_attrs = DamonAttrs()`), it misses the fact that `DamonAttrs.__init__` still uses a mutable default (`intervals_goal=IntervalsGoal()`).
> 
> Because `DamonAttrs()` is now called multiple times (once per `DamonCtx`), multiple `DamonAttrs` instances are created, but they all silently share the exact same `IntervalsGoal` instance. During `DamonAttrs.__init__`, the shared `IntervalsGoal` instance's `attrs` back-reference is overwritten to point to the newest `DamonAttrs` (`self.intervals_goal.attrs = self`). 
> 
> As a result, when an older `DamonCtx` calls `stage()`, its base attributes correctly route to its own sysfs directory, but its `intervals_goal` writes route to the sysfs directory of the most recently created `DamonCtx`. This is a newly introduced inconsistency, as prior to the patch, all properties incorrectly routed to the same directory together. Additionally, the patch leaves identical mutable default bugs untouched in `DamosFilters`, `DamosDests`, `Damos`, `Kdamond`, and `Kdamonds`.
> --
> 
> commit 7d9280d7450287d38ab4b080317de0093ab71119
> Author: Kunwu Chan <chentao@kylinos.cn>
> 
> selftests/damon: prevent cross-context state pollution in DamonCtx
> 
> DamonCtx.__init__() uses mutable default values for monitoring_attrs, targets,
> and schemes. In Python these are evaluated once at function definition time,
> so multiple DamonCtx instances can unintentionally share the same lists and
> DamonAttrs instance. Replace the mutable defaults with None sentinels and
> initialize the objects when needed.
> 
> Link: https://lore.kernel.org/20260601032314.424013-2-kunwu.chan@linux.dev
> 
> > diff --git a/tools/testing/selftests/damon/_damon_sysfs.py b/tools/testing/selftests/damon/_damon_sysfs.py
> > index 8b12cc0484405..2f6f2699db256 100644
> > --- a/tools/testing/selftests/damon/_damon_sysfs.py
> > +++ b/tools/testing/selftests/damon/_damon_sysfs.py
> > @@ -624,17 +624,23 @@ class DamonCtx:
> >      pause = None
> >      idx = None
> >  
> > -    def __init__(self, ops='paddr', monitoring_attrs=DamonAttrs(), targets=[],
> > -            schemes=[], pause=False):
> > +    def __init__(self, ops='paddr', monitoring_attrs=None, targets=None,
> > +            schemes=None, pause=False):
> >          self.ops = ops
> > +        if monitoring_attrs is None:
> > +            monitoring_attrs = DamonAttrs()
> 
> [Severity: Medium]
> Does this incomplete fix introduce a split-brain state regression?
> 
> Because DamonAttrs() is now called multiple times (once per DamonCtx),
> multiple DamonAttrs instances are created. However, DamonAttrs.__init__
> still uses a mutable default for intervals_goal:
> 
> tools/testing/selftests/damon/_damon_sysfs.py:DamonAttrs.__init__() {
>     def __init__(self, sample_us=5000, aggr_us=100000,
>                  intervals_goal=IntervalsGoal(), update_us=1000000,
> ...
>         self.intervals_goal.attrs = self
> }
> 
> This causes all newly created DamonCtx instances to silently share the
> exact same IntervalsGoal instance. During DamonAttrs.__init__, the shared
> IntervalsGoal instance's attrs back-reference is overwritten to point to
> the newest DamonAttrs.
> 
> As a result, when an older DamonCtx calls stage(), its base attributes
> correctly route to its own sysfs directory, but its intervals_goal writes
> route to the sysfs directory of the most recently created DamonCtx.
> 
> Additionally, could there be other similar mutable default bugs left
> untouched, such as in Damos.__init__:
> 
> tools/testing/selftests/damon/_damon_sysfs.py:Damos.__init__() {
>     def __init__(self, action='stat', access_pattern=DamosAccessPattern(),
>                  quota=DamosQuota(), watermarks=DamosWatermarks(),
>                  core_filters=[], ops_filters=[], filters=[], target_nid=0,
> }

Good findings.  But no blocker of this patch that makes some improvements.  And
fortunately there is no real callers that causing the issue for now, to my best
knowledge.  So not real urgent.  Someone who get interested in this change will
take time to fix this.


Thanks,
SJ

[...]