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: <16d2d53d-a2d0-4cd8-9323-eb91ba51b061@lucifer.local>
Date: Mon, 1 Sep 2025 15:32:49 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Max Kellermann <max.kellermann@...os.com>
Cc: akpm@...ux-foundation.org, david@...hat.com, axelrasmussen@...gle.com,
        yuanchu@...gle.com, willy@...radead.org, hughd@...gle.com,
        mhocko@...e.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Liam.Howlett@...cle.com, vbabka@...e.cz, rppt@...nel.org,
        surenb@...gle.com, vishal.moola@...il.com, linux@...linux.org.uk,
        James.Bottomley@...senpartnership.com, deller@....de,
        agordeev@...ux.ibm.com, gerald.schaefer@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com, borntraeger@...ux.ibm.com,
        svens@...ux.ibm.com, davem@...emloft.net, andreas@...sler.com,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, x86@...nel.org,
        hpa@...or.com, chris@...kel.net, jcmvbkbc@...il.com,
        viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
        weixugc@...gle.com, baolin.wang@...ux.alibaba.com, rientjes@...gle.com,
        shakeel.butt@...ux.dev, thuth@...hat.com, broonie@...nel.org,
        osalvador@...e.de, jfalempe@...hat.com, mpe@...erman.id.au,
        nysal@...ux.ibm.com, linux-arm-kernel@...ts.infradead.org,
        linux-parisc@...r.kernel.org, linux-s390@...r.kernel.org,
        sparclinux@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v5 00/12] mm: establish const-correctness for pointer
 parameters

On Mon, Sep 01, 2025 at 02:30:16PM +0200, Max Kellermann wrote:
> For improved const-correctness.
>
> This patch series systematically adds const qualifiers to pointer
> parameters throughout the memory management subsystem, establishing a
> foundation for improved const-correctness across the entire Linux
> kernel.
>
> Const-correctness provides multiple benefits:
>
> 1. Type Safety: The compiler enforces that functions marked as taking
>    const parameters cannot accidentally modify the data, catching
>    potential bugs at compile time rather than runtime.
>
> 2. Compiler Optimizations: When the compiler knows data won't be
>    modified, it can generate more efficient code through better
>    register allocation, code motion, and aliasing analysis.
>
> 3. API Documentation: Const qualifiers serve as self-documenting code,
>    making it immediately clear to developers which functions are
>    read-only operations versus those that modify state.
>
> 4. Maintenance Safety: Future modifications to const-correct code are
>    less likely to introduce subtle bugs, as the compiler will reject
>    attempts to modify data that should remain unchanged.

I think all of the above is really a lot of noise that doesn't add a huge
amount of value.

Please in your own words say why you are doing it, and also please mention
why you feel it's justified to do:

const <type> *const param

As mentioned on review of 2/12.

I'm pretty well leaning towards - let's just not do the 2nd const at all,
unless there's a really good reason to do so.

There are also legit cases where you might want to reassign a local
variable.

Largely, granted, you shouldn't be reassinging params, but it's pretty
constraining.

>
> The memory management subsystem is a fundamental building block of the
> kernel.  Most higher-level kernel subsystems (filesystems, drivers,
> networking) depend on mm interfaces.  By establishing
> const-correctness at this foundational level:
>
> 1. Enables Propagation: Higher-level subsystems can adopt
>    const-correctness in their own interfaces.  Without const-correct
>    mm functions, filesystems cannot mark their own parameters const
>    when they need to call mm functions.
>
> 2. Maximum Impact: Changes to core mm APIs benefit the entire kernel, as
>    these functions are called from virtually every subsystem.
>
> 3. Prevents Impedance Mismatch: Without const-correctness in mm, other
>    subsystems must either cast away const (dangerous) or avoid using
>    const altogether (missing optimization opportunities).
>
> Each patch focuses on a specific header or subsystem component to ease review
> and bisection.

All this is unnecessary noise, can you summarise more succinctly.

More words than 'const-ify everything' doesn't mean 'several paragraphs of
noise'.

