Skip to contentSkip to conversation

user_events: Use alloc_pages instead of kzalloc for register pages

PATCH In review Beau Belgrave 2 participants 4 messages Mar 11, 2022

Subsystems (2)
Linux-next tree Tracing
lore
Share & tools
lore

Beau Belgrave

author
#0 lore raw

PATCH 1/1 user_events: Use alloc_pages instead of kzalloc for register pages

kzalloc virtual addresses do not work with SetPageReserved, use the actual page virtual addresses instead via alloc_pages.

The issue is reported when booting with user_events and DEBUG_VM_PGFLAGS=y.

Link https://lore.kernel.org/linux-trace-devel/CADYN=9+xY5Vku3Ws5E9S60SM5dCFfeGeRBkmDFbcxX0ZMoFing@mail.gmail.com/#R open here
Reported-by Anders Roxell <[email protected]>
Signed-off-by Beau Belgrave <[email protected]>
---
 kernel/trace/trace_events_user.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
kernel/trace/trace_events_user.c +7 -3
index 2b5e9fdb63a0..59c900789757 100644
@@ -1587,16 +1587,20 @@ static void set_page_reservations(bool set)
 static int __init trace_events_user_init(void)
 {
 	int ret;
+	struct page *register_pages;
 
 	/* Zero all bits beside 0 (which is reserved for failures) */
 	bitmap_zero(page_bitmap, MAX_EVENTS);
 	set_bit(0, page_bitmap);
 
-	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
+	register_pages = alloc_pages(GFP_KERNEL | __GFP_ZERO,
+				     get_order(MAX_EVENTS));
 
-	if (!register_page_data)
+	if (!register_pages)
 		return -ENOMEM;
 
+	register_page_data = page_address(register_pages);
+
 	set_page_reservations(true);
 
 	ret = create_user_tracefs();

@@ -1604,7 +1608,7 @@ static int __init trace_events_user_init(void)
 	if (ret) {
 		pr_warn("user_events could not register with tracefs\n");
 		set_page_reservations(false);
-		kfree(register_page_data);
+		__free_pages(register_pages, get_order(MAX_EVENTS));
 		return ret;
 	}

base-commit: 864ea0e10cc90416a01b46f0d47a6f26dc020820

Signature · 1 line
2.17.1
maintainer
#1 lore raw

kernel/trace/trace_events_user.c static void set_page_reservations(bool set) · line 1590

Quoted 9 lines · On Fri, 11 Mar 2022 14:30:28 -0800 Beau Belgrave <[email protected]>…
kernel/trace/trace_events_user.c +1 -0
index 2b5e9fdb63a0..59c900789757 100644
@@ -1587,16 +1587,20 @@ static void set_page_reservations(bool set)
 static int __init trace_events_user_init(void)
 {
 	int ret;
+	struct page *register_pages;

The int ret should come last.

Quoted 19 lines
kernel/trace/trace_events_user.c +5 -2
 	/* Zero all bits beside 0 (which is reserved for failures) */
 	bitmap_zero(page_bitmap, MAX_EVENTS);
 	set_bit(0, page_bitmap);
 
-	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
+	register_pages = alloc_pages(GFP_KERNEL | __GFP_ZERO,
+				     get_order(MAX_EVENTS));
 
-	if (!register_page_data)
+	if (!register_pages)
 		return -ENOMEM;
 
+	register_page_data = page_address(register_pages);
+
 	set_page_reservations(true);
 
 	ret = create_user_tracefs();

kernel/trace/trace_events_user.c static int __init trace_events_user_init(void) · line 1614

Quoted 8 lines
kernel/trace/trace_events_user.c +1 -1
@@ -1604,7 +1608,7 @@ static int __init trace_events_user_init(void)
 	if (ret) {
 		pr_warn("user_events could not register with tracefs\n");
 		set_page_reservations(false);
-		kfree(register_page_data);
+		__free_pages(register_pages, get_order(MAX_EVENTS));
 		return ret;
 	}

I tried it slightly differently. Why waist bits if MAX_EVENTS is greater than the order. That is, make MAX_EVENTS depend on the order, not the other way around.

-- Steve

kernel/trace/trace_events_user.c +8 -23
index 4febc1d6ae72..6941d0794347 100644
@@ -30,9 +30,10 @@
 
 /*
  * Limits how many trace_event calls user processes can create:
- * Must be multiple of PAGE_SIZE.
+ * Must be a power of two of PAGE_SIZE.
  */
-#define MAX_PAGES 1
+#define MAX_PAGE_ORDER 0
+#define MAX_PAGES (1 << MAX_PAGE_ORDER)
 #define MAX_EVENTS (MAX_PAGES * PAGE_SIZE)
 
 /* Limit how long of an event name plus args within the subsystem. */

@@ -1606,41 +1607,25 @@ static int create_user_tracefs(void)
 	return -ENODEV;
 }
 
-static void set_page_reservations(bool set)
-{
-	int page;
-
-	for (page = 0; page < MAX_PAGES; ++page) {
-		void *addr = register_page_data + (PAGE_SIZE * page);
-
-		if (set)
-			SetPageReserved(virt_to_page(addr));
-		else
-			ClearPageReserved(virt_to_page(addr));
-	}
-}
-
 static int __init trace_events_user_init(void)
 {
+	struct page pages;
 	int ret;
 
 	/* Zero all bits beside 0 (which is reserved for failures) */
 	bitmap_zero(page_bitmap, MAX_EVENTS);
 	set_bit(0, page_bitmap);
 
-	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
-
-	if (!register_page_data)
+	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, MAX_PAGE_ORDER);
+	if (!pages)
 		return -ENOMEM;
-
-	set_page_reservations(true);
+	register_page_data = page_address(pages);
 
 	ret = create_user_tracefs();
 
 	if (ret) {
 		pr_warn("user_events could not register with tracefs\n");
-		set_page_reservations(false);
-		kfree(register_page_data);
+		free_page((unsigned long)register_page_data);
 		return ret;
 	}
maintainer
#2 lore raw
Quoted 3 lines · On Fri, 11 Mar 2022 18:33:43 -0500 Steven Rostedt <[email protected]> wrote:

I tried it slightly differently. Why waist bits if MAX_EVENTS is greater than the order. That is, make MAX_EVENTS depend on the order, not the other way around.

Here's a version that keeps the reserving part as well as some bug fixes (I didn't even compile the previous version ;-)

