Re: [PATCH v4 4/5] cgroup: use separate rstat trees for each subsystem

[JP Kobryn <inwardvessel@gmail.com>]

On 4/17/25 12:05 PM, JP Kobryn wrote:
> On 4/17/25 2:26 AM, Michal Koutný wrote:
>> On Wed, Apr 16, 2025 at 02:43:57PM -0700, JP Kobryn 
>> <inwardvessel@gmail.com> wrote:
>>> Hmmm I checked my initial assumptions. I'm still finding that css's from
>>> any subsystem regardless of rstat usage can reach this call to exit.
>>> Without the guard there will be undefined behavior.
>>
>> At which place is the UB? (I saw that all funnels to css_rstat_flush()
>> that does the check but I may have overlooked something in the diffs.)
> 
> It would occur on access to the per-cpu rstat pointer during the tree
> building in the sequence below.
> 
> css_rstat_exit(css)
>      css_rstat_flush(css)
>          css_rstat_updated_list(css, cpu)
>              rstatc = css_rstat_cpu(css, cpu)
>                  per_cpu_ptr(css->rstat_cpu, cpu)
> 
> Since I'm doing the early checks in css_rstat_flush() in the next rev
> though, I was thinking of this:
> 
> void css_rstat_flush(css)
> {
>      bool is_cgroup = css_is_cgroup(css);
> 
>      if (!is_cgroup && !css->ss->css_rstat_flush)
>          return;
> 
>      ...
> 
>      for (...) {
>          if (is_cgroup)
>              /* flush base stats and bpf */
>          else
>              /* flush via css_rstat_flush */
>      }
> }
> 
> Then we could remove the two conditional guards in css_release_work_fn()
> and css_free_rwork_fn(). Thoughts?

Correction: just the one in css_release_work_fn(). In the case of
css_free_rwork_fn(), there is a call to css_rstat_exit() which would
still need to be guarded (or it will touch the per-cpu rstat pointer).

> 
> Note that I was also thinking of doing the same early check in
> css_rstat_updated() since it is exposed as a kfunc and someone could
> pass in a non-rstat css other than cgroup::self.