[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

Hi ,

At 2013-04-26 04:52:31,"Thomas Gleixner" <tglx@linutronix.de> wrote:
>
>Unfortunately this did not work out very well.
>
>1. Your patch now lacks a proper changelog which explains the change
>
>2. Your patch lacks any newline characters as you can see below
>

I am so sorry for my mistakes. : )




The futex-keys of processes share futex determined by page-offset, mapping-host, and
mapping-index of the user space address.
User appications using hugepage for futex may lead to futex-key conflict.
Assume there are two or more futexes in diffrent normal pages of the hugepage,
and each futex has the same offset in its normal page, causing all the futexes have the same futex-key.
In that case, futex may not work well.

This patch adds the normal page index in the compound page into the offset of futex-key.

Steps to reproduce the bug:
1. The 1st thread map a file of hugetlbfs, and use the return address as the 1st mutex's
address, and use the return address with PAGE_SIZE added as the 2nd mutex's address;
2. The 1st thread initialize the two mutexes with pshared attribute, and lock the two mutexes.
3. The 1st thread create the 2nd thread, and the 2nd thread block on the 1st mutex.
4. The 1st thread create the 3rd thread, and the 3rd thread block on the 2nd mutex.
5. The 1st thread unlock the 2nd mutex, the 3rd thread can not take the 2nd mutex, and
may block forever.


Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>


diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h
--- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
+++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000
@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st
  * The key type depends on whether it's a shared or private mapping.
  * Don't rearrange members without looking at hash_futex().
  *
- * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.
+ * There are three components in offset:
  * We use the two low order bits of offset to tell what is the kind of key :
  *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)
  *       (no reference on an inode or mm)
@@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st
  *	mapped on a file (reference on the underlying inode)
  *  10 : Shared futex (PTHREAD_PROCESS_SHARED)
  *       (but private mapping on an mm, and reference taken on it)
+ * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.
+ * The rest hign order bits indicates the index if the page is a
+ * subpage of a compound page.
 */

 #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
@@ -36,17 +39,17 @@ union futex_key {
 	struct {
 		unsigned long pgoff;
 		struct inode *inode;
-		int offset;
+		long offset;
 	} shared;
 	struct {
 		unsigned long address;
 		struct mm_struct *mm;
-		int offset;
+		long offset;
 	} private;
 	struct {
 		unsigned long word;
 		void *ptr;
-		int offset;
+		long offset;
 	} both;
 };

diff -uprN orig/linux3.9-rc7/kernel/futex.c new/linux3.9-rc7/kernel/futex.c
--- orig/linux3.9-rc7/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
+++ new/linux3.9-rc7/kernel/futex.c	2013-04-19 16:24:05.629143000 +0000
@@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
 	}
 }

+/*
+* Get subpage index in compound page, for futex_key.
+*/
+static inline int get_page_compound_index(struct page *page)
+{
+	struct page *head_page;
+	if (PageHead(page))
+		return 0;
+
+	head_page = compound_head(page);
+	if (compound_order(head_page) >= MAX_ORDER)
+		return page_to_pfn(page) - page_to_pfn(head_page);
+	else
+		return page - compound_head(page);
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr:	virtual address of the futex
@@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu
  * The key words are stored in *key on success.
  *
  * For shared mappings, it's (page->index, file_inode(vma->vm_file),
- * offset_within_page).  For private mappings, it's (uaddr, current->mm).
+ * page_compound_index and offset_within_page).
+ * For private mappings, it's (uaddr, current->mm).
  * We can usually work out the index without swapping in the page.
  *
  * lock_page() might sleep, the caller should not hold a spinlock.
@@ -239,7 +256,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 	unsigned long address = (unsigned long)uaddr;
 	struct mm_struct *mm = current->mm;
 	struct page *page, *page_head;
-	int err, ro = 0;
+	int err, ro = 0, comp_idx = 0;

 	/*
 	 * The futex address must be "naturally" aligned.
@@ -299,6 +316,7 @@ again:
 			 * freed from under us.
 			 */
 			if (page != page_head) {
+				comp_idx = get_page_compound_index(page);
 				get_page(page_head);
 				put_page(page);
 			}
@@ -311,6 +329,7 @@ again:
 #else
 	page_head = compound_head(page);
 	if (page != page_head) {
+		comp_idx = get_page_compound_index(page);
 		get_page(page_head);
 		put_page(page);
 	}
@@ -363,7 +382,9 @@ again:
 		key->private.mm = mm;
 		key->private.address = address;
 	} else {
-		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
+		/* inode-based key */
+		key->both.offset |= ((long)comp_idx << PAGE_SHIFT)
+				   | FUT_OFF_INODE;
 		key->shared.inode = page_head->mapping->host;
 		key->shared.pgoff = page_head->index;
 	}

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Thomas Gleixner]

Zhang,

On Fri, 26 Apr 2013, Zhang Yi wrote:
> At 2013-04-26 04:52:31,"Thomas Gleixner" <tglx@linutronix.de> wrote:
> >
> >Unfortunately this did not work out very well.
> >
> >1. Your patch now lacks a proper changelog which explains the change
> >
> >2. Your patch lacks any newline characters as you can see below
> >
> 
> I am so sorry for my mistakes. : )

Nothing to worry about. We all make mistakes! :)

> The futex-keys of processes share futex determined by page-offset, mapping-host, and
> mapping-index of the user space address.
> User appications using hugepage for futex may lead to futex-key conflict.
> Assume there are two or more futexes in diffrent normal pages of the hugepage,
> and each futex has the same offset in its normal page, causing all the futexes have the same futex-key.

Nit-pick: Please format changelog text with a linebreak around 78
characters. So it looks like this:

The futex-keys of processes share futex determined by page-offset,
mapping-host, and mapping-index of the user space address. User
appications using hugepage for futex may lead to futex-key conflict.

Assume there are two or more futexes in diffrent normal pages of the
hugepage, and each futex has the same offset in its normal page,
causing all the futexes have the same futex-key.

> In that case, futex may not work well.

Very nice detective work!
 
> diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h
> --- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
> +++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000

The canonical diff for patch submission is

  diff -uprN linux3.9-rc7/ linux3.9-rc7.orig/

That results in a patch which can be applied with "patch -p1" from the
kernel base directory and that's how all our scripts work.

Your's needs to be applied with -p2, so it requires manual
interaction.

You can verify that by cd'ing into the kernel tree base directory and
run "patch -p1 < your.patch".

You might have a look at quilt or simply use git, which will do the
right thing for you and in both cases you do not need a separate
kernel tree to diff against.

>  #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
> @@ -36,17 +39,17 @@ union futex_key {
>  	struct {
>  		unsigned long pgoff;
>  		struct inode *inode;
> -		int offset;
> +		long offset;

unsigned long please, offset can't be negative. The "int" type of
offset was silly already.

> +/*
> +* Get subpage index in compound page, for futex_key.
> +*/
> +static inline int get_page_compound_index(struct page *page)
> +{
> +	struct page *head_page;
> +	if (PageHead(page))
> +		return 0;

If you look at the callsite, then you'll see that this is only called
when page != page_head. And page_head = compound_head(page). So you
don't need to double check that.

> +
> +	head_page = compound_head(page);

Again. The head page is already known, so you can hand it into the
function.

> +	if (compound_order(head_page) >= MAX_ORDER)
> +		return page_to_pfn(page) - page_to_pfn(head_page);
> +	else
> +		return page - compound_head(page);
> +}
> +

Now instead of returning that value, I'd rather hand the futex key
pointer to the function and let the function add the index
value. Something like:

static void key_add_compound_idx(key, page, page_head)
{
	...
}

That makes the code simpler and easier to read.

Thanks,

	tglx

RE: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, April 27, 2013 2:26 AM
> To: Zhang Yi
> Cc: linux-kernel@vger.kernel.org; 'Peter Zijlstra'; 'Darren Hart'; 'Ingo Molnar'; 'Dave Hansen';
zhang.yi20@zte.com.cn;
> wetpzy@163.com
> Subject: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
>
> Zhang,
>
> On Fri, 26 Apr 2013, Zhang Yi wrote:
> > At 2013-04-26 04:52:31,"Thomas Gleixner" <tglx@linutronix.de> wrote:
> > >
> > >Unfortunately this did not work out very well.
> > >
> > >1. Your patch now lacks a proper changelog which explains the change
> > >
> > >2. Your patch lacks any newline characters as you can see below
> > >
> >
> > I am so sorry for my mistakes. : )
>
> Nothing to worry about. We all make mistakes! :)
>
> > The futex-keys of processes share futex determined by page-offset, mapping-host, and
> > mapping-index of the user space address.
> > User appications using hugepage for futex may lead to futex-key conflict.
> > Assume there are two or more futexes in diffrent normal pages of the hugepage,
> > and each futex has the same offset in its normal page, causing all the futexes have the same futex-key.
>
> Nit-pick: Please format changelog text with a linebreak around 78
> characters. So it looks like this:
>
> The futex-keys of processes share futex determined by page-offset,
> mapping-host, and mapping-index of the user space address. User
> appications using hugepage for futex may lead to futex-key conflict.
>
> Assume there are two or more futexes in diffrent normal pages of the
> hugepage, and each futex has the same offset in its normal page,
> causing all the futexes have the same futex-key.
>
> > In that case, futex may not work well.
>
> Very nice detective work!
>
> > diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h
> > --- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
> > +++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000
>
> The canonical diff for patch submission is
>
>   diff -uprN linux3.9-rc7/ linux3.9-rc7.orig/
>
> That results in a patch which can be applied with "patch -p1" from the
> kernel base directory and that's how all our scripts work.
>
> Your's needs to be applied with -p2, so it requires manual
> interaction.
>
> You can verify that by cd'ing into the kernel tree base directory and
> run "patch -p1 < your.patch".
>
> You might have a look at quilt or simply use git, which will do the
> right thing for you and in both cases you do not need a separate
> kernel tree to diff against.
>
> >  #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
> > @@ -36,17 +39,17 @@ union futex_key {
> >  	struct {
> >  		unsigned long pgoff;
> >  		struct inode *inode;
> > -		int offset;
> > +		long offset;
>
> unsigned long please, offset can't be negative. The "int" type of
> offset was silly already.
>
> > +/*
> > +* Get subpage index in compound page, for futex_key.
> > +*/
> > +static inline int get_page_compound_index(struct page *page)
> > +{
> > +	struct page *head_page;
> > +	if (PageHead(page))
> > +		return 0;
>
> If you look at the callsite, then you'll see that this is only called
> when page != page_head. And page_head = compound_head(page). So you
> don't need to double check that.
>
> > +
> > +	head_page = compound_head(page);
>
> Again. The head page is already known, so you can hand it into the
> function.
>
> > +	if (compound_order(head_page) >= MAX_ORDER)
> > +		return page_to_pfn(page) - page_to_pfn(head_page);
> > +	else
> > +		return page - compound_head(page);
> > +}
> > +
>
> Now instead of returning that value, I'd rather hand the futex key
> pointer to the function and let the function add the index
> value. Something like:
>
> static void key_add_compound_idx(key, page, page_head)
> {
> 	...
> }
>
> That makes the code simpler and easier to read.
>
> Thanks,
>
> 	tglx


The futex-keys of processes share futex determined by page-offset,
mapping-host, and mapping-index of the user space address. User
appications using hugepage for futex may lead to futex-key conflict.

Assume there are two or more futexes in diffrent normal pages of the
hugepage, and each futex has the same offset in its normal page,
causing all the futexes have the same futex-key.


Steps to reproduce the bug:
1. The 1st thread map a file of hugetlbfs, and use the return address
as the 1st mutex's
address, and use the return address with PAGE_SIZ
 added as the 2nd mutex's addres.;
2. The 1st thread initialize the two mutexes with pshared attribute
 and lock the two mutexes.
3. The 1st thread create the 2nd thread, and the 2nd thread block o
 the 1st mutex.
4. The 1st thread create the 3rd thread, and the 3rd thread block o
 the 2nd mutex.
5. The 1st thread unlock the 2nd mutex, the 3rd thread can not tak
 the 2nd mutex, an
may block forever.


Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.c
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>n>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>


diff -uprN linux3.9-orig/include/linux/futex.h linux3.9/include/linux/futex.h
--- linux3.9-orig/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
+++ linux3.9/include/linux/futex.h	2013-04-27 08:59:58.932078000 +0000
@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st
  * The key type depends on whether it's a shared or private mapping.
  * Don't rearrange members without looking at hash_futex().
  *
- * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.
+ * There are three cmponents in offset:
  * We use the two low order bits of offset to tell what is the kind of key :
  *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)
  *       (no reference on an inode or mm)
@@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st
  *	mapped on a file (reference on the underlying inode)
  *  10 : Shared futex (PTHREAD_PROCESS_SHARED)
  *       (but private mapping on an mm, and reference taken on it)
+ * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.
+ * The rest hign order bits indicates the index if the page is a
+ * subpage of a compound page.
 */

 #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
@@ -36,17 +39,17 @@ union futex_key {
 	struct {
 		unsigned long pgoff;
 		struct inode *inode;
-		int offset;
+		unsigned long offset;
 	} shared;
 	struct {
 		unsigned long address;
 		struct mm_struct *mm;
-		int offset;
+		unsigned long offset;
 	} private;
 	struct {
 		unsigned long word;
 		void *ptr;
-		int offset;
+		unsigned long offset;
 	} both;
 };

diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
--- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
+++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
@@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
 	}
 }

