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

Message-ID: <2dd7rwkxuirjqpxzdvplt26vgfydomu56j3dkmr37765zk67pd@aou7jw4d6wtq>
Date: Tue, 17 Jun 2025 11:08:19 +0200
From: Michal Koutný <mkoutny@...e.com>
To: Jemmy Wong <jemmywong512@...il.com>, Tejun Heo <tj@...nel.org>, 
	Johannes Weiner <hannes@...xchg.org>
Cc: 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

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


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)