ACPI/IPMI: Fix several issues in the current codes

This patch enhances sanity checks on message size to avoid potential buffer overflow.

The kernel IPMI message size is IPMI_MAX_MSG_LENGTH(272 bytes) while the ACPI specification defined IPMI message size is 64 bytes. The difference is not handled by the original codes. This may cause crash in the response handling codes.
This patch fixes this gap and also combines rx_data/tx_data to use single data/len pair since they need not be seperated.

Signed-off-by Lv Zheng <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |  100 ++++++++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 39 deletions(-)

From: [email protected] [mailto:[email protected]] On Behalf Of Greg KH
Sent: Tuesday, July 23, 2013 10:54 PM

Quoted 10 lines · Lv Zheng

This patch enhances sanity checks on message size to avoid potential buffer overflow.

The kernel IPMI message size is IPMI_MAX_MSG_LENGTH(272 bytes) while the ACPI specification defined IPMI message size is 64 bytes. The difference is not handled by the original codes. This may cause crash in the response handling codes.
This patch fixes this gap and also combines rx_data/tx_data to use single data/len pair since they need not be seperated.

Quoted 6 lines

Signed-off-by Lv Zheng <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |  100
++++++++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 39 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Same goes for the other patches you sent in this thread...

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

From: Zheng, Lv
Sent: Wednesday, July 24, 2013 8:22 AM

Quoted 28 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Greg KH
Sent: Tuesday, July 23, 2013 10:54 PM

Quoted 10 lines · Lv Zheng

This patch enhances sanity checks on message size to avoid potential buffer overflow.

The kernel IPMI message size is IPMI_MAX_MSG_LENGTH(272 bytes) while the ACPI specification defined IPMI message size is 64 bytes. The difference is not handled by the original codes. This may cause crash in the response handling codes.
This patch fixes this gap and also combines rx_data/tx_data to use single data/len pair since they need not be seperated.

Quoted 6 lines

Signed-off-by Lv Zheng <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |  100
++++++++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 39 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

Quoted 3 lines

</formletter>

Same goes for the other patches you sent in this thread...

OK, I'll add prerequisites for each that want to be accepted by the stable queue and re-send them (PATCH 01-06).

Thanks and best regards
-Lv

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c +4 -2

index b37c189..527ee43 100644
@@ -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 */

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 29 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c index renamed from drivers/acpi/acpi_ipmi.c +0 -0

b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

 	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)

Quoted 14 lines

drivers/acpi/acpi_ipmi.c +1 -2

 			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 @@

Quoted 1 line

static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

drivers/acpi/acpi_ipmi.c +2 -0

 	}
 	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?

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 301

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +2 -0

 	tx_msg->msg_done = 1;
 
 out_comp:
 	complete(&tx_msg->tx_complete);
+out_lock:
+	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 out_msg:
 	ipmi_free_recv_msg(msg);
 }

Rafael

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

-stable according to the previous conversation.

Quoted 74 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 29 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c index renamed from drivers/acpi/acpi_ipmi.c +0 -0

b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

 	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)

Quoted 14 lines

drivers/acpi/acpi_ipmi.c +1 -2

 			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 @@

Quoted 1 line

static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

drivers/acpi/acpi_ipmi.c +2 -0

 	}
 	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.

Quoted 2 lines

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.

Quoted 122 lines · Lv

-stable according to the previous conversation.

Quoted 73 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 29 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
  drivers/acpi/acpi_ipmi.c |   10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c index renamed from drivers/acpi/acpi_ipmi.c +0 -0

b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

  	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)

Quoted 14 lines

drivers/acpi/acpi_ipmi.c +1 -2

  			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 @@

Quoted 1 line

static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

  	}
  	tx_msg->rx_len = msg->msg.data_len;
  	memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);

drivers/acpi/acpi_ipmi.c +2 -0

+	/* tx_msg content must be valid before setting msg_done flag */
+	smp_wmb();

That's suspicious.

Quoted 2 lines

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.

Quoted 8 lines

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.

Thanks,
Rafael

Quoted 131 lines · Rafael J. Wysocki

Quoted 122 lines · Lv

-stable according to the previous conversation.

Quoted 73 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 29 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
  drivers/acpi/acpi_ipmi.c |   10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c index renamed from drivers/acpi/acpi_ipmi.c +0 -0

b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

  	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)

Quoted 14 lines

drivers/acpi/acpi_ipmi.c +1 -2

  			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 @@

Quoted 1 line

static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

  	}
  	tx_msg->rx_len = msg->msg.data_len;
  	memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);

drivers/acpi/acpi_ipmi.c +2 -0

+	/* tx_msg content must be valid before setting msg_done flag */
+	smp_wmb();

That's suspicious.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 8 lines

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.

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.

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.

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Sent: Friday, July 26, 2013 3:33 AM

Quoted 39 lines · Corey Minyard

Quoted 38 lines · Rafael J. Wysocki

Quoted 37 lines · Lv

-stable according to the previous conversation.

Quoted 35 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 30 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
  drivers/acpi/acpi_ipmi.c |   10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c +0 -0

index
b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi)

Quoted 7 lines

drivers/acpi/acpi_ipmi.c +2 -0

  	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) {

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 8 lines

drivers/acpi/acpi_ipmi.c +1 -0

  		/* 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)

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +2 -2

  			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

Quoted 1 line

*msg, void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

  	}
  	tx_msg->rx_len = msg->msg.data_len;
  	memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);

drivers/acpi/acpi_ipmi.c +2 -0

+	/* tx_msg content must be valid before setting msg_done flag */
+	smp_wmb();

That's suspicious.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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().

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Note that the rx_len is 0 during initialization and will never exceed the

sizeof(buffer->data), so the read is safe.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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].

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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 [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

From: Corey Minyard [mailto:[email protected]]
Sent: Friday, July 26, 2013 2:13 AM

Quoted 78 lines · Rafael J. Wysocki

Quoted 77 lines · Lv

-stable according to the previous conversation.

Quoted 75 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 30 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
  drivers/acpi/acpi_ipmi.c |   10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c +0 -0

index
b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

  	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)

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +2 -2

  			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,

