futex: bugfix for futex-key conflict when futex use hugepage

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 <[email protected]>

Tested-by Ma Chenggong <[email protected]>

Reviewed-by Thomas Gleixner <[email protected]>

Reviewed-by Darren Hart <[email protected]>

Reviewed-by Dave Hansen <[email protected]>

Reviewed-by Mel Gorman <[email protected]>

Reviewed-by Liu Dong <[email protected]>

Reviewed-by Cui Yunfeng <[email protected]>

Reviewed-by Lu Zhongjun <[email protected]>

Reviewed-by Jiang Biao <[email protected]>

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

Quoted 25 lines · 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.

Quoted 11 lines

Signed-off-by Zhang Yi <[email protected]>

Tested-by Ma Chenggong <[email protected]>

Reviewed-by Thomas Gleixner <[email protected]>

Reviewed-by Darren Hart <[email protected]>

Reviewed-by Dave Hansen <[email protected]>

Reviewed-by Mel Gorman <[email protected]>

Reviewed-by Liu Dong <[email protected]>

Reviewed-by Cui Yunfeng <[email protected]>

Reviewed-by Lu Zhongjun <[email protected]>

Reviewed-by Jiang Biao <[email protected]>

Did all these people really review it? I just whinged about the last

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 <[email protected]>

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

Mel Gorman <[email protected]> wrote on 2013/05/15 22:20:35:

Quoted 40 lines

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

Quoted 36 lines · 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 <[email protected]>

Tested-by Ma Chenggong <[email protected]>

Reviewed-by Thomas Gleixner <[email protected]>

Reviewed-by Darren Hart <[email protected]>

Reviewed-by Dave Hansen <[email protected]>

Reviewed-by Mel Gorman <[email protected]>

Reviewed-by Liu Dong <[email protected]>

Reviewed-by Cui Yunfeng <[email protected]>

Reviewed-by Lu Zhongjun <[email protected]>

Reviewed-by Jiang Biao <[email protected]>

Quoted 1 line

Did all these people really review it? I just whinged about the last

patch

Quoted 3 lines

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?

Quoted 6 lines

Reviewed-by Mel Gorman <[email protected]>

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.

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.

Darren Hart <[email protected]> wrote on 2013/05/16 09:30:31:

Quoted 7 lines

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.

No other specific reason. I just think that int is enough.

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 <[email protected]>

Tested-by Ma Chenggong <[email protected]>

Reviewed-by Jiang Biao <[email protected]>

diff -uprN linux-3.10-rc7.org/include/linux/hugetlb.h linux-3.10-rc7/include/linux/hugetlb.h

include/linux/hugetlb.h +16 -0

@@ -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-rc7.org/kernel/futex.c linux-3.10-rc7/kernel/futex.c

kernel/futex.c +2 -1

@@ -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-rc7.org/mm/hugetlb.c linux-3.10-rc7/mm/hugetlb.c

mm/hugetlb.c +17 -0

@@ -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);
+	unsigned long 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;

Otherwise this looks ready to me. Thomas, do you want a resend with commit message corrections or do you prefer to integrate those yourself?

With the above fixes:

Acked-by Darren Hart <[email protected]>