* [PATCH] ipmi: fix unsigned long overflow @ 2017-07-29 2:42 Weilong Chen 2017-07-30 2:20 ` [PATCH] ipmi: fix unsigned long underflow minyard 0 siblings, 1 reply; 4+ messages in thread From: Weilong Chen @ 2017-07-29 2:42 UTC (permalink / raw) To: minyard; +Cc: openipmi-developer, linux-kernel 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. Signed-off-by: Weilong Chen <chenweilong@huawei.com> --- drivers/char/ipmi/ipmi_msghandler.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 810b138..4c806e9 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -135,8 +135,8 @@ struct seq_table { unsigned int inuse : 1; unsigned int broadcast : 1; - unsigned long timeout; - unsigned long orig_timeout; + long timeout; + long orig_timeout; unsigned int retries_left; /* -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] ipmi: fix unsigned long underflow 2017-07-29 2:42 [PATCH] ipmi: fix unsigned long overflow Weilong Chen @ 2017-07-30 2:20 ` minyard 2017-08-07 8:41 ` Weilong Chen 0 siblings, 1 reply; 4+ messages in thread From: minyard @ 2017-07-30 2:20 UTC (permalink / raw) To: Weilong Chen; +Cc: openipmi-developer, linux-kernel, Corey Minyard From: Corey Minyard <cminyard@mvista.com> 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 <chenweilong@huawei.com> Signed-off-by: Corey Minyard <cminyard@mvista.com> --- 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ipmi: fix unsigned long underflow 2017-07-30 2:20 ` [PATCH] ipmi: fix unsigned long underflow minyard @ 2017-08-07 8:41 ` Weilong Chen 2017-08-07 12:24 ` Corey Minyard 0 siblings, 1 reply; 4+ messages in thread From: Weilong Chen @ 2017-08-07 8:41 UTC (permalink / raw) To: minyard; +Cc: openipmi-developer, linux-kernel, Corey Minyard Hi Minyard, I test this patch, it works. Thanks. On 2017/7/30 10:20, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > 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 <chenweilong@huawei.com> > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > 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; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipmi: fix unsigned long underflow 2017-08-07 8:41 ` Weilong Chen @ 2017-08-07 12:24 ` Corey Minyard 0 siblings, 0 replies; 4+ messages in thread From: Corey Minyard @ 2017-08-07 12:24 UTC (permalink / raw) To: Weilong Chen; +Cc: openipmi-developer, linux-kernel, Corey Minyard 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 <cminyard@mvista.com> >> >> 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 <chenweilong@huawei.com> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> --- >> 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; >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-07 12:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-29 2:42 [PATCH] ipmi: fix unsigned long overflow Weilong Chen 2017-07-30 2:20 ` [PATCH] ipmi: fix unsigned long underflow minyard 2017-08-07 8:41 ` Weilong Chen 2017-08-07 12:24 ` Corey Minyard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).