+/*
+* Get subpage index in compound page, and add it into futex_key.
+*/
+static void key_add_compound_idx(union futex_key *key,
+				 struct page *head_page, struct page *page)
+{
+	int compound_idx;
+
+	if (compound_order(head_page) >= MAX_ORDER)
+		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
+	else
+		compound_idx = page - head_page;
+
+	key->both.offset |= compound_idx << PAGE_SHIFT;
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr:	virtual address of the futex
@@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu
  * The key words are stored in *key on success.
  *
  * For shared mappings, it's (page->index, file_inode(vma->vm_file),
- * offset_within_page).  For private mappings, it's (uaddr, current->mm).
+ * page_compound_index and offset_within_page).
+ * For private mappings, it's (uaddr, current->mm).
  * We can usually work out the index without swapping in the page.
  *
  * lock_page() might sleep, the caller should not hold a spinlock.
@@ -366,6 +383,8 @@ again:
 		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
 		key->shared.inode = page_head->mapping->host;
 		key->shared.pgoff = page_head->index;
+		if (page != page_head)
+			key_add_compound_idx(key, page_head, page);
 	}

 	get_futex_key_refs(key);

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Mel Gorman]

On Tue, May 07, 2013 at 08:23:48PM +0800, Zhang Yi wrote:
> diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> --- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
> +++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
> @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
>  	}
>  }
> 
> +/*
> +* Get subpage index in compound page, and add it into futex_key.
> +*/
> +static void key_add_compound_idx(union futex_key *key,
> +				 struct page *head_page, struct page *page)
> +{
> +	int compound_idx;
> +
> +	if (compound_order(head_page) >= MAX_ORDER)
> +		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> +	else
> +		compound_idx = page - head_page;
> +
> +	key->both.offset |= compound_idx << PAGE_SHIFT;
> +}
> +

This implicitely assumies it is dealing with a hugetlbfs page. Today, it
is the case that an inode-based futex with PageCompound is a hugetlbfs
page but that could change in the future if THP ever backs files. This
would then break again except it would be harder to fix because THP pages
can be collapsed underneath you after the futex key has been generated.

As this problem is hugetlbfs-specific should the fix be firmly in hugetlbfs
land? Something like the following untested and only partial diff? Is the
use of PageCompound in the futex path like this going to be problematic?

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 16e4e9a..f9c33d3 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -348,6 +348,17 @@ static inline int hstate_index(struct hstate *h)
 	return h - hstates;
 }
 
+pgoff_t __basepage_index(struct page *page);
+
+/* Return page->index in PAGE_SIZE units */
+static inline pgoff_t basepage_index(struct page *page)
+{
+	if (!PageCompound(page))
+		return page->index;
+
+	return __basepage_index(page);
+}
+
 #else
 struct hstate {};
 #define alloc_huge_page_node(h, nid) NULL
@@ -365,6 +376,10 @@ static inline unsigned int pages_per_huge_page(struct hstate *h)
 {
 	return 1;
 }
+static inline pgoff_t basepage_index(struct page *page)
+{
+	return page->index;
+}
 #define hstate_index_to_shift(index) 0
 #define hstate_index(h) 0
 #endif
diff --git a/kernel/futex.c b/kernel/futex.c
index b26dcfc..97beb5d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -61,6 +61,7 @@
 #include <linux/nsproxy.h>
 #include <linux/ptrace.h>
 #include <linux/sched/rt.h>
+#include <linux/hugetlb.h>
 
 #include <asm/futex.h>
 
@@ -365,7 +366,7 @@ again:
 	} else {
 		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
 		key->shared.inode = page_head->mapping->host;
-		key->shared.pgoff = page_head->index;
+		key->shared.pgoff = basepage_index(page_head);
 	}
 
 	get_futex_key_refs(key);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1a12f5b..ddbad35 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -690,6 +690,23 @@ int PageHuge(struct page *page)
 }
 EXPORT_SYMBOL_GPL(PageHuge);
 
+pgoff_t __basepage_index(struct page *page)
+{
+	struct page *page_head = compound_head(page);
+	pgoff_t index = page_index(page_head);
+	int compound_idx;
+
+	if (!PageHuge(page_head))
+		return page_index(page);
+
+	if (compound_order(page_head) >= MAX_ORDER)
+		compound_idx = page_to_pfn(page) - page_to_pfn(page_head);
+	else
+		compound_idx = page - head_page;
+
+	return (index << page_hstate(page_head)->order) + compound_idx;
+}
+
 static struct page *alloc_fresh_huge_page_node(struct hstate *h, int nid)
 {
 	struct page *page;

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Thomas Gleixner]

On Tue, 7 May 2013, Mel Gorman wrote:

> On Tue, May 07, 2013 at 08:23:48PM +0800, Zhang Yi wrote:
> > diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> > --- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
> > +++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
> > @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
> >  	}
> >  }
> > 
> > +/*
> > +* Get subpage index in compound page, and add it into futex_key.
> > +*/
> > +static void key_add_compound_idx(union futex_key *key,
> > +				 struct page *head_page, struct page *page)
> > +{
> > +	int compound_idx;
> > +
> > +	if (compound_order(head_page) >= MAX_ORDER)
> > +		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> > +	else
> > +		compound_idx = page - head_page;
> > +
> > +	key->both.offset |= compound_idx << PAGE_SHIFT;
> > +}
> > +
> 
> This implicitely assumies it is dealing with a hugetlbfs page. Today, it
> is the case that an inode-based futex with PageCompound is a hugetlbfs
> page but that could change in the future if THP ever backs files. This
> would then break again except it would be harder to fix because THP pages
> can be collapsed underneath you after the futex key has been generated.
> 
> As this problem is hugetlbfs-specific should the fix be firmly in hugetlbfs
> land? Something like the following untested and only partial diff? Is the
> use of PageCompound in the futex path like this going to be problematic?

Why should it ?
 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 16e4e9a..f9c33d3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -348,6 +348,17 @@ static inline int hstate_index(struct hstate *h)
>  	return h - hstates;
>  }
>  
> +pgoff_t __basepage_index(struct page *page);
> +
> +/* Return page->index in PAGE_SIZE units */
> +static inline pgoff_t basepage_index(struct page *page)
> +{
> +	if (!PageCompound(page))
> +		return page->index;
> +
> +	return __basepage_index(page);
> +}
> +
>  #else
>  struct hstate {};
>  #define alloc_huge_page_node(h, nid) NULL
> @@ -365,6 +376,10 @@ static inline unsigned int pages_per_huge_page(struct hstate *h)
>  {
>  	return 1;
>  }
> +static inline pgoff_t basepage_index(struct page *page)
> +{
> +	return page->index;
> +}
>  #define hstate_index_to_shift(index) 0
>  #define hstate_index(h) 0
>  #endif
> diff --git a/kernel/futex.c b/kernel/futex.c
> index b26dcfc..97beb5d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -61,6 +61,7 @@
>  #include <linux/nsproxy.h>
>  #include <linux/ptrace.h>
>  #include <linux/sched/rt.h>
> +#include <linux/hugetlb.h>
>  
>  #include <asm/futex.h>
>  
> @@ -365,7 +366,7 @@ again:
>  	} else {
>  		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
>  		key->shared.inode = page_head->mapping->host;
> -		key->shared.pgoff = page_head->index;
> +		key->shared.pgoff = basepage_index(page_head);

  That want's to be  basepage_index(page), right ?

>  	}
>  
>  	get_futex_key_refs(key);
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1a12f5b..ddbad35 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -690,6 +690,23 @@ int PageHuge(struct page *page)
>  }
>  EXPORT_SYMBOL_GPL(PageHuge);
>  
> +pgoff_t __basepage_index(struct page *page)
> +{
> +	struct page *page_head = compound_head(page);
> +	pgoff_t index = page_index(page_head);
> +	int compound_idx;
> +
> +	if (!PageHuge(page_head))
> +		return page_index(page);
> +
> +	if (compound_order(page_head) >= MAX_ORDER)
> +		compound_idx = page_to_pfn(page) - page_to_pfn(page_head);
> +	else
> +		compound_idx = page - head_page;
> +
> +	return (index << page_hstate(page_head)->order) + compound_idx;
> +}
> +
>  static struct page *alloc_fresh_huge_page_node(struct hstate *h, int nid)
>  {
>  	struct page *page;
> 

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Mel Gorman]

