linux-kernel - Re: [PATCH] x86/resctrl: fix an imbalance in domain_remove

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: <CADjb_WRmyE3rRN2sLAh9ZOqAg0E3WeCkj9SwSM9dorvx4TGC2A@mail.gmail.com>
Date:   Tue, 10 Dec 2019 15:55:57 +0800
From:   Ryan Chen <yu.chen.surf@...il.com>
To:     Qian Cai <cai@....pw>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        John Stultz <john.stultz@...aro.org>, sboyd@...nel.org,
        Tony Luck <tony.luck@...el.com>, tj@...nel.org,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/resctrl: fix an imbalance in domain_remove_cpu

Hi Qian,

On Sun, Dec 8, 2019 at 12:14 PM Qian Cai <cai@....pw> wrote:
>
> domain_add_cpu() calls domain_setup_mon_state() only when r->mon_capable
> is true where it will initialize d->mbm_over. However,
> domain_remove_cpu() calls cancel_delayed_work(&d->mbm_over) without
> checking r->mon_capable. Hence, it triggers a debugobjects warning when
> offlining CPUs because those timer debugobjects are never initialized.
>
Could you elaborate a little more on the failure symptom?
If I understand correctly, the error you described was  due to
r->mon_capable set to false while is_mbm_enabled() returns true?
Which means on this platform rdt_mon_features is non zero?
And in get_rdt_mon_resources() it will invoke rdt_get_mon_l3_config(),
however the only possible failure to do not set r->mon_capable is that it
 failed in dom_data_init() due to kcalloc() failure?  Then the logic in
get_rdt_resources() is that it will ignore the return error if rdt allocate
feature is supported on this platform?  If this is the case, the r->mon_capable
is not an indicator for whether the overflow thread has been created, right?
Can we simply remove the check of r->mon_capable in domain_add_cpu() and
invoke  domain_setup_mon_state() directly?
> ODEBUG: assert_init not available (active state 0) object type:
> timer_list hint: 0x0
> WARNING: CPU: 143 PID: 789 at lib/debugobjects.c:484
> debug_print_object+0xfe/0x140
> Hardware name: HP Synergy 680 Gen9/Synergy 680 Gen9 Compute Module, BIOS
> I40 05/23/2018
> RIP: 0010:debug_print_object+0xfe/0x140
> Call Trace:
> debug_object_assert_init+0x1f5/0x240
> del_timer+0x6f/0xf0
> try_to_grab_pending+0x42/0x3c0
> cancel_delayed_work+0x7d/0x150
> resctrl_offline_cpu+0x3c0/0x520
> cpuhp_invoke_callback+0x197/0x1120
> cpuhp_thread_fun+0x252/0x2f0
> smpboot_thread_fn+0x255/0x440
> kthread+0x1e6/0x210
> ret_from_fork+0x3a/0x50
>
> Fixes: e33026831bdb ("x86/intel_rdt/mbm: Handle counter overflow")
> Signed-off-by: Qian Cai <cai@....pw>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 03eb90d00af0..89049b343c7a 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -618,7 +618,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>                 if (static_branch_unlikely(&rdt_mon_enable_key))
>                         rmdir_mondata_subdir_allrdtgrp(r, d->id);
>                 list_del(&d->list);
> -               if (is_mbm_enabled())
> +               if (r->mon_capable && is_mbm_enabled())
>                         cancel_delayed_work(&d->mbm_over);
Humm, it looks like there are two places within this function
invoked cancel_delayed_work(&d->mbm_over),
why not adding the check for both of them?

thanks,
Y