linux-kernel - Re: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with blockable invalidate callbacks

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20171216113645.GG16951@dhcp22.suse.cz>
Date:   Sat, 16 Dec 2017 12:36:45 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Oded Gabbay <oded.gabbay@...il.com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        David Airlie <airlied@...ux.ie>,
        Joerg Roedel <joro@...tes.org>,
        Doug Ledford <dledford@...hat.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Mike Marciniszyn <mike.marciniszyn@...el.com>,
        Sean Hefty <sean.hefty@...el.com>,
        Dimitri Sivanich <sivanich@....com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with
 blockable invalidate callbacks

On Sat 16-12-17 16:14:07, Tetsuo Handa wrote:
> On 2017/12/16 8:04, Andrew Morton wrote:
> >> The implementation is steered toward an expensive slowpath, such as after
> >> the oom reaper has grabbed mm->mmap_sem of a still alive oom victim.
> > 
> > some tweakage, please review.
> > 
> > From: Andrew Morton <akpm@...ux-foundation.org>
> > Subject: mm-mmu_notifier-annotate-mmu-notifiers-with-blockable-invalidate-callbacks-fix
> > 
> > make mm_has_blockable_invalidate_notifiers() return bool, use rwsem_is_locked()
> > 
> 
> > @@ -240,13 +240,13 @@ EXPORT_SYMBOL_GPL(__mmu_notifier_invalid
> >   * Must be called while holding mm->mmap_sem for either read or write.
> >   * The result is guaranteed to be valid until mm->mmap_sem is dropped.
> >   */
> > -int mm_has_blockable_invalidate_notifiers(struct mm_struct *mm)
> > +bool mm_has_blockable_invalidate_notifiers(struct mm_struct *mm)
> >  {
> >  	struct mmu_notifier *mn;
> >  	int id;
> > -	int ret = 0;
> > +	bool ret = false;
> >  
> > -	WARN_ON_ONCE(down_write_trylock(&mm->mmap_sem));
> > +	WARN_ON_ONCE(!rwsem_is_locked(&mm->mmap_sem));
> >  
> >  	if (!mm_has_notifiers(mm))
> >  		return ret;
> 
> rwsem_is_locked() test isn't equivalent with __mutex_owner() == current test, is it?
> If rwsem_is_locked() returns true because somebody else has locked it, there is
> no guarantee that current thread has locked it before calling this function.
> 
> down_write_trylock() test isn't equivalent with __mutex_owner() == current test, is it?
> What if somebody else held it for read or write (the worst case is registration path),
> down_write_trylock() will return false even if current thread has not locked it for
> read or write.
> 
> I think this WARN_ON_ONCE() can not detect incorrect call to this function.

Yes it cannot catch _all_ cases. This is an inherent problem of
rwsem_is_locked because semaphores do not really have the owner concept.
The core idea behind this, I guess, is to catch obviously incorrect
usage and as such it gives us a reasonabe coverage. I could live without
the annotation but rwsem_is_locked looks better than down_write_trylock
to me.

-- 
Michal Hocko
SUSE Labs