On Tue, May 07, 2013 at 05:24:57PM +0200, Thomas Gleixner wrote:
> 
> 
> On Tue, 7 May 2013, Mel Gorman wrote:
> 
> > On Tue, May 07, 2013 at 08:23:48PM +0800, Zhang Yi wrote:
> > > diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> > > --- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
> > > +++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
> > > @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
> > >  	}
> > >  }
> > > 
> > > +/*
> > > +* Get subpage index in compound page, and add it into futex_key.
> > > +*/
> > > +static void key_add_compound_idx(union futex_key *key,
> > > +				 struct page *head_page, struct page *page)
> > > +{
> > > +	int compound_idx;
> > > +
> > > +	if (compound_order(head_page) >= MAX_ORDER)
> > > +		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> > > +	else
> > > +		compound_idx = page - head_page;
> > > +
> > > +	key->both.offset |= compound_idx << PAGE_SHIFT;
> > > +}
> > > +
> > 
> > This implicitely assumies it is dealing with a hugetlbfs page. Today, it
> > is the case that an inode-based futex with PageCompound is a hugetlbfs
> > page but that could change in the future if THP ever backs files. This
> > would then break again except it would be harder to fix because THP pages
> > can be collapsed underneath you after the futex key has been generated.
> > 
> > As this problem is hugetlbfs-specific should the fix be firmly in hugetlbfs
> > land? Something like the following untested and only partial diff? Is the
> > use of PageCompound in the futex path like this going to be problematic?
> 
> Why should it ?
>  

The comment for it states that it is "generally not used in hot code
paths" but it's a light-weight check that the cache lines should already
be fetched for. I doubt that the overhead of this check versus
page_head == page is noticable.

> > @@ -365,7 +366,7 @@ again:
> >  	} else {
> >  		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
> >  		key->shared.inode = page_head->mapping->host;
> > -		key->shared.pgoff = page_head->index;
> > +		key->shared.pgoff = basepage_index(page_head);
> 
>   That want's to be  basepage_index(page), right ?
> 

BAH, yes.

-- 
Mel Gorman
SUSE Labs

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

Mel Gorman <mgorman@suse.de> wrote on 2013/05/07 23:20:07:

>
> Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
>
> On Tue, May 07, 2013 at 08:23:48PM +0800, Zhang Yi wrote:
> > diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> > --- linux3.9-orig/kernel/futex.c   2013-04-15 00:45:16.000000000 +0000
> > +++ linux3.9/kernel/futex.c   2013-05-06 16:24:40.403525000 +0000
> > @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
> >     }
> >  }
> >
> > +/*
> > +* Get subpage index in compound page, and add it into futex_key.
> > +*/
> > +static void key_add_compound_idx(union futex_key *key,
> > +             struct page *head_page, struct page *page)
> > +{
> > +   int compound_idx;
> > +
> > +   if (compound_order(head_page) >= MAX_ORDER)
> > +      compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> > +   else
> > +      compound_idx = page - head_page;
> > +
> > +   key->both.offset |= compound_idx << PAGE_SHIFT;
> > +}
> > +
>
> This implicitely assumies it is dealing with a hugetlbfs page. Today, it
> is the case that an inode-based futex with PageCompound is a hugetlbfs
> page but that could change in the future if THP ever backs files. This
> would then break again except it would be harder to fix because THP pages
> can be collapsed underneath you after the futex key has been generated.
>
> As this problem is hugetlbfs-specific should the fix be firmly in
hugetlbfs

I think we should do.
Eg, user applications want high performance, they may use DPDK  which using
hugetlbfs.


Should I rework the patch like the following code, and test it?

> land? Something like the following untested and only partial diff? Is the
> use of PageCompound in the futex path like this going to be problematic?
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 16e4e9a..f9c33d3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -348,6 +348,17 @@ static inline int hstate_index(struct hstate *h)
>     return h - hstates;
>  }
>
> +pgoff_t __basepage_index(struct page *page);
> +
> +/* Return page->index in PAGE_SIZE units */
> +static inline pgoff_t basepage_index(struct page *page)
> +{
> +   if (!PageCompound(page))
> +      return page->index;
> +
> +   return __basepage_index(page);
> +}
> +
>  #else
>  struct hstate {};
>  #define alloc_huge_page_node(h, nid) NULL
> @@ -365,6 +376,10 @@ static inline unsigned int pages_per_huge_page
> (struct hstate *h)
>  {
>     return 1;
>  }
> +static inline pgoff_t basepage_index(struct page *page)
> +{
> +   return page->index;
> +}
>  #define hstate_index_to_shift(index) 0
>  #define hstate_index(h) 0
>  #endif
> diff --git a/kernel/futex.c b/kernel/futex.c
> index b26dcfc..97beb5d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -61,6 +61,7 @@
>  #include <linux/nsproxy.h>
>  #include <linux/ptrace.h>
>  #include <linux/sched/rt.h>
> +#include <linux/hugetlb.h>
>
>  #include <asm/futex.h>
>
> @@ -365,7 +366,7 @@ again:
>     } else {
>        key->both.offset |= FUT_OFF_INODE; /* inode-based key */
>        key->shared.inode = page_head->mapping->host;
> -      key->shared.pgoff = page_head->index;
> +      key->shared.pgoff = basepage_index(page_head);
>     }
>
>     get_futex_key_refs(key);
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1a12f5b..ddbad35 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -690,6 +690,23 @@ int PageHuge(struct page *page)
>  }
>  EXPORT_SYMBOL_GPL(PageHuge);
>
> +pgoff_t __basepage_index(struct page *page)
> +{
> +   struct page *page_head = compound_head(page);
> +   pgoff_t index = page_index(page_head);
> +   int compound_idx;
> +
> +   if (!PageHuge(page_head))
> +      return page_index(page);
> +
> +   if (compound_order(page_head) >= MAX_ORDER)
> +      compound_idx = page_to_pfn(page) - page_to_pfn(page_head);
> +   else
> +      compound_idx = page - head_page;
> +
> +   return (index << page_hstate(page_head)->order) + compound_idx;
> +}
> +
>  static struct page *alloc_fresh_huge_page_node(struct hstate *h, int
nid)
>  {
>     struct page *page;

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Mel Gorman]

On Fri, May 10, 2013 at 05:08:30PM +0800, zhang.yi20@zte.com.cn wrote:
> 
> 
> Mel Gorman <mgorman@suse.de> wrote on 2013/05/07 23:20:07:
> 
> >
> > Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
> >
> > On Tue, May 07, 2013 at 08:23:48PM +0800, Zhang Yi wrote:
> > > diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> > > --- linux3.9-orig/kernel/futex.c   2013-04-15 00:45:16.000000000 +0000
> > > +++ linux3.9/kernel/futex.c   2013-05-06 16:24:40.403525000 +0000
> > > @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
> > >     }
> > >  }
> > >
> > > +/*
> > > +* Get subpage index in compound page, and add it into futex_key.
> > > +*/
> > > +static void key_add_compound_idx(union futex_key *key,
> > > +             struct page *head_page, struct page *page)
> > > +{
> > > +   int compound_idx;
> > > +
> > > +   if (compound_order(head_page) >= MAX_ORDER)
> > > +      compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> > > +   else
> > > +      compound_idx = page - head_page;
> > > +
> > > +   key->both.offset |= compound_idx << PAGE_SHIFT;
> > > +}
> > > +
> >
> > This implicitely assumies it is dealing with a hugetlbfs page. Today, it
> > is the case that an inode-based futex with PageCompound is a hugetlbfs
> > page but that could change in the future if THP ever backs files. This
> > would then break again except it would be harder to fix because THP pages
> > can be collapsed underneath you after the futex key has been generated.
> >
> > As this problem is hugetlbfs-specific should the fix be firmly in
> hugetlbfs
> 
> I think we should do.
> Eg, user applications want high performance, they may use DPDK  which using
> hugetlbfs.
> 
> 
> Should I rework the patch like the following code, and test it?
> 

Yes please, making sure to fix the bug Thomas pointed out.

-- 
Mel Gorman
SUSE Labs

RE: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

It is OK that I send the mail to myself , but there are some wrong while sending to you.
Ignore this mail ,please, I will check and send it again.

