From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1953C10F14 for ; Thu, 3 Oct 2019 16:53:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6B93A2086A for ; Thu, 3 Oct 2019 16:53:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570121583; bh=NJRqgMFb42gdLNeJHVaCND2GhjGKkjOwcp7MoysuOtY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=djbsUTFj1yXVBDRClxbmu1+qcb2Ee/HPblw5RUZqDYxRDR26M9YF2uBLWqhWWkXu3 FNYGUf9mvUaDNVgUmDw+RCb3TNV5bAIeK72fP9trXvFMSO+M4ZWJ5Zs63I6JRsNj/W 0j7LIUwLxTbON0eJryTKiE5pxH2cfb/rO1sTe/9g= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2406279AbfJCQw7 (ORCPT ); Thu, 3 Oct 2019 12:52:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:40700 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2406256AbfJCQwx (ORCPT ); Thu, 3 Oct 2019 12:52:53 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9ADB420867; Thu, 3 Oct 2019 16:52:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570121572; bh=NJRqgMFb42gdLNeJHVaCND2GhjGKkjOwcp7MoysuOtY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=mPaeI+xxlp8whK4lQAQhJIgkBI1uoH9rkaMx3cvpbGWpqJqrp5kLPJfj88JS9yVmK GeeHobPr1MbRD86RKOTomHvEfDzWTXorplpXLrQ1jx/9M1BV2J33gJ1FeZ4QDByIMn d9cEG7as7xpgmep0W8PGt/jvXzmVb/VFs9kZj4cc= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Tony Camuso , Corey Minyard Subject: [PATCH 5.3 328/344] ipmi: move message error checking to avoid deadlock Date: Thu, 3 Oct 2019 17:54:53 +0200 Message-Id: <20191003154611.188543007@linuxfoundation.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191003154540.062170222@linuxfoundation.org> References: <20191003154540.062170222@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Tony Camuso commit 383035211c79d4d98481a09ad429b31c7dbf22bd upstream. V1->V2: in handle_one_rcv_msg, if data_size > 2, set requeue to zero and goto out instead of calling ipmi_free_msg. Kosuke Tatsukawa In the source stack trace below, function set_need_watch tries to take out the same si_lock that was taken earlier by ipmi_thread. ipmi_thread() [drivers/char/ipmi/ipmi_si_intf.c:995] smi_event_handler() [drivers/char/ipmi/ipmi_si_intf.c:765] handle_transaction_done() [drivers/char/ipmi/ipmi_si_intf.c:555] deliver_recv_msg() [drivers/char/ipmi/ipmi_si_intf.c:283] ipmi_smi_msg_received() [drivers/char/ipmi/ipmi_msghandler.c:4503] intf_err_seq() [drivers/char/ipmi/ipmi_msghandler.c:1149] smi_remove_watch() [drivers/char/ipmi/ipmi_msghandler.c:999] set_need_watch() [drivers/char/ipmi/ipmi_si_intf.c:1066] Upstream commit e1891cffd4c4896a899337a243273f0e23c028df adds code to ipmi_smi_msg_received() to call smi_remove_watch() via intf_err_seq() and this seems to be causing the deadlock. commit e1891cffd4c4896a899337a243273f0e23c028df Author: Corey Minyard Date: Wed Oct 24 15:17:04 2018 -0500 ipmi: Make the smi watcher be disabled immediately when not needed The fix is to put all messages in the queue and move the message checking code out of ipmi_smi_msg_received and into handle_one_recv_msg, which processes the message checking after ipmi_thread releases its locks. Additionally,Kosuke Tatsukawa reported that handle_new_recv_msgs calls ipmi_free_msg when handle_one_rcv_msg returns zero, so that the call to ipmi_free_msg in handle_one_rcv_msg introduced another panic when "ipmitool sensor list" was run in a loop. He submitted this part of the patch. +free_msg: + requeue = 0; + goto out; Reported by: Osamu Samukawa Characterized by: Kosuke Tatsukawa Signed-off-by: Tony Camuso Fixes: e1891cffd4c4 ("ipmi: Make the smi watcher be disabled immediately when not needed") Cc: stable@vger.kernel.org # 5.1 Signed-off-by: Corey Minyard Signed-off-by: Greg Kroah-Hartman --- drivers/char/ipmi/ipmi_msghandler.c | 114 ++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 57 deletions(-) --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -4215,7 +4215,53 @@ static int handle_one_recv_msg(struct ip int chan; ipmi_debug_msg("Recv:", msg->rsp, msg->rsp_size); - if (msg->rsp_size < 2) { + + if ((msg->data_size >= 2) + && (msg->data[0] == (IPMI_NETFN_APP_REQUEST << 2)) + && (msg->data[1] == IPMI_SEND_MSG_CMD) + && (msg->user_data == NULL)) { + + if (intf->in_shutdown) + goto free_msg; + + /* + * This is the local response to a command send, start + * the timer for these. The user_data will not be + * NULL if this is a response send, and we will let + * response sends just go through. + */ + + /* + * Check for errors, if we get certain errors (ones + * that mean basically we can try again later), we + * ignore them and start the timer. Otherwise we + * report the error immediately. + */ + if ((msg->rsp_size >= 3) && (msg->rsp[2] != 0) + && (msg->rsp[2] != IPMI_NODE_BUSY_ERR) + && (msg->rsp[2] != IPMI_LOST_ARBITRATION_ERR) + && (msg->rsp[2] != IPMI_BUS_ERR) + && (msg->rsp[2] != IPMI_NAK_ON_WRITE_ERR)) { + int ch = msg->rsp[3] & 0xf; + struct ipmi_channel *chans; + + /* Got an error sending the message, handle it. */ + + chans = READ_ONCE(intf->channel_list)->c; + if ((chans[ch].medium == IPMI_CHANNEL_MEDIUM_8023LAN) + || (chans[ch].medium == IPMI_CHANNEL_MEDIUM_ASYNC)) + ipmi_inc_stat(intf, sent_lan_command_errs); + else + ipmi_inc_stat(intf, sent_ipmb_command_errs); + intf_err_seq(intf, msg->msgid, msg->rsp[2]); + } else + /* The message was sent, start the timer. */ + intf_start_seq_timer(intf, msg->msgid); +free_msg: + requeue = 0; + goto out; + + } else if (msg->rsp_size < 2) { /* Message is too small to be correct. */ dev_warn(intf->si_dev, "BMC returned too small a message for netfn %x cmd %x, got %d bytes\n", @@ -4472,62 +4518,16 @@ void ipmi_smi_msg_received(struct ipmi_s unsigned long flags = 0; /* keep us warning-free. */ int run_to_completion = intf->run_to_completion; - if ((msg->data_size >= 2) - && (msg->data[0] == (IPMI_NETFN_APP_REQUEST << 2)) - && (msg->data[1] == IPMI_SEND_MSG_CMD) - && (msg->user_data == NULL)) { - - if (intf->in_shutdown) - goto free_msg; - - /* - * This is the local response to a command send, start - * the timer for these. The user_data will not be - * NULL if this is a response send, and we will let - * response sends just go through. - */ - - /* - * Check for errors, if we get certain errors (ones - * that mean basically we can try again later), we - * ignore them and start the timer. Otherwise we - * report the error immediately. - */ - if ((msg->rsp_size >= 3) && (msg->rsp[2] != 0) - && (msg->rsp[2] != IPMI_NODE_BUSY_ERR) - && (msg->rsp[2] != IPMI_LOST_ARBITRATION_ERR) - && (msg->rsp[2] != IPMI_BUS_ERR) - && (msg->rsp[2] != IPMI_NAK_ON_WRITE_ERR)) { - int ch = msg->rsp[3] & 0xf; - struct ipmi_channel *chans; - - /* Got an error sending the message, handle it. */ - - chans = READ_ONCE(intf->channel_list)->c; - if ((chans[ch].medium == IPMI_CHANNEL_MEDIUM_8023LAN) - || (chans[ch].medium == IPMI_CHANNEL_MEDIUM_ASYNC)) - ipmi_inc_stat(intf, sent_lan_command_errs); - else - ipmi_inc_stat(intf, sent_ipmb_command_errs); - intf_err_seq(intf, msg->msgid, msg->rsp[2]); - } else - /* The message was sent, start the timer. */ - intf_start_seq_timer(intf, msg->msgid); - -free_msg: - ipmi_free_smi_msg(msg); - } else { - /* - * To preserve message order, we keep a queue and deliver from - * a tasklet. - */ - if (!run_to_completion) - spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); - list_add_tail(&msg->link, &intf->waiting_rcv_msgs); - if (!run_to_completion) - spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock, - flags); - } + /* + * To preserve message order, we keep a queue and deliver from + * a tasklet. + */ + if (!run_to_completion) + spin_lock_irqsave(&intf->waiting_rcv_msgs_lock, flags); + list_add_tail(&msg->link, &intf->waiting_rcv_msgs); + if (!run_to_completion) + spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock, + flags); if (!run_to_completion) spin_lock_irqsave(&intf->xmit_msgs_lock, flags);