On Thursday, July 25, 2013 01:12:38 PM Corey Minyard wrote: > On 07/25/2013 07:06 AM, Rafael J. Wysocki wrote: > > On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote: > >> -stable according to the previous conversation. > >> > >>> From: Rafael J. Wysocki [mailto:rjw@xxxxxxx] > >>> Sent: Thursday, July 25, 2013 7:38 AM > >>> > >>> On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote: > >>>> This patch fixes races caused by unprotected ACPI IPMI transfers. > >>>> > >>>> We can see the following crashes may occur: > >>>> 1. There is no tx_msg_lock held for iterating tx_msg_list in > >>>> ipmi_flush_tx_msg() while it is parellel unlinked on failure in > >>>> acpi_ipmi_space_handler() under protection of tx_msg_lock. > >>>> 2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler() > >>>> while it is parellel accessed in ipmi_flush_tx_msg() and > >>>> ipmi_msg_handler(). > >>>> > >>>> This patch enhances tx_msg_lock to protect all tx_msg accesses to > >>>> solve this issue. Then tx_msg_lock is always held around complete() > >>>> and tx_msg accesses. > >>>> Calling smp_wmb() before setting msg_done flag so that messages > >>>> completed due to flushing will not be handled as 'done' messages while > >>>> their contents are not vaild. > >>>> > >>>> Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > >>>> Cc: Zhao Yakui <yakui.zhao@xxxxxxxxx> > >>>> Reviewed-by: Huang Ying <ying.huang@xxxxxxxxx> > >>>> --- > >>>> drivers/acpi/acpi_ipmi.c | 10 ++++++++-- > >>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index > >>>> b37c189..527ee43 100644 > >>>> --- a/drivers/acpi/acpi_ipmi.c > >>>> +++ b/drivers/acpi/acpi_ipmi.c > >>>> @@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct > >>> acpi_ipmi_device *ipmi) > >>>> struct acpi_ipmi_msg *tx_msg, *temp; > >>>> int count = HZ / 10; > >>>> struct pnp_dev *pnp_dev = ipmi->pnp_dev; > >>>> + unsigned long flags; > >>>> > >>>> + spin_lock_irqsave(&ipmi->tx_msg_lock, flags); > >>>> list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) { > >>>> /* wake up the sleep thread on the Tx msg */ > >>>> complete(&tx_msg->tx_complete); > >>>> } > >>>> + spin_unlock_irqrestore(&ipmi->tx_msg_lock, flags); > >>>> > >>>> /* wait for about 100ms to flush the tx message list */ > >>>> while (count--) { > >>>> @@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct > >>> ipmi_recv_msg *msg, void *user_msg_data) > >>>> break; > >>>> } > >>>> } > >>>> - spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags); > >>>> > >>>> if (!msg_found) { > >>>> dev_warn(&pnp_dev->dev, > >>>> "Unexpected response (msg id %ld) is returned.\n", > >>>> msg->msgid); > >>>> - goto out_msg; > >>>> + goto out_lock; > >>>> } > >>>> > >>>> /* copy the response data to Rx_data buffer */ @@ -286,10 +288,14 @@ > >>>> static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void > >>> *user_msg_data) > >>>> } > >>>> tx_msg->rx_len = msg->msg.data_len; > >>>> memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len); > >>>> + /* tx_msg content must be valid before setting msg_done flag */ > >>>> + smp_wmb(); > >>> That's suspicious. > >>> > >>> If you need the write barrier here, you'll most likely need a read barrier > >>> somewhere else. Where's that? > >> It might depend on whether the content written before the smp_wmb() is used or not by the other side codes under the condition set after the smp_wmb(). > >> > >> So comment could be treated as 2 parts: > >> 1. do we need a paired smp_rmb(). > >> 2. do we need a smp_wmb(). > >> > >> For 1. > >> If we want a paired smp_rmb(), then it will appear in this function: > >> > >> 186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg, > >> 187 acpi_integer *value, int rem_time) > >> 188 { > >> 189 struct acpi_ipmi_buffer *buffer; > >> 190 > >> 191 /* > >> 192 * value is also used as output parameter. It represents the response > >> 193 * IPMI message returned by IPMI command. > >> 194 */ > >> 195 buffer = (struct acpi_ipmi_buffer *)value; > >> 196 if (!rem_time && !msg->msg_done) { > >> 197 buffer->status = ACPI_IPMI_TIMEOUT; > >> 198 return; > >> 199 } > >> 200 /* > >> 201 * If the flag of msg_done is not set or the recv length is zero, it > >> 202 * means that the IPMI command is not executed correctly. > >> 203 * The status code will be ACPI_IPMI_UNKNOWN. > >> 204 */ > >> 205 if (!msg->msg_done || !msg->rx_len) { > >> 206 buffer->status = ACPI_IPMI_UNKNOWN; > >> 207 return; > >> 208 } > >> + smp_rmb(); > >> 209 /* > >> 210 * If the IPMI response message is obtained correctly, the status code > >> 211 * will be ACPI_IPMI_OK > >> 212 */ > >> 213 buffer->status = ACPI_IPMI_OK; > >> 214 buffer->length = msg->rx_len; > >> 215 memcpy(buffer->data, msg->rx_data, msg->rx_len); > >> 216 } > >> > >> If we don't then there will only be msg content not correctly read from msg->rx_data. > >> Note that the rx_len is 0 during initialization and will never exceed the sizeof(buffer->data), so the read is safe. > >> > >> Being without smp_rmb() is also OK in this case, since: > >> 1. buffer->data will never be used when buffer->status is not ACPI_IPMI_OK and > >> 2. the smp_rmb()/smp_wmb() added in this patch will be deleted in [PATCH 07]. > >> > >> So IMO, we needn't add the smp_rmb(), what do you think of this? > >> > >> For 2. > >> If we don't add smp_wmb() in the ipmi_msg_handler(), then the codes running on other thread in the acpi_format_ipmi_response() may read wrong msg->rx_data (a timeout triggers this function, but when acpi_format_ipmi_response() is entered, the msg->msg_done flag could be seen as 1 but the msg->rx_data is not ready), this is what we want to avoid in this quick fix. > > Using smp_wmb() without the complementary smp_rmb() doesn't makes sense, > > because each of them prevents only one flow of control from being > > speculatively reordered, either by the CPU or by the compiler. If only one > > of them is used without the other, then the flow of control without the > > barrier may be reordered in a way that will effectively cancel the effect of > > the barrier in the second flow of control. > > > > So, either we need *both* smp_wmb() and smp_rmb(), or we don't need them at all. > > If I understand this correctly, the problem would be if: > > rem_time = wait_for_completion_timeout(&tx_msg->tx_complete, > IPMI_TIMEOUT); > > returns on a timeout, then checks msg_done and races with something > setting msg_done. If that is the case, you would need the smp_rmb() > before checking msg_done. I believe so. > However, the timeout above is unnecessary. You are using > ipmi_request_settime(), so you can set the timeout when the IPMI command > fails and returns a failure message. The driver guarantees a return > message for each request. Just remove the timeout from the completion, > set the timeout and retries in the ipmi request, and the completion > should handle the barrier issues. Good point. > Plus, from a quick glance at the code, it doesn't look like it will > properly handle a situation where the timeout occurs and is handled then > the response comes in later. Lv, what about this? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
