linux-kernel - Re: [PATCH v3 00/11] Introduces new count-based method for monitoring lockless pagetable walks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Sep 2019 11:46:04 -0300
From:   Leonardo Bras <leonardo@...ux.ibm.com>
To:     jhubbard@...dia.com, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, kvm-ppc@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-mm@...ck.org
Cc:     benh@...nel.crashing.org, paulus@...ba.org, mpe@...erman.id.au,
        arnd@...db.de, aneesh.kumar@...ux.ibm.com, christophe.leroy@....fr,
        akpm@...ux-foundation.org, dan.j.williams@...el.com,
        npiggin@...il.com, mahesh@...ux.vnet.ibm.com,
        gregkh@...uxfoundation.org, tglx@...utronix.de,
        ganeshgr@...ux.ibm.com, allison@...utok.net, rppt@...ux.ibm.com,
        yuehaibing@...wei.com, ira.weiny@...el.com, jgg@...pe.ca,
        keith.busch@...el.com
Subject: Re: [PATCH v3 00/11] Introduces new count-based method for
 monitoring lockless pagetable walks

John Hubbard <jhubbard@...dia.com> writes:

> Hi Leonardo,
>
> Thanks for adding linux-mm to CC for this next round of reviews. For the benefit
> of any new reviewers, I'd like to add that there are some issues that were discovered
> while reviewing the v2 patchset, that are not (yet) addressed in this v3 series.

> Since those issues are not listed in the cover letter above, I'll list them here

Thanks for bringing that.
The cover letter is a great place to put this info, I will keep that in
mind for future patchsets.

>
> 1. The locking model requires a combination of disabling interrupts and
> atomic counting and memory barriers, but
>
> 	a) some memory barriers are missing
> 	(start/end_lockless_pgtbl_walk), and

It seems that it works fine today because of the amount of intructions
executed between the irq_disable / start_lockless_pgtbl_walk and where
the THP collapse/split can happen. (It's very unlikely that it reorders
that much).

But I don't think it would be so bad to put a memory barrier after
irq_disable just in case.

> 	b) some cases (patch #8) fail to disable interrupts

I have done some looking into that, and it seems that some uses of
{start,end}_lockless_pgtbl_walk are unneeded, because they operate in
(nested) guest pgd and I was told it's safe against THP split/collapse.

In other uses, there is no interrupt disable because the function is
called in real mode, with MSR_EE=0, and there we have instructions
disabled, so there is no need to disable them again.

>
> ...so the synchronization appears to be inadequate. (And if it *is* adequate, then
> definitely we need the next item, to explain it.)

>
> 2. Documentation of the synchronization/locking model needs to exist, once we
> figure out the exact details of (1).

I will add the missing doc in the code, so it may be easier to
understand in the future.

>
> 3. Related to (1), I've asked to change things so that interrupt controls and 
> atomic inc/dec are in the same start/end calls--assuming, of course, that the
> caller can tolerate that. 

I am not sure if it would be ok to use irq_{save,restore} in real mode,
I will do some more reading of the docs before addressing this. 
>
> 4. Please see the v2 series for any other details I've missed.
>
> thanks,
> -- 
> John Hubbard
> NVIDIA
>

Thank you for helping, John!

Best regards,
Leonardo Bras

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)