| Message ID | 20240807124103.85644-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] mm: Add optional close() to struct vm_special_mapping | expand |
On 07.08.24 14:41, Michael Ellerman wrote: > 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); > + 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); > */ > static void special_mapping_close(struct vm_area_struct *vma) > { > + const struct vm_special_mapping *sm = vma->vm_private_data; I'm old-fashioned, I enjoy an empty line here ;) > + if (sm->close) > + sm->close(sm, vma); Reviewed-by: David Hildenbrand <david@redhat.com>
* Michael Ellerman <mpe@ellerman.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@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? > + 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
"Liam R. Howlett" <Liam.Howlett@oracle.com> writes: > * Michael Ellerman <mpe@ellerman.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@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. >> + 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. >> 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
David Hildenbrand <david@redhat.com> writes: > On 07.08.24 14:41, Michael Ellerman wrote: >> 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/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); >> */ >> static void special_mapping_close(struct vm_area_struct *vma) >> { >> + const struct vm_special_mapping *sm = vma->vm_private_data; > > I'm old-fashioned, I enjoy an empty line here ;) Ack. >> + if (sm->close) >> + sm->close(sm, vma); > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks. cheers
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); + 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); */ 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); } static const char *special_mapping_name(struct vm_area_struct *vma)
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(+)