* [PATCH v4 0/2] Fix issues with vmalloc flush flag @ 2019-05-21 20:51 Rick Edgecombe 2019-05-21 20:51 ` [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe 2019-05-21 20:51 ` [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements Rick Edgecombe 0 siblings, 2 replies; 6+ messages in thread From: Rick Edgecombe @ 2019-05-21 20:51 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, Rick Edgecombe These two patches address issues with the recently added VM_FLUSH_RESET_PERMS vmalloc flag. Patch 1 is the most critical and addresses an issue that could cause a crash on x86. Patch 2 addresses an issue where in a rare case strange arguments could be provided to flush_tlb_kernel_range(). Most arch's should be fine with it, but some may not like them, so we should avoid doing this. v3->v4: - Drop patch that switched vm_unmap_alias() calls to regular flush - Add patch to address correctness previously fixed in dropped patch v2->v3: - Split into two patches v1->v2: - Update commit message with more detail - Fix flush end range on !CONFIG_ARCH_HAS_SET_DIRECT_MAP case Rick Edgecombe (2): vmalloc: Fix calculation of direct map addr range vmalloc: Avoid rare case of flushing tlb with weird arguements mm/vmalloc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range 2019-05-21 20:51 [PATCH v4 0/2] Fix issues with vmalloc flush flag Rick Edgecombe @ 2019-05-21 20:51 ` Rick Edgecombe 2019-05-27 12:20 ` Peter Zijlstra 2019-05-21 20:51 ` [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements Rick Edgecombe 1 sibling, 1 reply; 6+ messages in thread From: Rick Edgecombe @ 2019-05-21 20:51 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, Rick Edgecombe, Meelis Roos, David S. Miller, Borislav Petkov, Ingo Molnar The calculation of the direct map address range to flush was wrong. This could cause problems on x86 if a RO direct map alias ever got loaded into the TLB. This shouldn't normally happen, but it could cause the permissions to remain RO on the direct map alias, and then the page would return from the page allocator to some other component as RO and cause a crash. So fix fix the address range calculation so the flush will include the direct map range. Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions") Cc: Meelis Roos <mroos@linux.ee> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Nadav Amit <namit@vmware.com> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- mm/vmalloc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mm/vmalloc.c b/mm/vmalloc.c index c42872ed82ac..836888ae01f6 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2159,9 +2159,10 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) * the vm_unmap_aliases() flush includes the direct map. */ for (i = 0; i < area->nr_pages; i++) { - if (page_address(area->pages[i])) { + addr = (unsigned long)page_address(area->pages[i]); + if (addr) { start = min(addr, start); - end = max(addr, end); + end = max(addr + PAGE_SIZE, end); } } -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range 2019-05-21 20:51 ` [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe @ 2019-05-27 12:20 ` Peter Zijlstra 2019-05-27 20:05 ` Edgecombe, Rick P 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2019-05-27 12:20 UTC (permalink / raw) To: Rick Edgecombe Cc: linux-kernel, sparclinux, linux-mm, netdev, luto, dave.hansen, namit, Meelis Roos, David S. Miller, Borislav Petkov, Ingo Molnar On Tue, May 21, 2019 at 01:51:36PM -0700, Rick Edgecombe wrote: > The calculation of the direct map address range to flush was wrong. > This could cause problems on x86 if a RO direct map alias ever got loaded > into the TLB. This shouldn't normally happen, but it could cause the > permissions to remain RO on the direct map alias, and then the page > would return from the page allocator to some other component as RO and > cause a crash. > > So fix fix the address range calculation so the flush will include the > direct map range. > > Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions") > Cc: Meelis Roos <mroos@linux.ee> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Nadav Amit <namit@vmware.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > mm/vmalloc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index c42872ed82ac..836888ae01f6 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2159,9 +2159,10 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) > * the vm_unmap_aliases() flush includes the direct map. > */ > for (i = 0; i < area->nr_pages; i++) { > - if (page_address(area->pages[i])) { > + addr = (unsigned long)page_address(area->pages[i]); > + if (addr) { > start = min(addr, start); > - end = max(addr, end); > + end = max(addr + PAGE_SIZE, end); > } > } > Indeed; howevr I'm thinking this bug was caused to exist by the dual use of @addr in this function, so should we not, perhaps, do something like the below instead? Also; having looked at this, it makes me question the use of flush_tlb_kernel_range() in _vm_unmap_aliases() and __purge_vmap_area_lazy(), it's potentially combining multiple ranges, which never really works well. Arguably, we should just do flush_tlb_all() here, but that's for another patch I'm thinking. --- --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2123,7 +2123,6 @@ static inline void set_area_direct_map(c /* Handle removing and resetting vm mappings related to the vm_struct. */ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) { - unsigned long addr = (unsigned long)area->addr; unsigned long start = ULONG_MAX, end = 0; int flush_reset = area->flags & VM_FLUSH_RESET_PERMS; int i; @@ -2135,8 +2134,8 @@ static void vm_remove_mappings(struct vm * execute permissions, without leaving a RW+X window. */ if (flush_reset && !IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) { - set_memory_nx(addr, area->nr_pages); - set_memory_rw(addr, area->nr_pages); + set_memory_nx((unsigned long)area->addr, area->nr_pages); + set_memory_rw((unsigned long)area->addr, area->nr_pages); } remove_vm_area(area->addr); @@ -2160,9 +2159,10 @@ static void vm_remove_mappings(struct vm * the vm_unmap_aliases() flush includes the direct map. */ for (i = 0; i < area->nr_pages; i++) { - if (page_address(area->pages[i])) { + unsigned long addr = (unsigned long)page_address(area->pages[i]); + if (addr) { start = min(addr, start); - end = max(addr, end); + end = max(addr + PAGE_SIZE, end); } } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range 2019-05-27 12:20 ` Peter Zijlstra @ 2019-05-27 20:05 ` Edgecombe, Rick P 0 siblings, 0 replies; 6+ messages in thread From: Edgecombe, Rick P @ 2019-05-27 20:05 UTC (permalink / raw) To: peterz Cc: linux-kernel, linux-mm, mroos, mingo, namit, luto, netdev, Hansen, Dave, bp, davem, sparclinux On Mon, 2019-05-27 at 14:20 +0200, Peter Zijlstra wrote: > On Tue, May 21, 2019 at 01:51:36PM -0700, Rick Edgecombe wrote: > > The calculation of the direct map address range to flush was wrong. > > This could cause problems on x86 if a RO direct map alias ever got > > loaded > > into the TLB. This shouldn't normally happen, but it could cause > > the > > permissions to remain RO on the direct map alias, and then the page > > would return from the page allocator to some other component as RO > > and > > cause a crash. > > > > So fix fix the address range calculation so the flush will include > > the > > direct map range. > > > > Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special > > permsissions") > > Cc: Meelis Roos <mroos@linux.ee> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Dave Hansen <dave.hansen@intel.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Nadav Amit <namit@vmware.com> > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > mm/vmalloc.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index c42872ed82ac..836888ae01f6 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -2159,9 +2159,10 @@ static void vm_remove_mappings(struct > > vm_struct *area, int deallocate_pages) > > * the vm_unmap_aliases() flush includes the direct map. > > */ > > for (i = 0; i < area->nr_pages; i++) { > > - if (page_address(area->pages[i])) { > > + addr = (unsigned long)page_address(area->pages[i]); > > + if (addr) { > > start = min(addr, start); > > - end = max(addr, end); > > + end = max(addr + PAGE_SIZE, end); > > } > > } > > > > Indeed; howevr I'm thinking this bug was caused to exist by the dual > use > of @addr in this function, so should we not, perhaps, do something > like > the below instead? > > Also; having looked at this, it makes me question the use of > flush_tlb_kernel_range() in _vm_unmap_aliases() and > __purge_vmap_area_lazy(), it's potentially combining multiple ranges, > which never really works well. > > Arguably, we should just do flush_tlb_all() here, but that's for > another > patch I'm thinking. Thanks. It mostly got broken implementing a style suggestion late in the series. I'll change the addr variable around like you suggest to make it more resistant. The flush_tlb_all() suggestion makes sense to me, but I'll leave it for now. > --- > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2123,7 +2123,6 @@ static inline void set_area_direct_map(c > /* Handle removing and resetting vm mappings related to the > vm_struct. */ > static void vm_remove_mappings(struct vm_struct *area, int > deallocate_pages) > { > - unsigned long addr = (unsigned long)area->addr; > unsigned long start = ULONG_MAX, end = 0; > int flush_reset = area->flags & VM_FLUSH_RESET_PERMS; > int i; > @@ -2135,8 +2134,8 @@ static void vm_remove_mappings(struct vm > * execute permissions, without leaving a RW+X window. > */ > if (flush_reset && !IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) > { > - set_memory_nx(addr, area->nr_pages); > - set_memory_rw(addr, area->nr_pages); > + set_memory_nx((unsigned long)area->addr, area- > >nr_pages); > + set_memory_rw((unsigned long)area->addr, area- > >nr_pages); > } > > remove_vm_area(area->addr); > @@ -2160,9 +2159,10 @@ static void vm_remove_mappings(struct vm > * the vm_unmap_aliases() flush includes the direct map. > */ > for (i = 0; i < area->nr_pages; i++) { > - if (page_address(area->pages[i])) { > + unsigned long addr = (unsigned long)page_address(area- > >pages[i]); > + if (addr) { > start = min(addr, start); > - end = max(addr, end); > + end = max(addr + PAGE_SIZE, end); > } > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements 2019-05-21 20:51 [PATCH v4 0/2] Fix issues with vmalloc flush flag Rick Edgecombe 2019-05-21 20:51 ` [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe @ 2019-05-21 20:51 ` Rick Edgecombe 2019-05-27 12:24 ` Peter Zijlstra 1 sibling, 1 reply; 6+ messages in thread From: Rick Edgecombe @ 2019-05-21 20:51 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, Rick Edgecombe, Meelis Roos, David S. Miller, Borislav Petkov, Ingo Molnar In a rare case, flush_tlb_kernel_range() could be called with a start higher than the end. Most architectures should be fine with with this, but some may not like it, so avoid doing this. In vm_remove_mappings(), in case page_address() returns 0 for all pages, _vm_unmap_aliases() will be called with start = ULONG_MAX, end = 0 and flush = 1. If at the same time, the vmalloc purge operation is triggered by something else while the current operation is between remove_vm_area() and _vm_unmap_aliases(), then the vm mapping just removed will be already purged. In this case the call of vm_unmap_aliases() may not find any other mappings to flush and so end up flushing start = ULONG_MAX, end = 0. So only set flush = true if we find something in the direct mapping that we need to flush, and this way this can't happen. Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions") Cc: Meelis Roos <mroos@linux.ee> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Nadav Amit <namit@vmware.com> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- mm/vmalloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 836888ae01f6..537d1134b40e 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2125,6 +2125,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) unsigned long addr = (unsigned long)area->addr; unsigned long start = ULONG_MAX, end = 0; int flush_reset = area->flags & VM_FLUSH_RESET_PERMS; + int flush_dmap = 0; int i; /* @@ -2163,6 +2164,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) if (addr) { start = min(addr, start); end = max(addr + PAGE_SIZE, end); + flush_dmap = 1; } } @@ -2172,7 +2174,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) * reset the direct map permissions to the default. */ set_area_direct_map(area, set_direct_map_invalid_noflush); - _vm_unmap_aliases(start, end, 1); + _vm_unmap_aliases(start, end, flush_dmap); set_area_direct_map(area, set_direct_map_default_noflush); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements 2019-05-21 20:51 ` [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements Rick Edgecombe @ 2019-05-27 12:24 ` Peter Zijlstra 0 siblings, 0 replies; 6+ messages in thread From: Peter Zijlstra @ 2019-05-27 12:24 UTC (permalink / raw) To: Rick Edgecombe Cc: linux-kernel, sparclinux, linux-mm, netdev, luto, dave.hansen, namit, Meelis Roos, David S. Miller, Borislav Petkov, Ingo Molnar On Tue, May 21, 2019 at 01:51:37PM -0700, Rick Edgecombe wrote: > In a rare case, flush_tlb_kernel_range() could be called with a start > higher than the end. Most architectures should be fine with with this, but > some may not like it, so avoid doing this. > > In vm_remove_mappings(), in case page_address() returns 0 for all pages, > _vm_unmap_aliases() will be called with start = ULONG_MAX, end = 0 and > flush = 1. > > If at the same time, the vmalloc purge operation is triggered by something > else while the current operation is between remove_vm_area() and > _vm_unmap_aliases(), then the vm mapping just removed will be already > purged. In this case the call of vm_unmap_aliases() may not find any other > mappings to flush and so end up flushing start = ULONG_MAX, end = 0. So > only set flush = true if we find something in the direct mapping that we > need to flush, and this way this can't happen. > > Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions") > Cc: Meelis Roos <mroos@linux.ee> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Nadav Amit <namit@vmware.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > mm/vmalloc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 836888ae01f6..537d1134b40e 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2125,6 +2125,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) > unsigned long addr = (unsigned long)area->addr; > unsigned long start = ULONG_MAX, end = 0; > int flush_reset = area->flags & VM_FLUSH_RESET_PERMS; > + int flush_dmap = 0; > int i; > > /* > @@ -2163,6 +2164,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) > if (addr) { > start = min(addr, start); > end = max(addr + PAGE_SIZE, end); > + flush_dmap = 1; > } > } > > @@ -2172,7 +2174,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) > * reset the direct map permissions to the default. > */ > set_area_direct_map(area, set_direct_map_invalid_noflush); > - _vm_unmap_aliases(start, end, 1); > + _vm_unmap_aliases(start, end, flush_dmap); > set_area_direct_map(area, set_direct_map_default_noflush); > } Hurmph.. another clue that this range flushing is crap I feel. The phys addrs of the page array can be scattered all over the place, a range doesn't properly represent things. But yes, this seems like a minimal fix in spirit with the existing code. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-27 20:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-21 20:51 [PATCH v4 0/2] Fix issues with vmalloc flush flag Rick Edgecombe 2019-05-21 20:51 ` [PATCH v4 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe 2019-05-27 12:20 ` Peter Zijlstra 2019-05-27 20:05 ` Edgecombe, Rick P 2019-05-21 20:51 ` [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements Rick Edgecombe 2019-05-27 12:24 ` Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).