Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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 > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Signed-off-by: Yang Shi > --- > 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. >
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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 > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Signed-off-by: Yang Shi > > --- > > 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. -- Michal Hocko SUSE Labs
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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 >>> Cc: Thomas Gleixner >>> Cc: Ingo Molnar >>> Cc: "H. Peter Anvin" >>> Signed-off-by: Yang Shi >>> --- >>> 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.
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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 > >>> Cc: Thomas Gleixner > >>> Cc: Ingo Molnar > >>> Cc: "H. Peter Anvin" > >>> Signed-off-by: Yang Shi > >>> --- > >>> 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. -- Michal Hocko SUSE Labs
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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 > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Signed-off-by: Yang Shi > --- > 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 ;)
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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. -- Michal Hocko SUSE Labs
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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? -- Michal Hocko SUSE Labs
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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.
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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). -- Michal Hocko SUSE Labs
Re: [RFC v3 PATCH 5/5] x86: check VM_DEAD flag in page fault
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