Re: [PATCH] ipmi: Remove smi_msg from waiting_rcv_msgs list before handle_one_recv_msg()

[Corey Minyard <minyard@acm.org>]

I actually just wrote this exact patch, moments ago.  But you deserve 
credit, I'll use yours :).

-corey

On 06/09/2016 11:31 PM, Junichi Nomura wrote:
> Commit 7ea0ed2b5be8 ("ipmi: Make the message handler easier to use for
> SMI interfaces") changed handle_new_recv_msgs() to call handle_one_recv_msg()
> for a smi_msg while the smi_msg is still connected to waiting_rcv_msgs list.
> That could lead to following list corruption problems:
>
> 1) low-level function treats smi_msg as not connected to list
>
>    handle_one_recv_msg() could end up calling smi_send(), which
>    assumes the msg is not connected to list.
>
>    For example, the following sequence could corrupt list by
>    doing list_add_tail() for the entry still connected to other list.
>
>      handle_new_recv_msgs()
>        msg = list_entry(waiting_rcv_msgs)
>        handle_one_recv_msg(msg)
>          handle_ipmb_get_msg_cmd(msg)
>            smi_send(msg)
>              spin_lock(xmit_msgs_lock)
>              list_add_tail(msg)
>              spin_unlock(xmit_msgs_lock)
>
> 2) race between multiple handle_new_recv_msgs() instances
>
>    handle_new_recv_msgs() once releases waiting_rcv_msgs_lock before calling
>    handle_one_recv_msg() then retakes the lock and list_del() it.
>
>    If others call handle_new_recv_msgs() during the window shown below
>    list_del() will be done twice for the same smi_msg.
>
>    handle_new_recv_msgs()
>      spin_lock(waiting_rcv_msgs_lock)
>      msg = list_entry(waiting_rcv_msgs)
>      spin_unlock(waiting_rcv_msgs_lock)
>    |
>    | handle_one_recv_msg(msg)
>    |
>      spin_lock(waiting_rcv_msgs_lock)
>      list_del(msg)
>      spin_unlock(waiting_rcv_msgs_lock)
>
> Fixes: 7ea0ed2b5be8 ("ipmi: Make the message handler easier to use for SMI interfaces")
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 94fb407..94e4a88 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3820,6 +3820,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)
>   	while (!list_empty(&intf->waiting_rcv_msgs)) {
>   		smi_msg = list_entry(intf->waiting_rcv_msgs.next,
>   				     struct ipmi_smi_msg, link);
> +		list_del(&smi_msg->link);
>   		if (!run_to_completion)
>   			spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock,
>   					       flags);
> @@ -3831,9 +3832,9 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)
>   			 * To preserve message order, quit if we
>   			 * can't handle a message.
>   			 */
> +			list_add(&smi_msg->link, &intf->waiting_rcv_msgs);
>   			break;
>   		} else {
> -			list_del(&smi_msg->link);
>   			if (rv == 0)
>   				/* Message handled */
>   				ipmi_free_smi_msg(smi_msg);