> -----Original Message-----
> From: Zhang Yi [mailto:wetpzy@gmail.com]
> Sent: Tuesday, May 07, 2013 8:24 PM
> To: 'Thomas Gleixner'
> Cc: 'linux-kernel@vger.kernel.org'; 'Peter Zijlstra'; 'Darren Hart'; 'Ingo Molnar'; 'Dave Hansen';
'zhang.yi20@zte.com.cn';
> 'wetpzy@163.com'
> Subject: RE: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
>
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Saturday, April 27, 2013 2:26 AM
> > To: Zhang Yi
> > Cc: linux-kernel@vger.kernel.org; 'Peter Zijlstra'; 'Darren Hart'; 'Ingo Molnar'; 'Dave Hansen';
zhang.yi20@zte.com.cn;
> > wetpzy@163.com
> > Subject: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
> >
> > Zhang,
> >
> > On Fri, 26 Apr 2013, Zhang Yi wrote:
> > > At 2013-04-26 04:52:31,"Thomas Gleixner" <tglx@linutronix.de> wrote:
> > > >
> > > >Unfortunately this did not work out very well.
> > > >
> > > >1. Your patch now lacks a proper changelog which explains the change
> > > >
> > > >2. Your patch lacks any newline characters as you can see below
> > > >
> > >
> > > I am so sorry for my mistakes. : )
> >
> > Nothing to worry about. We all make mistakes! :)
> >
> > > The futex-keys of processes share futex determined by page-offset, mapping-host, and
> > > mapping-index of the user space address.
> > > User appications using hugepage for futex may lead to futex-key conflict.
> > > Assume there are two or more futexes in diffrent normal pages of the hugepage,
> > > and each futex has the same offset in its normal page, causing all the futexes have the same futex-key.
> >
> > Nit-pick: Please format changelog text with a linebreak around 78
> > characters. So it looks like this:
> >
> > The futex-keys of processes share futex determined by page-offset,
> > mapping-host, and mapping-index of the user space address. User
> > appications using hugepage for futex may lead to futex-key conflict.
> >
> > Assume there are two or more futexes in diffrent normal pages of the
> > hugepage, and each futex has the same offset in its normal page,
> > causing all the futexes have the same futex-key.
> >
> > > In that case, futex may not work well.
> >
> > Very nice detective work!
> >
> > > diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h
> > > --- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
> > > +++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000
> >
> > The canonical diff for patch submission is
> >
> >   diff -uprN linux3.9-rc7/ linux3.9-rc7.orig/
> >
> > That results in a patch which can be applied with "patch -p1" from the
> > kernel base directory and that's how all our scripts work.
> >
> > Your's needs to be applied with -p2, so it requires manual
> > interaction.
> >
> > You can verify that by cd'ing into the kernel tree base directory and
> > run "patch -p1 < your.patch".
> >
> > You might have a look at quilt or simply use git, which will do the
> > right thing for you and in both cases you do not need a separate
> > kernel tree to diff against.
> >
> > >  #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
> > > @@ -36,17 +39,17 @@ union futex_key {
> > >  	struct {
> > >  		unsigned long pgoff;
> > >  		struct inode *inode;
> > > -		int offset;
> > > +		long offset;
> >
> > unsigned long please, offset can't be negative. The "int" type of
> > offset was silly already.
> >
> > > +/*
> > > +* Get subpage index in compound page, for futex_key.
> > > +*/
> > > +static inline int get_page_compound_index(struct page *page)
> > > +{
> > > +	struct page *head_page;
> > > +	if (PageHead(page))
> > > +		return 0;
> >
> > If you look at the callsite, then you'll see that this is only called
> > when page != page_head. And page_head = compound_head(page). So you
> > don't need to double check that.
> >
> > > +
> > > +	head_page = compound_head(page);
> >
> > Again. The head page is already known, so you can hand it into the
> > function.
> >
> > > +	if (compound_order(head_page) >= MAX_ORDER)
> > > +		return page_to_pfn(page) - page_to_pfn(head_page);
> > > +	else
> > > +		return page - compound_head(page);
> > > +}
> > > +
> >
> > Now instead of returning that value, I'd rather hand the futex key
> > pointer to the function and let the function add the index
> > value. Something like:
> >
> > static void key_add_compound_idx(key, page, page_head)
> > {
> > 	...
> > }
> >
> > That makes the code simpler and easier to read.
> >
> > Thanks,
> >
> > 	tglx
>
>
> The futex-keys of processes share futex determined by page-offset,
> mapping-host, and mapping-index of the user space address. User
> appications using hugepage for futex may lead to futex-key conflict.
>
> Assume there are two or more futexes in diffrent normal pages of the
> hugepage, and each futex has the same offset in its normal page,
> causing all the futexes have the same futex-key.
>
>
> Steps to reproduce the bug:
> 1. The 1st thread map a file of hugetlbfs, and use the return address
> as the 1st mutex's
> address, and use the return address with PAGE_SIZ
>  added as the 2nd mutex's addres.;
> 2. The 1st thread initialize the two mutexes with pshared attribute
>  and lock the two mutexes.
> 3. The 1st thread create the 2nd thread, and the 2nd thread block o
>  the 1st mutex.
> 4. The 1st thread create the 3rd thread, and the 3rd thread block o
>  the 2nd mutex.
> 5. The 1st thread unlock the 2nd mutex, the 3rd thread can not tak
>  the 2nd mutex, an
> may block forever.
>
>
> Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
> Tested-by: Ma Chenggong <ma.chenggong@zte.com.c
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>n>
> Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
> Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
> Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
>
>
> diff -uprN linux3.9-orig/include/linux/futex.h linux3.9/include/linux/futex.h
> --- linux3.9-orig/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
> +++ linux3.9/include/linux/futex.h	2013-04-27 08:59:58.932078000 +0000
> @@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st
>   * The key type depends on whether it's a shared or private mapping.
>   * Don't rearrange members without looking at hash_futex().
>   *
> - * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.
> + * There are three cmponents in offset:
>   * We use the two low order bits of offset to tell what is the kind of key :
>   *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)
>   *       (no reference on an inode or mm)
> @@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st
>   *	mapped on a file (reference on the underlying inode)
>   *  10 : Shared futex (PTHREAD_PROCESS_SHARED)
>   *       (but private mapping on an mm, and reference taken on it)
> + * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.
> + * The rest hign order bits indicates the index if the page is a
> + * subpage of a compound page.
>  */
>
>  #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
> @@ -36,17 +39,17 @@ union futex_key {
>  	struct {
>  		unsigned long pgoff;
>  		struct inode *inode;
> -		int offset;
> +		unsigned long offset;
>  	} shared;
>  	struct {
>  		unsigned long address;
>  		struct mm_struct *mm;
> -		int offset;
> +		unsigned long offset;
>  	} private;
>  	struct {
>  		unsigned long word;
>  		void *ptr;
> -		int offset;
> +		unsigned long offset;
>  	} both;
>  };
>
> diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
> --- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
> +++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
> @@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
>  	}
>  }
>
> +/*
> +* Get subpage index in compound page, and add it into futex_key.
> +*/
> +static void key_add_compound_idx(union futex_key *key,
> +				 struct page *head_page, struct page *page)
> +{
> +	int compound_idx;
> +
> +	if (compound_order(head_page) >= MAX_ORDER)
> +		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
> +	else
> +		compound_idx = page - head_page;
> +
> +	key->both.offset |= compound_idx << PAGE_SHIFT;
> +}
> +
>  /**
>   * get_futex_key() - Get parameters which are the keys for a futex
>   * @uaddr:	virtual address of the futex
> @@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu
>   * The key words are stored in *key on success.
>   *
>   * For shared mappings, it's (page->index, file_inode(vma->vm_file),
> - * offset_within_page).  For private mappings, it's (uaddr, current->mm).
> + * page_compound_index and offset_within_page).
> + * For private mappings, it's (uaddr, current->mm).
>   * We can usually work out the index without swapping in the page.
>   *
>   * lock_page() might sleep, the caller should not hold a spinlock.
> @@ -366,6 +383,8 @@ again:
>  		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
>  		key->shared.inode = page_head->mapping->host;
>  		key->shared.pgoff = page_head->index;
> +		if (page != page_head)
> +			key_add_compound_idx(key, page_head, page);
>  	}
>
>  	get_futex_key_refs(key);

[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

The futex-keys of processes share futex determined by page-offset,
mapping-host, and mapping-index of the user space address. User
appications using hugepage for futex may lead to futex-key conflict.

Assume there are two or more futexes in diffrent normal pages of the
hugepage, and each futex has the same offset in its normal page,
causing all the futexes have the same futex-key.

This patch adds the normal page index in the compound page into
the pgoff of futex-key.

Steps to reproduce the bug:
1. The 1st thread map a file of hugetlbfs, and use the return address
as the 1st mutex's address, and use the return address with PAGE_SIZE
added as the 2nd mutex's address.
2. The 1st thread initialize the two mutexes with pshared attribute,
and lock the two mutexes.
3. The 1st thread create the 2nd thread, and the 2nd thread block on
the 1st mutex.
4. The 1st thread create the 3rd thread, and the 3rd thread block on
the 2nd mutex.
5. The 1st thread unlock the 2nd mutex, the 3rd thread cannot take
the 2nd mutex, and may block forever.


Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Mel Gorman <mgorman@suse.de>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>


diff -uprN linux-3.10-rc1.org/include/linux/hugetlb.h linux-3.10-rc1/include/linux/hugetlb.h
--- linux-3.10-rc1.org/include/linux/hugetlb.h	2013-05-12 00:14:08.000000000 +0000
+++ linux-3.10-rc1/include/linux/hugetlb.h	2013-05-14 10:15:49.849389000 +0000
@@ -358,6 +358,17 @@ static inline int hstate_index(struct hs
 	return h - hstates;
 }

+pgoff_t __basepage_index(struct page *page);
+
+/* Return page->index in PAGE_SIZE units */
+static inline pgoff_t basepage_index(struct page *page)
+{
+	if (!PageCompound(page))
+		return page->index;
+
+	return __basepage_index(page);
+}
+
 #else	/* CONFIG_HUGETLB_PAGE */
 struct hstate {};
 #define alloc_huge_page_node(h, nid) NULL
@@ -378,6 +389,11 @@ static inline unsigned int pages_per_hug
 }
 #define hstate_index_to_shift(index) 0
 #define hstate_index(h) 0
+
+static inline pgoff_t basepage_index(struct page *page)
+{
+	return page->index;
+}
 #endif	/* CONFIG_HUGETLB_PAGE */

 #endif /* _LINUX_HUGETLB_H */
diff -uprN linux-3.10-rc1.org/kernel/futex.c linux-3.10-rc1/kernel/futex.c
--- linux-3.10-rc1.org/kernel/futex.c	2013-05-12 00:14:08.000000000 +0000
+++ linux-3.10-rc1/kernel/futex.c	2013-05-14 10:15:04.804350000 +0000
@@ -61,6 +61,7 @@
 #include <linux/nsproxy.h>
 #include <linux/ptrace.h>
 #include <linux/sched/rt.h>
+#include <linux/hugetlb.h>

 #include <asm/futex.h>

@@ -365,7 +366,7 @@ again:
 	} else {
 		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
 		key->shared.inode = page_head->mapping->host;
-		key->shared.pgoff = page_head->index;
+		key->shared.pgoff = basepage_index(page);
 	}

 	get_futex_key_refs(key);
diff -uprN linux-3.10-rc1.org/mm/hugetlb.c linux-3.10-rc1/mm/hugetlb.c
--- linux-3.10-rc1.org/mm/hugetlb.c	2013-05-12 00:14:08.000000000 +0000
+++ linux-3.10-rc1/mm/hugetlb.c	2013-05-14 10:15:37.470175000 +0000
@@ -690,6 +690,23 @@ int PageHuge(struct page *page)
 }
 EXPORT_SYMBOL_GPL(PageHuge);