-- Steve

kernel/trace/trace_events_user.c +8 -6
index 4febc1d6ae72..e10ad057e797 100644
@@ -30,9 +30,10 @@
 
 /*
  * Limits how many trace_event calls user processes can create:
- * Must be multiple of PAGE_SIZE.
+ * Must be a power of two of PAGE_SIZE.
  */
-#define MAX_PAGES 1
+#define MAX_PAGE_ORDER 0
+#define MAX_PAGES (1 << MAX_PAGE_ORDER)
 #define MAX_EVENTS (MAX_PAGES * PAGE_SIZE)
 
 /* Limit how long of an event name plus args within the subsystem. */

@@ -1622,16 +1623,17 @@ static void set_page_reservations(bool set)
 
 static int __init trace_events_user_init(void)
 {
+	struct page *pages;
 	int ret;
 
 	/* Zero all bits beside 0 (which is reserved for failures) */
 	bitmap_zero(page_bitmap, MAX_EVENTS);
 	set_bit(0, page_bitmap);
 
-	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
-
-	if (!register_page_data)
+	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, MAX_PAGE_ORDER);
+	if (!pages)
 		return -ENOMEM;
+	register_page_data = page_address(pages);
 
 	set_page_reservations(true);
 

@@ -1640,7 +1642,7 @@ static int __init trace_events_user_init(void)
 	if (ret) {
 		pr_warn("user_events could not register with tracefs\n");
 		set_page_reservations(false);
-		kfree(register_page_data);
+		__free_pages(pages, MAX_PAGE_ORDER);
 		return ret;
 	}
author
#3 lore raw
Quoted 51 lines · Steven Rostedt
Quoted 3 lines · On Fri, 11 Mar 2022 18:33:43 -0500 Steven Rostedt <[email protected]> wrote:

I tried it slightly differently. Why waist bits if MAX_EVENTS is greater than the order. That is, make MAX_EVENTS depend on the order, not the other way around.

Here's a version that keeps the reserving part as well as some bug fixes (I didn't even compile the previous version ;-)

-- Steve

kernel/trace/trace_events_user.c +7 -5
index 4febc1d6ae72..e10ad057e797 100644
@@ -30,9 +30,10 @@
 
 /*
  * Limits how many trace_event calls user processes can create:
- * Must be multiple of PAGE_SIZE.
+ * Must be a power of two of PAGE_SIZE.
  */
-#define MAX_PAGES 1
+#define MAX_PAGE_ORDER 0
+#define MAX_PAGES (1 << MAX_PAGE_ORDER)
 #define MAX_EVENTS (MAX_PAGES * PAGE_SIZE)
 
 /* Limit how long of an event name plus args within the subsystem. */

@@ -1622,16 +1623,17 @@ static void set_page_reservations(bool set)
 
 static int __init trace_events_user_init(void)
 {
+	struct page *pages;
 	int ret;
 
 	/* Zero all bits beside 0 (which is reserved for failures) */
 	bitmap_zero(page_bitmap, MAX_EVENTS);
 	set_bit(0, page_bitmap);
 
-	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
-
-	if (!register_page_data)
+	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, MAX_PAGE_ORDER);
+	if (!pages)
 		return -ENOMEM;
+	register_page_data = page_address(pages);
 
 	set_page_reservations(true);

kernel/trace/trace_events_user.c static int __init trace_events_user_init(void) · line 1648

Quoted 8 lines
kernel/trace/trace_events_user.c +1 -1
@@ -1640,7 +1642,7 @@ static int __init trace_events_user_init(void)
 	if (ret) {
 		pr_warn("user_events could not register with tracefs\n");
 		set_page_reservations(false);
-		kfree(register_page_data);
+		__free_pages(pages, MAX_PAGE_ORDER);
 		return ret;
 	}

This looks good to me, I agree having the max events aligning to page order makes more sense going forward.

Thanks,
-Beau