Quoted 1 line

void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

  	}
  	tx_msg->rx_len = msg->msg.data_len;
  	memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);

drivers/acpi/acpi_ipmi.c +2 -0

+	/* tx_msg content must be valid before setting msg_done flag */
+	smp_wmb();

That's suspicious.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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().

Quoted 8 lines

Quoted 8 lines

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:

Quoted 7 lines

Quoted 7 lines

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

Quoted 9 lines

Quoted 9 lines

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

Quoted 10 lines

Quoted 10 lines

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

Quoted 8 lines

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line

Note that the rx_len is 0 during initialization and will never exceed the

sizeof(buffer->data), so the read is safe.

Quoted 4 lines

Quoted 4 lines

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].

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

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.

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.

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.

-corey

Quoted 5 lines

Thanks,
Rafael

Quoted 10 lines

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.

Quoted 6 lines

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.

It's just difficult for me to determine retry count and timeout value, maybe retry=0, timeout=IPMI_TIMEOUT is OK. The code of the timeout completion is already there, I think the quick fix code should not introduce this logic. I'll add a new patch to apply your comment.

Quoted 4 lines

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.

PATCH 07 fixed this issue.
Here we just need the smp_rmb() or holding tx_msg_lock() around the acpi_format_ipmi_response().

Thanks for commenting.

From: Corey Minyard [mailto:[email protected]]
Sent: Friday, July 26, 2013 8:48 AM

Quoted 10 lines · Lv

Quoted 10 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

It's just difficult for me to determine retry count and timeout value, maybe

retry=0, timeout=IPMI_TIMEOUT is OK.

Quoted 1 line

The code of the timeout completion is already there, I think the quick fix code

should not introduce this logic.

Quoted 1 line

I'll add a new patch to apply your comment.

Since it is a local BMC, I doubt a retry is required. That is probably fine. Or you could set retry=1 and timeout=IPMI_TIMEOUT/2 if you wanted to be more sure, but I doubt it would make a difference. The only time you really need to worry about retries is if you are resetting the BMC or it is being overloaded.

Quoted 7 lines

Quoted 5 lines

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.

PATCH 07 fixed this issue.
Here we just need the smp_rmb() or holding tx_msg_lock() around the

acpi_format_ipmi_response().

If you apply the fix like I suggest, then the race goes away. If there's no timeout and it just waits for the completion, things get a lot simpler.

Quoted 2 lines

Thanks for commenting.

No problem, thanks for working on this.

-corey

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 8:07 PM

Quoted 78 lines · Lv

-stable according to the previous conversation.

Quoted 76 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, July 25, 2013 7:38 AM

drivers/acpi/acpi_ipmi.c static void ipmi_flush_tx_msg(struct

Quoted 30 lines · Lv Zheng

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 <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/acpi/acpi_ipmi.c +0 -0

index
b37c189..527ee43 100644

drivers/acpi/acpi_ipmi.c +0 -0

@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +3 -0

 	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)

Quoted 15 lines

drivers/acpi/acpi_ipmi.c +2 -2

 			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,

Quoted 1 line

void

*user_msg_data)

drivers/acpi/acpi_ipmi.c static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) · line 292

Quoted 5 lines

drivers/acpi/acpi_ipmi.c +2 -0

 	}
 	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.

Quoted 4 lines

Quoted 2 lines

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().

Quoted 8 lines

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:

Quoted 7 lines

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

Quoted 9 lines

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

Quoted 10 lines

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

Quoted 8 lines

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.

Quoted 1 line

Note that the rx_len is 0 during initialization and will never exceed the

sizeof(buffer->data), so the read is safe.

Quoted 4 lines

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].

Quoted 5 lines

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.

Thanks,
Rafael

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

This patch uses reference counting to fix the race caused by the unprotected ACPI IPMI user.

As the acpi_ipmi_device->user_interface check in acpi_ipmi_space_handler() can happen before setting user_interface to NULL and codes after the check in acpi_ipmi_space_handler() can happen after user_interface becoming NULL, then the on-going acpi_ipmi_space_handler() still can pass an invalid acpi_ipmi_device->user_interface to ipmi_request_settime(). Such race condition is not allowed by the IPMI layer's API design as crash will happen in ipmi_request_settime().
In IPMI layer, smi_gone()/new_smi() callbacks are protected by smi_watchers_mutex, thus their invocations are serialized. But as a new smi can re-use the freed intf_num, it requires that the callback implementation must not use intf_num as an identification mean or it must ensure all references to the previous smi are all dropped before exiting smi_gone() callback. In case of acpi_ipmi module, this means ipmi_flush_tx_msg() must ensure all on-going IPMI transfers are completed before exiting ipmi_flush_tx_msg().

This patch follows ipmi_devintf.c design:
1. Invoking ipmi_destroy_user() after the reference count of acpi_ipmi_device dropping to 0, this matches IPMI layer's API calling rule on ipmi_destroy_user() and ipmi_request_settime().
2. References of acpi_ipmi_device dropping to 1 means tx_msg related to this acpi_ipmi_device are all freed, this can be used to implement the new flushing mechanism. Note complete() must be retried so that the on-going tx_msg won't block flushing at the point to add tx_msg into tx_msg_list where reference of acpi_ipmi_device is held. This matches the IPMI layer's callback rule on smi_gone()/new_smi() serialization.
3. ipmi_flush_tx_msg() is performed after deleting acpi_ipmi_device from the list so that no new tx_msg can be created after entering flushing process.
4. The flushing of tx_msg is also moved out of ipmi_lock in this patch.

