Re: [PATCH 1/4] mm: Add optional close() to struct vm_special_mapping
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2024-08-12 08:22:47
Also in:
linux-mm, lkml
"Liam R. Howlett" [off-list ref] writes:
* Michael Ellerman [off-list ref] [240807 08:41]:quoted
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@linux-foundation.org> Signed-off-by: Michael Ellerman <mpe@ellerman.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?
Ack.
quoted
+ 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.
I expanded it slightly, happy for others to wordsmith it further.
quoted
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. ... 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?
I took this as more of a meta comment/rant :) Yes I agree the implementation should eventually be generic, but this series is just about moving the existing powerpc behaviour from arch_unmap() into this hook. cheers