| 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
| ||
|
Message-ID: <3d5b2b55-381b-562a-9c02-52a8bdfc1a0d@acm.org>
Date: Mon, 25 Jun 2018 14:34:50 -0500
From: Corey Minyard <minyard@....org>
To: Haiyue Wang <haiyue.wang@...ux.intel.com>, arnd@...db.de,
gregkh@...uxfoundation.org,
openipmi-developer@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Cc: luis.a.silva@...l.com, avi.fishman@...oton.com,
openbmc@...ts.ozlabs.org
Subject: Re: [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not
open
On 06/23/2018 08:51 AM, Haiyue Wang wrote:
> When kcs_bmc_handle_event calls kcs_force_abort function to handle the
> not open (no user running) KCS channel transaction, the returned status
> value -ENODEV causes the low level IRQ handler indicating that the irq
> was not for him by returning IRQ_NONE. After some time, this IRQ will
> be treated to be spurious one, and the exception dump happens.
>
> irq 30: nobody cared (try booting with the "irqpoll" option)
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.10.15-npcm750 #1
> Hardware name: NPCMX50 Chip family
> [<c010b264>] (unwind_backtrace) from [<c0106930>] (show_stack+0x20/0x24)
> [<c0106930>] (show_stack) from [<c03dad38>] (dump_stack+0x8c/0xa0)
> [<c03dad38>] (dump_stack) from [<c0168810>] (__report_bad_irq+0x3c/0xdc)
> [<c0168810>] (__report_bad_irq) from [<c0168c34>] (note_interrupt+0x29c/0x2ec)
> [<c0168c34>] (note_interrupt) from [<c0165c80>] (handle_irq_event_percpu+0x5c/0x68)
> [<c0165c80>] (handle_irq_event_percpu) from [<c0165cd4>] (handle_irq_event+0x48/0x6c)
> [<c0165cd4>] (handle_irq_event) from [<c0169664>] (handle_fasteoi_irq+0xc8/0x198)
> [<c0169664>] (handle_fasteoi_irq) from [<c016529c>] (__handle_domain_irq+0x90/0xe8)
> [<c016529c>] (__handle_domain_irq) from [<c01014bc>] (gic_handle_irq+0x58/0x9c)
> [<c01014bc>] (gic_handle_irq) from [<c010752c>] (__irq_svc+0x6c/0x90)
> Exception stack(0xc0a01de8 to 0xc0a01e30)
> 1de0: 00002080 c0a6fbc0 00000000 00000000 00000000 c096d294
> 1e00: 00000000 00000001 dc406400 f03ff100 00000082 c0a01e94 c0a6fbc0 c0a01e38
> 1e20: 00200102 c01015bc 60000113 ffffffff
> [<c010752c>] (__irq_svc) from [<c01015bc>] (__do_softirq+0xbc/0x358)
> [<c01015bc>] (__do_softirq) from [<c011c798>] (irq_exit+0xb8/0xec)
> [<c011c798>] (irq_exit) from [<c01652a0>] (__handle_domain_irq+0x94/0xe8)
> [<c01652a0>] (__handle_domain_irq) from [<c01014bc>] (gic_handle_irq+0x58/0x9c)
> [<c01014bc>] (gic_handle_irq) from [<c010752c>] (__irq_svc+0x6c/0x90)
> Exception stack(0xc0a01ef8 to 0xc0a01f40)
> 1ee0: 00000000 000003ae
> 1f00: dcc0f338 c0111060 c0a00000 c0a0cc44 c0a0cbe4 c0a1c22b c07bc218 00000001
> 1f20: dcffca40 c0a01f54 c0a01f58 c0a01f48 c0103524 c0103528 60000013 ffffffff
> [<c010752c>] (__irq_svc) from [<c0103528>] (arch_cpu_idle+0x48/0x4c)
> [<c0103528>] (arch_cpu_idle) from [<c0681390>] (default_idle_call+0x30/0x3c)
> [<c0681390>] (default_idle_call) from [<c0156f24>] (do_idle+0xc8/0x134)
> [<c0156f24>] (do_idle) from [<c015722c>] (cpu_startup_entry+0x28/0x2c)
> [<c015722c>] (cpu_startup_entry) from [<c067ad74>] (rest_init+0x84/0x88)
> [<c067ad74>] (rest_init) from [<c0900d44>] (start_kernel+0x388/0x394)
> [<c0900d44>] (start_kernel) from [<0000807c>] (0x807c)
> handlers:
> [<c041c5dc>] npcm7xx_kcs_irq
> Disabling IRQ #30
>
> It needs to change the returned status from -ENODEV to 0. The -ENODEV
> was originally used to tell the low level IRQ handler that no user was
> running, but not consider the IRQ handling desgin.
>
> And multiple KCS channels share one IRQ handler, it needs to check the
> IBF flag before doing force abort. If the IBF is set, after handling,
> return 0 to low level IRQ handler to indicate that the IRQ is handled.
I've looked at this some more, and I think it's ok, especially with the
better description.
I'm going to submit this for the current release after it sits in next
for a bit.
Thanks,
-corey
> Signed-off-by: Haiyue Wang <haiyue.wang@...ux.intel.com>
> ---
> v1 -> v2:
> - Change the commit message to be more understandable.
> ---
> drivers/char/ipmi/kcs_bmc.c | 31 ++++++++++---------------------
> 1 file changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index fbfc05e..bb882ab1 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -210,34 +210,23 @@ static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> {
> unsigned long flags;
> - int ret = 0;
> + int ret = -ENODATA;
> u8 status;
>
> spin_lock_irqsave(&kcs_bmc->lock, flags);
>
> - if (!kcs_bmc->running) {
> - kcs_force_abort(kcs_bmc);
> - ret = -ENODEV;
> - goto out_unlock;
> - }
> -
> - status = read_status(kcs_bmc) & (KCS_STATUS_IBF | KCS_STATUS_CMD_DAT);
> -
> - switch (status) {
> - case KCS_STATUS_IBF | KCS_STATUS_CMD_DAT:
> - kcs_bmc_handle_cmd(kcs_bmc);
> - break;
> -
> - case KCS_STATUS_IBF:
> - kcs_bmc_handle_data(kcs_bmc);
> - break;
> + status = read_status(kcs_bmc);
> + if (status & KCS_STATUS_IBF) {
> + if (!kcs_bmc->running)
> + kcs_force_abort(kcs_bmc);
> + else if (status & KCS_STATUS_CMD_DAT)
> + kcs_bmc_handle_cmd(kcs_bmc);
> + else
> + kcs_bmc_handle_data(kcs_bmc);
>
> - default:
> - ret = -ENODATA;
> - break;
> + ret = 0;
> }
>
> -out_unlock:
> spin_unlock_irqrestore(&kcs_bmc->lock, flags);
>
> return ret;
Powered by blists - more mailing lists