linux-kernel - Re: [PATCH] user_events: Use alloc_pages instead of kzalloc for register pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220311183343.4d8c94a5@gandalf.local.home>
Date:   Fri, 11 Mar 2022 18:33:43 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Beau Belgrave <beaub@...ux.microsoft.com>
Cc:     mhiramat@...nel.org, anders.roxell@...aro.org,
        linux-trace-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-next@...r.kernel.org
Subject: Re: [PATCH] user_events: Use alloc_pages instead of kzalloc for
 register pages

On Fri, 11 Mar 2022 14:30:28 -0800
Beau Belgrave <beaub@...ux.microsoft.com> wrote:

> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 2b5e9fdb63a0..59c900789757 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -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.

>  
>  	/* 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;
>  	}
>  

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

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 4febc1d6ae72..6941d0794347 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -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;
 	}