linux-kernel - Re: [PATCH] mm: extend prefault helpers to fault in more than PAGE

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: <20120224124003.93780408.akpm@linux-foundation.org>
Date:	Fri, 24 Feb 2012 12:40:03 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Daniel Vetter <daniel@...ll.ch>
Cc:	Daniel Vetter <daniel.vetter@...ll.ch>,
	Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
	DRI Development <dri-devel@...ts.freedesktop.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH] mm: extend prefault helpers to fault in more than
 PAGE_SIZE

On Fri, 24 Feb 2012 14:34:31 +0100
Daniel Vetter <daniel@...ll.ch> wrote:

> > > --- a/include/linux/pagemap.h
> > > +++ b/include/linux/pagemap.h
> > > @@ -408,6 +408,7 @@ extern void add_page_wait_queue(struct page *page, wait_queue_t *waiter);
> > >  static inline int fault_in_pages_writeable(char __user *uaddr, int size)
> > >  {
> > >  	int ret;
> > > +	char __user *end = uaddr + size - 1;
> > >  
> > >  	if (unlikely(size == 0))
> > >  		return 0;
> > > @@ -416,17 +417,20 @@ static inline int fault_in_pages_writeable(char __user *uaddr, int size)
> > >  	 * Writing zeroes into userspace here is OK, because we know that if
> > >  	 * the zero gets there, we'll be overwriting it.
> > >  	 */
> > > -	ret = __put_user(0, uaddr);
> > > +	while (uaddr <= end) {
> > > +		ret = __put_user(0, uaddr);
> > > +		if (ret != 0)
> > > +			return ret;
> > > +		uaddr += PAGE_SIZE;
> > > +	}
> > 
> > The callsites in filemap.c are pretty hot paths, which is why this
> > thing remains explicitly inlined.  I think it would be worth adding a
> > bit of code here to avoid adding a pointless test-n-branch and larger
> > cache footprint to read() and write().
> > 
> > A way of doing that is to add another argument to these functions, say
> > "bool multipage".  Change the code to do
> > 
> > 	if (multipage) {
> > 		while (uaddr <= end) {
> > 			...
> > 		}
> > 	}
> > 
> > and change the callsites to pass in constant "true" or "false".  Then
> > compile it up and manually check that the compiler completely removed
> > the offending code from the filemap.c callsites.
> > 
> > Wanna have a think about that?  If it all looks OK then please be sure
> > to add code comments explaining why we did this.
> 
> I wasn't really happy with the added branch either, but failed to come up
> with a trick to avoid it. Imho adding new _multipage variants of these
> functions instead of adding a constant argument is simpler because the
> functions don't really share much thanks to the block below. I'll see what
> it looks like (and obviously add a comment explaining what's going on).

well... that's just syntactic sugar:

static inline int __fault_in_pages_writeable(char __user *uaddr, int size, bool multipage)
{
	...
}

static inline int fault_in_pages_writeable(char __user *uaddr, int size)
{
	return __fault_in_pages_writeable(uaddr, size, false);
}

static inline int fault_in_multipages_writeable(char __user *uaddr, int size)
{
	return __fault_in_pages_writeable(uaddr, size, true);
}

which I don't think is worth bothering with given the very small number
of callsites.

--
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/