| Message ID | 20240311112330.372158-1-changbin.du@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: kmsan: fix instrumentation recursion on preempt_count | expand |
On Mon, Mar 11, 2024 at 07:23:30PM +0800, Changbin Du wrote: > This disables msan check for preempt_count_{add,sub} to fix a > instrumentation recursion issue on preempt_count: > > __msan_metadata_ptr_for_load_4() -> kmsan_virt_addr_valid() -> > preempt_disable() -> __msan_metadata_ptr_for_load_4() > > With this fix, I was able to run kmsan kernel with: > o CONFIG_DEBUG_KMEMLEAK=n > o CONFIG_KFENCE=n > o CONFIG_LOCKDEP=n > > KMEMLEAK and KFENCE generate too many false positives in unwinding code. > LOCKDEP still introduces instrumenting recursions issue. But these are > other issues expected to be fixed. > > Cc: Marco Elver <elver@google.com> > Signed-off-by: Changbin Du <changbin.du@huawei.com> > --- > kernel/sched/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9116bcc90346..5b63bb98e60a 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -5848,7 +5848,7 @@ static inline void preempt_latency_start(int val) > } > } > > -void preempt_count_add(int val) > +void __no_kmsan_checks preempt_count_add(int val) > { > #ifdef CONFIG_DEBUG_PREEMPT > /* > @@ -5880,7 +5880,7 @@ static inline void preempt_latency_stop(int val) > trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); > } What prevents a larger loop via one of the calles of preempt_count_{add,sub}() For example, via preempt_latency_{start,stop}() ? ... or via some *other* instrumentation that might be placed in those? I suspect we should be using noinstr or __always_inline in a bunch of places to clean this up properly. Mark.
On Mon, Mar 11, 2024 at 11:42:29AM +0000, Mark Rutland wrote: > On Mon, Mar 11, 2024 at 07:23:30PM +0800, Changbin Du wrote: > > This disables msan check for preempt_count_{add,sub} to fix a > > instrumentation recursion issue on preempt_count: > > > > __msan_metadata_ptr_for_load_4() -> kmsan_virt_addr_valid() -> > > preempt_disable() -> __msan_metadata_ptr_for_load_4() > > > > With this fix, I was able to run kmsan kernel with: > > o CONFIG_DEBUG_KMEMLEAK=n > > o CONFIG_KFENCE=n > > o CONFIG_LOCKDEP=n > > > > KMEMLEAK and KFENCE generate too many false positives in unwinding code. > > LOCKDEP still introduces instrumenting recursions issue. But these are > > other issues expected to be fixed. > > > > Cc: Marco Elver <elver@google.com> > > Signed-off-by: Changbin Du <changbin.du@huawei.com> > > --- > > kernel/sched/core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 9116bcc90346..5b63bb98e60a 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -5848,7 +5848,7 @@ static inline void preempt_latency_start(int val) > > } > > } > > > > -void preempt_count_add(int val) > > +void __no_kmsan_checks preempt_count_add(int val) > > { > > #ifdef CONFIG_DEBUG_PREEMPT > > /* > > @@ -5880,7 +5880,7 @@ static inline void preempt_latency_stop(int val) > > trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); > > } > > What prevents a larger loop via one of the calles of preempt_count_{add,sub}() > > For example, via preempt_latency_{start,stop}() ? > > ... or via some *other* instrumentation that might be placed in those? > > I suspect we should be using noinstr or __always_inline in a bunch of places to > clean this up properly. > In my local build, these two are not that small for inlining. (I has preempt_off tracer enabled). $ readelf -s vmlinux | grep -sw -E 'preempt_count_add|preempt_count_sub' 157043: ffffffff81174de0 186 FUNC GLOBAL DEFAULT 1 preempt_count_add 157045: ffffffff81174eb0 216 FUNC GLOBAL DEFAULT 1 preempt_count_sub The noinstr adds __no_sanitize_memory to the tagged functions so we might see many false positives. > Mark.
On Mon, Mar 11, 2024 at 11:42:29AM +0000, Mark Rutland wrote: > On Mon, Mar 11, 2024 at 07:23:30PM +0800, Changbin Du wrote: > > This disables msan check for preempt_count_{add,sub} to fix a > > instrumentation recursion issue on preempt_count: > > > > __msan_metadata_ptr_for_load_4() -> kmsan_virt_addr_valid() -> > > preempt_disable() -> __msan_metadata_ptr_for_load_4() > > > > With this fix, I was able to run kmsan kernel with: > > o CONFIG_DEBUG_KMEMLEAK=n > > o CONFIG_KFENCE=n > > o CONFIG_LOCKDEP=n > > > > KMEMLEAK and KFENCE generate too many false positives in unwinding code. > > LOCKDEP still introduces instrumenting recursions issue. But these are > > other issues expected to be fixed. > > > > Cc: Marco Elver <elver@google.com> > > Signed-off-by: Changbin Du <changbin.du@huawei.com> > > --- > > kernel/sched/core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 9116bcc90346..5b63bb98e60a 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -5848,7 +5848,7 @@ static inline void preempt_latency_start(int val) > > } > > } > > > > -void preempt_count_add(int val) > > +void __no_kmsan_checks preempt_count_add(int val) > > { > > #ifdef CONFIG_DEBUG_PREEMPT > > /* > > @@ -5880,7 +5880,7 @@ static inline void preempt_latency_stop(int val) > > trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); > > } > > What prevents a larger loop via one of the calles of preempt_count_{add,sub}() > > For example, via preempt_latency_{start,stop}() ? > > ... or via some *other* instrumentation that might be placed in those? > > I suspect we should be using noinstr or __always_inline in a bunch of places to > clean this up properly. > > Mark. Hi, I tried the patch with the ftrace testsuite, and this uncovered another loop, as predicted here: preempt_count_add():int3 function_trace_call() __msan_metadata_ptr_for_load_8() kmsan_get_shadow_origin_ptr() kmsan_get_metadata() virt_to_page_or_null() preempt_count_add() Best regards, Ilya
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 9116bcc90346..5b63bb98e60a 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -5848,7 +5848,7 @@ static inline void preempt_latency_start(int val) } } -void preempt_count_add(int val) +void __no_kmsan_checks preempt_count_add(int val) { #ifdef CONFIG_DEBUG_PREEMPT /* @@ -5880,7 +5880,7 @@ static inline void preempt_latency_stop(int val) trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); } -void preempt_count_sub(int val) +void __no_kmsan_checks preempt_count_sub(int val) { #ifdef CONFIG_DEBUG_PREEMPT /*
This disables msan check for preempt_count_{add,sub} to fix a instrumentation recursion issue on preempt_count: __msan_metadata_ptr_for_load_4() -> kmsan_virt_addr_valid() -> preempt_disable() -> __msan_metadata_ptr_for_load_4() With this fix, I was able to run kmsan kernel with: o CONFIG_DEBUG_KMEMLEAK=n o CONFIG_KFENCE=n o CONFIG_LOCKDEP=n KMEMLEAK and KFENCE generate too many false positives in unwinding code. LOCKDEP still introduces instrumenting recursions issue. But these are other issues expected to be fixed. Cc: Marco Elver <elver@google.com> Signed-off-by: Changbin Du <changbin.du@huawei.com> --- kernel/sched/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)