linux-kernel - Re: [patch v2 5/5] ntfs: remove dependency on __GFP

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: <alpine.DEB.2.00.1009051548570.5003@chino.kir.corp.google.com>
Date:	Sun, 5 Sep 2010 15:55:43 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Anton Altaparmakov <aia21@....ac.uk>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Anton Altaparmakov <aia21@...tab.net>,
	linux-ntfs-dev@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [patch v2 5/5] ntfs: remove dependency on __GFP_NOFAIL

On Thu, 2 Sep 2010, Anton Altaparmakov wrote:

> > @@ -76,11 +76,19 @@ static inline void *ntfs_malloc_nofs(unsigned long size)
> >  * This function guarantees that the allocation will succeed.  It will sleep
> >  * for as long as it takes to complete the allocation.
> >  *
> > - * If there was insufficient memory to complete the request, return NULL.
> > + * NOTE: no new callers of this function should be implemented!
> > + * All memory allocations should be failable whenever possible.
> >  */
> > static inline void *ntfs_malloc_nofs_nofail(unsigned long size)
> > {
> > -	return __ntfs_malloc(size, GFP_NOFS | __GFP_HIGHMEM | __GFP_NOFAIL);
> > +	void *ret;
> > +
> > +	for (;;) {
> > +		ret = ntfs_malloc_nofs(size);
> > +		if (ret)
> > +			return ret;
> > +		WARN_ON_ONCE(get_order(size) > PAGE_ALLOC_COSTLY_ORDER);
> 
> What is the relevance of the above get_order check?  ntfs_malloc_nofs() 
> does a vmalloc() if size >= PAGE_SIZE so I cannot see how that has 
> anything to do with get_order()?
> 

It probably shouldn't be doing vmalloc() for size == PAGE_SIZE, but I can 
understand how it would use it for higher order allocations.  I didn't 
look at that specifically because I assumed these were going through the 
page allocator (and no current user should have any order less than 
PAGE_ALLOC_COSTLY_ORDER, correct me if I'm wrong).

I'm pretty sure we want some sort of warning emitted once if this has the 
potential to loop forever, so it should probably be changed to WARN_ONCE() 
in this patch only.  Thanks for pointing that out.

> Other than that patch looks fine.
> 
> Note the _nofail function is only called from one place and there will 
> be no more call sites for it.  One day we ought to work something out so 
> it is not needed at all but given it only gets called in very obscure 
> circumstances (definitely not in the general code paths) it shouldn't 
> matter too much...
> 

That would be great!
--
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/