+pgoff_t __basepage_index(struct page *page)
+{
+	struct page *page_head = compound_head(page);
+	pgoff_t index = page_index(page_head);
+	int compound_idx;
+
+	if (!PageHuge(page_head))
+		return page_index(page);
+
+	if (compound_order(page_head) >= MAX_ORDER)
+		compound_idx = page_to_pfn(page) - page_to_pfn(page_head);
+	else
+		compound_idx = page - page_head;
+
+	return (index << compound_order(page_head)) + compound_idx;
+}
+
 static struct page *alloc_fresh_huge_page_node(struct hstate *h, int nid)
 {
 	struct page *page;

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Mel Gorman]

On Wed, May 15, 2013 at 09:57:03PM +0800, Zhang Yi wrote:
> The futex-keys of processes share futex determined by page-offset,
> mapping-host, and mapping-index of the user space address. User
> appications using hugepage for futex may lead to futex-key conflict.
> 
> Assume there are two or more futexes in diffrent normal pages of the
> hugepage, and each futex has the same offset in its normal page,
> causing all the futexes have the same futex-key.
> 
> This patch adds the normal page index in the compound page into
> the pgoff of futex-key.
> 
> Steps to reproduce the bug:
> 1. The 1st thread map a file of hugetlbfs, and use the return address
> as the 1st mutex's address, and use the return address with PAGE_SIZE
> added as the 2nd mutex's address.
> 2. The 1st thread initialize the two mutexes with pshared attribute,
> and lock the two mutexes.
> 3. The 1st thread create the 2nd thread, and the 2nd thread block on
> the 1st mutex.
> 4. The 1st thread create the 3rd thread, and the 3rd thread block on
> the 2nd mutex.
> 5. The 1st thread unlock the 2nd mutex, the 3rd thread cannot take
> the 2nd mutex, and may block forever.
> 
> 
> Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
> Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Reviewed-by: Mel Gorman <mgorman@suse.de>
> Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
> Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
> Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
> 

Did all these people really review it? I just whinged about the last patch
and didn't put a Reviewed-by on it. That said, I don't actually have a
problem with this patch and I assumed it passed your testing so

Reviewed-by: Mel Gorman <mgorman@suse.de>

The others might not agree though.

I note the conversion from int offset to long offset in futex_key appears
to have gotten lost. Is that in a separate cleanup patch now?

-- 
Mel Gorman
SUSE Labs

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

Mel Gorman <mgorman@suse.de> wrote on 2013/05/15 22:20:35:


> Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
>
> On Wed, May 15, 2013 at 09:57:03PM +0800, Zhang Yi wrote:
> > The futex-keys of processes share futex determined by page-offset,
> > mapping-host, and mapping-index of the user space address. User
> > appications using hugepage for futex may lead to futex-key conflict.
> >
> > Assume there are two or more futexes in diffrent normal pages of the
> > hugepage, and each futex has the same offset in its normal page,
> > causing all the futexes have the same futex-key.
> >
> > This patch adds the normal page index in the compound page into
> > the pgoff of futex-key.
> >
> > Steps to reproduce the bug:
> > 1. The 1st thread map a file of hugetlbfs, and use the return address
> > as the 1st mutex's address, and use the return address with PAGE_SIZE
> > added as the 2nd mutex's address.
> > 2. The 1st thread initialize the two mutexes with pshared attribute,
> > and lock the two mutexes.
> > 3. The 1st thread create the 2nd thread, and the 2nd thread block on
> > the 1st mutex.
> > 4. The 1st thread create the 3rd thread, and the 3rd thread block on
> > the 2nd mutex.
> > 5. The 1st thread unlock the 2nd mutex, the 3rd thread cannot take
> > the 2nd mutex, and may block forever.
> >
> >
> > Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
> > Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> > Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Reviewed-by: Mel Gorman <mgorman@suse.de>
> > Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
> > Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
> > Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
> > Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
> >
>
> Did all these people really review it? I just whinged about the last
patch
> and didn't put a Reviewed-by on it. That said, I don't actually have a
> problem with this patch and I assumed it passed your testing so
>
I mistakenly think that I should list all the people here. : )
Shall I cleanup the name list and send the patch again?

> Reviewed-by: Mel Gorman <mgorman@suse.de>
>
> The others might not agree though.
>
> I note the conversion from int offset to long offset in futex_key appears
> to have gotten lost. Is that in a separate cleanup patch now?

In old patch, I add the compound index into offset, so I make the offset
from int
to long. It is unnecessary for this patch.

>
> --
> Mel Gorman
> SUSE Labs


BTW, Does anyone have other advices for the patch?

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 05/15/2013 06:16 PM, zhang.yi20@zte.com.cn wrote:
> 
> 
> Mel Gorman <mgorman@suse.de> wrote on 2013/05/15 22:20:35:
> 
> 
>> Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage
>>
>> On Wed, May 15, 2013 at 09:57:03PM +0800, Zhang Yi wrote:
>>> The futex-keys of processes share futex determined by page-offset,
>>> mapping-host, and mapping-index of the user space address. User
>>> appications using hugepage for futex may lead to futex-key conflict.
>>>
>>> Assume there are two or more futexes in diffrent normal pages of the
>>> hugepage, and each futex has the same offset in its normal page,
>>> causing all the futexes have the same futex-key.
>>>
>>> This patch adds the normal page index in the compound page into
>>> the pgoff of futex-key.
>>>
>>> Steps to reproduce the bug:
>>> 1. The 1st thread map a file of hugetlbfs, and use the return address
>>> as the 1st mutex's address, and use the return address with PAGE_SIZE
>>> added as the 2nd mutex's address.
>>> 2. The 1st thread initialize the two mutexes with pshared attribute,
>>> and lock the two mutexes.
>>> 3. The 1st thread create the 2nd thread, and the 2nd thread block on
>>> the 1st mutex.
>>> 4. The 1st thread create the 3rd thread, and the 3rd thread block on
>>> the 2nd mutex.
>>> 5. The 1st thread unlock the 2nd mutex, the 3rd thread cannot take
>>> the 2nd mutex, and may block forever.
>>>
>>>
>>> Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
>>> Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
>>> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>>> Reviewed-by: Darren Hart <dvhart@linux.intel.com>
>>> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
>>> Reviewed-by: Mel Gorman <mgorman@suse.de>
>>> Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
>>> Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
>>> Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
>>> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
>>>
>>
>> Did all these people really review it? I just whinged about the last
> patch
>> and didn't put a Reviewed-by on it. That said, I don't actually have a
>> problem with this patch and I assumed it passed your testing so
>>
> I mistakenly think that I should list all the people here. : )
> Shall I cleanup the name list and send the patch again?
> 
>> Reviewed-by: Mel Gorman <mgorman@suse.de>
>>
>> The others might not agree though.
>>
>> I note the conversion from int offset to long offset in futex_key appears
>> to have gotten lost. Is that in a separate cleanup patch now?

In general, you should not add someone's signature unless they gave it
first or explicitly gave you permission to do so. If you want to
indicate they were contacted, you can use the "Cc:" tag instead of
"Reviewed-by".

> 
> In old patch, I add the compound index into offset, so I make the offset
> from int to long. It is unnecessary for this patch. 

pgoff_t is an unsigned long, and page_to_pfn() returns an unsigned long.
Since compound_idx can be assigned from page_to_pfn() and it is added
with index in the return value, unsigned long seems like a better choice
to me. Is there a specific reason you prefer an int? It might be "fine"
but it is likely to raise eyebrows whenever someone read through it.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

RE: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

Hi,

The futex-keys of processes share futex determined by page-offset,
mapping-host, and mapping-index of the user space address. User
appications using hugepage for futex may lead to futex-key conflict.

Assume there are two or more futexes in diffrent normal pages of the
hugepage, and each futex has the same offset in its normal page,
causing all the futexes have the same futex-key.


Steps to reproduce the bug:
1. The 1st thread map a file of hugetlbfs, and use the return address
as the 1st mutex's address, and use the return address with PAGE_SIZE
added as the 2nd mutex's address.
2. The 1st thread initialize the two mutexes with pshared attribute,
and lock the two mutexes.
3. The 1st thread create the 2nd thread, and the 2nd thread block on
the 1st mutex.
4. The 1st thread create the 3rd thread, and the 3rd thread block on
the 2nd mutex.
5. The 1st thread unlock the 2nd mutex, the 3rd thread cannot take
the 2nd mutex, and may block forever.


Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>


diff -uprN linux3.9-orig/include/linux/futex.h linux3.9/include/linux/futex.h
--- linux3.9-orig/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
+++ linux3.9/include/linux/futex.h	2013-04-27 08:59:58.932078000 +0000
@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st
  * The key type depends on whether it's a shared or private mapping.
  * Don't rearrange members without looking at hash_futex().
  *
- * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.
+ * There are three cmponents in offset:
  * We use the two low order bits of offset to tell what is the kind of key :
  *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)
  *       (no reference on an inode or mm)
@@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st
  *	mapped on a file (reference on the underlying inode)
  *  10 : Shared futex (PTHREAD_PROCESS_SHARED)
  *       (but private mapping on an mm, and reference taken on it)
+ * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.
+ * The rest hign order bits indicates the index if the page is a
+ * subpage of a compound page.
 */

 #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
@@ -36,17 +39,17 @@ union futex_key {
 	struct {
 		unsigned long pgoff;
 		struct inode *inode;
-		int offset;
+		unsigned long offset;
 	} shared;
 	struct {
 		unsigned long address;
 		struct mm_struct *mm;
-		int offset;
+		unsigned long offset;
 	} private;
 	struct {
 		unsigned long word;
 		void *ptr;
-		int offset;
+		unsigned long offset;
 	} both;
 };

diff -uprN linux3.9-orig/kernel/futex.c linux3.9/kernel/futex.c
--- linux3.9-orig/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
+++ linux3.9/kernel/futex.c	2013-05-06 16:24:40.403525000 +0000
@@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
 	}
 }

