lkml.org 
[lkml]   [2021]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v11 2/6] arm64: kvm: Introduce MTE VM feature
    On Wed, May 12, 2021 at 04:46:48PM +0100, Steven Price wrote:
    > On 10/05/2021 19:35, Catalin Marinas wrote:
    > > On Fri, May 07, 2021 at 07:25:39PM +0100, Catalin Marinas wrote:
    > > > On Thu, May 06, 2021 at 05:15:25PM +0100, Steven Price wrote:
    > > > > On 04/05/2021 18:40, Catalin Marinas wrote:
    > > > > > On Thu, Apr 29, 2021 at 05:06:41PM +0100, Steven Price wrote:
    > > > > > > Given the changes to set_pte_at() which means that tags are restored from
    > > > > > > swap even if !PROT_MTE, the only race I can see remaining is the creation of
    > > > > > > new PROT_MTE mappings. As you mention an attempt to change mappings in the
    > > > > > > VMM memory space should involve a mmu notifier call which I think serialises
    > > > > > > this. So the remaining issue is doing this in a separate address space.
    > > > > > >
    > > > > > > So I guess the potential problem is:
    > > > > > >
    > > > > > > * allocate memory MAP_SHARED but !PROT_MTE
    > > > > > > * fork()
    > > > > > > * VM causes a fault in parent address space
    > > > > > > * child does a mprotect(PROT_MTE)
    > > > > > >
    > > > > > > With the last two potentially racing. Sadly I can't see a good way of
    > > > > > > handling that.
    [...]
    > > Options:
    > >
    > > 1. Change the mte_sync_tags() code path to set the flag after clearing
    > > and avoid reading stale tags. We document that mprotect() on
    > > MAP_SHARED may lead to tag loss. Maybe we can intercept this in the
    > > arch code and return an error.
    >
    > This is the best option I've come up with so far - but it's not a good
    > one! We can replace the set_bit() with a test_and_set_bit() to catch the
    > race after it has occurred - but I'm not sure what we can do about it
    > then (we've already wiped the data). Returning an error doesn't seem
    > particularly useful at that point, a message in dmesg is about the best
    > I can come up with.

    What I meant about intercepting is on something like
    arch_validate_flags() to prevent VM_SHARED and VM_MTE together but only
    for mprotect(), not mmap(). However, arch_validate_flags() is currently
    called on both mmap() and mprotect() paths.

    We can't do much in set_pte_at() to prevent the race with only a single
    bit.

    > > 2. Figure out some other locking in the core code. However, if
    > > mprotect() in one process can race with a handle_pte_fault() in
    > > another, on the same shared mapping, it's not trivial.
    > > filemap_map_pages() would take the page lock before calling
    > > do_set_pte(), so mprotect() would need the same page lock.
    >
    > I can't see how this is going to work without harming the performance of
    > non-MTE work. Ultimately we're trying to add some sort of locking for
    > two (mostly) unrelated processes doing page table operations, which will
    > hurt scalability.

    Another option is to have an arch callback to force re-faulting on the
    pte. That means we don't populate it back after the invalidation in the
    change_protection() path. We could do this only if the new pte is tagged
    and the page doesn't have PG_mte_tagged. The faulting path takes the
    page lock IIUC.

    Well, at least for stage 1, I haven't thought much about stage 2.

    > > 3. Use another PG_arch_3 bit as a lock to spin on in the arch code (i.e.
    > > set it around the other PG_arch_* bit setting).
    >
    > This is certainly tempting, although sadly the existing
    > wait_on_page_bit() is sleeping - so this would either be a literal spin,
    > or we'd need to implement a new non-sleeping wait mechanism.

    Yeah, it would have to be a custom spinning mechanism, something like:

    /* lock the page */
    while (test_and_set_bit(PG_arch_3, &page->flags))
    smp_cond_load_relaxed(&page->flags, !(VAL & PG_arch_3));
    ...
    /* unlock the page */
    clear_bit(PG_arch_3, &page->flags);

    > 4. Sledgehammer locking in mte_sync_page_tags(), add a spinlock only for
    > the MTE case where we have to sync tags (see below). What the actual
    > performance impact of this is I've no idea. It could certainly be bad
    > if there are a lot of pages with MTE enabled, which sadly is exactly
    > the case if KVM is used with MTE :(
    >
    > --->8----
    > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
    > index 0d320c060ebe..389ad40256f6 100644
    > --- a/arch/arm64/kernel/mte.c
    > +++ b/arch/arm64/kernel/mte.c
    > @@ -25,23 +25,33 @@
    > u64 gcr_kernel_excl __ro_after_init;
    > static bool report_fault_once = true;
    > +static spinlock_t tag_sync_lock;
    > static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap,
    > bool pte_is_tagged)
    > {
    > pte_t old_pte = READ_ONCE(*ptep);
    > + if (!is_swap_pte(old_pte) && !pte_is_tagged)
    > + return;
    > +
    > + spin_lock_irqsave(&tag_sync_lock, flags);
    > +
    > + /* Recheck with the lock held */
    > + if (test_bit(PG_mte_tagged, &page->flags))
    > + goto out;

    Can we skip the lock if the page already has the PG_mte_tagged set?
    That's assuming that we set the flag only after clearing the tags. The
    normal case where mprotect() is called on a page already mapped with
    PROT_MTE would not be affected.

    --
    Catalin

    \
     
     \ /
      Last update: 2021-05-12 23:31    [W:2.744 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site