| Message ID | 1530311985-31251-6-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/06/2018 00:39, Yang Shi wrote: > Check VM_DEAD flag of vma in page fault handler, if it is set, trigger > SIGSEGV. > > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > --- > arch/x86/mm/fault.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 9a84a0d..3fd2da5 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) > bad_area(regs, error_code, address); > return; > } > + if (unlikely(vma->vm_flags & VM_DEAD)) { > + bad_area(regs, error_code, address); > + return; > + } This will have to be done for all the supported architectures, what about doing this check in handle_mm_fault() and return VM_FAULT_SIGSEGV ? > if (error_code & X86_PF_USER) { > /* > * Accessing the stack below %sp is always a bug. >
On Mon 02-07-18 10:45:03, Laurent Dufour wrote: > On 30/06/2018 00:39, Yang Shi wrote: > > Check VM_DEAD flag of vma in page fault handler, if it is set, trigger > > SIGSEGV. > > > > Cc: Michal Hocko <mhocko@kernel.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > > --- > > arch/x86/mm/fault.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > > index 9a84a0d..3fd2da5 100644 > > --- a/arch/x86/mm/fault.c > > +++ b/arch/x86/mm/fault.c > > @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) > > bad_area(regs, error_code, address); > > return; > > } > > + if (unlikely(vma->vm_flags & VM_DEAD)) { > > + bad_area(regs, error_code, address); > > + return; > > + } > > This will have to be done for all the supported architectures, what about doing > this check in handle_mm_fault() and return VM_FAULT_SIGSEGV ? We already do have a model for that. Have a look at MMF_UNSTABLE.
On 02/07/2018 14:15, Michal Hocko wrote: > On Mon 02-07-18 10:45:03, Laurent Dufour wrote: >> On 30/06/2018 00:39, Yang Shi wrote: >>> Check VM_DEAD flag of vma in page fault handler, if it is set, trigger >>> SIGSEGV. >>> >>> Cc: Michal Hocko <mhocko@kernel.org> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: "H. Peter Anvin" <hpa@zytor.com> >>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>> --- >>> arch/x86/mm/fault.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >>> index 9a84a0d..3fd2da5 100644 >>> --- a/arch/x86/mm/fault.c >>> +++ b/arch/x86/mm/fault.c >>> @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) >>> bad_area(regs, error_code, address); >>> return; >>> } >>> + if (unlikely(vma->vm_flags & VM_DEAD)) { >>> + bad_area(regs, error_code, address); >>> + return; >>> + } >> >> This will have to be done for all the supported architectures, what about doing >> this check in handle_mm_fault() and return VM_FAULT_SIGSEGV ? > > We already do have a model for that. Have a look at MMF_UNSTABLE. MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked.
On Mon 02-07-18 14:26:09, Laurent Dufour wrote: > On 02/07/2018 14:15, Michal Hocko wrote: > > On Mon 02-07-18 10:45:03, Laurent Dufour wrote: > >> On 30/06/2018 00:39, Yang Shi wrote: > >>> Check VM_DEAD flag of vma in page fault handler, if it is set, trigger > >>> SIGSEGV. > >>> > >>> Cc: Michal Hocko <mhocko@kernel.org> > >>> Cc: Thomas Gleixner <tglx@linutronix.de> > >>> Cc: Ingo Molnar <mingo@redhat.com> > >>> Cc: "H. Peter Anvin" <hpa@zytor.com> > >>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > >>> --- > >>> arch/x86/mm/fault.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > >>> index 9a84a0d..3fd2da5 100644 > >>> --- a/arch/x86/mm/fault.c > >>> +++ b/arch/x86/mm/fault.c > >>> @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) > >>> bad_area(regs, error_code, address); > >>> return; > >>> } > >>> + if (unlikely(vma->vm_flags & VM_DEAD)) { > >>> + bad_area(regs, error_code, address); > >>> + return; > >>> + } > >> > >> This will have to be done for all the supported architectures, what about doing > >> this check in handle_mm_fault() and return VM_FAULT_SIGSEGV ? > > > > We already do have a model for that. Have a look at MMF_UNSTABLE. > > MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. Yeah, and we have the VMA ready for all places where we do check the flag. check_stable_address_space can be made to get vma rather than mm.
On 02/07/2018 14:45, Michal Hocko wrote: > On Mon 02-07-18 14:26:09, Laurent Dufour wrote: >> On 02/07/2018 14:15, Michal Hocko wrote: >>> On Mon 02-07-18 10:45:03, Laurent Dufour wrote: >>>> On 30/06/2018 00:39, Yang Shi wrote: >>>>> Check VM_DEAD flag of vma in page fault handler, if it is set, trigger >>>>> SIGSEGV. >>>>> >>>>> Cc: Michal Hocko <mhocko@kernel.org> >>>>> Cc: Thomas Gleixner <tglx@linutronix.de> >>>>> Cc: Ingo Molnar <mingo@redhat.com> >>>>> Cc: "H. Peter Anvin" <hpa@zytor.com> >>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>>>> --- >>>>> arch/x86/mm/fault.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >>>>> index 9a84a0d..3fd2da5 100644 >>>>> --- a/arch/x86/mm/fault.c >>>>> +++ b/arch/x86/mm/fault.c >>>>> @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) >>>>> bad_area(regs, error_code, address); >>>>> return; >>>>> } >>>>> + if (unlikely(vma->vm_flags & VM_DEAD)) { >>>>> + bad_area(regs, error_code, address); >>>>> + return; >>>>> + } >>>> >>>> This will have to be done for all the supported architectures, what about doing >>>> this check in handle_mm_fault() and return VM_FAULT_SIGSEGV ? >>> >>> We already do have a model for that. Have a look at MMF_UNSTABLE. >> >> MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. > > Yeah, and we have the VMA ready for all places where we do check the > flag. check_stable_address_space can be made to get vma rather than mm. Yeah, this would have been more efficient to check that flag at the beginning of the page fault handler rather than the end, but this way it will be easier to handle the speculative page fault too ;)
On Mon 02-07-18 15:33:11, Laurent Dufour wrote: > > > On 02/07/2018 14:45, Michal Hocko wrote: > > On Mon 02-07-18 14:26:09, Laurent Dufour wrote: > >> On 02/07/2018 14:15, Michal Hocko wrote: [...] > >>> We already do have a model for that. Have a look at MMF_UNSTABLE. > >> > >> MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. > > > > Yeah, and we have the VMA ready for all places where we do check the > > flag. check_stable_address_space can be made to get vma rather than mm. > > Yeah, this would have been more efficient to check that flag at the beginning > of the page fault handler rather than the end, but this way it will be easier > to handle the speculative page fault too ;) The thing is that it doesn't really need to be called earlier. You are not risking data corruption on file backed mappings.
On 7/2/18 6:37 AM, Michal Hocko wrote: > On Mon 02-07-18 15:33:11, Laurent Dufour wrote: >> >> On 02/07/2018 14:45, Michal Hocko wrote: >>> On Mon 02-07-18 14:26:09, Laurent Dufour wrote: >>>> On 02/07/2018 14:15, Michal Hocko wrote: > [...] >>>>> We already do have a model for that. Have a look at MMF_UNSTABLE. >>>> MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. >>> Yeah, and we have the VMA ready for all places where we do check the >>> flag. check_stable_address_space can be made to get vma rather than mm. >> Yeah, this would have been more efficient to check that flag at the beginning >> of the page fault handler rather than the end, but this way it will be easier >> to handle the speculative page fault too ;) > The thing is that it doesn't really need to be called earlier. You are > not risking data corruption on file backed mappings. OK, I just think it could save a few cycles to check the flag earlier. If nobody think it is necessary, we definitely could re-use check_stable_address_space(), just return VM_FAULT_SIGSEGV for VM_DEAD vma, and check for both shared and non-shared. Thanks, Yang >
On Mon 02-07-18 10:24:27, Yang Shi wrote: > > > On 7/2/18 6:37 AM, Michal Hocko wrote: > > On Mon 02-07-18 15:33:11, Laurent Dufour wrote: > > > > > > On 02/07/2018 14:45, Michal Hocko wrote: > > > > On Mon 02-07-18 14:26:09, Laurent Dufour wrote: > > > > > On 02/07/2018 14:15, Michal Hocko wrote: > > [...] > > > > > > We already do have a model for that. Have a look at MMF_UNSTABLE. > > > > > MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. > > > > Yeah, and we have the VMA ready for all places where we do check the > > > > flag. check_stable_address_space can be made to get vma rather than mm. > > > Yeah, this would have been more efficient to check that flag at the beginning > > > of the page fault handler rather than the end, but this way it will be easier > > > to handle the speculative page fault too ;) > > The thing is that it doesn't really need to be called earlier. You are > > not risking data corruption on file backed mappings. > > OK, I just think it could save a few cycles to check the flag earlier. This should be an extremely rare case. Just think about it. It should only ever happen when an access races with munmap which itself is questionable if not an outright bug. > If nobody think it is necessary, we definitely could re-use > check_stable_address_space(), If we really need this whole VM_DEAD thing then it should be better handled at the same place rather than some ad-hoc places. > just return VM_FAULT_SIGSEGV for VM_DEAD vma, > and check for both shared and non-shared. Why would you even care about shared mappings?
On 7/2/18 10:57 AM, Michal Hocko wrote: > On Mon 02-07-18 10:24:27, Yang Shi wrote: >> >> On 7/2/18 6:37 AM, Michal Hocko wrote: >>> On Mon 02-07-18 15:33:11, Laurent Dufour wrote: >>>> On 02/07/2018 14:45, Michal Hocko wrote: >>>>> On Mon 02-07-18 14:26:09, Laurent Dufour wrote: >>>>>> On 02/07/2018 14:15, Michal Hocko wrote: >>> [...] >>>>>>> We already do have a model for that. Have a look at MMF_UNSTABLE. >>>>>> MMF_UNSTABLE is a mm's flag, here this is a VMA's flag which is checked. >>>>> Yeah, and we have the VMA ready for all places where we do check the >>>>> flag. check_stable_address_space can be made to get vma rather than mm. >>>> Yeah, this would have been more efficient to check that flag at the beginning >>>> of the page fault handler rather than the end, but this way it will be easier >>>> to handle the speculative page fault too ;) >>> The thing is that it doesn't really need to be called earlier. You are >>> not risking data corruption on file backed mappings. >> OK, I just think it could save a few cycles to check the flag earlier. > This should be an extremely rare case. Just think about it. It should > only ever happen when an access races with munmap which itself is > questionable if not an outright bug. > >> If nobody think it is necessary, we definitely could re-use >> check_stable_address_space(), > If we really need this whole VM_DEAD thing then it should be better > handled at the same place rather than some ad-hoc places. > >> just return VM_FAULT_SIGSEGV for VM_DEAD vma, >> and check for both shared and non-shared. > Why would you even care about shared mappings? Just thought about we are dealing with VM_DEAD, which means the vma will be tore down soon regardless it is shared or non-shared. MMF_UNSTABLE doesn't care about !shared case.
On Mon 02-07-18 11:10:23, Yang Shi wrote: > On 7/2/18 10:57 AM, Michal Hocko wrote: [...] > > Why would you even care about shared mappings? > > Just thought about we are dealing with VM_DEAD, which means the vma will be > tore down soon regardless it is shared or non-shared. > > MMF_UNSTABLE doesn't care about !shared case. Let me clarify some more. MMF_UNSTABLE is there to prevent from unexpected page faults when the mm is torn down by the oom reaper. And oom reaper only cares about private mappings because we do not touch shared ones. Disk based shared mappings should be a non-issue for VM_DEAD because even if you race and refault a page back then you know it is the same one you have seen before. Memory backed shared mappings are a different story because you can get a fresh new page. oom_reaper doesn't care because it doesn't tear those down. You would have to but my primary point was that we already have MMF_UNSTABLE so all you need is to extend it to memory backed shared mappings (shmem and hugetlb).
On 7/2/18 11:17 PM, Michal Hocko wrote: > On Mon 02-07-18 11:10:23, Yang Shi wrote: >> On 7/2/18 10:57 AM, Michal Hocko wrote: > [...] >>> Why would you even care about shared mappings? >> Just thought about we are dealing with VM_DEAD, which means the vma will be >> tore down soon regardless it is shared or non-shared. >> >> MMF_UNSTABLE doesn't care about !shared case. Sorry, this is a typo, it should be "shared". > Let me clarify some more. MMF_UNSTABLE is there to prevent from > unexpected page faults when the mm is torn down by the oom reaper. And > oom reaper only cares about private mappings because we do not touch > shared ones. Disk based shared mappings should be a non-issue for > VM_DEAD because even if you race and refault a page back then you know > it is the same one you have seen before. Memory backed shared mappings > are a different story because you can get a fresh new page. oom_reaper > doesn't care because it doesn't tear those down. You would have to but > my primary point was that we already have MMF_UNSTABLE so all you need > is to extend it to memory backed shared mappings (shmem and hugetlb). Yes, sure. I think I got your point. Thanks for the elaboration. Yang >
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 9a84a0d..3fd2da5 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1357,6 +1357,10 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs) bad_area(regs, error_code, address); return; } + if (unlikely(vma->vm_flags & VM_DEAD)) { + bad_area(regs, error_code, address); + return; + } if (error_code & X86_PF_USER) { /* * Accessing the stack below %sp is always a bug.
Check VM_DEAD flag of vma in page fault handler, if it is set, trigger SIGSEGV. Cc: Michal Hocko <mhocko@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- arch/x86/mm/fault.c | 4 ++++ 1 file changed, 4 insertions(+)