* [PATCH v2 0/2] Fix issues with vmalloc flush flag @ 2019-05-20 23:38 Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Rick Edgecombe @ 2019-05-20 23:38 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, davem, Rick Edgecombe These two patches address issues with the recently added VM_FLUSH_RESET_PERMS vmalloc flag. It is now split into two patches, which made sense to me, but can split it further if desired. Patch 1 is the most critical and addresses an issue that could cause a crash on x86. Patch 2 is to try to reduce the work done in the free operation to push it to allocation time where it would be more expected. This shouldn't be a big issue most of the time, but I thought it was slightly better. 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: Remove work as from vfree path mm/vmalloc.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] vmalloc: Fix calculation of direct map addr range 2019-05-20 23:38 [PATCH v2 0/2] Fix issues with vmalloc flush flag Rick Edgecombe @ 2019-05-20 23:38 ` Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 2/2] vmalloc: Remove work as from vfree path Rick Edgecombe 2019-05-20 23:46 ` [PATCH v2 0/2] Fix issues with vmalloc flush flag Edgecombe, Rick P 2 siblings, 0 replies; 8+ messages in thread From: Rick Edgecombe @ 2019-05-20 23:38 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, davem, Rick Edgecombe, Meelis Roos, Borislav Petkov, Andy Lutomirski, Ingo Molnar, Rick Edgecombe From: Rick Edgecombe <redgecombe.lkml@gmail.com> 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] 8+ messages in thread
* [PATCH v2 2/2] vmalloc: Remove work as from vfree path 2019-05-20 23:38 [PATCH v2 0/2] Fix issues with vmalloc flush flag Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe @ 2019-05-20 23:38 ` Rick Edgecombe 2019-05-21 16:17 ` Andy Lutomirski 2019-05-20 23:46 ` [PATCH v2 0/2] Fix issues with vmalloc flush flag Edgecombe, Rick P 2 siblings, 1 reply; 8+ messages in thread From: Rick Edgecombe @ 2019-05-20 23:38 UTC (permalink / raw) To: linux-kernel, peterz, sparclinux, linux-mm, netdev, luto Cc: dave.hansen, namit, davem, Rick Edgecombe, Meelis Roos, Borislav Petkov, Andy Lutomirski, Ingo Molnar, Rick Edgecombe From: Rick Edgecombe <redgecombe.lkml@gmail.com> Calling vm_unmap_alias() in vm_remove_mappings() could potentially be a lot of work to do on a free operation. Simply flushing the TLB instead of the whole vm_unmap_alias() operation makes the frees faster and pushes the heavy work to happen on allocation where it would be more expected. In addition to the extra work, vm_unmap_alias() takes some locks including a long hold of vmap_purge_lock, which will make all other VM_FLUSH_RESET_PERMS vfrees wait while the purge operation happens. Lastly, page_address() can involve locking and lookups on some configurations, so skip calling this by exiting out early when !CONFIG_ARCH_HAS_SET_DIRECT_MAP. 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 | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 836888ae01f6..8d03427626dc 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2122,9 +2122,10 @@ static inline void set_area_direct_map(const struct vm_struct *area, /* Handle removing and resetting vm mappings related to the vm_struct. */ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) { + const bool has_set_direct = IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP); + const bool flush_reset = area->flags & VM_FLUSH_RESET_PERMS; unsigned long addr = (unsigned long)area->addr; - unsigned long start = ULONG_MAX, end = 0; - int flush_reset = area->flags & VM_FLUSH_RESET_PERMS; + unsigned long start = addr, end = addr + area->size; int i; /* @@ -2133,7 +2134,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) * This is concerned with resetting the direct map any an vm alias with * execute permissions, without leaving a RW+X window. */ - if (flush_reset && !IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) { + if (flush_reset && !has_set_direct) { set_memory_nx(addr, area->nr_pages); set_memory_rw(addr, area->nr_pages); } @@ -2146,17 +2147,18 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages) /* * If not deallocating pages, just do the flush of the VM area and - * return. + * return. If the arch doesn't have set_direct_map_(), also skip the + * below work. */ - if (!deallocate_pages) { - vm_unmap_aliases(); + if (!deallocate_pages || !has_set_direct) { + flush_tlb_kernel_range(start, end); return; } /* * If execution gets here, flush the vm mapping and reset the direct * map. Find the start and end range of the direct mappings to make sure - * the vm_unmap_aliases() flush includes the direct map. + * the flush_tlb_kernel_range() includes the direct map. */ for (i = 0; i < area->nr_pages; i++) { addr = (unsigned long)page_address(area->pages[i]); @@ -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); + flush_tlb_kernel_range(start, end); set_area_direct_map(area, set_direct_map_default_noflush); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path 2019-05-20 23:38 ` [PATCH v2 2/2] vmalloc: Remove work as from vfree path Rick Edgecombe @ 2019-05-21 16:17 ` Andy Lutomirski 2019-05-21 16:51 ` Edgecombe, Rick P 0 siblings, 1 reply; 8+ messages in thread From: Andy Lutomirski @ 2019-05-21 16:17 UTC (permalink / raw) To: Rick Edgecombe Cc: LKML, Peter Zijlstra, sparclinux, Linux-MM, Network Development, Dave Hansen, Nadav Amit, David S. Miller, Rick Edgecombe, Meelis Roos, Borislav Petkov, Andy Lutomirski, Ingo Molnar On Mon, May 20, 2019 at 4:39 PM Rick Edgecombe <rick.p.edgecombe@intel.com> wrote: > > From: Rick Edgecombe <redgecombe.lkml@gmail.com> > > Calling vm_unmap_alias() in vm_remove_mappings() could potentially be a > lot of work to do on a free operation. Simply flushing the TLB instead of > the whole vm_unmap_alias() operation makes the frees faster and pushes > the heavy work to happen on allocation where it would be more expected. > In addition to the extra work, vm_unmap_alias() takes some locks including > a long hold of vmap_purge_lock, which will make all other > VM_FLUSH_RESET_PERMS vfrees wait while the purge operation happens. > > Lastly, page_address() can involve locking and lookups on some > configurations, so skip calling this by exiting out early when > !CONFIG_ARCH_HAS_SET_DIRECT_MAP. Hmm. I would have expected that the major cost of vm_unmap_aliases() would be the flush, and at least informing the code that the flush happened seems valuable. So would guess that this patch is actually a loss in throughput. --Andy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path 2019-05-21 16:17 ` Andy Lutomirski @ 2019-05-21 16:51 ` Edgecombe, Rick P 2019-05-21 17:00 ` Andy Lutomirski 0 siblings, 1 reply; 8+ messages in thread From: Edgecombe, Rick P @ 2019-05-21 16:51 UTC (permalink / raw) To: luto Cc: linux-kernel, peterz, linux-mm, mroos, redgecombe.lkml, mingo, namit, netdev, Hansen, Dave, bp, davem, sparclinux On Tue, 2019-05-21 at 09:17 -0700, Andy Lutomirski wrote: > On Mon, May 20, 2019 at 4:39 PM Rick Edgecombe > <rick.p.edgecombe@intel.com> wrote: > > From: Rick Edgecombe <redgecombe.lkml@gmail.com> > > > > Calling vm_unmap_alias() in vm_remove_mappings() could potentially > > be a > > lot of work to do on a free operation. Simply flushing the TLB > > instead of > > the whole vm_unmap_alias() operation makes the frees faster and > > pushes > > the heavy work to happen on allocation where it would be more > > expected. > > In addition to the extra work, vm_unmap_alias() takes some locks > > including > > a long hold of vmap_purge_lock, which will make all other > > VM_FLUSH_RESET_PERMS vfrees wait while the purge operation happens. > > > > Lastly, page_address() can involve locking and lookups on some > > configurations, so skip calling this by exiting out early when > > !CONFIG_ARCH_HAS_SET_DIRECT_MAP. > > Hmm. I would have expected that the major cost of vm_unmap_aliases() > would be the flush, and at least informing the code that the flush > happened seems valuable. So would guess that this patch is actually > a > loss in throughput. > You are probably right about the flush taking the longest. The original idea of using it was exactly to improve throughput by saving a flush. However with vm_unmap_aliases() the flush will be over a larger range than before for most arch's since it will likley span from the module space to vmalloc. From poking around the sparc tlb flush history, I guess the lazy purges used to be (still are?) a problem for them because it would try to flush each page individually for some CPUs. Not sure about all of the other architectures, but for any implementation like that, using vm_unmap_alias() would turn an occasional long operation into a more frequent one. On x86, it shouldn't be a problem to use it. We already used to call this function several times around a exec permission vfree. I guess its a tradeoff that depends on how fast large range TLB flushes usually are compared to small ones. I am ok dropping it, if it doesn't seem worth it. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path 2019-05-21 16:51 ` Edgecombe, Rick P @ 2019-05-21 17:00 ` Andy Lutomirski 2019-05-21 19:47 ` Edgecombe, Rick P 0 siblings, 1 reply; 8+ messages in thread From: Andy Lutomirski @ 2019-05-21 17:00 UTC (permalink / raw) To: Edgecombe, Rick P Cc: luto, linux-kernel, peterz, linux-mm, mroos, redgecombe.lkml, mingo, namit, netdev, Hansen, Dave, bp, davem, sparclinux On Tue, May 21, 2019 at 9:51 AM Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote: > > On Tue, 2019-05-21 at 09:17 -0700, Andy Lutomirski wrote: > > On Mon, May 20, 2019 at 4:39 PM Rick Edgecombe > > <rick.p.edgecombe@intel.com> wrote: > > > From: Rick Edgecombe <redgecombe.lkml@gmail.com> > > > > > > Calling vm_unmap_alias() in vm_remove_mappings() could potentially > > > be a > > > lot of work to do on a free operation. Simply flushing the TLB > > > instead of > > > the whole vm_unmap_alias() operation makes the frees faster and > > > pushes > > > the heavy work to happen on allocation where it would be more > > > expected. > > > In addition to the extra work, vm_unmap_alias() takes some locks > > > including > > > a long hold of vmap_purge_lock, which will make all other > > > VM_FLUSH_RESET_PERMS vfrees wait while the purge operation happens. > > > > > > Lastly, page_address() can involve locking and lookups on some > > > configurations, so skip calling this by exiting out early when > > > !CONFIG_ARCH_HAS_SET_DIRECT_MAP. > > > > Hmm. I would have expected that the major cost of vm_unmap_aliases() > > would be the flush, and at least informing the code that the flush > > happened seems valuable. So would guess that this patch is actually > > a > > loss in throughput. > > > You are probably right about the flush taking the longest. The original > idea of using it was exactly to improve throughput by saving a flush. > However with vm_unmap_aliases() the flush will be over a larger range > than before for most arch's since it will likley span from the module > space to vmalloc. From poking around the sparc tlb flush history, I > guess the lazy purges used to be (still are?) a problem for them > because it would try to flush each page individually for some CPUs. Not > sure about all of the other architectures, but for any implementation > like that, using vm_unmap_alias() would turn an occasional long > operation into a more frequent one. > > On x86, it shouldn't be a problem to use it. We already used to call > this function several times around a exec permission vfree. > > I guess its a tradeoff that depends on how fast large range TLB flushes > usually are compared to small ones. I am ok dropping it, if it doesn't > seem worth it. On x86, a full flush is probably not much slower than just flushing a page or two -- the main cost is in the TLB refill. I don't know about other architectures. I would drop this patch unless you have numbers suggesting that it's a win. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path 2019-05-21 17:00 ` Andy Lutomirski @ 2019-05-21 19:47 ` Edgecombe, Rick P 0 siblings, 0 replies; 8+ messages in thread From: Edgecombe, Rick P @ 2019-05-21 19:47 UTC (permalink / raw) To: luto Cc: linux-kernel, peterz, linux-mm, mroos, redgecombe.lkml, mingo, namit, netdev, Hansen, Dave, bp, davem, sparclinux On Tue, 2019-05-21 at 10:00 -0700, Andy Lutomirski wrote: > On Tue, May 21, 2019 at 9:51 AM Edgecombe, Rick P > <rick.p.edgecombe@intel.com> wrote: > > On Tue, 2019-05-21 at 09:17 -0700, Andy Lutomirski wrote: > > > On Mon, May 20, 2019 at 4:39 PM Rick Edgecombe > > > <rick.p.edgecombe@intel.com> wrote: > > > > From: Rick Edgecombe <redgecombe.lkml@gmail.com> > > > > > > > > Calling vm_unmap_alias() in vm_remove_mappings() could > > > > potentially > > > > be a > > > > lot of work to do on a free operation. Simply flushing the TLB > > > > instead of > > > > the whole vm_unmap_alias() operation makes the frees faster and > > > > pushes > > > > the heavy work to happen on allocation where it would be more > > > > expected. > > > > In addition to the extra work, vm_unmap_alias() takes some > > > > locks > > > > including > > > > a long hold of vmap_purge_lock, which will make all other > > > > VM_FLUSH_RESET_PERMS vfrees wait while the purge operation > > > > happens. > > > > > > > > Lastly, page_address() can involve locking and lookups on some > > > > configurations, so skip calling this by exiting out early when > > > > !CONFIG_ARCH_HAS_SET_DIRECT_MAP. > > > > > > Hmm. I would have expected that the major cost of > > > vm_unmap_aliases() > > > would be the flush, and at least informing the code that the > > > flush > > > happened seems valuable. So would guess that this patch is > > > actually > > > a > > > loss in throughput. > > > > > You are probably right about the flush taking the longest. The > > original > > idea of using it was exactly to improve throughput by saving a > > flush. > > However with vm_unmap_aliases() the flush will be over a larger > > range > > than before for most arch's since it will likley span from the > > module > > space to vmalloc. From poking around the sparc tlb flush history, I > > guess the lazy purges used to be (still are?) a problem for them > > because it would try to flush each page individually for some CPUs. > > Not > > sure about all of the other architectures, but for any > > implementation > > like that, using vm_unmap_alias() would turn an occasional long > > operation into a more frequent one. > > > > On x86, it shouldn't be a problem to use it. We already used to > > call > > this function several times around a exec permission vfree. > > > > I guess its a tradeoff that depends on how fast large range TLB > > flushes > > usually are compared to small ones. I am ok dropping it, if it > > doesn't > > seem worth it. > > On x86, a full flush is probably not much slower than just flushing a > page or two -- the main cost is in the TLB refill. I don't know > about > other architectures. I would drop this patch unless you have numbers > suggesting that it's a win. Ok. This patch also inadvertently improved some correctness in calls to flush_tlb_kernel_range() for a rare situation. I'll work that into a different patch. Thanks, Rick ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/2] Fix issues with vmalloc flush flag 2019-05-20 23:38 [PATCH v2 0/2] Fix issues with vmalloc flush flag Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 2/2] vmalloc: Remove work as from vfree path Rick Edgecombe @ 2019-05-20 23:46 ` Edgecombe, Rick P 2 siblings, 0 replies; 8+ messages in thread From: Edgecombe, Rick P @ 2019-05-20 23:46 UTC (permalink / raw) To: linux-kernel, peterz, linux-mm, netdev, sparclinux, luto Cc: davem, namit, Hansen, Dave On Mon, 2019-05-20 at 16:38 -0700, Rick Edgecombe wrote: > These two patches address issues with the recently added > VM_FLUSH_RESET_PERMS vmalloc flag. It is now split into two patches, > which > made sense to me, but can split it further if desired. > Oops, this was supposed to say PATCH v3. Let me know if I should resend. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-21 19:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-20 23:38 [PATCH v2 0/2] Fix issues with vmalloc flush flag Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 1/2] vmalloc: Fix calculation of direct map addr range Rick Edgecombe 2019-05-20 23:38 ` [PATCH v2 2/2] vmalloc: Remove work as from vfree path Rick Edgecombe 2019-05-21 16:17 ` Andy Lutomirski 2019-05-21 16:51 ` Edgecombe, Rick P 2019-05-21 17:00 ` Andy Lutomirski 2019-05-21 19:47 ` Edgecombe, Rick P 2019-05-20 23:46 ` [PATCH v2 0/2] Fix issues with vmalloc flush flag Edgecombe, Rick P
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).