linux-kernel - Re: [PATCH v1 0/3] cgroup: Add lock guard support

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] [day] [month] [year] [list]

Message-Id: <9BDD726A-2EAE-46C3-9D00-004E051B8F5B@gmail.com>
Date: Fri, 20 Jun 2025 18:45:54 +0800
From: Jemmy Wong <jemmywong512@...il.com>
To: Michal Koutný <mkoutny@...e.com>,
 tj@...nel.org,
 hannes@...xchg.org
Cc: Jemmy <jemmywong512@...il.com>,
 Waiman Long <longman@...hat.com>,
 cgroups@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 0/3] cgroup: Add lock guard support


Hi Michal, Tejun, Johannes,

Thank you, Michal, for supporting this modernization effort to adopt guard constructs.

With 3,326 instances already in use across the kernel upstream,
guards offer improved safety and readability.

I look forward to working with the community to integrate them into cgroup 
and welcome feedback to ensure a smooth transition.

Best,
Jemmy

> On Jun 17, 2025, at 5:08 PM, Michal Koutný <mkoutny@...e.com> wrote:
> 
> Hello.
> 
> I understand why this might have been controversial but I'm surprised it
> remains so when the lock guards are already used in the kernel code.
> Documentation/process/coding-style.rst isn't partisan in either way.
> 
> Johannes:
>> heebeejeebees - it's asymmetric and critical sections don't stand out
>> visually at all.
> - I'd say that's the point of it for functions that are made to
>  manipulate data structures under the lock. Not to spoil the code.
> - Yes, it's a different coding style that one has to get used to.
> 
>> extra indentation
> - deeper indentation == deeper locking, wary of that
> - although I admit, in some cases of the reworked code, it's overly deep
> 
> Further reasoning is laid out in include/linux/cleanup.h. I noticed
> there exists no_free_ptr() macro and that suggests an idea for analogous
> no_exit_class() (or unguard()) macro (essential an unlock + signal for
> compiler to not call cleanup function after this BB).
> 
> Tejun:
>> There are no practical benefits to converting the code base at this point.
> 
> I'd expect future backports (into such code) to be more robust wrt
> pairing errors.
> At the same time this is also my biggest concern about this change, the
> wide-spread diff would make current backporting more difficult.  (But
> I'd counter argue that one should think forward here.)
> 
> 
> Further benefits I see:
> - it is fewer lines (code is to the point),
> - reliable cleanup paths,
> - it is modern and evolution step forward (given such constructs
>  eventually emerge in various languages).
> 
> 
> On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong <jemmywong512@...il.com> wrote:
>> v1 changes:
>> - remove guard support for BPF
>> - split patch into parts
>> 
>> v0 link:
>> https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/
>> 
>> Jemmy Wong (3):
>>  cgroup: add lock guard support for cgroup_muetx
>>  cgroup: add lock guard support for css_set_lock and rcu
>>  cgroup: add lock guard support for others
> 
> As for the series in general
> - I'm still in favor of pursuing it (with remarks to individual
>  patches),
> - it's a good opportunity to also to append sparse __acquires/__releases
>  cleanup to it (see also [1]).
> 
> Regards,
> Michal
> 
> [1] https://lore.kernel.org/r/Z_6z2-qqLI7dbl8h@slm.duckdns.org
>