>
> This work was initially posted as a single large patch:
>  https://lore.kernel.org/lkml/20250827192233.447920-1-max.kellermann@ionos.com/
>
> Following feedback from Lorenzo Stoakes and David Hildenbrand, it has been
> split into focused, reviewable chunks. The approach was validated with a
> smaller patch that received agreement:
>  https://lore.kernel.org/lkml/20250828130311.772993-1-max.kellermann@ionos.com/
>

^--- All of this should be below the line and is associated with versions
not the series as a whole really.

> Signed-off-by: Max Kellermann <max.kellermann@...os.com>
> ---
> v1 -> v2:
> - made several parameter values const (i.e. the pointer address, not
>   just the pointed-to memory), as suggested by Andrew Morton and
>   Yuanchu Xie
> - drop existing+obsolete "extern" keywords on lines modified by these
>   patches (suggested by Vishal Moola)
> - add missing parameter names on lines modified by these patches
>   (suggested by Vishal Moola)
> - more "const" pointers (e.g. the task_struct passed to
>   process_shares_mm())
> - add missing "const" to s390, fixing s390 build failure
> - moved the mmap_is_legacy() change in arch/s390/mm/mmap.c from 08/12
>   to 06/12 (suggested by Vishal Moola)
>
> v2 -> v3:
> - remove garbage from 06/12
> - changed tags on subject line (suggested by Matthew Wilcox)
>
> v3 -> v4:
> - more verbose commit messages including a listing of function names
>   (suggested by David Hildenbrand and Lorenzo Stoakes)
>
> v4 -> v5:
> - back to shorter commit messages after an agreement between David
>   Hildenbrand and Lorenzo Stoakes was found

I did ask you to do this in reverse order and add lore links :) I mean
these aren't big things but it's REALLY helpful for reviewers.

Thanks.

>
> Max Kellermann (12):
>   mm: constify shmem related test functions for improved
>     const-correctness
>   mm: constify pagemap related test functions for improved
>     const-correctness
>   mm: constify zone related test functions for improved
>     const-correctness
>   fs: constify mapping related test functions for improved
>     const-correctness
>   mm: constify process_shares_mm() for improved const-correctness
>   mm, s390: constify mapping related test functions for improved
>     const-correctness
>   parisc: constify mmap_upper_limit() parameter for improved
>     const-correctness
>   mm: constify arch_pick_mmap_layout() for improved const-correctness
>   mm: constify ptdesc_pmd_pts_count() and folio_get_private()
>   mm: constify various inline test functions for improved
>     const-correctness
>   mm: constify assert/test functions in mm.h
>   mm: constify highmem related functions for improved const-correctness
>
>  arch/arm/include/asm/highmem.h      |  6 +--
>  arch/parisc/include/asm/processor.h |  2 +-
>  arch/parisc/kernel/sys_parisc.c     |  2 +-
>  arch/s390/mm/mmap.c                 |  7 ++--
>  arch/sparc/kernel/sys_sparc_64.c    |  3 +-
>  arch/x86/mm/mmap.c                  |  7 ++--
>  arch/xtensa/include/asm/highmem.h   |  2 +-
>  include/linux/fs.h                  |  7 ++--
>  include/linux/highmem-internal.h    | 44 +++++++++++----------
>  include/linux/highmem.h             |  8 ++--
>  include/linux/mm.h                  | 56 +++++++++++++--------------
>  include/linux/mm_inline.h           | 26 +++++++------
>  include/linux/mm_types.h            |  4 +-
>  include/linux/mmzone.h              | 42 ++++++++++----------
>  include/linux/pagemap.h             | 59 +++++++++++++++--------------
>  include/linux/sched/mm.h            |  4 +-
>  include/linux/shmem_fs.h            |  4 +-
>  mm/highmem.c                        | 10 ++---
>  mm/oom_kill.c                       |  7 ++--
>  mm/shmem.c                          |  6 +--
>  mm/util.c                           | 20 ++++++----
>  21 files changed, 171 insertions(+), 155 deletions(-)
>
> --
> 2.47.2
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