The forthcoming IPMI operation region handler installation changes also requires acpi_ipmi_device be handled in the reference counting style.

Authorship is also updated due to this design change.

Signed-off-by Lv Zheng <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |  249 +++++++++++++++++++++++++++-------------------
 1 file changed, 149 insertions(+), 100 deletions(-)

drivers/acpi/acpi_ipmi.c +3 -2

index 527ee43..cbf25e0 100644
@@ -1,8 +1,9 @@
 /*
  *  acpi_ipmi.c - ACPI IPMI opregion
  *
- *  Copyright (C) 2010 Intel Corporation
- *  Copyright (C) 2010 Zhao Yakui <[email protected]>
+ *  Copyright (C) 2010, 2013 Intel Corporation
+ *    Author: Zhao Yakui <[email protected]>
+ *            Lv Zheng <[email protected]>
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *

drivers/acpi/acpi_ipmi.c +53 -2

 };
 
 struct ipmi_driver_data {

@@ -107,8 +109,8 @@ struct acpi_ipmi_buffer {
 static void ipmi_register_bmc(int iface, struct device *dev);
 static void ipmi_bmc_gone(int iface);
 static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
-static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device);
-static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device);
+static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi);
+static void ipmi_remove_space_handler(struct acpi_ipmi_device *ipmi);
 
 static struct ipmi_driver_data driver_data = {
 	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),

@@ -122,6 +124,80 @@ static struct ipmi_driver_data driver_data = {
 	},
 };
 
+static struct acpi_ipmi_device *
+ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle handle)
+{
+	struct acpi_ipmi_device *ipmi_device;
+	int err;
+	ipmi_user_t user;
+
+	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+	if (!ipmi_device)
+		return NULL;
+
+	atomic_set(&ipmi_device->refcnt, 1);
+	INIT_LIST_HEAD(&ipmi_device->head);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+	spin_lock_init(&ipmi_device->tx_msg_lock);
+
+	ipmi_device->handle = handle;
+	ipmi_device->pnp_dev = to_pnp_dev(get_device(smi_data->dev));
+	memcpy(&ipmi_device->smi_data, smi_data, sizeof(struct ipmi_smi_info));
+	ipmi_device->ipmi_ifnum = iface;
+
+	err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+			       ipmi_device, &user);
+	if (err) {
+		put_device(smi_data->dev);
+		kfree(ipmi_device);
+		return NULL;
+	}
+	ipmi_device->user_interface = user;
+	ipmi_install_space_handler(ipmi_device);
+
+	return ipmi_device;
+}
+
+static struct acpi_ipmi_device *
+acpi_ipmi_dev_get(struct acpi_ipmi_device *ipmi_device)
+{
+	if (ipmi_device)
+		atomic_inc(&ipmi_device->refcnt);
+	return ipmi_device;
+}
+
+static void ipmi_dev_release(struct acpi_ipmi_device *ipmi_device)
+{
+	ipmi_remove_space_handler(ipmi_device);
+	ipmi_destroy_user(ipmi_device->user_interface);
+	put_device(ipmi_device->smi_data.dev);
+	kfree(ipmi_device);
+}
+
+static void acpi_ipmi_dev_put(struct acpi_ipmi_device *ipmi_device)

drivers/acpi/acpi_ipmi.c +2 -11

+}
+
 static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
 {
 	struct acpi_ipmi_msg *ipmi_msg;

@@ -228,25 +304,24 @@ static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
 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--) {

drivers/acpi/acpi_ipmi.c +27 -34

+		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);
+		schedule_timeout_uninterruptible(msecs_to_jiffies(1));
 	}
-	if (!list_empty(&ipmi->tx_msg_list))
-		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
 }
 
 static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)

@@ -304,22 +379,26 @@ static void ipmi_register_bmc(int iface, struct device *dev)
 {
 	struct acpi_ipmi_device *ipmi_device, *temp;
 	struct pnp_dev *pnp_dev;
-	ipmi_user_t		user;
 	int err;
 	struct ipmi_smi_info smi_data;
 	acpi_handle handle;
 
 	err = ipmi_get_smi_info(iface, &smi_data);
-
 	if (err)
 		return;
 
-	if (smi_data.addr_src != SI_ACPI) {
-		put_device(smi_data.dev);
-		return;
-	}
-
+	if (smi_data.addr_src != SI_ACPI)
+		goto err_ref;
 	handle = smi_data.addr_info.acpi_info.acpi_handle;
+	if (!handle)
+		goto err_ref;
+	pnp_dev = to_pnp_dev(smi_data.dev);
+
+	ipmi_device = ipmi_dev_alloc(iface, &smi_data, handle);
+	if (!ipmi_device) {
+		dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
+		goto err_ref;
+	}
 
 	mutex_lock(&driver_data.ipmi_lock);
 	list_for_each_entry(temp, &driver_data.ipmi_devices, head) {

@@ -328,54 +407,42 @@ static void ipmi_register_bmc(int iface, struct device *dev)
 		 * to the device list, don't add it again.
 		 */
 		if (temp->handle == handle)
-			goto out;
+			goto err_lock;
 	}
 
-	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
-
-	if (!ipmi_device)
-		goto out;
-
-	pnp_dev = to_pnp_dev(smi_data.dev);
-	ipmi_device->handle = handle;
-	ipmi_device->pnp_dev = pnp_dev;
-
-	err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
-					ipmi_device, &user);
-	if (err) {
-		dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
-		kfree(ipmi_device);
-		goto out;
-	}
-	acpi_add_ipmi_device(ipmi_device);
-	ipmi_device->user_interface = user;
-	ipmi_device->ipmi_ifnum = iface;
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
 	mutex_unlock(&driver_data.ipmi_lock);
