linux-kernel - Re: [PATCH 1/4] mm: Add optional close() to struct vm_special

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 for Android: free password hash cracker in your pocket

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

Message-ID: <shiq5v3jrmyi6ncwke7wgl76ojysgbhrchsk32q4lbx2hadqqc@kzyy2igem256>
Date: Wed, 7 Aug 2024 11:52:58 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Michael Ellerman <mpe@...erman.id.au>
Cc: linux-mm@...ck.org, linuxppc-dev@...ts.ozlabs.org,
        torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
        christophe.leroy@...roup.eu, jeffxu@...omium.org, jeffxu@...gle.com,
        linux-kernel@...r.kernel.org, npiggin@...il.com, oliver.sang@...el.com,
        pedro.falcato@...il.com
Subject: Re: [PATCH 1/4] mm: Add optional close() to struct vm_special_mapping

* Michael Ellerman <mpe@...erman.id.au> [240807 08:41]:
> Add an optional close() callback to struct vm_special_mapping. It will
> be used, by powerpc at least, to handle unmapping of the VDSO.
> 
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Michael Ellerman <mpe@...erman.id.au>
> ---
>  include/linux/mm_types.h | 2 ++
>  mm/mmap.c                | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 485424979254..ef32d87a3adc 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1313,6 +1313,8 @@ struct vm_special_mapping {
>  
>  	int (*mremap)(const struct vm_special_mapping *sm,
>  		     struct vm_area_struct *new_vma);

nit: missing new line?

> +	void (*close)(const struct vm_special_mapping *sm,
> +		      struct vm_area_struct *vma);
>  };
>  
>  enum tlb_flush_reason {
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d0dfc85b209b..24bd6aa9155c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3624,6 +3624,9 @@ static vm_fault_t special_mapping_fault(struct vm_fault *vmf);
>   */

The above comment should probably be expanded to explain what this is
about, or removed.

>  static void special_mapping_close(struct vm_area_struct *vma)
>  {
> +	const struct vm_special_mapping *sm = vma->vm_private_data;
> +	if (sm->close)
> +		sm->close(sm, vma);

Right now we have the same sort of situation for mremap calls on
special: we have a call to the specific vma mremap() function.

However, every single one of the vdso mremap() calls that I see:
s390, riscv, powerppc, parisc, loongarch, arm64, arm

seems to do the same thing, except ppc which verifies the size is okay
before doing the same thing.

So, are we missing an opportunity to avoid every arch having the same
implementation here (that will evolve into random bugs existing in some
archs for years before someone realises the cloned code wasn't fixed)?
Do we already have a fix in ppc for the size checking that doesn't exist
in the other archs in the case of mremap?

That is, if it's a special mapping that has the same start as the vdso,
can't all platforms do the same thing and set it to NULL and avoid every
platform cloning the same function?

Since this deals with mm_context_t, which is per-platform data, I think
the easiest way to make this more generic is to make a
generic_vdso_close() and set it in specific vmas on a per-platform
basis.  At least then we can use the same close function across multiple
platforms and make this less error prone to cloned code not receiving
fixes.

...

Thanks,
Liam