diff mbox series

profiling: initialize prof_cpu_mask from profile_online_cpu()

Message ID 85edf211-aa30-4671-93e0-5173b3f7adf2@I-love.SAKURA.ne.jp
State New
Headers
Series profiling: initialize prof_cpu_mask from profile_online_cpu() |

Commit Message

Tetsuo Handa Jan. 31, 2024, 2:06 p.m. UTC 
  syzbot is reporting uninit-value at profile_hits(), for commit acd895795d35
("profiling: fix broken profiling regression") by error initialized
prof_cpu_mask too early.

do_profile_hits() is called from profile_tick() from timer interrupt
only if cpumask_test_cpu(smp_processor_id(), prof_cpu_mask) is true and
prof_buffer is not NULL. But the syzbot's report says that profile_hits()
was called while current thread is still doing vzalloc(buffer_bytes)
where prof_buffer is NULL at this moment. This indicates two things.

One is that cpumask_set_cpu(cpu, prof_cpu_mask) should have been called
 from profile_online_cpu() from cpuhp_setup_state() only after
profile_init() completed. Fix this by explicitly calling cpumask_copy()
 from create_proc_profile() on only UP kernels.

The other is that multiple threads concurrently tried to write to
/sys/kernel/profiling interface, which caused that somebody else tried
to re-initialize prof_buffer despite somebody has already initialized
prof_buffer. Fix this by using serialization.

Reported-by: syzbot+b1a83ab2a9eb9321fbdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b1a83ab2a9eb9321fbdd
Fixes: acd895795d35 ("profiling: fix broken profiling regression")
Tested-by: syzbot+b1a83ab2a9eb9321fbdd@syzkaller.appspotmail.com
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/ksysfs.c  | 27 ++++++++++++++++++++++-----
 kernel/profile.c |  6 +++---
 2 files changed, 25 insertions(+), 8 deletions(-)

Comments

Tetsuo Handa Feb. 15, 2024, 2:41 p.m. UTC | #1 
Hello, Linus.

It seems that nobody maintains this code.

Can I directly send this patch to you, as "THE REST" rule of MAINTAINERS file?
Is some tag like [PATCH ORPHANED] available?

On 2024/01/31 23:06, Tetsuo Handa wrote:
> syzbot is reporting uninit-value at profile_hits(), for commit acd895795d35
> ("profiling: fix broken profiling regression") by error initialized
> prof_cpu_mask too early.
> 
> do_profile_hits() is called from profile_tick() from timer interrupt
> only if cpumask_test_cpu(smp_processor_id(), prof_cpu_mask) is true and
> prof_buffer is not NULL. But the syzbot's report says that profile_hits()
> was called while current thread is still doing vzalloc(buffer_bytes)
> where prof_buffer is NULL at this moment. This indicates two things.
> 
> One is that cpumask_set_cpu(cpu, prof_cpu_mask) should have been called
>  from profile_online_cpu() from cpuhp_setup_state() only after
> profile_init() completed. Fix this by explicitly calling cpumask_copy()
>  from create_proc_profile() on only UP kernels.
> 
> The other is that multiple threads concurrently tried to write to
> /sys/kernel/profiling interface, which caused that somebody else tried
> to re-initialize prof_buffer despite somebody has already initialized
> prof_buffer. Fix this by using serialization.
> 
> Reported-by: syzbot+b1a83ab2a9eb9321fbdd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b1a83ab2a9eb9321fbdd
> Fixes: acd895795d35 ("profiling: fix broken profiling regression")
> Tested-by: syzbot+b1a83ab2a9eb9321fbdd@syzkaller.appspotmail.com
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  kernel/ksysfs.c  | 27 ++++++++++++++++++++++-----
>  kernel/profile.c |  6 +++---
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 1d4bc493b2f4..66bc712f590c 100644
> --- a/kernel/ksysfs.c
> +++ b/kernel/ksysfs.c
> @@ -91,10 +91,23 @@ static ssize_t profiling_store(struct kobject *kobj,
>  				   struct kobj_attribute *attr,
>  				   const char *buf, size_t count)
>  {
> +	static DEFINE_MUTEX(lock);
>  	int ret;
>  
> -	if (prof_on)
> -		return -EEXIST;
> +	/*
> +	 * We need serialization, for profile_setup() initializes prof_on
> +	 * value. Also, use killable wait in case memory allocation from
> +	 * profile_init() triggered the OOM killer and chose current thread
> +	 * blocked here.
> +	 */
> +	if (mutex_lock_killable(&lock))
> +		return -EINTR;
> +
> +	if (prof_on) {
> +		count = -EEXIST;
> +		goto out;
> +	}
> +
>  	/*
>  	 * This eventually calls into get_option() which
>  	 * has a ton of callers and is not const.  It is
> @@ -102,11 +115,15 @@ static ssize_t profiling_store(struct kobject *kobj,
>  	 */
>  	profile_setup((char *)buf);
>  	ret = profile_init();
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		count = ret;
> +		goto out;
> +	}
>  	ret = create_proc_profile();
>  	if (ret)
> -		return ret;
> +		count = ret;
> +out:
> +	mutex_unlock(&lock);
>  	return count;
>  }
>  KERNEL_ATTR_RW(profiling);
> diff --git a/kernel/profile.c b/kernel/profile.c
> index 8a77769bc4b4..7575747e2ac6 100644
> --- a/kernel/profile.c
> +++ b/kernel/profile.c
> @@ -114,11 +114,9 @@ int __ref profile_init(void)
>  
>  	buffer_bytes = prof_len*sizeof(atomic_t);
>  
> -	if (!alloc_cpumask_var(&prof_cpu_mask, GFP_KERNEL))
> +	if (!zalloc_cpumask_var(&prof_cpu_mask, GFP_KERNEL))
>  		return -ENOMEM;
>  
> -	cpumask_copy(prof_cpu_mask, cpu_possible_mask);
> -
>  	prof_buffer = kzalloc(buffer_bytes, GFP_KERNEL|__GFP_NOWARN);
>  	if (prof_buffer)
>  		return 0;
> @@ -481,6 +479,8 @@ int __ref create_proc_profile(void)
>  		goto err_state_prep;
>  	online_state = err;
>  	err = 0;
> +#else
> +	cpumask_copy(prof_cpu_mask, cpu_possible_mask);
>  #endif
>  	entry = proc_create("profile", S_IWUSR | S_IRUGO,
>  			    NULL, &profile_proc_ops);
diff mbox series

