LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "luto@amacapital.net" <luto@amacapital.net>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"mroos@linux.ee" <mroos@linux.ee>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"namit@vmware.com" <namit@vmware.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>
Subject: Re: [PATCH v2] vmalloc: Fix issues with flush flag
Date: Mon, 20 May 2019 21:48:32 +0000	[thread overview]
Message-ID: <19b6787ce974b07d0d32d2422d0feef557ab443e.camel@intel.com> (raw)
In-Reply-To: <28F28A46-C57B-483A-A5CB-8BEA06AF15F8@amacapital.net>

On Mon, 2019-05-20 at 14:25 -0700, Andy Lutomirski wrote:
> 
> 
> > On May 20, 2019, at 1:07 PM, Rick Edgecombe <
> > rick.p.edgecombe@intel.com> wrote:
> > 
> > Switch VM_FLUSH_RESET_PERMS to use a regular TLB flush intead of
> > vm_unmap_aliases() and fix calculation of the direct map for the
> > CONFIG_ARCH_HAS_SET_DIRECT_MAP case.
> > 
> > Meelis Roos reported issues with the new VM_FLUSH_RESET_PERMS flag
> > on a
> > sparc machine. On investigation some issues were noticed:
> > 
> 
> Can you split this into a few (3?) patches, each fixing one issue?
Sure, I just did one because because it was all in the same function
and the address range calculation needs to be done differently for pure
TLB flush, so its kind of intertwined.

> > 1. 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.
> > 
> > 2. Calling vm_unmap_alias() on vfree 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.
> > 
> > 3. page_address() can have locking on some configurations, so skip
> > calling
> > this when possible to further speed this up.
> > 
> > Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special
> > permsissions")
> > Reported-by: Meelis Roos <mroos@linux.ee>
> > 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>
> > ---
> > 
> > Changes since v1:
> > - Update commit message with more detail
> > - Fix flush end range on !CONFIG_ARCH_HAS_SET_DIRECT_MAP case
> > 
> > mm/vmalloc.c | 23 +++++++++++++----------
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index c42872ed82ac..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,22 +2147,24 @@ 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++) {
> > -        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);
> >        }
> >    }
> > 
> > @@ -2171,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
> > 

  reply	other threads:[~2019-05-20 21:48 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-20 20:07 Rick Edgecombe
2019-05-20 21:25 ` Andy Lutomirski
2019-05-20 21:48   ` Edgecombe, Rick P [this message]
2019-05-20 21:36 ` Meelis Roos
2019-05-20 22:17   ` Edgecombe, Rick P
2019-05-20 22:48     ` David Miller
2019-05-21  0:20       ` Edgecombe, Rick P
2019-05-21  0:33         ` David Miller
2019-05-21  1:20           ` Edgecombe, Rick P
2019-05-21  1:43             ` David Miller
2019-05-21  1:59               ` Edgecombe, Rick P
2019-05-22 17:40                 ` David Miller
2019-05-22 19:26                   ` Edgecombe, Rick P
2019-05-22 22:40                     ` Edgecombe, Rick P
2019-05-24 15:50                       ` Edgecombe, Rick P

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=19b6787ce974b07d0d32d2422d0feef557ab443e.camel@intel.com \
    --to=rick.p.edgecombe@intel.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=davem@davemloft.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mroos@linux.ee \
    --cc=namit@vmware.com \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=sparclinux@vger.kernel.org \
    --subject='Re: [PATCH v2] vmalloc: Fix issues with flush flag' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).