-	memcpy(&ipmi_device->smi_data, &smi_data, sizeof(struct ipmi_smi_info));
+	put_device(smi_data.dev);
 	return;
 
-out:
+err_lock:
 	mutex_unlock(&driver_data.ipmi_lock);
+	ipmi_dev_release(ipmi_device);
+err_ref:
 	put_device(smi_data.dev);
 	return;
 }
 
 static void ipmi_bmc_gone(int iface)
 {
-	struct acpi_ipmi_device *ipmi_device, *temp;
+	int dev_found = 0;
+	struct acpi_ipmi_device *ipmi_device;
 
 	mutex_lock(&driver_data.ipmi_lock);
-	list_for_each_entry_safe(ipmi_device, temp,
-				&driver_data.ipmi_devices, head) {

drivers/acpi/acpi_ipmi.c +32 -41

+			break;
+		}
 	}
+	if (dev_found)
+		list_del(&ipmi_device->head);
 	mutex_unlock(&driver_data.ipmi_lock);
+
+	if (dev_found) {
+		ipmi_flush_tx_msg(ipmi_device);
+		acpi_ipmi_dev_put(ipmi_device);
+	}
 }
 
 /* --------------------------------------------------------------------------

@@ -400,7 +467,8 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
 			void *handler_context, void *region_context)
 {
 	struct acpi_ipmi_msg *tx_msg;
-	struct acpi_ipmi_device *ipmi_device = handler_context;
+	int iface = (long)handler_context;
+	struct acpi_ipmi_device *ipmi_device;
 	int err, rem_time;
 	acpi_status status;
 	unsigned long flags;

@@ -414,12 +482,15 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
 	if ((function & ACPI_IO_MASK) == ACPI_READ)
 		return AE_TYPE;
 
-	if (!ipmi_device->user_interface)
+	ipmi_device = acpi_ipmi_get_targeted_smi(iface);
+	if (!ipmi_device)
 		return AE_NOT_EXIST;
 
 	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
-	if (!tx_msg)
-		return AE_NO_MEMORY;
+	if (!tx_msg) {
+		status = AE_NO_MEMORY;
+		goto out_ref;
+	}
 
 	if (acpi_format_ipmi_request(tx_msg, address, value) != 0) {
 		status = AE_TYPE;

@@ -449,6 +520,8 @@ out_list:
 	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 out_msg:
 	kfree(tx_msg);
+out_ref:
+	acpi_ipmi_dev_put(ipmi_device);
 	return status;
 }
 

@@ -473,7 +546,7 @@ static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi)
 	status = acpi_install_address_space_handler(ipmi->handle,
 						    ACPI_ADR_SPACE_IPMI,
 						    &acpi_ipmi_space_handler,
-						    NULL, ipmi);
+						    NULL, (void *)((long)ipmi->ipmi_ifnum));
 	if (ACPI_FAILURE(status)) {
 		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
 		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion space "

@@ -484,36 +557,6 @@ static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi)
 	return 0;
 }
 
-static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device)
-{
-
-	INIT_LIST_HEAD(&ipmi_device->head);
-
-	spin_lock_init(&ipmi_device->tx_msg_lock);
-	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
-	ipmi_install_space_handler(ipmi_device);
-
-	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
-}
-
-static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device)
-{
-	/*
-	 * If the IPMI user interface is created, it should be
-	 * destroyed.
-	 */
-	if (ipmi_device->user_interface) {
-		ipmi_destroy_user(ipmi_device->user_interface);
-		ipmi_device->user_interface = NULL;
-	}
-	/* flush the Tx_msg list */
-	if (!list_empty(&ipmi_device->tx_msg_list))
-		ipmi_flush_tx_msg(ipmi_device);
-
-	list_del(&ipmi_device->head);
-	ipmi_remove_space_handler(ipmi_device);
-}
-
 static int __init acpi_ipmi_init(void)
 {
 	int result = 0;

@@ -530,7 +573,7 @@ static int __init acpi_ipmi_init(void)
 
 static void __exit acpi_ipmi_exit(void)
 {
-	struct acpi_ipmi_device *ipmi_device, *temp;
+	struct acpi_ipmi_device *ipmi_device;
 
 	if (acpi_disabled)
 		return;

@@ -544,11 +587,17 @@ static void __exit acpi_ipmi_exit(void)
 	 * handler and free it.
 	 */
 	mutex_lock(&driver_data.ipmi_lock);
-	list_for_each_entry_safe(ipmi_device, temp,
-				&driver_data.ipmi_devices, head) {
-		acpi_remove_ipmi_device(ipmi_device);
-		put_device(ipmi_device->smi_data.dev);
-		kfree(ipmi_device);
+	while (!list_empty(&driver_data.ipmi_devices)) {
+		ipmi_device = list_first_entry(&driver_data.ipmi_devices,
+					       struct acpi_ipmi_device,
+					       head);
+		list_del(&ipmi_device->head);
+		mutex_unlock(&driver_data.ipmi_lock);
+
+		ipmi_flush_tx_msg(ipmi_device);
+		acpi_ipmi_dev_put(ipmi_device);
+
+		mutex_lock(&driver_data.ipmi_lock);
 	}
 	mutex_unlock(&driver_data.ipmi_lock);
 }

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Friday, July 26, 2013 5:59 AM

Quoted 10 lines · Lv Zheng

This patch uses reference counting to fix the race caused by the unprotected ACPI IPMI user.

As the acpi_ipmi_device->user_interface check in acpi_ipmi_space_handler() can happen before setting user_interface to NULL and codes after the check in acpi_ipmi_space_handler() can happen after user_interface becoming NULL, then the on-going acpi_ipmi_space_handler() still can pass an invalid acpi_ipmi_device->user_interface to ipmi_request_settime(). Such race condition is not allowed by the IPMI layer's API design as crash will happen in

ipmi_request_settime().

Quoted 43 lines

In IPMI layer, smi_gone()/new_smi() callbacks are protected by smi_watchers_mutex, thus their invocations are serialized. But as a new smi can re-use the freed intf_num, it requires that the callback implementation must not use intf_num as an identification mean or it must ensure all references to the previous smi are all dropped before exiting
smi_gone() callback. In case of acpi_ipmi module, this means ipmi_flush_tx_msg() must ensure all on-going IPMI transfers are completed before exiting ipmi_flush_tx_msg().

This patch follows ipmi_devintf.c design:
1. Invoking ipmi_destroy_user() after the reference count of acpi_ipmi_device dropping to 0, this matches IPMI layer's API calling rule on ipmi_destroy_user() and ipmi_request_settime().
2. References of acpi_ipmi_device dropping to 1 means tx_msg related to this acpi_ipmi_device are all freed, this can be used to implement the new flushing mechanism. Note complete() must be retried so that the on-going tx_msg won't block flushing at the point to add tx_msg into tx_msg_list where reference of acpi_ipmi_device is held. This matches the IPMI layer's callback rule on smi_gone()/new_smi() serialization.
3. ipmi_flush_tx_msg() is performed after deleting acpi_ipmi_device from the list so that no new tx_msg can be created after entering flushing process.
4. The flushing of tx_msg is also moved out of ipmi_lock in this patch.

The forthcoming IPMI operation region handler installation changes also requires acpi_ipmi_device be handled in the reference counting style.

Authorship is also updated due to this design change.

Signed-off-by Lv Zheng <[email protected]>

Cc Zhao Yakui <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |  249
+++++++++++++++++++++++++++-------------------
 1 file changed, 149 insertions(+), 100 deletions(-)

drivers/acpi/acpi_ipmi.c index renamed from drivers/acpi/acpi_ipmi.c +0 -0

527ee43..cbf25e0 100644

drivers/acpi/acpi_ipmi.c line 8

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +3 -2

@@ -1,8 +1,9 @@
 /*
  *  acpi_ipmi.c - ACPI IPMI opregion
  *
- *  Copyright (C) 2010 Intel Corporation
- *  Copyright (C) 2010 Zhao Yakui <[email protected]>
+ *  Copyright (C) 2010, 2013 Intel Corporation
+ *    Author: Zhao Yakui <[email protected]>
+ *            Lv Zheng <[email protected]>
  *
  *

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~

drivers/acpi/acpi_ipmi.c struct acpi_ipmi_device { · line 71

Quoted 6 lines

drivers/acpi/acpi_ipmi.c +1 -0

  *

@@ -67,6 +68,7 @@ struct acpi_ipmi_device {
 	long curr_msgid;
 	unsigned long flags;
 	struct ipmi_smi_info smi_data;
+	atomic_t refcnt;

Can you use a kref instead?

Quoted 28 lines

drivers/acpi/acpi_ipmi.c +0 -0

 };
 
 struct ipmi_driver_data {

@@ -107,8 +109,8 @@ struct acpi_ipmi_buffer {  static void

ipmi_register_bmc(int iface, struct device *dev); static void ipmi_bmc_gone(int iface); static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data); -static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device); -static void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device);

drivers/acpi/acpi_ipmi.c +11 -0

+static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi);
+static void ipmi_remove_space_handler(struct acpi_ipmi_device *ipmi);
 
 static struct ipmi_driver_data driver_data = {
 	.ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),

@@ -122,6 +124,80 @@ static struct ipmi_driver_data driver_data = {
 	},
 };
 
+static struct acpi_ipmi_device *
+ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle
+handle) {
+	struct acpi_ipmi_device *ipmi_device;
+	int err;
+	ipmi_user_t user;
+
+	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
+	if (!ipmi_device)

drivers/acpi/acpi_ipmi.c static struct ipmi_driver_data driver_data = { · line 145

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +10 -0

+		return NULL;
+
+	atomic_set(&ipmi_device->refcnt, 1);
+	INIT_LIST_HEAD(&ipmi_device->head);
+	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
+	spin_lock_init(&ipmi_device->tx_msg_lock);
+
+	ipmi_device->handle = handle;
+	ipmi_device->pnp_dev = to_pnp_dev(get_device(smi_data->dev));
+	memcpy(&ipmi_device->smi_data, smi_data, sizeof(struct

ipmi_smi_info));

Quoted 29 lines

drivers/acpi/acpi_ipmi.c +29 -0

+	ipmi_device->ipmi_ifnum = iface;
+
+	err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
+			       ipmi_device, &user);
+	if (err) {
+		put_device(smi_data->dev);
+		kfree(ipmi_device);
+		return NULL;
+	}
+	ipmi_device->user_interface = user;
+	ipmi_install_space_handler(ipmi_device);
+
+	return ipmi_device;
+}
+
+static struct acpi_ipmi_device *
+acpi_ipmi_dev_get(struct acpi_ipmi_device *ipmi_device) {
+	if (ipmi_device)
+		atomic_inc(&ipmi_device->refcnt);
+	return ipmi_device;
+}
+
+static void ipmi_dev_release(struct acpi_ipmi_device *ipmi_device) {
+	ipmi_remove_space_handler(ipmi_device);
+	ipmi_destroy_user(ipmi_device->user_interface);
+	put_device(ipmi_device->smi_data.dev);
+	kfree(ipmi_device);
+}
+

drivers/acpi/acpi_ipmi.c static struct ipmi_driver_data driver_data = { · line 187

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +10 -0

+static void acpi_ipmi_dev_put(struct acpi_ipmi_device *ipmi_device) {
+	if (ipmi_device && atomic_dec_and_test(&ipmi_device->refcnt))
+		ipmi_dev_release(ipmi_device);
+}
+
+static struct acpi_ipmi_device *acpi_ipmi_get_targeted_smi(int iface)
+{
+	int dev_found = 0;
+	struct acpi_ipmi_device *ipmi_device;
+

Why don't you do

	struct acpi_ipmi_device *ipmi_device, *ret = NULL;

and then ->

drivers/acpi/acpi_ipmi.c static struct ipmi_driver_data driver_data = { · line 190

Quoted 3 lines

drivers/acpi/acpi_ipmi.c +3 -0

+	mutex_lock(&driver_data.ipmi_lock);
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->ipmi_ifnum == iface) {

-> ret = ipmi_device; ->

drivers/acpi/acpi_ipmi.c static struct ipmi_driver_data driver_data = { · line 198

Quoted 8 lines

drivers/acpi/acpi_ipmi.c +8 -0

+			dev_found = 1;
+			acpi_ipmi_dev_get(ipmi_device);
+			break;
+		}
+	}
+	mutex_unlock(&driver_data.ipmi_lock);
+
+	return dev_found ? ipmi_device : NULL;

-> return ret;

Quoted 28 lines

drivers/acpi/acpi_ipmi.c +2 -0

+}
+
 static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct
 acpi_ipmi_device *ipmi)  {
 	struct acpi_ipmi_msg *ipmi_msg;

@@ -228,25 +304,24 @@ static void acpi_format_ipmi_response(struct

acpi_ipmi_msg *msg, static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi) {

drivers/acpi/acpi_ipmi.c +2 -14

 	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--) {
-		if (list_empty(&ipmi->tx_msg_list))
-			break;
-		schedule_timeout(1);
+	/*
+	 * NOTE: Synchronous Flushing

drivers/acpi/acpi_ipmi.c static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg, · line 315

Quoted 5 lines

drivers/acpi/acpi_ipmi.c +5 -0

+	 * Wait until refnct dropping to 1 - no other users unless this
+	 * context.  This function should always be called before
+	 * acpi_ipmi_device destruction.
+	 */
+	while (atomic_read(&ipmi->refcnt) > 1) {

Isn't this racy? What if we see that the refcount is 1 and break the loop, but someone else bumps up the refcount at the same time?

Quoted 40 lines

drivers/acpi/acpi_ipmi.c +8 -2

+		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);
+		schedule_timeout_uninterruptible(msecs_to_jiffies(1));
 	}
-	if (!list_empty(&ipmi->tx_msg_list))
-		dev_warn(&pnp_dev->dev, "tx msg list is not NULL\n");
 }
 
 static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void

