From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753434AbdHGMYi (ORCPT ); Mon, 7 Aug 2017 08:24:38 -0400 Received: from mail-pg0-f66.google.com ([74.125.83.66]:34667 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753203AbdHGMYg (ORCPT ); Mon, 7 Aug 2017 08:24:36 -0400 Reply-To: minyard@acm.org Subject: Re: [PATCH] ipmi: fix unsigned long underflow To: Weilong Chen Cc: openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, Corey Minyard References: <1501296161-74544-1-git-send-email-chenweilong@huawei.com> <1501381228-30958-1-git-send-email-minyard@acm.org> From: Corey Minyard Message-ID: <145b74c4-24a6-5eb0-d193-80f74dd7e81e@acm.org> Date: Mon, 7 Aug 2017 07:24:32 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/07/2017 03:41 AM, Weilong Chen wrote: > Hi Minyard, > > I test this patch, it works. > > Thanks. > Thanks, I added a Tested-by: for you and it's queued for the next release. If it's urgent I can send it in now. I also added this for the stable kernels 3.16 and later. -corey > On 2017/7/30 10:20, minyard@acm.org wrote: >> From: Corey Minyard >> >> When I set the timeout to a specific value such as 500ms, the timeout >> event will not happen in time due to the overflow in function >> check_msg_timeout: >> ... >> ent->timeout -= timeout_period; >> if (ent->timeout > 0) >> return; >> ... >> >> The type of timeout_period is long, but ent->timeout is unsigned long. >> This patch makes the type consistent. >> >> Reported-by: Weilong Chen >> Signed-off-by: Corey Minyard >> --- >> I like to keep things consistent (though I obviously messed up here) >> and keep variables that should be positive unsigned. >> >> But you are right, there is a bug here and some inconsistency. >> This patch changes timeout_period to be unsigned and fixes the >> check. Can you try this out? >> >> drivers/char/ipmi/ipmi_msghandler.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/char/ipmi/ipmi_msghandler.c >> b/drivers/char/ipmi/ipmi_msghandler.c >> index 810b138..c82d9fd 100644 >> --- a/drivers/char/ipmi/ipmi_msghandler.c >> +++ b/drivers/char/ipmi/ipmi_msghandler.c >> @@ -4030,7 +4030,8 @@ smi_from_recv_msg(ipmi_smi_t intf, struct >> ipmi_recv_msg *recv_msg, >> } >> >> static void check_msg_timeout(ipmi_smi_t intf, struct seq_table *ent, >> - struct list_head *timeouts, long timeout_period, >> + struct list_head *timeouts, >> + unsigned long timeout_period, >> int slot, unsigned long *flags, >> unsigned int *waiting_msgs) >> { >> @@ -4043,8 +4044,8 @@ static void check_msg_timeout(ipmi_smi_t intf, >> struct seq_table *ent, >> if (!ent->inuse) >> return; >> >> - ent->timeout -= timeout_period; >> - if (ent->timeout > 0) { >> + if (timeout_period < ent->timeout) { >> + ent->timeout -= timeout_period; >> (*waiting_msgs)++; >> return; >> } >> @@ -4110,7 +4111,8 @@ static void check_msg_timeout(ipmi_smi_t intf, >> struct seq_table *ent, >> } >> } >> >> -static unsigned int ipmi_timeout_handler(ipmi_smi_t intf, long >> timeout_period) >> +static unsigned int ipmi_timeout_handler(ipmi_smi_t intf, >> + unsigned long timeout_period) >> { >> struct list_head timeouts; >> struct ipmi_recv_msg *msg, *msg2; >> >