linux-kernel - Re: [PATCH v2 06/23] mm: introduce BPF struct ops for OOM handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87frb2octc.fsf@linux.dev>
Date: Tue, 28 Oct 2025 15:56:31 -0700
From: Roman Gushchin <roman.gushchin@...ux.dev>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,  LKML
 <linux-kernel@...r.kernel.org>,  Alexei Starovoitov <ast@...nel.org>,
  Suren Baghdasaryan <surenb@...gle.com>,  Michal Hocko
 <mhocko@...nel.org>,  Shakeel Butt <shakeel.butt@...ux.dev>,  Johannes
 Weiner <hannes@...xchg.org>,  Andrii Nakryiko <andrii@...nel.org>,  JP
 Kobryn <inwardvessel@...il.com>,  linux-mm <linux-mm@...ck.org>,  "open
 list:CONTROL GROUP (CGROUP)" <cgroups@...r.kernel.org>,  bpf
 <bpf@...r.kernel.org>,  Martin KaFai Lau <martin.lau@...nel.org>,  Song
 Liu <song@...nel.org>,  Kumar Kartikeya Dwivedi <memxor@...il.com>,  Tejun
 Heo <tj@...nel.org>
Subject: Re: [PATCH v2 06/23] mm: introduce BPF struct ops for OOM handling

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Tue, Oct 28, 2025 at 11:42 AM Roman Gushchin
> <roman.gushchin@...ux.dev> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@...il.com> writes:
>>
>> > On Mon, Oct 27, 2025 at 4:18 PM Roman Gushchin <roman.gushchin@...ux.dev> wrote:
>> >>
>> >> +bool bpf_handle_oom(struct oom_control *oc)
>> >> +{
>> >> +       struct bpf_oom_ops *bpf_oom_ops = NULL;
>> >> +       struct mem_cgroup __maybe_unused *memcg;
>> >> +       int idx, ret = 0;
>> >> +
>> >> +       /* All bpf_oom_ops structures are protected using bpf_oom_srcu */
>> >> +       idx = srcu_read_lock(&bpf_oom_srcu);
>> >> +
>> >> +#ifdef CONFIG_MEMCG
>> >> +       /* Find the nearest bpf_oom_ops traversing the cgroup tree upwards */
>> >> +       for (memcg = oc->memcg; memcg; memcg = parent_mem_cgroup(memcg)) {
>> >> +               bpf_oom_ops = READ_ONCE(memcg->bpf_oom);
>> >> +               if (!bpf_oom_ops)
>> >> +                       continue;
>> >> +
>> >> +               /* Call BPF OOM handler */
>> >> +               ret = bpf_ops_handle_oom(bpf_oom_ops, memcg, oc);
>> >> +               if (ret && oc->bpf_memory_freed)
>> >> +                       goto exit;
>> >> +       }
>> >> +#endif /* CONFIG_MEMCG */
>> >> +
>> >> +       /*
>> >> +        * System-wide OOM or per-memcg BPF OOM handler wasn't successful?
>> >> +        * Try system_bpf_oom.
>> >> +        */
>> >> +       bpf_oom_ops = READ_ONCE(system_bpf_oom);
>> >> +       if (!bpf_oom_ops)
>> >> +               goto exit;
>> >> +
>> >> +       /* Call BPF OOM handler */
>> >> +       ret = bpf_ops_handle_oom(bpf_oom_ops, NULL, oc);
>> >> +exit:
>> >> +       srcu_read_unlock(&bpf_oom_srcu, idx);
>> >> +       return ret && oc->bpf_memory_freed;
>> >> +}
>> >
>> > ...
>> >
>> >> +static int bpf_oom_ops_reg(void *kdata, struct bpf_link *link)
>> >> +{
>> >> +       struct bpf_struct_ops_link *ops_link = container_of(link, struct bpf_struct_ops_link, link);
>> >> +       struct bpf_oom_ops **bpf_oom_ops_ptr = NULL;
>> >> +       struct bpf_oom_ops *bpf_oom_ops = kdata;
>> >> +       struct mem_cgroup *memcg = NULL;
>> >> +       int err = 0;
>> >> +
>> >> +       if (IS_ENABLED(CONFIG_MEMCG) && ops_link->cgroup_id) {
>> >> +               /* Attach to a memory cgroup? */
>> >> +               memcg = mem_cgroup_get_from_ino(ops_link->cgroup_id);
>> >> +               if (IS_ERR_OR_NULL(memcg))
>> >> +                       return PTR_ERR(memcg);
>> >> +               bpf_oom_ops_ptr = bpf_oom_memcg_ops_ptr(memcg);
>> >> +       } else {
>> >> +               /* System-wide OOM handler */
>> >> +               bpf_oom_ops_ptr = &system_bpf_oom;
>> >> +       }
>> >
>> > I don't like the fallback and special case of cgroup_id == 0.
>> > imo it would be cleaner to require CONFIG_MEMCG for this feature
>> > and only allow attach to a cgroup.
>> > There is always a root cgroup that can be attached to and that
>> > handler will be acting as "system wide" oom handler.
>>
>> I thought about it, but then it can't be used on !CONFIG_MEMCG
>> configurations and also before cgroupfs is mounted, root cgroup
>> is created etc.
>
> before that bpf isn't viable either, and oom is certainly not an issue.
>
>> This is why system-wide things are often handled in a
>> special way, e.g. in by PSI (grep system_group_pcpu).
>>
>> I think supporting !CONFIG_MEMCG configurations might be useful for
>> some very stripped down VM's, for example.
>
> I thought I wouldn't need to convince the guy who converted bpf maps
> to memcg and it made it pretty much mandatory for the bpf subsystem :)
> I think the following is long overdue:
> diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
> index eb3de35734f0..af60be6d3d41 100644
> --- a/kernel/bpf/Kconfig
> +++ b/kernel/bpf/Kconfig
> @@ -34,6 +34,7 @@ config BPF_SYSCALL
>         select NET_SOCK_MSG if NET
>         select NET_XGRESS if NET
>         select PAGE_POOL if NET
> +       depends on MEMCG
>         default n
>
> With this we can cleanup a ton of code.
> Let's not add more hacks just because some weird thing
> still wants !MEMCG. If they do, they will survive without bpf.

Ok, this is bold, but why not?
Acked-by: Roman Gushchin <roman.gushchin@...ux.dev>

Are you going to land it separately, I guess?