From: Leon Romanovsky <[email protected]>
Hi Linus,
Both Colin in Ubuntu [1] and I in FC 32 are having same kernel crashes
while GCOV is enabled. The reason to it that n_fuction variable that
should be provided by GCC is not initialized (or wrongly set).
This patch is based on the RFC [2] which I sent to gather feedback, but
didn't get any response, so sending it to you in proper -rc format.
Bottom line, GCOV is broken on GCC 10.2.
Thanks
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1891288
[2] https://lore.kernel.org/lkml/[email protected]
Leon Romanovsky (4):
gcov: Open-code kmemdup() to work correctly with kernel and user space
pointers
gcov: Use proper duplication routine for const pointer
gcov: Protect from uninitialized number of functions provided by GCC
10.2
gcov: Don't print out-of-memory print for all failed files
kernel/gcov/fs.c | 5 +++--
kernel/gcov/gcc_4_7.c | 17 +++++++++--------
2 files changed, 12 insertions(+), 10 deletions(-)
--
2.26.2
From: Leon Romanovsky <[email protected]>
The kernel with KASAN and GCOV enabled generates the following splat
due to the situation that gcov_info can be both user and kernel pointer.
It is triggered by the memcpy() inside kmemdup(), so as a possible solution
let's copy fields manually.
==================================================================
BUG: KASAN: global-out-of-bounds in kmemdup+0x43/0x70
Read of size 120 at addr ffffffffa0d2c780 by task modprobe/296
CPU: 0 PID: 296 Comm: modprobe Not tainted 5.9.0-rc1+ #1860
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04 /01/2014
Call Trace:
dump_stack+0x128/0x1af
print_address_description.constprop.0+0x2c/0x3f0
_raw_spin_lock_irqsave+0x34/0xa0
__kasan_check_read+0x1d/0x30
kmemdup+0x43/0x70
kmemdup+0x43/0x70
gcov_info_dup+0x2d/0x730
__kasan_check_write+0x20/0x30
__mutex_unlock_slowpath+0x10d/0x740
gcov_event+0x88d/0xd30
gcov_module_notifier+0xe9/0x100
notifier_call_chain+0xeb/0x170
blocking_notifier_call_chain+0x75/0xc0
__x64_sys_delete_module+0x326/0x5a0
do_init_module+0x810/0x810
syscall_enter_from_user_mode+0x40/0x420
trace_hardirqs_on+0x45/0xb0
syscall_enter_from_user_mode+0x40/0x420
do_syscall_64+0x45/0x70
entry_SYSCALL_64_after_hwframe+0x44/0xa9
The buggy address belongs to the variable:
__gcov_.uverbs_attr_get_obj+0x60/0xfffffffffff778e0 [mlx5_ib]
Memory state around the buggy address:
ffffffffa0d2c680: 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 f9 f9 f9
ffffffffa0d2c700: f9 f9 f9 f9 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
>ffffffffa0d2c780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9
^
ffffffffa0d2c800: f9 f9 f9 f9 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
ffffffffa0d2c880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
Disabling lock debugging due to kernel taint
---[ end trace 065ea9cc2ba144a6 ]---
Cc: Colin Ian King <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
kernel/gcov/gcc_4_7.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
index 908fdf5098c3..6d706c5eed5c 100644
--- a/kernel/gcov/gcc_4_7.c
+++ b/kernel/gcov/gcc_4_7.c
@@ -275,13 +275,13 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info)
size_t fi_size; /* function info size */
size_t cv_size; /* counter values size */
- dup = kmemdup(info, sizeof(*dup), GFP_KERNEL);
+ dup = kzalloc(sizeof(*dup), GFP_KERNEL);
if (!dup)
return NULL;
- dup->next = NULL;
- dup->filename = NULL;
- dup->functions = NULL;
+ for (fi_idx = 0; fi_idx < GCOV_COUNTERS; fi_idx++)
+ dup->merge[fi_idx] = info->merge[fi_idx];
+ dup->n_functions = info->n_functions;
dup->filename = kstrdup(info->filename, GFP_KERNEL);
if (!dup->filename)
--
2.26.2
From: Leon Romanovsky <[email protected]>
Once GCOV fails to duplicate information, the following error is
printed:
gcov: could not save data for '/home/leonro/src/kernel/drivers/infiniband/hw/mlx5/std_types.gcda' (out of memory)
In the event of out-of-memory such prints are seen for almost every kernel
file, so instead of spamming dmesg, we print the first failure and inform
the user that future prints are suppressed.
Cc: Colin Ian King <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
kernel/gcov/fs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/gcov/fs.c b/kernel/gcov/fs.c
index 82babf5aa077..b74d426ca99e 100644
--- a/kernel/gcov/fs.c
+++ b/kernel/gcov/fs.c
@@ -685,8 +685,9 @@ static void save_info(struct gcov_node *node, struct gcov_info *info)
else {
node->unloaded_info = gcov_info_dup(info);
if (!node->unloaded_info) {
- pr_warn("could not save data for '%s' "
- "(out of memory)\n",
+ pr_warn_once(
+ "could not save data for first file '%s' "
+ "(out of memory), other files are suppressed\n",
gcov_info_filename(info));
}
}
--
2.26.2
From: Leon Romanovsky <[email protected]>
The filename is a const pointer, so use the proper string duplication
routine that takes into account const identifier.
Cc: Colin Ian King <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
kernel/gcov/gcc_4_7.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
index 6d706c5eed5c..318211deb903 100644
--- a/kernel/gcov/gcc_4_7.c
+++ b/kernel/gcov/gcc_4_7.c
@@ -283,7 +283,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info)
dup->merge[fi_idx] = info->merge[fi_idx];
dup->n_functions = info->n_functions;
- dup->filename = kstrdup(info->filename, GFP_KERNEL);
+ dup->filename = kstrdup_const(info->filename, GFP_KERNEL);
if (!dup->filename)
goto err_free;
@@ -359,7 +359,7 @@ void gcov_info_free(struct gcov_info *info)
free_info:
kfree(info->functions);
- kfree(info->filename);
+ kfree_const(info->filename);
kfree(info);
}
--
2.26.2
From: Leon Romanovsky <[email protected]>
The kernel compiled with GCC 10.2.1 and KASAN together with GCOV enabled
produces the following splat while reloading modules. The very similar
trace was reported by Colin [1].
------------[ cut here ]------------
WARNING: CPU: 0 PID: 296 at mm/page_alloc.c:4859 __alloc_pages_nodemask+0x670/0x3190
Modules linked in: mlx5_ib(-) mlx5_core mlxfw ptp ib_ipoib pps_core rdma_ucm rdma_cm iw_cm ib_cm ib_umad ib_uverbs ib_core
CPU: 0 PID: 296 Comm: modprobe Tainted: G B 5.9.0-rc1+ #1860
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04 /01/2014
RIP: 0010:__alloc_pages_nodemask+0x670/0x3190
Code: e9 af fc ff ff 48 83 05 fd 28 90 05 01 81 e7 00 20 00 00 48 c7 44 24 28 00 00 00 00 0f 85 fb fd ff ff 48 83 05 f0 28 90 05 01 <0f> 0b 48 83 05 ee 28 90 05 01 48 83 05 ee 28 90 05 01 e9 dc fd ff
RSP: 0018:ffff88805f7ffa28 EFLAGS: 00010202
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffff1100befff5e
RDX: 0000000000000000 RSI: 0000000000000017 RDI: 0000000000000000
RBP: 000000050695a900 R08: ffff888060fc7900 R09: ffff888060fc793b
R10: ffffed100c1f8f27 R11: ffffed100c1f8f28 R12: 0000000000040dc0
R13: 000000050695a900 R14: 0000000000000017 R15: 0000000000000001
FS: 00007f521f695740(0000) GS:ffff88806ce00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f31b013f000 CR3: 000000006637e001 CR4: 0000000000370eb0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
? __kmalloc_track_caller+0x17a/0x570
? gcov_info_dup+0xfe/0x730
? gcov_event+0x88d/0xd30
? gcov_module_notifier+0xe9/0x100
? blocking_notifier_call_chain+0x75/0xc0
? __x64_sys_delete_module+0x326/0x5a0
? entry_SYSCALL_64_after_hwframe+0x44/0xa9
? mark_lock+0xba0/0xba0
? mark_lock+0xba0/0xba0
? notifier_call_chain+0xeb/0x170
? blocking_notifier_call_chain+0x75/0xc0
? __x64_sys_delete_module+0x326/0x5a0
? do_syscall_64+0x45/0x70
? entry_SYSCALL_64_after_hwframe+0x44/0xa9
? warn_alloc+0x130/0x130
? lock_acquire+0x1f2/0xa30
? fs_reclaim_acquire+0x1f/0x70
? fs_reclaim_release+0x1f/0x50
? __kasan_check_read+0x1d/0x30
? reacquire_held_locks+0x420/0x420
? reacquire_held_locks+0x420/0x420
kmalloc_order+0x3f/0xc0
kmalloc_order_trace+0x24/0x220
__kmalloc+0x41b/0x5a0
? gcov_info_dup+0xfe/0x730
? memcpy+0x73/0xa0
gcov_info_dup+0x176/0x730
gcov_event+0x88d/0xd30
gcov_module_notifier+0xe9/0x100
notifier_call_chain+0xeb/0x170
blocking_notifier_call_chain+0x75/0xc0
__x64_sys_delete_module+0x326/0x5a0
? do_init_module+0x810/0x810
? syscall_enter_from_user_mode+0x40/0x420
? trace_hardirqs_on+0x45/0xb0
? syscall_enter_from_user_mode+0x40/0x420
do_syscall_64+0x45/0x70
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f521f7c531b
Code: 73 01 c3 48 8b 0d 7d 0b 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4d 0b 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffe1bd4af48 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 0000561a3eae0910 RCX: 00007f521f7c531b
RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000561a3eae0978
RBP: 0000561a3eae0910 R08: 1999999999999999 R09: 0000000000000000
R10: 00007f521f839ac0 R11: 0000000000000206 R12: 0000000000000000
R13: 0000561a3eae0978 R14: 0000000000000000 R15: 0000561a3eae84d0
irq event stamp: 326464
hardirqs last enabled at (326463): [<ffffffff832ecdde>] _raw_spin_unlock_irqrestore+0x8e/0xb0
hardirqs last disabled at (326464): [<ffffffff832ec994>] _raw_spin_lock_irqsave+0x34/0xa0
hardirqs last disabled at (326464): [<ffffffff832ec994>] _raw_spin_lock_irqsave+0x34/0xa0
softirqs last enabled at (320794): [<ffffffff83600931>] __do_softirq+0x931/0xbc4
softirqs last disabled at (320789): [<ffffffff83400f2f>] asm_call_on_stack+0xf/0x20
---[ end trace 065ea9cc2ba144a6 ]---
This trace is seen because n_function value provided by GCC through
__gcov_init() is ridiculously high, in my case it was 2698213824,
which probably means that the field is not initialized.
In order to overcome this, fail allocation silently and rely on GCOV print later:
gcov: could not save data for '/home/leonro/src/kernel/drivers/infiniband/hw/mlx5/std_types.gcda' (out of memory)
[1] https://bugzilla.kernel.org/show_bug.cgi?id=208885#c1
Cc: Colin Ian King <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
kernel/gcov/gcc_4_7.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
index 318211deb903..f2856896d12f 100644
--- a/kernel/gcov/gcc_4_7.c
+++ b/kernel/gcov/gcc_4_7.c
@@ -287,8 +287,9 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info)
if (!dup->filename)
goto err_free;
- dup->functions = kcalloc(info->n_functions,
- sizeof(struct gcov_fn_info *), GFP_KERNEL);
+ dup->functions =
+ kcalloc(info->n_functions, sizeof(struct gcov_fn_info *),
+ GFP_KERNEL | __GFP_NOWARN);
if (!dup->functions)
goto err_free;
--
2.26.2
On Wed, Sep 02, 2020 at 10:38:20AM -0700, Linus Torvalds wrote:
> On Wed, Sep 2, 2020 at 1:55 AM Leon Romanovsky <[email protected]> wrote:
> >
> > The kernel with KASAN and GCOV enabled generates the following splat
> > due to the situation that gcov_info can be both user and kernel pointer.
>
> I can't parse the above explanation..
>
> > It is triggered by the memcpy() inside kmemdup(), so as a possible solution
> > let's copy fields manually.
>
> .. and I don't see why copying the fields manually makes a difference.
>
> Can you explain more?
Definitely my explanation is wrong, but it was my interpretation of
"BUG: KASAN: global-out-of-bounds in kmemdup+0x43/0x70" line. I saw
that the failure was in memcpy() inside of kmemdup(), so I changed
from memcpy to be copy_from_user() and it solved the KASAN warning.
This is why I wrote "both user and kernel pointer".
Thanks
>
> Linus
On Wed, Sep 2, 2020 at 10:52 AM Leon Romanovsky <[email protected]> wrote:
>
> Are you suggesting something like this?
>
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 3110c77230c7..bc0e355f64aa 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -3,7 +3,7 @@ menu "GCOV-based kernel profiling"
>
> config GCOV_KERNEL
> bool "Enable gcov-based kernel profiling"
> - depends on DEBUG_FS
> + depends on DEBUG_FS && (GCC_VERSION >= XXX && GCC_VERSION < YYY)
> select CONSTRUCTORS if !UML
> default n
> help
Yes. Except please don't mix it up with DEBUG_FS. Just add a new
"depends on" line. They are separate issues.
Linus
On Wed, Sep 2, 2020 at 10:46 AM Leon Romanovsky <[email protected]> wrote:
>
> Definitely my explanation is wrong, but it was my interpretation of
> "BUG: KASAN: global-out-of-bounds in kmemdup+0x43/0x70" line. I saw
> that the failure was in memcpy() inside of kmemdup(), so I changed
> from memcpy to be copy_from_user() and it solved the KASAN warning.
But the actual patch attached to that explanation *doesn't* use
copy_from_user().
So your "changed from memcpy to be copy_from_user() solved the KASAN
warning" explanation makes even less sense. Because that's not at all
what the patch does.
Linus
On Wed, Sep 02, 2020 at 11:24:50AM -0700, Linus Torvalds wrote:
> On Wed, Sep 2, 2020 at 10:52 AM Leon Romanovsky <[email protected]> wrote:
> >
> > Are you suggesting something like this?
> >
> > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > index 3110c77230c7..bc0e355f64aa 100644
> > --- a/kernel/gcov/Kconfig
> > +++ b/kernel/gcov/Kconfig
> > @@ -3,7 +3,7 @@ menu "GCOV-based kernel profiling"
> >
> > config GCOV_KERNEL
> > bool "Enable gcov-based kernel profiling"
> > - depends on DEBUG_FS
> > + depends on DEBUG_FS && (GCC_VERSION >= XXX && GCC_VERSION < YYY)
> > select CONSTRUCTORS if !UML
> > default n
> > help
>
> Yes. Except please don't mix it up with DEBUG_FS. Just add a new
> "depends on" line. They are separate issues.
Thanks, I'll do and resend.
>
> Linus
On Wed, Sep 02, 2020 at 11:27:14AM -0700, Linus Torvalds wrote:
> On Wed, Sep 2, 2020 at 10:46 AM Leon Romanovsky <[email protected]> wrote:
> >
> > Definitely my explanation is wrong, but it was my interpretation of
> > "BUG: KASAN: global-out-of-bounds in kmemdup+0x43/0x70" line. I saw
> > that the failure was in memcpy() inside of kmemdup(), so I changed
> > from memcpy to be copy_from_user() and it solved the KASAN warning.
>
> But the actual patch attached to that explanation *doesn't* use
> copy_from_user().
>
> So your "changed from memcpy to be copy_from_user() solved the KASAN
> warning" explanation makes even less sense. Because that's not at all
> what the patch does.
I already don't remember why, but copy_from_user() caused to second flow
of gcov_info_dup() through gcov_event() to generate another set of warnings.
As a summary, I have a workaround, but I don't know why it works and not
proud about it.
Thanks
>
> Linus
On Wed, Sep 2, 2020 at 11:44 AM Leon Romanovsky <[email protected]> wrote:
>
> I already don't remember why, but copy_from_user() caused to second flow
> of gcov_info_dup() through gcov_event() to generate another set of warnings.
>
> As a summary, I have a workaround, but I don't know why it works and not
> proud about it.
Ok, it does sound like "let's just disable gcov for the affected
compilers" is the way to go.
If/when somebody figures out exactly what's up and how to work around
it properly, maybe we can then revisit the issue.
Thanks,
Linus
On Wed, Sep 2, 2020 at 1:55 AM Leon Romanovsky <[email protected]> wrote:
>
> The kernel with KASAN and GCOV enabled generates the following splat
> due to the situation that gcov_info can be both user and kernel pointer.
I can't parse the above explanation..
> It is triggered by the memcpy() inside kmemdup(), so as a possible solution
> let's copy fields manually.
.. and I don't see why copying the fields manually makes a difference.
Can you explain more?
Linus
On Wed, Sep 2, 2020 at 1:55 AM Leon Romanovsky <[email protected]> wrote:
>
> This trace is seen because n_function value provided by GCC through
> __gcov_init() is ridiculously high, in my case it was 2698213824,
> which probably means that the field is not initialized.
This seems to be wrong - since a different (smaller) uninitialized
value will succeed in the kcalloc, but will then walk some random
array size when copying and fail later instead.
So this doesn't actually seem to _fix_ anything, it just hides one
special case of bogus values.
Linus
On Wed, Sep 2, 2020 at 1:55 AM Leon Romanovsky <[email protected]> wrote:
>
> Bottom line, GCOV is broken on GCC 10.2.
The patches don't really make sense to me.
How about we just disable GCOV with the known-broken compiler version
instead? As mentioned in the replies to individual patches, it looks
like the "fixes" are random bandaids that don't _really_ fix anything.
Linus
On Wed, Sep 02, 2020 at 10:42:55AM -0700, Linus Torvalds wrote:
> On Wed, Sep 2, 2020 at 1:55 AM Leon Romanovsky <[email protected]> wrote:
> >
> > Bottom line, GCOV is broken on GCC 10.2.
>
> The patches don't really make sense to me.
>
> How about we just disable GCOV with the known-broken compiler version
> instead? As mentioned in the replies to individual patches, it looks
> like the "fixes" are random bandaids that don't _really_ fix anything.
Right, as I wrote in RFC "solution is wrong", I knew it, just didn't
get any feedback on how to do it correctly.
Are you suggesting something like this?
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 3110c77230c7..bc0e355f64aa 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -3,7 +3,7 @@ menu "GCOV-based kernel profiling"
config GCOV_KERNEL
bool "Enable gcov-based kernel profiling"
- depends on DEBUG_FS
+ depends on DEBUG_FS && (GCC_VERSION >= XXX && GCC_VERSION < YYY)
select CONSTRUCTORS if !UML
default n
help
~
Thanks
>
> Linus
On 02/09/2020 10.55, Leon Romanovsky wrote:
> From: Leon Romanovsky <[email protected]>
>
> The filename is a const pointer, so use the proper string duplication
> routine that takes into account const identifier.
This commit log makes no sense at all.
kstrdup_const is merely an optimization that can be used when there's a
good chance that the passed string lives in vmlinux' .rodata, in which
case it is known to be immortal, and we can avoid allocating heap memory
to contain a duplicate. [It also requires that the caller has no
intention of modifying the returned string.]
In the case of something called ->filename, I assume it's initialized
with __FILE__ somewhere, making the above true for built-in stuff but
not for modules. So if the gcov_info can live longer than the module,
it's of course necessary to duplicate the string, but OTOH making an
optimization for the built-in stuff makes sense. So this is certainly
one of the places where kstrdup_const() seems applicable. But it has
nothing whatsoever to do with the C-level qualifiers the argument may have.
Rasmus
On Thu, Sep 03, 2020 at 10:56:38AM +0200, Rasmus Villemoes wrote:
> On 02/09/2020 10.55, Leon Romanovsky wrote:
> > From: Leon Romanovsky <[email protected]>
> >
> > The filename is a const pointer, so use the proper string duplication
> > routine that takes into account const identifier.
>
> This commit log makes no sense at all.
>
> kstrdup_const is merely an optimization that can be used when there's a
> good chance that the passed string lives in vmlinux' .rodata, in which
> case it is known to be immortal, and we can avoid allocating heap memory
> to contain a duplicate. [It also requires that the caller has no
> intention of modifying the returned string.]
>
> In the case of something called ->filename, I assume it's initialized
> with __FILE__ somewhere, making the above true for built-in stuff but
> not for modules. So if the gcov_info can live longer than the module,
> it's of course necessary to duplicate the string, but OTOH making an
> optimization for the built-in stuff makes sense. So this is certainly
> one of the places where kstrdup_const() seems applicable. But it has
> nothing whatsoever to do with the C-level qualifiers the argument may have.
Thanks, GCOV can't be built as module.
>
> Rasmus