linux-kernel - Re: [PATCH 1/2] mm/mmu_notifier: init notifier if necessary

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>] [day] [month] [year] [list]

Date:	Thu, 30 Aug 2012 12:13:02 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Gavin Shan <shangw@...ux.vnet.ibm.com>
Cc:	Wanpeng Li <liwanp@...ux.vnet.ibm.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Michal Hocko <mhocko@...e.cz>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Minchan Kim <minchan@...nel.org>
Subject: Re: [PATCH 1/2] mm/mmu_notifier: init notifier if necessary

On Sat, 25 Aug 2012 17:47:50 +0800
Gavin Shan <shangw@...ux.vnet.ibm.com> wrote:

> >> --- a/mm/mmu_notifier.c
> >> +++ b/mm/mmu_notifier.c
> >> @@ -192,22 +192,23 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
> >>  
> >>  	BUG_ON(atomic_read(&mm->mm_users) <= 0);
> >>  
> >> -	ret = -ENOMEM;
> >> -	mmu_notifier_mm = kmalloc(sizeof(struct mmu_notifier_mm), GFP_KERNEL);
> >> -	if (unlikely(!mmu_notifier_mm))
> >> -		goto out;
> >> -
> >>  	if (take_mmap_sem)
> >>  		down_write(&mm->mmap_sem);
> >>  	ret = mm_take_all_locks(mm);
> >>  	if (unlikely(ret))
> >> -		goto out_cleanup;
> >> +		goto out;
> >>  
> >>  	if (!mm_has_notifiers(mm)) {
> >> +		mmu_notifier_mm = kmalloc(sizeof(struct mmu_notifier_mm),
> >> +					GFP_ATOMIC);
> >
> >Why was the code switched to the far weaker GFP_ATOMIC?  We can still
> >perform sleeping allocations inside mmap_sem.
> >
> 
> Yes, we can perform sleeping while allocating memory, but we're holding
> the "mmap_sem". GFP_KERNEL possiblly block somebody else who also waits
> on mmap_sem for long time even though the case should be rare :-)

GFP_ATOMIC allocations are unreliable.  If the allocation attempt fails
here, an entire kernel subsystem will have failed, quite probably
requiring a reboot.  It's a bad tradeoff.

Please fix this and retest.  With lockdep enabled, of course.

And please do not attempt to sneak changes like this into the kernel
without even mentioning them in the changelog.  If I hadn't have
happened to notice this, we'd have ended up with a less reliable
kernel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/