*user_msg_data) @@ -304,22 +379,26 @@ static void ipmi_register_bmc(int iface, struct device *dev) {

drivers/acpi/acpi_ipmi.c +5 -7

 	struct acpi_ipmi_device *ipmi_device, *temp;
 	struct pnp_dev *pnp_dev;
-	ipmi_user_t		user;
 	int err;
 	struct ipmi_smi_info smi_data;
 	acpi_handle handle;
 
 	err = ipmi_get_smi_info(iface, &smi_data);
-
 	if (err)
 		return;
 
-	if (smi_data.addr_src != SI_ACPI) {
-		put_device(smi_data.dev);
-		return;
-	}
-
+	if (smi_data.addr_src != SI_ACPI)
+		goto err_ref;
 	handle = smi_data.addr_info.acpi_info.acpi_handle;
+	if (!handle)
+		goto err_ref;
+	pnp_dev = to_pnp_dev(smi_data.dev);

drivers/acpi/acpi_ipmi.c static void ipmi_register_bmc(int iface, struct device *dev)

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +6 -1

+
+	ipmi_device = ipmi_dev_alloc(iface, &smi_data, handle);
+	if (!ipmi_device) {
+		dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
+		goto err_ref;
+	}
 
 	mutex_lock(&driver_data.ipmi_lock);
 	list_for_each_entry(temp, &driver_data.ipmi_devices, head) { @@
-328,54 +407,42 @@ static void ipmi_register_bmc(int iface, struct device

*dev)

Quoted 19 lines

drivers/acpi/acpi_ipmi.c +1 -13

 		 * to the device list, don't add it again.
 		 */
 		if (temp->handle == handle)
-			goto out;
+			goto err_lock;
 	}
 
-	ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
-
-	if (!ipmi_device)
-		goto out;
-
-	pnp_dev = to_pnp_dev(smi_data.dev);
-	ipmi_device->handle = handle;
-	ipmi_device->pnp_dev = pnp_dev;
-
-	err = ipmi_create_user(iface, &driver_data.ipmi_hndlrs,
-					ipmi_device, &user);
-	if (err) {

drivers/acpi/acpi_ipmi.c static void ipmi_register_bmc(int iface, struct device *dev)

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +1 -8

-		dev_warn(&pnp_dev->dev, "Can't create IPMI user interface\n");
-		kfree(ipmi_device);
-		goto out;
-	}
-	acpi_add_ipmi_device(ipmi_device);
-	ipmi_device->user_interface = user;
-	ipmi_device->ipmi_ifnum = iface;
+	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
 	mutex_unlock(&driver_data.ipmi_lock);
-	memcpy(&ipmi_device->smi_data, &smi_data, sizeof(struct

ipmi_smi_info));

Quoted 18 lines

drivers/acpi/acpi_ipmi.c +6 -2

+	put_device(smi_data.dev);
 	return;
 
-out:
+err_lock:
 	mutex_unlock(&driver_data.ipmi_lock);
+	ipmi_dev_release(ipmi_device);
+err_ref:
 	put_device(smi_data.dev);
 	return;
 }
 
 static void ipmi_bmc_gone(int iface)
 {
-	struct acpi_ipmi_device *ipmi_device, *temp;
+	int dev_found = 0;
+	struct acpi_ipmi_device *ipmi_device;

drivers/acpi/acpi_ipmi.c static void ipmi_register_bmc(int iface, struct device *dev) · line 434

Quoted 13 lines

drivers/acpi/acpi_ipmi.c +3 -9

 	mutex_lock(&driver_data.ipmi_lock);
-	list_for_each_entry_safe(ipmi_device, temp,
-				&driver_data.ipmi_devices, head) {
-		if (ipmi_device->ipmi_ifnum != iface)
-			continue;
-
-		acpi_remove_ipmi_device(ipmi_device);
-		put_device(ipmi_device->smi_data.dev);
-		kfree(ipmi_device);
-		break;
+	list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+		if (ipmi_device->ipmi_ifnum == iface) {
+			dev_found = 1;

You can do the list_del() here, because you're under the mutex, so others won't see the list in an inconsistens state and you're about to break anyway.

drivers/acpi/acpi_ipmi.c acpi_ipmi_space_handler(u32 function,

Quoted 27 lines

drivers/acpi/acpi_ipmi.c +9 -2

+			break;
+		}
 	}
+	if (dev_found)
+		list_del(&ipmi_device->head);
 	mutex_unlock(&driver_data.ipmi_lock);
+
+	if (dev_found) {
+		ipmi_flush_tx_msg(ipmi_device);
+		acpi_ipmi_dev_put(ipmi_device);
+	}
 }
 
 /*
----------------------------------------------------------------------
---- @@ -400,7 +467,8 @@ acpi_ipmi_space_handler(u32 function,

acpi_physical_address address,

drivers/acpi/acpi_ipmi.c +2 -1

 			void *handler_context, void *region_context)  {
 	struct acpi_ipmi_msg *tx_msg;
-	struct acpi_ipmi_device *ipmi_device = handler_context;
+	int iface = (long)handler_context;
+	struct acpi_ipmi_device *ipmi_device;
 	int err, rem_time;
 	acpi_status status;
 	unsigned long flags;

@@ -414,12 +482,15 @@ acpi_ipmi_space_handler(u32 function,

acpi_physical_address address,

drivers/acpi/acpi_ipmi.c static int ipmi_install_space_handler(struct

Quoted 28 lines

drivers/acpi/acpi_ipmi.c +8 -3

 	if ((function & ACPI_IO_MASK) == ACPI_READ)
 		return AE_TYPE;
 
-	if (!ipmi_device->user_interface)
+	ipmi_device = acpi_ipmi_get_targeted_smi(iface);
+	if (!ipmi_device)
 		return AE_NOT_EXIST;
 
 	tx_msg = acpi_alloc_ipmi_msg(ipmi_device);
-	if (!tx_msg)
-		return AE_NO_MEMORY;
+	if (!tx_msg) {
+		status = AE_NO_MEMORY;
+		goto out_ref;
+	}
 
 	if (acpi_format_ipmi_request(tx_msg, address, value) != 0) {
 		status = AE_TYPE;

@@ -449,6 +520,8 @@ out_list:
 	spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
 out_msg:
 	kfree(tx_msg);
+out_ref:
+	acpi_ipmi_dev_put(ipmi_device);
 	return status;
 }
 

@@ -473,7 +546,7 @@ static int ipmi_install_space_handler(struct

acpi_ipmi_device *ipmi)

drivers/acpi/acpi_ipmi.c static int ipmi_install_space_handler(struct

Quoted 9 lines

drivers/acpi/acpi_ipmi.c +1 -1

 	status = acpi_install_address_space_handler(ipmi->handle,
 						    ACPI_ADR_SPACE_IPMI,
 						    &acpi_ipmi_space_handler,
-						    NULL, ipmi);
+						    NULL, (void *)((long)ipmi->ipmi_ifnum));
 	if (ACPI_FAILURE(status)) {
 		struct pnp_dev *pnp_dev = ipmi->pnp_dev;
 		dev_warn(&pnp_dev->dev, "Can't register IPMI opregion space "

@@ -484,36 +557,6 @@ static int ipmi_install_space_handler(struct

acpi_ipmi_device *ipmi)

Quoted 57 lines

drivers/acpi/acpi_ipmi.c +5 -34

 	return 0;
 }
 
-static void acpi_add_ipmi_device(struct acpi_ipmi_device
 *ipmi_device) -{
-
-	INIT_LIST_HEAD(&ipmi_device->head);
-
-	spin_lock_init(&ipmi_device->tx_msg_lock);
-	INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
-	ipmi_install_space_handler(ipmi_device);
-
-	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
-}
-
-static void acpi_remove_ipmi_device(struct acpi_ipmi_device
 *ipmi_device) -{
-	/*
-	 * If the IPMI user interface is created, it should be
-	 * destroyed.
-	 */
-	if (ipmi_device->user_interface) {
-		ipmi_destroy_user(ipmi_device->user_interface);
-		ipmi_device->user_interface = NULL;
-	}
-	/* flush the Tx_msg list */
-	if (!list_empty(&ipmi_device->tx_msg_list))
-		ipmi_flush_tx_msg(ipmi_device);
-
-	list_del(&ipmi_device->head);
-	ipmi_remove_space_handler(ipmi_device);
-}
-
 static int __init acpi_ipmi_init(void)  {
 	int result = 0;

@@ -530,7 +573,7 @@ static int __init acpi_ipmi_init(void)
 
 static void __exit acpi_ipmi_exit(void)  {
-	struct acpi_ipmi_device *ipmi_device, *temp;
+	struct acpi_ipmi_device *ipmi_device;
 
 	if (acpi_disabled)
 		return;

@@ -544,11 +587,17 @@ static void __exit acpi_ipmi_exit(void)
 	 * handler and free it.
 	 */
 	mutex_lock(&driver_data.ipmi_lock);
-	list_for_each_entry_safe(ipmi_device, temp,
-				&driver_data.ipmi_devices, head) {
-		acpi_remove_ipmi_device(ipmi_device);
-		put_device(ipmi_device->smi_data.dev);
-		kfree(ipmi_device);
+	while (!list_empty(&driver_data.ipmi_devices)) {
+		ipmi_device = list_first_entry(&driver_data.ipmi_devices,
+					       struct acpi_ipmi_device,
+					       head);

drivers/acpi/acpi_ipmi.c static void __exit acpi_ipmi_exit(void) · line 603

Quoted 10 lines

drivers/acpi/acpi_ipmi.c +7 -0

+		list_del(&ipmi_device->head);
+		mutex_unlock(&driver_data.ipmi_lock);
+
+		ipmi_flush_tx_msg(ipmi_device);
+		acpi_ipmi_dev_put(ipmi_device);
+
+		mutex_lock(&driver_data.ipmi_lock);
 	}
 	mutex_unlock(&driver_data.ipmi_lock);
 }

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

This patch adds reference couting for ACPI operation region handlers to fix races caused by the ACPICA address space callback invocations.

ACPICA address space callback invocation is not suitable for Linux CONFIG_MODULE=y execution environment. This patch tries to protect the address space callbacks by invoking them under a module safe environment. The IPMI address space handler is also upgraded in this patch. The acpi_unregister_region() is designed to meet the following requirements:
1. It acts as a barrier for operation region callbacks - no callback will happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle->exit() functions.
Using reference counting rather than module referencing allows such benefits to be achieved even when acpi_unregister_region() is called in the environments other than module->exit().
The header file of include/acpi/acpi_bus.h should contain the declarations that have references to some ACPICA defined types.

Signed-off-by Lv Zheng <[email protected]>

Reviewed-by Huang Ying <[email protected]>

---
 drivers/acpi/acpi_ipmi.c |   16 ++--
 drivers/acpi/osl.c       |  224 ++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h  |    5 ++
 3 files changed, 235 insertions(+), 10 deletions(-)

drivers/acpi/acpi_ipmi.c +6 -10

index 5f8f495..2a09156 100644
@@ -539,20 +539,18 @@ out_ref:
 static int __init acpi_ipmi_init(void)
 {
 	int result = 0;
-	acpi_status status;
 
 	if (acpi_disabled)
 		return result;
 
 	mutex_init(&driver_data.ipmi_lock);
 
-	status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
-						    ACPI_ADR_SPACE_IPMI,
-						    &acpi_ipmi_space_handler,
-						    NULL, NULL);
-	if (ACPI_FAILURE(status)) {
+	result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
+				      &acpi_ipmi_space_handler,
+				      NULL, NULL);
+	if (result) {
 		pr_warn("Can't register IPMI opregion space handle\n");
-		return -EINVAL;
+		return result;
 	}
 
 	result = ipmi_smi_watcher_register(&driver_data.bmc_events);

@@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
 	}
 	mutex_unlock(&driver_data.ipmi_lock);
 
-	acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
-					  ACPI_ADR_SPACE_IPMI,
-					  &acpi_ipmi_space_handler);
+	acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
 }
 
 module_init(acpi_ipmi_init);