+/*
+* Get subpage index in compound page, and add it into futex_key.
+*/
+static void key_add_compound_idx(union futex_key *key,
+				 struct page *head_page, struct page *page)
+{
+	int compound_idx;
+
+	if (compound_order(head_page) >= MAX_ORDER)
+		compound_idx = page_to_pfn(page) - page_to_pfn(head_page);
+	else
+		compound_idx = page - head_page;
+
+	key->both.offset |= compound_idx << PAGE_SHIFT;
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr:	virtual address of the futex
@@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu
  * The key words are stored in *key on success.
  *
  * For shared mappings, it's (page->index, file_inode(vma->vm_file),
- * offset_within_page).  For private mappings, it's (uaddr, current->mm).
+ * page_compound_index and offset_within_page).
+ * For private mappings, it's (uaddr, current->mm).
  * We can usually work out the index without swapping in the page.
  *
  * lock_page() might sleep, the caller should not hold a spinlock.
@@ -366,6 +383,8 @@ again:
 		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
 		key->shared.inode = page_head->mapping->host;
 		key->shared.pgoff = page_head->index;
+		if (page != page_head)
+			key_add_compound_idx(key, page_head, page);
 	}

 	get_futex_key_refs(key);

[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

Hi all,

I reworked the patch base on your advices.
For the line-wrapped bug before, I use this mailbox to send the mail .



Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>


diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h
--- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000
+++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000
@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st
  * The key type depends on whether it's a shared or private mapping.
  * Don't rearrange members without looking at hash_futex().
  *
- * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.
+ * There are three cmponents in offset:
  * We use the two low order bits of offset to tell what is the kind of key :
  *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)
  *       (no reference on an inode or mm)
@@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st
  *	mapped on a file (reference on the underlying inode)
  *  10 : Shared futex (PTHREAD_PROCESS_SHARED)
  *       (but private mapping on an mm, and reference taken on it)
+ * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.
+ * The rest hign order bits indicates the index if the page is a
+ * subpage of a compound page.
 */

 #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */
@@ -36,17 +39,17 @@ union futex_key {
 	struct {
 		unsigned long pgoff;
 		struct inode *inode;
-		int offset;
+		long offset;
 	} shared;
 	struct {
 		unsigned long address;
 		struct mm_struct *mm;
-		int offset;
+		long offset;
 	} private;
 	struct {
 		unsigned long word;
 		void *ptr;
-		int offset;
+		long offset;
 	} both;
 };

diff -uprN orig/linux3.9-rc7/kernel/futex.c new/linux3.9-rc7/kernel/futex.c
--- orig/linux3.9-rc7/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000
+++ new/linux3.9-rc7/kernel/futex.c	2013-04-19 16:24:05.629143000 +0000
@@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu
 	}
 }

+/*
+* Get subpage index in compound page, for futex_key.
+*/
+static inline int get_page_compound_index(struct page *page)
+{
+	struct page *head_page;
+	if (PageHead(page))
+		return 0;
+
+	head_page = compound_head(page);
+	if (compound_order(head_page) >= MAX_ORDER)
+		return page_to_pfn(page) - page_to_pfn(head_page);
+	else
+		return page - compound_head(page);
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr:	virtual address of the futex
@@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu
  * The key words are stored in *key on success.
  *
  * For shared mappings, it's (page->index, file_inode(vma->vm_file),
- * offset_within_page).  For private mappings, it's (uaddr, current->mm).
+ * page_compound_index and offset_within_page).
+ * For private mappings, it's (uaddr, current->mm).
  * We can usually work out the index without swapping in the page.
  *
  * lock_page() might sleep, the caller should not hold a spinlock.
@@ -239,7 +256,7 @@ get_futex_key(u32 __user *uaddr, int fsh
 	unsigned long address = (unsigned long)uaddr;
 	struct mm_struct *mm = current->mm;
 	struct page *page, *page_head;
-	int err, ro = 0;
+	int err, ro = 0, comp_idx = 0;

 	/*
 	 * The futex address must be "naturally" aligned.
@@ -299,6 +316,7 @@ again:
 			 * freed from under us.
 			 */
 			if (page != page_head) {
+				comp_idx = get_page_compound_index(page);
 				get_page(page_head);
 				put_page(page);
 			}
@@ -311,6 +329,7 @@ again:
 #else
 	page_head = compound_head(page);
 	if (page != page_head) {
+		comp_idx = get_page_compound_index(page);
 		get_page(page_head);
 		put_page(page);
 	}
@@ -363,7 +382,9 @@ again:
 		key->private.mm = mm;
 		key->private.address = address;
 	} else {
-		key->both.offset |= FUT_OFF_INODE; /* inode-based key */
+		/* inode-based key */
+		key->both.offset |= ((long)comp_idx << PAGE_SHIFT)
+				   | FUT_OFF_INODE;
 		key->shared.inode = page_head->mapping->host;
 		key->shared.pgoff = page_head->index;
 	}

[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Zhang Yi]

Hi all,

I reworked the patch base on your advices。
For the line-wrapped bug before, I use this mailbox to send the mail .


Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>

diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h--- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000+++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st  * The key type depends on whether it's a shared or private mapping.  * Don't rearrange members without looking at hash_futex().  *- * offset is aligned to a multiple of sizeof(u32) (== 4) by definition.+ * There are three cmponents in offset:  * We use the two low order bits of offset to tell what is the kind of key :  *  00 : Private process futex (PTHREAD_PROCESS_PRIVATE)  *       (no reference on an inode or mm)@@ -27,6 +27,9 @@ handle_futex_death(u32 __user *uaddr, st  *	mapped on a file (reference on the underlying inode)  *  10 : Shared futex (PTHREAD_PROCESS_SHARED)  *       (but private mapping on an mm, and reference taken on it)+ * Bits 2 to (PAGE_SHIFT-1) indicates the offset of futex in its page.+ * The rest hign order bits indicates the index if the page is a+ * subpage of a compound page. */  #define FUT_OFF_INODE    1 /* We set bit 0 if key has a reference on inode */@@ -36,17 +39,17 @@ union futex_key { 	struct { 		unsigned long pgoff; 		struct inode *inode;-		int offset;+		long offset; 	} shared; 	struct { 		unsigned long address; 		struct mm_struct *mm;-		int offset;+		long offset; 	} private; 	struct { 		unsigned long word; 		void *ptr;-		int offset;+		long offset; 	} both; }; diff -uprN orig/linux3.9-rc7/kernel/futex.c new/linux3.9-rc7/kernel/futex.c--- orig/linux3.9-rc7/kernel/futex.c	2013-04-15 00:45:16.000000000 +0000+++ new/linux3.9-rc7/kernel/futex.c	2013-04-19 16:24:05.629143000 +0000@@ -215,6 +215,22 @@ static void drop_futex_key_refs(union fu 	} } +/*+* Get subpage index in compound page, for futex_key.+*/+static inline int get_page_compound_index(struct page *page)+{+	struct page *head_page;+	if (PageHead(page))+		return 0;++	head_page = compound_head(page);+	if (compound_order(head_page) >= MAX_ORDER)+		return page_to_pfn(page) - page_to_pfn(head_page);+	else+		return page - compound_head(page);+}+ /**  * get_futex_key() - Get parameters which are the keys for a futex  * @uaddr:	virtual address of the futex@@ -228,7 +244,8 @@ static void drop_futex_key_refs(union fu  * The key words are stored in *key on success.  *  * For shared mappings, it's (page->index, file_inode(vma->vm_file),- * offset_within_page).  For private mappings, it's (uaddr, current->mm).+ * page_compound_index and offset_within_page).+ * For private mappings, it's (uaddr, current->mm).  * We can usually work out the index without swapping in the page.  *  * lock_page() might sleep, the caller should not hold a spinlock.@@ -239,7 +256,7 @@ get_futex_key(u32 __user *uaddr, int fsh 	unsigned long address = (unsigned long)uaddr; 	struct mm_struct *mm = current->mm; 	struct page *page, *page_head;-	int err, ro = 0;+	int err, ro = 0, comp_idx = 0;  	/* 	 * The futex address must be "naturally" aligned.@@ -299,6 +316,7 @@ again: 			 * freed from under us. 			 */ 			if (page != page_head) {+				comp_idx = get_page_compound_index(page); 				get_page(page_head); 				put_page(page); 			}@@ -311,6 +329,7 @@ again: #else 	page_head = compound_head(page); 	if (page != page_head) {+		comp_idx = get_page_compound_index(page); 		get_page(page_head); 		put_page(page); 	}@@ -363,7 +382,9 @@ again: 		key->private.mm = mm; 		key->private.address = address; 	} else {-		key->both.offset |= FUT_OFF_INODE; /* inode-based key */+		/* inode-based key */+		key->both.offset |= ((long)comp_idx << PAGE_SHIFT)+				   | FUT_OFF_INODE; 		key->shared.inode = page_head->mapping->host; 		key->shared.pgoff = page_head->index; 	}

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1344 bytes --]

On Wed, 24 Apr 2013, Zhang Yi wrote:

> Hi all,
> 
> I reworked the patch base on your advices。
> For the line-wrapped bug before, I use this mailbox to send the mail .

Unfortunately this did not work out very well.

1. Your patch now lacks a proper changelog which explains the change

2. Your patch lacks any newline characters as you can see below
 
