linux-kernel - Re: Change in functionality of futex() system call.

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: <20110615185047.GA2171@BohrerMBP.rgmadvisors.com>
Date:	Wed, 15 Jun 2011 13:50:47 -0500
From:	Shawn Bohrer <sbohrer@...advisors.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	peterz@...radead.org, eric.dumazet@...il.com,
	david@...advisors.com, linux-kernel@...r.kernel.org,
	zvonler@...advisors.com, hughd@...gle.com, tglx@...utronix.de,
	dvhart@...ux.intel.com, mingo@...e.hu
Subject: Re: Change in functionality of futex() system call.

On Fri, Jun 10, 2011 at 09:10:03PM +0900, KOSAKI Motohiro wrote:
> >> Urgh,. maybe something like the below but with more conditionals that
> >> enable the extra logic only for FUTEX_WAIT..
> >>
> >> The idea is to try a RO gup() when the RW gup() fails so as not to slow
> >> down the common path of writable anonymous maps and bail when we used
> >> the RO path on anonymous memory.
> >>
> >> ---
> >> diff --git a/kernel/futex.c b/kernel/futex.c
> >> index fe28dc2..11f2ad1 100644
> >> --- a/kernel/futex.c
> >> +++ b/kernel/futex.c
> >> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
> >>  	unsigned long address = (unsigned long)uaddr;
> >>  	struct mm_struct *mm = current->mm;
> >>  	struct page *page, *page_head;
> >> -	int err;
> >> +	int err, ro = 0;
> >>  
> >>  	/*
> >>  	 * The futex address must be "naturally" aligned.
> >> @@ -262,6 +262,10 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
> >>  
> >>  again:
> >>  	err = get_user_pages_fast(address, 1, 1, &page);
> >> +	if (err == -EFAULT) {
> >> +		err = get_user_pages_fast(address, 1, 0, &page);
> >> +		ro = 1;
> >> +	}
> >>  	if (err < 0)
> >>  		return err;
> >>  
> >> @@ -316,6 +320,11 @@ again:
> >>  	 * the object not the particular process.
> >>  	 */
> >>  	if (PageAnon(page_head)) {
> >> +		if (ro) {
> >> +			err = -EFAULT;
> >> +			goto out;
> >> +		}
> >> +
> >>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
> >>  		key->private.mm = mm;
> >>  		key->private.address = address;
> >> @@ -327,9 +336,10 @@ again:
> >>  
> >>  	get_futex_key_refs(key);
> >>  
> 
> Need err=0 here. (note: get_user_pages_fast() return 1) Other than that looks
> good to me and this patch passed my test.
> 	Reviewed-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> 
> >> +out:
> >>  	unlock_page(page_head);
> >>  	put_page(page_head);
> >> -	return 0;
> >> +	return err;
> >>  }
> >>  
> >>  static inline void put_futex_key(union futex_key *key)
> >>

I've reviewed and tested Peter's change with KOSAKI's addition against
2.6.32.41 and this passes our tests using FUTEX_WAIT with read only
shared mappings.

Reviewed-and-tested-by: Shawn Bohrer <sbohrer@...advisors.com>

--
Shawn


---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.

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