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>
---
Can somebody test this patch? I don't know how not using
cpu_possible_mask affects expected profile data collection
(especially when written to /sys/kernel/profiling interface
when some of CPUs are offline).
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 495b69a71a5d..0540483ae7f0 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 2b775cc5c28f..1a6c1cf98485 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;
@@ -438,6 +436,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);
--
2.18.4
("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>
---
Can somebody test this patch? I don't know how not using
cpu_possible_mask affects expected profile data collection
(especially when written to /sys/kernel/profiling interface
when some of CPUs are offline).
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 495b69a71a5d..0540483ae7f0 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 2b775cc5c28f..1a6c1cf98485 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;
@@ -438,6 +436,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);
--
2.18.4