> 
> Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
> 
> diff -uprN orig/linux3.9-rc7/include/linux/futex.h new/linux3.9-rc7/include/linux/futex.h--- orig/linux3.9-rc7/include/linux/futex.h	2013-04-15 00:45:16.000000000 +0000+++ new/linux3.9-rc7/include/linux/futex.h	2013-04-19 16:33:58.725880000 +0000@@ -19,7 +19,7 @@ handle_futex_death(u32 __user *uaddr, st  * The key type depends on whether it's a shared or private mapping.  * Don't rearrange members without looking at hash_futex().  *- * offset is aligned to a multiple of sizeof(u32) (==

Please read Documentation/SubmittingPatches and
Documentation/email-clients.txt. And as I said before: Send the mail
to yourself or a coworker and verify that it can be applied.

Thanks,

	tglx

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/17 01:57:10:

> Again, a functional testcase in futextest would be a good idea. This
> helps validate the patch and also can be used to identify regressions in
> the future.

I will post the testcase code later.

> 
> What is the max value of comp_idx? Are we at risk of truncating it?
> Looks like not really from my initial look.
> 
> This also needs a comment in futex.h describing the usage of the offset
> field in union futex_key as well as above get_futex_key describing the
> key for shared mappings.
> 
> 

As far as I know , the max size of one hugepage is 1 GBytes for x86 cpu.
Can some other cpus support greater hugepage even more than 4 GBytes? If 
so, we can change the type of 'offset' from int to long to avoid 
truncating.

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/17/2013 02:55 AM, zhang.yi20@zte.com.cn wrote:
> Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/17 01:57:10:
> 
>> Again, a functional testcase in futextest would be a good idea. This
>> helps validate the patch and also can be used to identify regressions in
>> the future.
> 
> I will post the testcase code later.
> 
>>
>> What is the max value of comp_idx? Are we at risk of truncating it?
>> Looks like not really from my initial look.
>>
>> This also needs a comment in futex.h describing the usage of the offset
>> field in union futex_key as well as above get_futex_key describing the
>> key for shared mappings.
>>
>>
> 
> As far as I know , the max size of one hugepage is 1 GBytes for x86 cpu.
> Can some other cpus support greater hugepage even more than 4 GBytes? If 
> so, we can change the type of 'offset' from int to long to avoid 
> truncating.

I discussed this with Dave Hansen, on CC, and he thought we needed 9
bits, so even on x86 32b we should be covered.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Dave Hansen]

On 04/17/2013 07:18 AM, Darren Hart wrote:
>>> This also needs a comment in futex.h describing the usage of the offset
>>> field in union futex_key as well as above get_futex_key describing the
>>> key for shared mappings.
>>>
>> As far as I know , the max size of one hugepage is 1 GBytes for x86 cpu.
>> Can some other cpus support greater hugepage even more than 4 GBytes? If 
>> so, we can change the type of 'offset' from int to long to avoid 
>> truncating.
> 
> I discussed this with Dave Hansen, on CC, and he thought we needed 9
> bits, so even on x86 32b we should be covered.

I think the problem is actually on 64-bit since you still only have
32-bits in an 'int' there.

I guess it's remotely possible that we could have some
mega-super-huge-gigantic pages show up in hardware some day, or that
somebody would come up with software-only one.  I bet there's a lot more
code that will break in the kernel than this futex code, though.

The other option would be to start #defining some build-time constant
for what the largest possible huge page size is, then BUILD_BUG_ON() it.

Or you can just make it a long ;)

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/17/2013 08:26 AM, Dave Hansen wrote:
> On 04/17/2013 07:18 AM, Darren Hart wrote:
>>>> This also needs a comment in futex.h describing the usage of the offset
>>>> field in union futex_key as well as above get_futex_key describing the
>>>> key for shared mappings.
>>>>
>>> As far as I know , the max size of one hugepage is 1 GBytes for x86 cpu.
>>> Can some other cpus support greater hugepage even more than 4 GBytes? If 
>>> so, we can change the type of 'offset' from int to long to avoid 
>>> truncating.
>>
>> I discussed this with Dave Hansen, on CC, and he thought we needed 9
>> bits, so even on x86 32b we should be covered.
> 
> I think the problem is actually on 64-bit since you still only have
> 32-bits in an 'int' there.
> 
> I guess it's remotely possible that we could have some
> mega-super-huge-gigantic pages show up in hardware some day, or that
> somebody would come up with software-only one.  I bet there's a lot more
> code that will break in the kernel than this futex code, though.
> 
> The other option would be to start #defining some build-time constant
> for what the largest possible huge page size is, then BUILD_BUG_ON() it.
> 
> Or you can just make it a long ;)

If we make it a long I'd want to see futextest performance tests before
and after. Messing with the futex_key has been known to have bad results
in the past :-)

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/17 23:51:36:

> On 04/17/2013 08:26 AM, Dave Hansen wrote:
> > On 04/17/2013 07:18 AM, Darren Hart wrote:
> >>>> This also needs a comment in futex.h describing the usage of the
> >>>> offset field in union futex_key as well as above get_futex_key
> >>>> describing the key for shared mappings.
> >>>>
> >>> As far as I know , the max size of one hugepage is 1 GBytes for 
> >>> x86 cpu. Can some other cpus support greater hugepage even more 
> >>> than 4 GBytes? If so, we can change the type of 'offset' from int 
> >>>  to long to avoid truncating.
> >>
> >> I discussed this with Dave Hansen, on CC, and he thought we needed
> >> 9 bits, so even on x86 32b we should be covered.
> > 
> > I think the problem is actually on 64-bit since you still only have
> > 32-bits in an 'int' there.
> > 
> > I guess it's remotely possible that we could have some
> > mega-super-huge-gigantic pages show up in hardware some day, or that
> > somebody would come up with software-only one.  I bet there's a lot
> > more code that will break in the kernel than this futex code, though.
> > 
> > The other option would be to start #defining some build-time constant
> > for what the largest possible huge page size is, then BUILD_BUG_ON()
> > it.
> >
> > Or you can just make it a long ;)
> 
> If we make it a long I'd want to see futextest performance tests before
> and after. Messing with the futex_key has been known to have bad results
> in the past :-)
> 
> -- 
 
I have run futextest/performance/futex_wait for testing, 5 times before 
make it long:
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10215 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 9862 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10081 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10060 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10081 Kiter/s


And 5 times after make it long:
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 9940 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10204 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 9901 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10152 Kiter/s

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 10060 Kiter/s


Seems OK, is it?

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/18/2013 01:05 AM, zhang.yi20@zte.com.cn wrote:
> Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/17 23:51:36:
> 
>> On 04/17/2013 08:26 AM, Dave Hansen wrote:
>>> On 04/17/2013 07:18 AM, Darren Hart wrote:
>>>>>> This also needs a comment in futex.h describing the usage of the
>>>>>> offset field in union futex_key as well as above get_futex_key
>>>>>> describing the key for shared mappings.
>>>>>>
>>>>> As far as I know , the max size of one hugepage is 1 GBytes for 
>>>>> x86 cpu. Can some other cpus support greater hugepage even more 
>>>>> than 4 GBytes? If so, we can change the type of 'offset' from int 
>>>>>  to long to avoid truncating.
>>>>
>>>> I discussed this with Dave Hansen, on CC, and he thought we needed
>>>> 9 bits, so even on x86 32b we should be covered.
>>>
>>> I think the problem is actually on 64-bit since you still only have
>>> 32-bits in an 'int' there.
>>>
>>> I guess it's remotely possible that we could have some
>>> mega-super-huge-gigantic pages show up in hardware some day, or that
>>> somebody would come up with software-only one.  I bet there's a lot
>>> more code that will break in the kernel than this futex code, though.
>>>
>>> The other option would be to start #defining some build-time constant
>>> for what the largest possible huge page size is, then BUILD_BUG_ON()
>>> it.
>>>
>>> Or you can just make it a long ;)
>>
>> If we make it a long I'd want to see futextest performance tests before
>> and after. Messing with the futex_key has been known to have bad results
>> in the past :-)
>>
>> -- 
>  
> I have run futextest/performance/futex_wait for testing, 5 times before 
> make it long:
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10215 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 9862 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10081 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10060 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10081 Kiter/s
> 
> 
> And 5 times after make it long:
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 9940 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10204 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 9901 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10152 Kiter/s
> 
> futex_wait: Measure FUTEX_WAIT operations per second
>         Arguments: iterations=100000000 threads=256
> Result: 10060 Kiter/s
> 
> 
> Seems OK, is it?
> 

Changes appear to be in the noise, no impact with this load anyway.

How many CPUs on your test machine? I presume not 256?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="GB2312", Size: 2312 bytes --]

Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/18 22:34:29:

> On 04/18/2013 01:05 AM, zhang.yi20@zte.com.cn wrote:
> > 
> > I have run futextest/performance/futex_wait for testing, 
> >  5 times before make it long:
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10215 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 9862 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10081 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10060 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10081 Kiter/s
> > 
> > 
> > And 5 times after make it long:
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 9940 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10204 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 9901 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10152 Kiter/s
> > 
> > futex_wait: Measure FUTEX_WAIT operations per second
> >         Arguments: iterations=100000000 threads=256
> > Result: 10060 Kiter/s
> > 
> > 
> > Seems OK, is it?
> > 
> 
> Changes appear to be in the noise, no impact with this load 
> anyway.
> How many CPUs on your test machine? I presume not 256?
> 
> -- 

There are 16 CPUs£¬ and mode is:
Intel(R) Xeon(R) CPU           C5528  @ 2.13GHz

Shall I make the number of threads as the CPUS? I test again with argument 
'-n 16', the result is similar.

BTW, have you seen the testcase in my other mail?  It seems to be rejected 
by LKML.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/18/2013 07:13 PM, zhang.yi20@zte.com.cn wrote:
> Darren Hart <dvhart@linux.intel.com> wrote on 2013/04/18 22:34:29:
> 
>> On 04/18/2013 01:05 AM, zhang.yi20@zte.com.cn wrote:
>>>
>>> I have run futextest/performance/futex_wait for testing, 
>>>  5 times before make it long:
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10215 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 9862 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10081 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10060 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10081 Kiter/s
>>>
>>>
>>> And 5 times after make it long:
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 9940 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10204 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 9901 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10152 Kiter/s
>>>
>>> futex_wait: Measure FUTEX_WAIT operations per second
>>>         Arguments: iterations=100000000 threads=256
>>> Result: 10060 Kiter/s
>>>
>>>
>>> Seems OK, is it?
>>>
>>
>> Changes appear to be in the noise, no impact with this load 
>> anyway.
>> How many CPUs on your test machine? I presume not 256?
>>
>> -- 
> 
> There are 16 CPUs， and mode is:
> Intel(R) Xeon(R) CPU           C5528  @ 2.13GHz
> 
> Shall I make the number of threads as the CPUS? I test again with argument 
> '-n 16', the result is similar.

No, I just wanted to be sure you weren't running 256 threads on 1 CPU as
you wouldn't be likely to be stressing the bucket list much :-)

> BTW, have you seen the testcase in my other mail?  It seems to be rejected 
> by LKML.

Might have something to do with what appears to still be HTML email. You
really need to fix your email client.

See:  http://www.tux.org/lkml/
#12 in particular.


-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

> 
> BTW, have you seen the testcase in my other mail?  It seems to be rejected 
> by LKML.
> 

I did not receive it, did you also CC me?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[zhang.yi20]

Hello,

The futex-keys of processes share futex determined by page-offset, 
mapping-host, and 
mapping-index of the user space address. 
User appications using hugepage for futex may lead to futex-key conflict. 
Assume there 
are two or more futexes in diffrent normal pages of the hugepage, and each 
futex has 
the same offset in its normal page, causing all the futexes have the same 
futex-key. 
In that case, futex may not work well. 

This patch adds the normal page index in the compound page into the offset 
of futex-key. 