drivers/acpi/osl.c +0 -0

index 6ab2c35..8398e51 100644

drivers/acpi/osl.c +52 -0

+};
+
+static struct acpi_region acpi_regions[ACPI_NUM_PREDEFINED_REGIONS] = {
+	[ACPI_ADR_SPACE_SYSTEM_MEMORY] = {
+		.flags = ACPI_REGION_DEFAULT,
+	},
+	[ACPI_ADR_SPACE_SYSTEM_IO] = {
+		.flags = ACPI_REGION_DEFAULT,
+	},
+	[ACPI_ADR_SPACE_PCI_CONFIG] = {
+		.flags = ACPI_REGION_DEFAULT,
+	},
+	[ACPI_ADR_SPACE_IPMI] = {
+		.flags = ACPI_REGION_MANAGED,
+	},
+};
+static DEFINE_MUTEX(acpi_mutex_region);
+
 /*
  * This list of permanent mappings is for memory that may be accessed from
  * interrupt context, where we can't do the ioremap().

@@ -1799,3 +1835,191 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
 		kfree(hp_work);
 }
 EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
+
+static bool acpi_region_managed(struct acpi_region *rgn)
+{
+	/*
+	 * NOTE: Default and Managed
+	 * We only need to avoid region management on the regions managed
+	 * by ACPICA (ACPI_REGION_DEFAULT).  Currently, we need additional
+	 * check as many operation region handlers are not upgraded, so
+	 * only those known to be safe are managed (ACPI_REGION_MANAGED).
+	 */
+	return !(rgn->flags & ACPI_REGION_DEFAULT) &&
+	       (rgn->flags & ACPI_REGION_MANAGED);
+}
+
+static bool acpi_region_callable(struct acpi_region *rgn)
+{
+	return (rgn->flags & ACPI_REGION_REGISTERED) &&
+	       !(rgn->flags & ACPI_REGION_UNREGISTERING);
+}
+
+static acpi_status
+acpi_region_default_handler(u32 function,
+			    acpi_physical_address address,
+			    u32 bit_width, u64 *value,
+			    void *handler_context, void *region_context)
+{
+	acpi_adr_space_handler handler;
+	struct acpi_region *rgn = (struct acpi_region *)handler_context;
+	void *context;
+	acpi_status status = AE_NOT_EXIST;
+
+	mutex_lock(&acpi_mutex_region);
+	if (!acpi_region_callable(rgn) || !rgn->handler) {
+		mutex_unlock(&acpi_mutex_region);