Patch

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 1d4bc493b2f4..66bc712f590c 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -91,10 +91,23 @@  static ssize_t profiling_store(struct kobject *kobj,
 				   struct kobj_attribute *attr,
 				   const char *buf, size_t count)
 {
+	static DEFINE_MUTEX(lock);
 	int ret;
 
-	if (prof_on)
-		return -EEXIST;
+	/*
+	 * We need serialization, for profile_setup() initializes prof_on
+	 * value. Also, use killable wait in case memory allocation from
+	 * profile_init() triggered the OOM killer and chose current thread
+	 * blocked here.
+	 */
+	if (mutex_lock_killable(&lock))
+		return -EINTR;
+
+	if (prof_on) {
+		count = -EEXIST;
+		goto out;
+	}
+
 	/*
 	 * This eventually calls into get_option() which
 	 * has a ton of callers and is not const.  It is
@@ -102,11 +115,15 @@  static ssize_t profiling_store(struct kobject *kobj,
 	 */
 	profile_setup((char *)buf);
 	ret = profile_init();
-	if (ret)
-		return ret;
+	if (ret) {
+		count = ret;
+		goto out;
+	}
 	ret = create_proc_profile();
 	if (ret)
-		return ret;
+		count = ret;
+out:
+	mutex_unlock(&lock);
 	return count;
 }
 KERNEL_ATTR_RW(profiling);
diff --git a/kernel/profile.c b/kernel/profile.c
index 8a77769bc4b4..7575747e2ac6 100644
--- a/kernel/profile.c
+++ b/kernel/profile.c
@@ -114,11 +114,9 @@  int __ref profile_init(void)
 
 	buffer_bytes = prof_len*sizeof(atomic_t);
 
-	if (!alloc_cpumask_var(&prof_cpu_mask, GFP_KERNEL))
+	if (!zalloc_cpumask_var(&prof_cpu_mask, GFP_KERNEL))
 		return -ENOMEM;
 
-	cpumask_copy(prof_cpu_mask, cpu_possible_mask);
-
 	prof_buffer = kzalloc(buffer_bytes, GFP_KERNEL|__GFP_NOWARN);
 	if (prof_buffer)
 		return 0;
@@ -481,6 +479,8 @@  int __ref create_proc_profile(void)
 		goto err_state_prep;
 	online_state = err;
 	err = 0;
+#else
+	cpumask_copy(prof_cpu_mask, cpu_possible_mask);
 #endif
 	entry = proc_create("profile", S_IWUSR | S_IRUGO,
 			    NULL, &profile_proc_ops);