Steps to reproduce the bug: 
1. The 1st thread map a file of hugetlbfs, and use the return address as 
the 1st mutex's 
address, and use the return address with PAGE_SIZE added as the 2nd 
mutex's address; 
2. The 1st thread initialize the two mutexes with pshared attribute, and 
lock the two mutexes. 
3. The 1st thread create the 2nd thread, and the 2nd thread block on the 
1st mutex. 
4. The 1st thread create the 3rd thread, and the 3rd thread block on the 
2nd mutex. 
5. The 1st thread unlock the 2nd mutex, the 3rd thread can not take the 
2nd mutex, and 
may block forever. 

Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>

diff -uprN orig/linux-3.9-rc7/include/linux/mm.h 
new/linux-3.9-rc7/include/linux/mm.h
--- orig/linux-3.9-rc7/include/linux/mm.h       2013-04-15 
00:45:16.000000000 +0000
+++ new/linux-3.9-rc7/include/linux/mm.h        2013-04-16 
11:21:59.573458000 +0000
@@ -502,6 +502,20 @@ static inline void set_compound_order(st
        page[1].lru.prev = (void *)order;
 }
 
+static inline void set_page_compound_index(struct page *page, int index)
+{
+       if (PageHead(page))
+               return;
+       page->index = index;
+}
+
+static inline int get_page_compound_index(struct page *page)
+{
+       if (PageHead(page))
+               return 0;
+       return page->index;
+}
+
 #ifdef CONFIG_MMU
 /*
  * Do pte_mkwrite, but only if the vma says VM_WRITE.  We do this when
diff -uprN orig/linux-3.9-rc7/kernel/futex.c 
new/linux-3.9-rc7/kernel/futex.c
--- orig/linux-3.9-rc7/kernel/futex.c   2013-04-15 00:45:16.000000000 
+0000
+++ new/linux-3.9-rc7/kernel/futex.c    2013-04-16 11:13:30.069887000 
+0000
@@ -239,7 +239,7 @@ get_futex_key(u32 __user *uaddr, int fsh
        unsigned long address = (unsigned long)uaddr;
        struct mm_struct *mm = current->mm;
        struct page *page, *page_head;
-       int err, ro = 0;
+       int err, ro = 0, comp_idx = 0;
 
        /*
         * The futex address must be "naturally" aligned.
@@ -299,6 +299,7 @@ again:
                         * freed from under us.
                         */
                        if (page != page_head) {
+                               comp_idx = get_page_compound_index(page);
                                get_page(page_head);
                                put_page(page);
                        }
@@ -311,6 +312,7 @@ again:
 #else
        page_head = compound_head(page);
        if (page != page_head) {
+               comp_idx = get_page_compound_index(page);
                get_page(page_head);
                put_page(page);
        }
@@ -363,7 +365,8 @@ again:
                key->private.mm = mm;
                key->private.address = address;
        } else {
-               key->both.offset |= FUT_OFF_INODE; /* inode-based key */
+               key->both.offset |= (comp_idx << PAGE_SHIFT)
+                                   | FUT_OFF_INODE; /* inode-based key */
                key->shared.inode = page_head->mapping->host;
                key->shared.pgoff = page_head->index;
        }
diff -uprN orig/linux-3.9-rc7/mm/hugetlb.c new/linux-3.9-rc7/mm/hugetlb.c
--- orig/linux-3.9-rc7/mm/hugetlb.c     2013-04-15 00:45:16.000000000 
+0000
+++ new/linux-3.9-rc7/mm/hugetlb.c      2013-04-16 10:23:02.658531000 
+0000
@@ -667,6 +667,7 @@ static void prep_compound_gigantic_page(
        for (i = 1; i < nr_pages; i++, p = mem_map_next(p, page, i)) {
                __SetPageTail(p);
                set_page_count(p, 0);
+               set_page_compound_index(p, i);
                p->first_page = page;
        }
 }
diff -uprN orig/linux-3.9-rc7/mm/page_alloc.c 
new/linux-3.9-rc7/mm/page_alloc.c
--- orig/linux-3.9-rc7/mm/page_alloc.c  2013-04-15 00:45:16.000000000 
+0000
+++ new/linux-3.9-rc7/mm/page_alloc.c   2013-04-16 10:23:16.452393000 
+0000
@@ -361,6 +361,7 @@ void prep_compound_page(struct page *pag
                struct page *p = page + i;
                __SetPageTail(p);
                set_page_count(p, 0);
+               set_page_compound_index(p, i);
                p->first_page = page;
        }
 }

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/15/2013 08:37 PM, zhang.yi20@zte.com.cn wrote:
> Hello,
>

Hi Zhang,

I've rewrapped your plain text here for legibility, please adjust your
mail client accordingly.

> The futex-keys of processes share futex determined by page-offset,
> mapping-host, and mapping-index of the user space address.  User appications
> using hugepage for futex may lead to futex-key conflict.  Assume there are two
> or more futexes in diffrent normal pages of the hugepage, and each futex has
> the same offset in its normal page, causing all the futexes have the same
> futex-key.  In that case, futex may not work well.
>
> This patch adds the normal page index in the compound page into the offset of
> futex-key.

It also modifies the mm prep_compound*page() routines to set the page
compound index. You didn't modify the structure itself, I'm curious why
this information wasn't set before? Something for the MM folks I guess..

>
> Steps to reproduce the bug:
> 1. The 1st thread map a file of hugetlbfs, and use the return address as the
>    1st mutex's address, and use the return address with PAGE_SIZE added as the
>    2nd mutex's address;
> 2. The 1st thread initialize the two mutexes with pshared attribute, and lock
>    the two mutexes.
> 3. The 1st thread create the 2nd thread, and the 2nd thread block on the 1st
>    mutex.
> 4. The 1st thread create the 3rd thread, and the 3rd thread block on the 2nd
>    mutex.
> 5. The 1st thread unlock the 2nd mutex, the 3rd thread can not take the 2nd
>    mutex, and may block forever.


Again, a functional testcase in futextest would be a good idea. This
helps validate the patch and also can be used to identify regressions in
the future.


> Signed-off-by: Zhang Yi <zhang.yi20@zte.com.cn>
> Tested-by: Ma Chenggong <ma.chenggong@zte.com.cn>
> Reviewed-by: Liu Dong <liu.dong3@zte.com.cn>
> Reviewed-by: Cui Yunfeng <cui.yunfeng@zte.com.cn>
> Reviewed-by: Lu Zhongjun <lu.zhongjun@zte.com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
>
> diff -uprN orig/linux-3.9-rc7/include/linux/mm.h
> new/linux-3.9-rc7/include/linux/mm.h
> --- orig/linux-3.9-rc7/include/linux/mm.h       2013-04-15
> 00:45:16.000000000 +0000
> +++ new/linux-3.9-rc7/include/linux/mm.h        2013-04-16
> 11:21:59.573458000 +0000
> @@ -502,6 +502,20 @@ static inline void set_compound_order(st
>         page[1].lru.prev = (void *)order;
>  }
>
> +static inline void set_page_compound_index(struct page *page, int index)
> +{
> +       if (PageHead(page))
> +               return;
> +       page->index = index;
> +}

I presume the spaces instead of tabs is a result of your mailer mangling
whitespace?

> +
> +static inline int get_page_compound_index(struct page *page)
> +{
> +       if (PageHead(page))
> +               return 0;
> +       return page->index;
> +}
> +
>  #ifdef CONFIG_MMU
>  /*
>   * Do pte_mkwrite, but only if the vma says VM_WRITE.  We do this when
> diff -uprN orig/linux-3.9-rc7/kernel/futex.c
> new/linux-3.9-rc7/kernel/futex.c
> --- orig/linux-3.9-rc7/kernel/futex.c   2013-04-15 00:45:16.000000000
> +0000
> +++ new/linux-3.9-rc7/kernel/futex.c    2013-04-16 11:13:30.069887000
> +0000
> @@ -239,7 +239,7 @@ get_futex_key(u32 __user *uaddr, int fsh
>         unsigned long address = (unsigned long)uaddr;
>         struct mm_struct *mm = current->mm;
>         struct page *page, *page_head;
> -       int err, ro = 0;
> +       int err, ro = 0, comp_idx = 0;
>
>         /*
>          * The futex address must be "naturally" aligned.
> @@ -299,6 +299,7 @@ again:
>                          * freed from under us.
>                          */
>                         if (page != page_head) {
> +                               comp_idx = get_page_compound_index(page);
>                                 get_page(page_head);
>                                 put_page(page);
>                         }
> @@ -311,6 +312,7 @@ again:
>  #else
>         page_head = compound_head(page);
>         if (page != page_head) {
> +               comp_idx = get_page_compound_index(page);
>                 get_page(page_head);
>                 put_page(page);
>         }
> @@ -363,7 +365,8 @@ again:
>                 key->private.mm = mm;
>                 key->private.address = address;
>         } else {
> -               key->both.offset |= FUT_OFF_INODE; /* inode-based key */
> +               key->both.offset |= (comp_idx << PAGE_SHIFT)
> +                                   | FUT_OFF_INODE; /* inode-based key */

Comments at the end of lines are bad form already, when moving to
multi-line, please move the comment just above the statements.

What is the max value of comp_idx? Are we at risk of truncating it?
Looks like not really from my initial look.

This also needs a comment in futex.h describing the usage of the offset
field in union futex_key as well as above get_futex_key describing the
key for shared mappings.


Thanks,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Dave Hansen]

Instead of bothering to store the index, why not just calculate it, like:

On 04/15/2013 08:37 PM, zhang.yi20@zte.com.cn wrote:
> +static inline int get_page_compound_index(struct page *page)
> +{
> +       if (PageHead(page))
> +               return 0;
> +       return compound_head(page) - page;
> +}

BTW, you've really got to get your mail client fixed.  Your patch is
still line-wrapped.

Re: [PATCH] futex: bugfix for futex-key conflict when futex use hugepage

[Darren Hart]

On 04/16/2013 11:37 AM, Dave Hansen wrote:
> Instead of bothering to store the index, why not just calculate it, like:
> 
> On 04/15/2013 08:37 PM, zhang.yi20@zte.com.cn wrote:
>> +static inline int get_page_compound_index(struct page *page)
>> +{
>> +       if (PageHead(page))
>> +               return 0;
>> +       return compound_head(page) - page;
>> +}
> 
> BTW, you've really got to get your mail client fixed.  Your patch is
> still line-wrapped.

And with this it would no longer be necessary to store this index at
all, eliminating all changes to the MM other than this accessor function
- which if not needed there could be added to futex.c, or even replaced with
"page_head - page" in get_futex_key() right?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel