* [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
@ 2013-07-23 8:08 ` Lv Zheng
2013-07-23 14:54 ` Greg KH
2013-07-23 8:09 ` [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler() Lv Zheng
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:08 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
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 <lv.zheng@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/acpi_ipmi.c | 100 ++++++++++++++++++++++++++++------------------
1 file changed, 61 insertions(+), 39 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index f40acef..28e2b4c 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -51,6 +51,7 @@ MODULE_LICENSE("GPL");
#define ACPI_IPMI_UNKNOWN 0x07
/* the IPMI timeout is 5s */
#define IPMI_TIMEOUT (5 * HZ)
+#define ACPI_IPMI_MAX_MSG_LENGTH 64
struct acpi_ipmi_device {
/* the device list attached to driver_data.ipmi_devices */
@@ -89,11 +90,9 @@ struct acpi_ipmi_msg {
struct completion tx_complete;
struct kernel_ipmi_msg tx_message;
int msg_done;
- /* tx data . And copy it from ACPI object buffer */
- u8 tx_data[64];
- int tx_len;
- u8 rx_data[64];
- int rx_len;
+ /* tx/rx data . And copy it from/to ACPI object buffer */
+ u8 data[ACPI_IPMI_MAX_MSG_LENGTH];
+ u8 rx_len;
struct acpi_ipmi_device *device;
};
@@ -101,7 +100,7 @@ struct acpi_ipmi_msg {
struct acpi_ipmi_buffer {
u8 status;
u8 length;
- u8 data[64];
+ u8 data[ACPI_IPMI_MAX_MSG_LENGTH];
};
static void ipmi_register_bmc(int iface, struct device *dev);
@@ -140,9 +139,9 @@ static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
#define IPMI_OP_RGN_NETFN(offset) ((offset >> 8) & 0xff)
#define IPMI_OP_RGN_CMD(offset) (offset & 0xff)
-static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
- acpi_physical_address address,
- acpi_integer *value)
+static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
+ acpi_physical_address address,
+ acpi_integer *value)
{
struct kernel_ipmi_msg *msg;
struct acpi_ipmi_buffer *buffer;
@@ -155,15 +154,21 @@ static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
*/
msg->netfn = IPMI_OP_RGN_NETFN(address);
msg->cmd = IPMI_OP_RGN_CMD(address);
- msg->data = tx_msg->tx_data;
+ msg->data = tx_msg->data;
/*
* value is the parameter passed by the IPMI opregion space handler.
* It points to the IPMI request message buffer
*/
buffer = (struct acpi_ipmi_buffer *)value;
/* copy the tx message data */
+ if (buffer->length > ACPI_IPMI_MAX_MSG_LENGTH) {
+ dev_WARN_ONCE(&tx_msg->device->pnp_dev->dev, true,
+ "Unexpected request (msg len %d).\n",
+ buffer->length);
+ return -EINVAL;
+ }
msg->data_len = buffer->length;
- memcpy(tx_msg->tx_data, buffer->data, msg->data_len);
+ memcpy(tx_msg->data, buffer->data, msg->data_len);
/*
* now the default type is SYSTEM_INTERFACE and channel type is BMC.
* If the netfn is APP_REQUEST and the cmd is SEND_MESSAGE,
@@ -181,10 +186,12 @@ static void acpi_format_ipmi_msg(struct acpi_ipmi_msg *tx_msg,
device->curr_msgid++;
tx_msg->tx_msgid = device->curr_msgid;
mutex_unlock(&device->tx_msg_lock);
+
+ return 0;
}
static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
- acpi_integer *value, int rem_time)
+ acpi_integer *value, int rem_time)
{
struct acpi_ipmi_buffer *buffer;
@@ -206,13 +213,14 @@ static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
buffer->status = ACPI_IPMI_UNKNOWN;
return;
}
+
/*
* If the IPMI response message is obtained correctly, the status code
* will be ACPI_IPMI_OK
*/
buffer->status = ACPI_IPMI_OK;
buffer->length = msg->rx_len;
- memcpy(buffer->data, msg->rx_data, msg->rx_len);
+ memcpy(buffer->data, msg->data, msg->rx_len);
}
static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi)
@@ -244,12 +252,12 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
if (msg->user != ipmi_device->user_interface) {
- dev_warn(&pnp_dev->dev, "Unexpected response is returned. "
- "returned user %p, expected user %p\n",
- msg->user, ipmi_device->user_interface);
- ipmi_free_recv_msg(msg);
- return;
+ dev_warn(&pnp_dev->dev,
+ "Unexpected response is returned. returned user %p, expected user %p\n",
+ msg->user, ipmi_device->user_interface);
+ goto out_msg;
}
+
mutex_lock(&ipmi_device->tx_msg_lock);
list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
if (msg->msgid == tx_msg->tx_msgid) {
@@ -257,24 +265,31 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
break;
}
}
-
mutex_unlock(&ipmi_device->tx_msg_lock);
+
if (!msg_found) {
- dev_warn(&pnp_dev->dev, "Unexpected response (msg id %ld) is "
- "returned.\n", msg->msgid);
- ipmi_free_recv_msg(msg);
- return;
+ dev_warn(&pnp_dev->dev,
+ "Unexpected response (msg id %ld) is returned.\n",
+ msg->msgid);
+ goto out_msg;
}
- if (msg->msg.data_len) {
- /* copy the response data to Rx_data buffer */
- memcpy(tx_msg->rx_data, msg->msg_data, msg->msg.data_len);
- tx_msg->rx_len = msg->msg.data_len;
- tx_msg->msg_done = 1;
+ /* copy the response data to Rx_data buffer */
+ if (msg->msg.data_len > ACPI_IPMI_MAX_MSG_LENGTH) {
+ dev_WARN_ONCE(&pnp_dev->dev, true,
+ "Unexpected response (msg len %d).\n",
+ msg->msg.data_len);
+ goto out_comp;
}
+ tx_msg->rx_len = msg->msg.data_len;
+ memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);
+ tx_msg->msg_done = 1;
+
+out_comp:
complete(&tx_msg->tx_complete);
+out_msg:
ipmi_free_recv_msg(msg);
-};
+}
static void ipmi_register_bmc(int iface, struct device *dev)
{
@@ -353,6 +368,7 @@ static void ipmi_bmc_gone(int iface)
}
mutex_unlock(&driver_data.ipmi_lock);
}
+
/* --------------------------------------------------------------------------
* Address Space Management
* -------------------------------------------------------------------------- */
@@ -371,13 +387,14 @@ static void ipmi_bmc_gone(int iface)
static acpi_status
acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
- u32 bits, acpi_integer *value,
- void *handler_context, void *region_context)
+ u32 bits, acpi_integer *value,
+ void *handler_context, void *region_context)
{
struct acpi_ipmi_msg *tx_msg;
struct acpi_ipmi_device *ipmi_device = handler_context;
int err, rem_time;
acpi_status status;
+
/*
* IPMI opregion message.
* IPMI message is firstly written to the BMC and system software
@@ -394,28 +411,33 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
if (!tx_msg)
return AE_NO_MEMORY;
- acpi_format_ipmi_msg(tx_msg, address, value);
+ if (acpi_format_ipmi_request(tx_msg, address, value) != 0) {
+ status = AE_TYPE;
+ goto out_msg;
+ }
+
mutex_lock(&ipmi_device->tx_msg_lock);
list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
mutex_unlock(&ipmi_device->tx_msg_lock);
err = ipmi_request_settime(ipmi_device->user_interface,
- &tx_msg->addr,
- tx_msg->tx_msgid,
- &tx_msg->tx_message,
- NULL, 0, 0, 0);
+ &tx_msg->addr,
+ tx_msg->tx_msgid,
+ &tx_msg->tx_message,
+ NULL, 0, 0, 0);
if (err) {
status = AE_ERROR;
- goto end_label;
+ goto out_list;
}
rem_time = wait_for_completion_timeout(&tx_msg->tx_complete,
- IPMI_TIMEOUT);
+ IPMI_TIMEOUT);
acpi_format_ipmi_response(tx_msg, value, rem_time);
status = AE_OK;
-end_label:
+out_list:
mutex_lock(&ipmi_device->tx_msg_lock);
list_del(&tx_msg->head);
mutex_unlock(&ipmi_device->tx_msg_lock);
+out_msg:
kfree(tx_msg);
return status;
}
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow
2013-07-23 8:08 ` [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow Lv Zheng
@ 2013-07-23 14:54 ` Greg KH
2013-07-24 0:21 ` Zheng, Lv
2013-07-24 0:44 ` Zheng, Lv
0 siblings, 2 replies; 18+ messages in thread
From: Greg KH @ 2013-07-23 14:54 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui,
linux-kernel, stable, linux-acpi, openipmi-developer
On Tue, Jul 23, 2013 at 04:08:59PM +0800, Lv Zheng wrote:
> 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 <lv.zheng@intel.com>
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> ---
> 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...
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow
2013-07-23 14:54 ` Greg KH
@ 2013-07-24 0:21 ` Zheng, Lv
2013-07-24 0:44 ` Zheng, Lv
1 sibling, 0 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-24 0:21 UTC (permalink / raw)
To: Greg KH
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> From: linux-acpi-owner@vger.kernel.org
> [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Greg KH
> Sent: Tuesday, July 23, 2013 10:54 PM
>
> On Tue, Jul 23, 2013 at 04:08:59PM +0800, Lv Zheng wrote:
> > 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 <lv.zheng@intel.com>
> > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > ---
> > 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...
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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow
2013-07-23 14:54 ` Greg KH
2013-07-24 0:21 ` Zheng, Lv
@ 2013-07-24 0:44 ` Zheng, Lv
1 sibling, 0 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-24 0:44 UTC (permalink / raw)
To: Greg KH
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> From: Zheng, Lv
> Sent: Wednesday, July 24, 2013 8:22 AM
>
> > From: linux-acpi-owner@vger.kernel.org
> > [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Greg KH
> > Sent: Tuesday, July 23, 2013 10:54 PM
> >
> > On Tue, Jul 23, 2013 at 04:08:59PM +0800, Lv Zheng wrote:
> > > 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 <lv.zheng@intel.com>
> > > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > > ---
> > > 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...
>
> OK, I'll add prerequisites for each that want to be accepted by the stable queue
> and re-send them (PATCH 01-06).
Maybe I shouldn't.
I looks it is not possible to add commit ID prerequisites for patch series that has not been accepted by the mainline.
As the patches haven't been merged by the mainline, it is likely that the commit IDs in this series will be changed.
Please ignore [PATCH 01-06] that have been sent to the stable mailing list.
I'll just let ACPI maintainers know which patches I think that can go for stable tree and let they make the decision after the mainline acceptance.
Thanks and best regards
-Lv
>
> Thanks and best regards
> -Lv
>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler()
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
2013-07-23 8:08 ` [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow Lv Zheng
@ 2013-07-23 8:09 ` Lv Zheng
2013-07-23 8:09 ` [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers Lv Zheng
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:09 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
This patch quick fixes the issues indicated by the test results that
ipmi_msg_handler() is invoked in atomic context.
BUG: scheduling while atomic: kipmi0/18933/0x10000100
Modules linked in: ipmi_si acpi_ipmi ...
CPU: 3 PID: 18933 Comm: kipmi0 Tainted: G AW 3.10.0-rc7+ #2
Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS QSSC-S4R.QCI.01.00.0027.070120100606 07/01/2010
ffff8838245eea00 ffff88103fc63c98 ffffffff814c4a1e ffff88103fc63ca8
ffffffff814bfbab ffff88103fc63d28 ffffffff814c73e0 ffff88103933cbd4
0000000000000096 ffff88103fc63ce8 ffff88102f618000 ffff881035c01fd8
Call Trace:
<IRQ> [<ffffffff814c4a1e>] dump_stack+0x19/0x1b
[<ffffffff814bfbab>] __schedule_bug+0x46/0x54
[<ffffffff814c73e0>] __schedule+0x83/0x59c
[<ffffffff81058853>] __cond_resched+0x22/0x2d
[<ffffffff814c794b>] _cond_resched+0x14/0x1d
[<ffffffff814c6d82>] mutex_lock+0x11/0x32
[<ffffffff8101e1e9>] ? __default_send_IPI_dest_field.constprop.0+0x53/0x58
[<ffffffffa09e3f9c>] ipmi_msg_handler+0x23/0x166 [ipmi_si]
[<ffffffff812bf6e4>] deliver_response+0x55/0x5a
[<ffffffff812c0fd4>] handle_new_recv_msgs+0xb67/0xc65
[<ffffffff81007ad1>] ? read_tsc+0x9/0x19
[<ffffffff814c8620>] ? _raw_spin_lock_irq+0xa/0xc
[<ffffffffa09e1128>] ipmi_thread+0x5c/0x146 [ipmi_si]
...
Known issues:
- Replacing tx_msg_lock with spinlock is not performance friendly
Current solution works but does not have the best performance because it
is better to make atomic context run as fast as possible. Given there
are no many IPMI messages created by ACPI, performance of current
solution may be OK. It can be better via linking ipmi_recv_msg into an
RX message queue and process it in other contexts.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/acpi_ipmi.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index 28e2b4c..b37c189 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -39,6 +39,7 @@
#include <linux/ipmi.h>
#include <linux/device.h>
#include <linux/pnp.h>
+#include <linux/spinlock.h>
MODULE_AUTHOR("Zhao Yakui");
MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
@@ -58,7 +59,7 @@ struct acpi_ipmi_device {
struct list_head head;
/* the IPMI request message list */
struct list_head tx_msg_list;
- struct mutex tx_msg_lock;
+ spinlock_t tx_msg_lock;
acpi_handle handle;
struct pnp_dev *pnp_dev;
ipmi_user_t user_interface;
@@ -146,6 +147,7 @@ static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
struct kernel_ipmi_msg *msg;
struct acpi_ipmi_buffer *buffer;
struct acpi_ipmi_device *device;
+ unsigned long flags;
msg = &tx_msg->tx_message;
/*
@@ -182,10 +184,10 @@ static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
/* Get the msgid */
device = tx_msg->device;
- mutex_lock(&device->tx_msg_lock);
+ spin_lock_irqsave(&device->tx_msg_lock, flags);
device->curr_msgid++;
tx_msg->tx_msgid = device->curr_msgid;
- mutex_unlock(&device->tx_msg_lock);
+ spin_unlock_irqrestore(&device->tx_msg_lock, flags);
return 0;
}
@@ -250,6 +252,7 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
int msg_found = 0;
struct acpi_ipmi_msg *tx_msg;
struct pnp_dev *pnp_dev = ipmi_device->pnp_dev;
+ unsigned long flags;
if (msg->user != ipmi_device->user_interface) {
dev_warn(&pnp_dev->dev,
@@ -258,14 +261,14 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
goto out_msg;
}
- mutex_lock(&ipmi_device->tx_msg_lock);
+ spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
list_for_each_entry(tx_msg, &ipmi_device->tx_msg_list, head) {
if (msg->msgid == tx_msg->tx_msgid) {
msg_found = 1;
break;
}
}
- mutex_unlock(&ipmi_device->tx_msg_lock);
+ spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
if (!msg_found) {
dev_warn(&pnp_dev->dev,
@@ -394,6 +397,7 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
struct acpi_ipmi_device *ipmi_device = handler_context;
int err, rem_time;
acpi_status status;
+ unsigned long flags;
/*
* IPMI opregion message.
@@ -416,9 +420,9 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
goto out_msg;
}
- mutex_lock(&ipmi_device->tx_msg_lock);
+ spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
list_add_tail(&tx_msg->head, &ipmi_device->tx_msg_list);
- mutex_unlock(&ipmi_device->tx_msg_lock);
+ spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
err = ipmi_request_settime(ipmi_device->user_interface,
&tx_msg->addr,
tx_msg->tx_msgid,
@@ -434,9 +438,9 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
status = AE_OK;
out_list:
- mutex_lock(&ipmi_device->tx_msg_lock);
+ spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags);
list_del(&tx_msg->head);
- mutex_unlock(&ipmi_device->tx_msg_lock);
+ spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
out_msg:
kfree(tx_msg);
return status;
@@ -479,7 +483,7 @@ static void acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device)
INIT_LIST_HEAD(&ipmi_device->head);
- mutex_init(&ipmi_device->tx_msg_lock);
+ spin_lock_init(&ipmi_device->tx_msg_lock);
INIT_LIST_HEAD(&ipmi_device->tx_msg_list);
ipmi_install_space_handler(ipmi_device);
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
2013-07-23 8:08 ` [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow Lv Zheng
2013-07-23 8:09 ` [PATCH 02/13] ACPI/IPMI: Fix atomic context requirement of ipmi_msg_handler() Lv Zheng
@ 2013-07-23 8:09 ` Lv Zheng
2013-07-24 23:38 ` Rafael J. Wysocki
2013-07-23 8:09 ` [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user Lv Zheng
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:09 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
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@intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
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();
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);
}
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
2013-07-23 8:09 ` [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers Lv Zheng
@ 2013-07-24 23:38 ` Rafael J. Wysocki
0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2013-07-24 23:38 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui,
linux-kernel, stable, linux-acpi, openipmi-developer
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@intel.com>
> Cc: Zhao Yakui <yakui.zhao@intel.com>
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> ---
> 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?
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
` (2 preceding siblings ...)
2013-07-23 8:09 ` [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers Lv Zheng
@ 2013-07-23 8:09 ` Lv Zheng
2013-07-25 21:59 ` Rafael J. Wysocki
2013-07-23 8:09 ` [PATCH 05/13] ACPI/IPMI: Fix issue caused by the per-device registration of the IPMI operation region handler Lv Zheng
2013-07-23 8:09 ` [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers Lv Zheng
5 siblings, 1 reply; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:09 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
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 <lv.zheng@intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/acpi_ipmi.c | 249 +++++++++++++++++++++++++++-------------------
1 file changed, 149 insertions(+), 100 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index 527ee43..cbf25e0 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -1,8 +1,9 @@
/*
* acpi_ipmi.c - ACPI IPMI opregion
*
- * Copyright (C) 2010 Intel Corporation
- * Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
+ * Copyright (C) 2010, 2013 Intel Corporation
+ * Author: Zhao Yakui <yakui.zhao@intel.com>
+ * Lv Zheng <lv.zheng@intel.com>
*
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*
@@ -67,6 +68,7 @@ struct acpi_ipmi_device {
long curr_msgid;
unsigned long flags;
struct ipmi_smi_info smi_data;
+ atomic_t refcnt;
};
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)
+{
+ 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;
+
+ mutex_lock(&driver_data.ipmi_lock);
+ list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
+ if (ipmi_device->ipmi_ifnum == iface) {
+ dev_found = 1;
+ acpi_ipmi_dev_get(ipmi_device);
+ break;
+ }
+ }
+ mutex_unlock(&driver_data.ipmi_lock);
+
+ return dev_found ? ipmi_device : NULL;
+}
+
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--) {
- if (list_empty(&ipmi->tx_msg_list))
- break;
- schedule_timeout(1);
+ /*
+ * NOTE: Synchronous Flushing
+ * 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) {
+ 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) {
- 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;
+ 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);
}
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user
2013-07-23 8:09 ` [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user Lv Zheng
@ 2013-07-25 21:59 ` Rafael J. Wysocki
2013-07-26 1:17 ` Zheng, Lv
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2013-07-25 21:59 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui,
linux-kernel, stable, linux-acpi, openipmi-developer
On Tuesday, July 23, 2013 04:09:26 PM Lv Zheng wrote:
> 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 <lv.zheng@intel.com>
> Cc: Zhao Yakui <yakui.zhao@intel.com>
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> ---
> drivers/acpi/acpi_ipmi.c | 249 +++++++++++++++++++++++++++-------------------
> 1 file changed, 149 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> index 527ee43..cbf25e0 100644
> --- a/drivers/acpi/acpi_ipmi.c
> +++ b/drivers/acpi/acpi_ipmi.c
> @@ -1,8 +1,9 @@
> /*
> * acpi_ipmi.c - ACPI IPMI opregion
> *
> - * Copyright (C) 2010 Intel Corporation
> - * Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
> + * Copyright (C) 2010, 2013 Intel Corporation
> + * Author: Zhao Yakui <yakui.zhao@intel.com>
> + * Lv Zheng <lv.zheng@intel.com>
> *
> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> *
> @@ -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?
> };
>
> 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)
> +{
> + 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 ->
> + 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; ->
> + dev_found = 1;
> + acpi_ipmi_dev_get(ipmi_device);
> + break;
> + }
> + }
> + mutex_unlock(&driver_data.ipmi_lock);
> +
> + return dev_found ? ipmi_device : NULL;
-> return ret;
> +}
> +
> 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--) {
> - if (list_empty(&ipmi->tx_msg_list))
> - break;
> - schedule_timeout(1);
> + /*
> + * NOTE: Synchronous Flushing
> + * 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?
> + 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) {
> - 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.
> + 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);
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user
2013-07-25 21:59 ` Rafael J. Wysocki
@ 2013-07-26 1:17 ` Zheng, Lv
0 siblings, 0 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-26 1:17 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, July 26, 2013 5:59 AM
>
> On Tuesday, July 23, 2013 04:09:26 PM Lv Zheng wrote:
> > 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 <lv.zheng@intel.com>
> > Cc: Zhao Yakui <yakui.zhao@intel.com>
> > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > ---
> > drivers/acpi/acpi_ipmi.c | 249
> > +++++++++++++++++++++++++++-------------------
> > 1 file changed, 149 insertions(+), 100 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > 527ee43..cbf25e0 100644
> > --- a/drivers/acpi/acpi_ipmi.c
> > +++ b/drivers/acpi/acpi_ipmi.c
> > @@ -1,8 +1,9 @@
> > /*
> > * acpi_ipmi.c - ACPI IPMI opregion
> > *
> > - * Copyright (C) 2010 Intel Corporation
> > - * Copyright (C) 2010 Zhao Yakui <yakui.zhao@intel.com>
> > + * Copyright (C) 2010, 2013 Intel Corporation
> > + * Author: Zhao Yakui <yakui.zhao@intel.com>
> > + * Lv Zheng <lv.zheng@intel.com>
> > *
> > *
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~
> > *
> > @@ -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?
Please see my concerns in another email.
>
> > };
> >
> > 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) {
> > + 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 ->
>
> > + 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; ->
>
> > + dev_found = 1;
> > + acpi_ipmi_dev_get(ipmi_device);
> > + break;
> > + }
> > + }
> > + mutex_unlock(&driver_data.ipmi_lock);
> > +
> > + return dev_found ? ipmi_device : NULL;
>
> -> return ret;
OK.
>
> > +}
> > +
> > 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--) {
> > - if (list_empty(&ipmi->tx_msg_list))
> > - break;
> > - schedule_timeout(1);
> > + /*
> > + * NOTE: Synchronous Flushing
> > + * 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?
No, it's not racy.
Flushing codes here is invoked after acpi_ipmi_device disappearing from the object managers.
Please look at the ipmi_bmc_gone() and acpi_ipmi_exit().
The ipmi_flush_tx_msg() will only be called after a "list_del()".
There will be no new transfers created in the acpi_ipmi_space_handler() as acpi_ipmi_get_targeted_smi() will return NULL after the "list_del()".
So there are no chances that it reaches to 1 and go back again as the refcount will only increases from 1 to > 1 unless it is still in an object managers.
The trick here is to drop all of the object managers' reference and only hold the "call chain" reference here (thus it is 1) in the ipmi_bmc_gone() and acpi_ipmi_exit().
In case of this patch, the object reference count is converted into "call chain" reference count in the ipmi_bmc_gone() and acpi_ipmi_exit().
The waiting codes here then can wait the reference count dropping to 1 which indicates all on-going transfer references are also get dropped.
>
> > + 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) {
> > - 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.
I'm trying to improve the code maintainability (hence the software internal quality) here for the reviewers.
If we introduce a list_del()/break inside a list_for_each_entry(), then it is pretty much likely that the list_for_each_entry() does not appear in a future patch that deletes the "break".
And reviewers could not detect such bug.
The coding style like what I'm showing here can avoid such issue.
I was thinking maintainers would be happy with such codes - it can prevent many unhappy small mistakes from happening.
Thanks for commenting.
Best regards
-Lv
>
> > + 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);
> > }
> >
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 05/13] ACPI/IPMI: Fix issue caused by the per-device registration of the IPMI operation region handler
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
` (3 preceding siblings ...)
2013-07-23 8:09 ` [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user Lv Zheng
@ 2013-07-23 8:09 ` Lv Zheng
2013-07-23 8:09 ` [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers Lv Zheng
5 siblings, 0 replies; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:09 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
It is found on a real machine, in its ACPI namespace, the IPMI
OperationRegions (in the ACPI000D - ACPI power meter) are not defined under
the IPMI system interface device (the IPI0001 with KCS type returned from
_IFT control method):
Device (PMI0)
{
Name (_HID, "ACPI000D") // _HID: Hardware ID
OperationRegion (SYSI, IPMI, 0x0600, 0x0100)
Field (SYSI, BufferAcc, Lock, Preserve)
{
AccessAs (BufferAcc, 0x01),
Offset (0x58),
SCMD, 8,
GCMD, 8
}
OperationRegion (POWR, IPMI, 0x3000, 0x0100)
Field (POWR, BufferAcc, Lock, Preserve)
{
AccessAs (BufferAcc, 0x01),
Offset (0xB3),
GPMM, 8
}
}
Device (PCI0)
{
Device (ISA)
{
Device (NIPM)
{
Name (_HID, EisaId ("IPI0001")) // _HID: Hardware ID
Method (_IFT, 0, NotSerialized) // _IFT: IPMI Interface Type
{
Return (0x01)
}
}
}
}
Current ACPI_IPMI code registers IPMI operation region handler on a
per-device basis, so that for above namespace, the IPMI operation region
handler is registered only under the scope of \_SB.PCI0.ISA.NIPM. Thus
when an IPMI operation region field of \PMI0 is accessed, there are errors
reported on such platform:
ACPI Error: No handlers for Region [IPMI]
ACPI Error: Region IPMI(7) has no handler
The solution is to install IPMI operation region handler from root node so
that every object that defines IPMI OperationRegion can get an address
space handler registered.
When an IPMI operation region field is accessed, the Network Function
(0x06 for SYSI and 0x30 for POWR) and the Command (SCMD, GCMD, GPMM) are
passed to the operation region handler, there is no system interface
specified by the BIOS. The patch tries to select one system interface by
monitoring the system interface notification. IPMI messages passed from
the ACPI codes are sent to this selected global IPMI system interface.
Known issues:
- How to select the IPMI system interface:
Currently, the ACPI_IPMI always selects the first registered one with the
ACPI handle set (i.e., defined in the ACPI namespace). It's hard to
determine the selection when there are multiple IPMI system interfaces
defined in the ACPI namespace.
According to the IPMI specification:
A BMC device may make available multiple system interfaces, but only one
management controller is allowed to be 'active' BMC that provides BMC
functionality for the system (in case of a 'partitioned' system, there
can be only one active BMC per partition). Only the system interface(s)
for the active BMC allowed to respond to the 'Get Device Id' command.
According to the ipmi_si desigin:
The ipmi_si registeration notifications can only happen after a
successful "Get Device ID" command.
Thus it should be OK for non-partitioned systems to do such selection.
But we do not have too much knowledges on 'partitioned' systems.
- Lack of smi_gone()/new_smi() testability:
It is not possible to do module(ipmi_si) load/unload test, and I can't
find any multiple IPMI system interfaces platforms available for testing.
There might be issues in the untested code path.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=46741
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/acpi_ipmi.c | 111 +++++++++++++++++++++++-----------------------
1 file changed, 55 insertions(+), 56 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index cbf25e0..5f8f495 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -46,7 +46,8 @@ MODULE_AUTHOR("Zhao Yakui");
MODULE_DESCRIPTION("ACPI IPMI Opregion driver");
MODULE_LICENSE("GPL");
-#define IPMI_FLAGS_HANDLER_INSTALL 0
+#undef PREFIX
+#define PREFIX "ACPI: IPMI: "
#define ACPI_IPMI_OK 0
#define ACPI_IPMI_TIMEOUT 0x10
@@ -66,7 +67,6 @@ struct acpi_ipmi_device {
ipmi_user_t user_interface;
int ipmi_ifnum; /* IPMI interface number */
long curr_msgid;
- unsigned long flags;
struct ipmi_smi_info smi_data;
atomic_t refcnt;
};
@@ -76,6 +76,14 @@ struct ipmi_driver_data {
struct ipmi_smi_watcher bmc_events;
struct ipmi_user_hndl ipmi_hndlrs;
struct mutex ipmi_lock;
+ /*
+ * NOTE: IPMI System Interface Selection
+ * There is no system interface specified by the IPMI operation
+ * region access. We try to select one system interface with ACPI
+ * handle set. IPMI messages passed from the ACPI codes are sent
+ * to this selected global IPMI system interface.
+ */
+ struct acpi_ipmi_device *selected_smi;
};
struct acpi_ipmi_msg {
@@ -109,8 +117,6 @@ 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 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),
@@ -153,7 +159,6 @@ ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle handle)
return NULL;
}
ipmi_device->user_interface = user;
- ipmi_install_space_handler(ipmi_device);
return ipmi_device;
}
@@ -168,7 +173,6 @@ acpi_ipmi_dev_get(struct acpi_ipmi_device *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);
@@ -180,22 +184,15 @@ static void acpi_ipmi_dev_put(struct acpi_ipmi_device *ipmi_device)
ipmi_dev_release(ipmi_device);
}
-static struct acpi_ipmi_device *acpi_ipmi_get_targeted_smi(int iface)
+static struct acpi_ipmi_device *acpi_ipmi_get_selected_smi(void)
{
- int dev_found = 0;
struct acpi_ipmi_device *ipmi_device;
mutex_lock(&driver_data.ipmi_lock);
- list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
- if (ipmi_device->ipmi_ifnum == iface) {
- dev_found = 1;
- acpi_ipmi_dev_get(ipmi_device);
- break;
- }
- }
+ ipmi_device = acpi_ipmi_dev_get(driver_data.selected_smi);
mutex_unlock(&driver_data.ipmi_lock);
- return dev_found ? ipmi_device : NULL;
+ return ipmi_device;
}
static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device *ipmi)
@@ -410,6 +407,9 @@ static void ipmi_register_bmc(int iface, struct device *dev)
goto err_lock;
}
+ if (!driver_data.selected_smi)
+ driver_data.selected_smi = acpi_ipmi_dev_get(ipmi_device);
+
list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
mutex_unlock(&driver_data.ipmi_lock);
put_device(smi_data.dev);
@@ -426,22 +426,34 @@ err_ref:
static void ipmi_bmc_gone(int iface)
{
int dev_found = 0;
- struct acpi_ipmi_device *ipmi_device;
+ struct acpi_ipmi_device *ipmi_gone, *ipmi_new;
mutex_lock(&driver_data.ipmi_lock);
- list_for_each_entry(ipmi_device, &driver_data.ipmi_devices, head) {
- if (ipmi_device->ipmi_ifnum == iface) {
+ list_for_each_entry(ipmi_gone, &driver_data.ipmi_devices, head) {
+ if (ipmi_gone->ipmi_ifnum == iface) {
dev_found = 1;
break;
}
}
- if (dev_found)
- list_del(&ipmi_device->head);
+ if (dev_found) {
+ list_del(&ipmi_gone->head);
+ if (driver_data.selected_smi == ipmi_gone) {
+ acpi_ipmi_dev_put(ipmi_gone);
+ driver_data.selected_smi = NULL;
+ }
+ }
+ if (!driver_data.selected_smi &&
+ !list_empty(&driver_data.ipmi_devices)) {
+ ipmi_new = list_first_entry(&driver_data.ipmi_devices,
+ struct acpi_ipmi_device,
+ head);
+ driver_data.selected_smi = acpi_ipmi_dev_get(ipmi_new);
+ }
mutex_unlock(&driver_data.ipmi_lock);
if (dev_found) {
- ipmi_flush_tx_msg(ipmi_device);
- acpi_ipmi_dev_put(ipmi_device);
+ ipmi_flush_tx_msg(ipmi_gone);
+ acpi_ipmi_dev_put(ipmi_gone);
}
}
@@ -467,7 +479,6 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
void *handler_context, void *region_context)
{
struct acpi_ipmi_msg *tx_msg;
- int iface = (long)handler_context;
struct acpi_ipmi_device *ipmi_device;
int err, rem_time;
acpi_status status;
@@ -482,7 +493,7 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address,
if ((function & ACPI_IO_MASK) == ACPI_READ)
return AE_TYPE;
- ipmi_device = acpi_ipmi_get_targeted_smi(iface);
+ ipmi_device = acpi_ipmi_get_selected_smi();
if (!ipmi_device)
return AE_NOT_EXIST;
@@ -525,48 +536,28 @@ out_ref:
return status;
}
-static void ipmi_remove_space_handler(struct acpi_ipmi_device *ipmi)
-{
- if (!test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
- return;
-
- acpi_remove_address_space_handler(ipmi->handle,
- ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler);
-
- clear_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
-}
-
-static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi)
+static int __init acpi_ipmi_init(void)
{
+ int result = 0;
acpi_status status;
- if (test_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags))
- return 0;
+ if (acpi_disabled)
+ return result;
- status = acpi_install_address_space_handler(ipmi->handle,
+ mutex_init(&driver_data.ipmi_lock);
+
+ status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
ACPI_ADR_SPACE_IPMI,
&acpi_ipmi_space_handler,
- NULL, (void *)((long)ipmi->ipmi_ifnum));
+ NULL, NULL);
if (ACPI_FAILURE(status)) {
- struct pnp_dev *pnp_dev = ipmi->pnp_dev;
- dev_warn(&pnp_dev->dev, "Can't register IPMI opregion space "
- "handle\n");
+ pr_warn("Can't register IPMI opregion space handle\n");
return -EINVAL;
}
- set_bit(IPMI_FLAGS_HANDLER_INSTALL, &ipmi->flags);
- return 0;
-}
-
-static int __init acpi_ipmi_init(void)
-{
- int result = 0;
-
- if (acpi_disabled)
- return result;
-
- mutex_init(&driver_data.ipmi_lock);
result = ipmi_smi_watcher_register(&driver_data.bmc_events);
+ if (result)
+ pr_err("Can't register IPMI system interface watcher\n");
return result;
}
@@ -592,6 +583,10 @@ static void __exit acpi_ipmi_exit(void)
struct acpi_ipmi_device,
head);
list_del(&ipmi_device->head);
+ if (ipmi_device == driver_data.selected_smi) {
+ acpi_ipmi_dev_put(driver_data.selected_smi);
+ driver_data.selected_smi = NULL;
+ }
mutex_unlock(&driver_data.ipmi_lock);
ipmi_flush_tx_msg(ipmi_device);
@@ -600,6 +595,10 @@ static void __exit acpi_ipmi_exit(void)
mutex_lock(&driver_data.ipmi_lock);
}
mutex_unlock(&driver_data.ipmi_lock);
+
+ acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
+ ACPI_ADR_SPACE_IPMI,
+ &acpi_ipmi_space_handler);
}
module_init(acpi_ipmi_init);
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-23 8:08 ` [PATCH 00/13] ACPI/IPMI: Fix several issues in the current codes Lv Zheng
` (4 preceding siblings ...)
2013-07-23 8:09 ` [PATCH 05/13] ACPI/IPMI: Fix issue caused by the per-device registration of the IPMI operation region handler Lv Zheng
@ 2013-07-23 8:09 ` Lv Zheng
2013-07-25 20:27 ` Rafael J. Wysocki
5 siblings, 1 reply; 18+ messages in thread
From: Lv Zheng @ 2013-07-23 8:09 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui
Cc: Lv Zheng, linux-kernel, stable, linux-acpi, openipmi-developer
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 <lv.zheng@intel.com>
Reviewed-by: Huang Ying <ying.huang@intel.com>
---
drivers/acpi/acpi_ipmi.c | 16 ++--
drivers/acpi/osl.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 5 ++
3 files changed, 235 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index 5f8f495..2a09156 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -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);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..8398e51 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
static struct workqueue_struct *kacpi_notify_wq;
static struct workqueue_struct *kacpi_hotplug_wq;
+struct acpi_region {
+ unsigned long flags;
+#define ACPI_REGION_DEFAULT 0x01
+#define ACPI_REGION_INSTALLED 0x02
+#define ACPI_REGION_REGISTERED 0x04
+#define ACPI_REGION_UNREGISTERING 0x08
+#define ACPI_REGION_INSTALLING 0x10
+ /*
+ * NOTE: Upgrading All Region Handlers
+ * This flag is only used during the period where not all of the
+ * region handers are upgraded to the new interfaces.
+ */
+#define ACPI_REGION_MANAGED 0x80
+ acpi_adr_space_handler handler;
+ acpi_adr_space_setup setup;
+ void *context;
+ /* Invoking references */
+ atomic_t refcnt;
+};
+
+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);
+ return status;
+ }
+
+ atomic_inc(&rgn->refcnt);
+ handler = rgn->handler;
+ context = rgn->context;
+ mutex_unlock(&acpi_mutex_region);
+
+ status = handler(function, address, bit_width, value, context,
+ region_context);
+ atomic_dec(&rgn->refcnt);
+
+ return status;
+}
+
+static acpi_status
+acpi_region_default_setup(acpi_handle handle, u32 function,
+ void *handler_context, void **region_context)
+{
+ acpi_adr_space_setup setup;
+ struct acpi_region *rgn = (struct acpi_region *)handler_context;
+ void *context;
+ acpi_status status = AE_OK;
+
+ mutex_lock(&acpi_mutex_region);
+ if (!acpi_region_callable(rgn) || !rgn->setup) {
+ mutex_unlock(&acpi_mutex_region);
+ return status;
+ }
+
+ atomic_inc(&rgn->refcnt);
+ setup = rgn->setup;
+ context = rgn->context;
+ mutex_unlock(&acpi_mutex_region);
+
+ status = setup(handle, function, context, region_context);
+ atomic_dec(&rgn->refcnt);
+
+ return status;
+}
+
+static int __acpi_install_region(struct acpi_region *rgn,
+ acpi_adr_space_type space_id)
+{
+ int res = 0;
+ acpi_status status;
+ int installing = 0;
+
+ mutex_lock(&acpi_mutex_region);
+ if (rgn->flags & ACPI_REGION_INSTALLED)
+ goto out_lock;
+ if (rgn->flags & ACPI_REGION_INSTALLING) {
+ res = -EBUSY;
+ goto out_lock;
+ }
+
+ installing = 1;
+ rgn->flags |= ACPI_REGION_INSTALLING;
+ status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT, space_id,
+ acpi_region_default_handler,
+ acpi_region_default_setup,
+ rgn);
+ rgn->flags &= ~ACPI_REGION_INSTALLING;
+ if (ACPI_FAILURE(status))
+ res = -EINVAL;
+ else
+ rgn->flags |= ACPI_REGION_INSTALLED;
+
+out_lock:
+ mutex_unlock(&acpi_mutex_region);
+ if (installing) {
+ if (res)
+ pr_err("Failed to install region %d\n", space_id);
+ else
+ pr_info("Region %d installed\n", space_id);
+ }
+ return res;
+}
+
+int acpi_register_region(acpi_adr_space_type space_id,
+ acpi_adr_space_handler handler,
+ acpi_adr_space_setup setup, void *context)
+{
+ int res;
+ struct acpi_region *rgn;
+
+ if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
+ return -EINVAL;
+
+ rgn = &acpi_regions[space_id];
+ if (!acpi_region_managed(rgn))
+ return -EINVAL;
+
+ res = __acpi_install_region(rgn, space_id);
+ if (res)
+ return res;
+
+ mutex_lock(&acpi_mutex_region);
+ if (rgn->flags & ACPI_REGION_REGISTERED) {
+ mutex_unlock(&acpi_mutex_region);
+ return -EBUSY;
+ }
+
+ rgn->handler = handler;
+ rgn->setup = setup;
+ rgn->context = context;
+ rgn->flags |= ACPI_REGION_REGISTERED;
+ atomic_set(&rgn->refcnt, 1);
+ mutex_unlock(&acpi_mutex_region);
+
+ pr_info("Region %d registered\n", space_id);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_register_region);
+
+void acpi_unregister_region(acpi_adr_space_type space_id)
+{
+ struct acpi_region *rgn;
+
+ if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
+ return;
+
+ rgn = &acpi_regions[space_id];
+ if (!acpi_region_managed(rgn))
+ return;
+
+ mutex_lock(&acpi_mutex_region);
+ if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
+ mutex_unlock(&acpi_mutex_region);
+ return;
+ }
+ if (rgn->flags & ACPI_REGION_UNREGISTERING) {
+ mutex_unlock(&acpi_mutex_region);
+ return;
+ }
+
+ rgn->flags |= ACPI_REGION_UNREGISTERING;
+ rgn->handler = NULL;
+ rgn->setup = NULL;
+ rgn->context = NULL;
+ mutex_unlock(&acpi_mutex_region);
+
+ while (atomic_read(&rgn->refcnt) > 1)
+ schedule_timeout_uninterruptible(usecs_to_jiffies(5));
+ atomic_dec(&rgn->refcnt);
+
+ mutex_lock(&acpi_mutex_region);
+ rgn->flags &= ~(ACPI_REGION_REGISTERED | ACPI_REGION_UNREGISTERING);
+ mutex_unlock(&acpi_mutex_region);
+
+ pr_info("Region %d unregistered\n", space_id);
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_region);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a2c2fbb..15fad0d 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; }
#endif /* CONFIG_ACPI */
+int acpi_register_region(acpi_adr_space_type space_id,
+ acpi_adr_space_handler handler,
+ acpi_adr_space_setup setup, void *context);
+void acpi_unregister_region(acpi_adr_space_type space_id);
+
#endif /*__ACPI_BUS_H__*/
--
1.7.10
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-23 8:09 ` [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers Lv Zheng
@ 2013-07-25 20:27 ` Rafael J. Wysocki
2013-07-26 0:47 ` Zheng, Lv
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2013-07-25 20:27 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J. Wysocki, Len Brown, Corey Minyard, Zhao Yakui,
linux-kernel, stable, linux-acpi, openipmi-developer
On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> 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 <lv.zheng@intel.com>
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> ---
> drivers/acpi/acpi_ipmi.c | 16 ++--
> drivers/acpi/osl.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 5 ++
> 3 files changed, 235 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> index 5f8f495..2a09156 100644
> --- a/drivers/acpi/acpi_ipmi.c
> +++ b/drivers/acpi/acpi_ipmi.c
> @@ -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);
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6ab2c35..8398e51 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
> static struct workqueue_struct *kacpi_notify_wq;
> static struct workqueue_struct *kacpi_hotplug_wq;
>
> +struct acpi_region {
> + unsigned long flags;
> +#define ACPI_REGION_DEFAULT 0x01
> +#define ACPI_REGION_INSTALLED 0x02
> +#define ACPI_REGION_REGISTERED 0x04
> +#define ACPI_REGION_UNREGISTERING 0x08
> +#define ACPI_REGION_INSTALLING 0x10
What about (1UL << 1), (1UL << 2) etc.?
Also please remove the #defines out of the struct definition.
> + /*
> + * NOTE: Upgrading All Region Handlers
> + * This flag is only used during the period where not all of the
> + * region handers are upgraded to the new interfaces.
> + */
> +#define ACPI_REGION_MANAGED 0x80
> + acpi_adr_space_handler handler;
> + acpi_adr_space_setup setup;
> + void *context;
> + /* Invoking references */
> + atomic_t refcnt;
Actually, why don't you use krefs?
> +};
> +
> +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);
> + return status;
> + }
> +
> + atomic_inc(&rgn->refcnt);
> + handler = rgn->handler;
> + context = rgn->context;
> + mutex_unlock(&acpi_mutex_region);
> +
> + status = handler(function, address, bit_width, value, context,
> + region_context);
Why don't we call the handler under the mutex?
What exactly prevents context from becoming NULL before the call above?
> + atomic_dec(&rgn->refcnt);
> +
> + return status;
> +}
> +
> +static acpi_status
> +acpi_region_default_setup(acpi_handle handle, u32 function,
> + void *handler_context, void **region_context)
> +{
> + acpi_adr_space_setup setup;
> + struct acpi_region *rgn = (struct acpi_region *)handler_context;
> + void *context;
> + acpi_status status = AE_OK;
> +
> + mutex_lock(&acpi_mutex_region);
> + if (!acpi_region_callable(rgn) || !rgn->setup) {
> + mutex_unlock(&acpi_mutex_region);
> + return status;
> + }
> +
> + atomic_inc(&rgn->refcnt);
> + setup = rgn->setup;
> + context = rgn->context;
> + mutex_unlock(&acpi_mutex_region);
> +
> + status = setup(handle, function, context, region_context);
Can setup drop rgn->refcnt ?
> + atomic_dec(&rgn->refcnt);
> +
> + return status;
> +}
> +
> +static int __acpi_install_region(struct acpi_region *rgn,
> + acpi_adr_space_type space_id)
> +{
> + int res = 0;
> + acpi_status status;
> + int installing = 0;
> +
> + mutex_lock(&acpi_mutex_region);
> + if (rgn->flags & ACPI_REGION_INSTALLED)
> + goto out_lock;
> + if (rgn->flags & ACPI_REGION_INSTALLING) {
> + res = -EBUSY;
> + goto out_lock;
> + }
> +
> + installing = 1;
> + rgn->flags |= ACPI_REGION_INSTALLING;
> + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT, space_id,
> + acpi_region_default_handler,
> + acpi_region_default_setup,
> + rgn);
> + rgn->flags &= ~ACPI_REGION_INSTALLING;
> + if (ACPI_FAILURE(status))
> + res = -EINVAL;
> + else
> + rgn->flags |= ACPI_REGION_INSTALLED;
> +
> +out_lock:
> + mutex_unlock(&acpi_mutex_region);
> + if (installing) {
> + if (res)
> + pr_err("Failed to install region %d\n", space_id);
> + else
> + pr_info("Region %d installed\n", space_id);
> + }
> + return res;
> +}
> +
> +int acpi_register_region(acpi_adr_space_type space_id,
> + acpi_adr_space_handler handler,
> + acpi_adr_space_setup setup, void *context)
> +{
> + int res;
> + struct acpi_region *rgn;
> +
> + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> + return -EINVAL;
> +
> + rgn = &acpi_regions[space_id];
> + if (!acpi_region_managed(rgn))
> + return -EINVAL;
> +
> + res = __acpi_install_region(rgn, space_id);
> + if (res)
> + return res;
> +
> + mutex_lock(&acpi_mutex_region);
> + if (rgn->flags & ACPI_REGION_REGISTERED) {
> + mutex_unlock(&acpi_mutex_region);
> + return -EBUSY;
> + }
> +
> + rgn->handler = handler;
> + rgn->setup = setup;
> + rgn->context = context;
> + rgn->flags |= ACPI_REGION_REGISTERED;
> + atomic_set(&rgn->refcnt, 1);
> + mutex_unlock(&acpi_mutex_region);
> +
> + pr_info("Region %d registered\n", space_id);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_register_region);
> +
> +void acpi_unregister_region(acpi_adr_space_type space_id)
> +{
> + struct acpi_region *rgn;
> +
> + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> + return;
> +
> + rgn = &acpi_regions[space_id];
> + if (!acpi_region_managed(rgn))
> + return;
> +
> + mutex_lock(&acpi_mutex_region);
> + if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
> + mutex_unlock(&acpi_mutex_region);
> + return;
> + }
> + if (rgn->flags & ACPI_REGION_UNREGISTERING) {
> + mutex_unlock(&acpi_mutex_region);
> + return;
What about
if ((rgn->flags & ACPI_REGION_UNREGISTERING)
|| !(rgn->flags & ACPI_REGION_REGISTERED)) {
mutex_unlock(&acpi_mutex_region);
return;
}
> + }
> +
> + rgn->flags |= ACPI_REGION_UNREGISTERING;
> + rgn->handler = NULL;
> + rgn->setup = NULL;
> + rgn->context = NULL;
> + mutex_unlock(&acpi_mutex_region);
> +
> + while (atomic_read(&rgn->refcnt) > 1)
> + schedule_timeout_uninterruptible(usecs_to_jiffies(5));
Wouldn't it be better to use a wait queue here?
> + atomic_dec(&rgn->refcnt);
> +
> + mutex_lock(&acpi_mutex_region);
> + rgn->flags &= ~(ACPI_REGION_REGISTERED | ACPI_REGION_UNREGISTERING);
> + mutex_unlock(&acpi_mutex_region);
> +
> + pr_info("Region %d unregistered\n", space_id);
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_region);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index a2c2fbb..15fad0d 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; }
>
> #endif /* CONFIG_ACPI */
>
> +int acpi_register_region(acpi_adr_space_type space_id,
> + acpi_adr_space_handler handler,
> + acpi_adr_space_setup setup, void *context);
> +void acpi_unregister_region(acpi_adr_space_type space_id);
> +
> #endif /*__ACPI_BUS_H__*/
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-25 20:27 ` Rafael J. Wysocki
@ 2013-07-26 0:47 ` Zheng, Lv
2013-07-26 8:09 ` Zheng, Lv
2013-07-26 14:00 ` Rafael J. Wysocki
0 siblings, 2 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-26 0:47 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, July 26, 2013 4:27 AM
>
> On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > 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 <lv.zheng@intel.com>
> > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > ---
> > drivers/acpi/acpi_ipmi.c | 16 ++--
> > drivers/acpi/osl.c | 224
> ++++++++++++++++++++++++++++++++++++++++++++++
> > include/acpi/acpi_bus.h | 5 ++
> > 3 files changed, 235 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > 5f8f495..2a09156 100644
> > --- a/drivers/acpi/acpi_ipmi.c
> > +++ b/drivers/acpi/acpi_ipmi.c
> > @@ -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);
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > 6ab2c35..8398e51 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq; static
> > struct workqueue_struct *kacpi_notify_wq; static struct
> > workqueue_struct *kacpi_hotplug_wq;
> >
> > +struct acpi_region {
> > + unsigned long flags;
> > +#define ACPI_REGION_DEFAULT 0x01
> > +#define ACPI_REGION_INSTALLED 0x02
> > +#define ACPI_REGION_REGISTERED 0x04
> > +#define ACPI_REGION_UNREGISTERING 0x08
> > +#define ACPI_REGION_INSTALLING 0x10
>
> What about (1UL << 1), (1UL << 2) etc.?
>
> Also please remove the #defines out of the struct definition.
OK.
>
> > + /*
> > + * NOTE: Upgrading All Region Handlers
> > + * This flag is only used during the period where not all of the
> > + * region handers are upgraded to the new interfaces.
> > + */
> > +#define ACPI_REGION_MANAGED 0x80
> > + acpi_adr_space_handler handler;
> > + acpi_adr_space_setup setup;
> > + void *context;
> > + /* Invoking references */
> > + atomic_t refcnt;
>
> Actually, why don't you use krefs?
If you take a look at other piece of my codes, you'll find there are two reasons:
1. I'm using while (atomic_read() > 1) to implement the objects' flushing and there is no kref API to do so.
I just think it is not suitable for me to introduce such an API into kref.h and start another argument around kref designs in this bug fix patch. :-)
I'll start a discussion about kref design using another thread.
2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's kind of atomic_t coding style.
If atomic_t is changed to struct kref, I will need to implement two API, __ipmi_dev_release() to take a struct kref as parameter and call ipmi_dev_release inside it.
By not using kref, I needn't write codes to implement such API.
>
> > +};
> > +
> > +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);
> > + return status;
> > + }
> > +
> > + atomic_inc(&rgn->refcnt);
> > + handler = rgn->handler;
> > + context = rgn->context;
> > + mutex_unlock(&acpi_mutex_region);
> > +
> > + status = handler(function, address, bit_width, value, context,
> > + region_context);
>
> Why don't we call the handler under the mutex?
>
> What exactly prevents context from becoming NULL before the call above?
It's a kind of programming style related concern.
IMO, using locks around callback function is a buggy programming style that could lead to dead locks.
Let me explain this using an example.
Object A exports a register/unregister API for other objects.
Object B calls A's register/unregister API to register/unregister B's callback.
It's likely that object B will hold lock_of_B around unregister/register when object B is destroyed/created, the lock_of_B is likely also used inside the callback.
So when object A holds the lock_of_A around the callback invocation, it leads to dead lock since:
1. the locking order for the register/unregister side will be: lock(lock_of_B), lock(lock_of_A)
2. the locking order for the callback side will be: lock(lock_of_A), lock(lock_of_B)
They are in the reversed order!
IMO, Linux may need to introduce __callback, __api as decelerators for the functions, and use sparse to enforce this rule, sparse knows if a callback is invoked under some locks.
In the case of ACPICA space_handlers, as you may know, when an ACPI operation region handler is invoked, there will be no lock held inside ACPICA (interpreter lock must be freed before executing operation region handlers).
So the likelihood of the dead lock is pretty much high here!
>
> > + atomic_dec(&rgn->refcnt);
> > +
> > + return status;
> > +}
> > +
> > +static acpi_status
> > +acpi_region_default_setup(acpi_handle handle, u32 function,
> > + void *handler_context, void **region_context) {
> > + acpi_adr_space_setup setup;
> > + struct acpi_region *rgn = (struct acpi_region *)handler_context;
> > + void *context;
> > + acpi_status status = AE_OK;
> > +
> > + mutex_lock(&acpi_mutex_region);
> > + if (!acpi_region_callable(rgn) || !rgn->setup) {
> > + mutex_unlock(&acpi_mutex_region);
> > + return status;
> > + }
> > +
> > + atomic_inc(&rgn->refcnt);
> > + setup = rgn->setup;
> > + context = rgn->context;
> > + mutex_unlock(&acpi_mutex_region);
> > +
> > + status = setup(handle, function, context, region_context);
>
> Can setup drop rgn->refcnt ?
The reason is same as the handler, as a setup is also a callback.
>
> > + atomic_dec(&rgn->refcnt);
> > +
> > + return status;
> > +}
> > +
> > +static int __acpi_install_region(struct acpi_region *rgn,
> > + acpi_adr_space_type space_id)
> > +{
> > + int res = 0;
> > + acpi_status status;
> > + int installing = 0;
> > +
> > + mutex_lock(&acpi_mutex_region);
> > + if (rgn->flags & ACPI_REGION_INSTALLED)
> > + goto out_lock;
> > + if (rgn->flags & ACPI_REGION_INSTALLING) {
> > + res = -EBUSY;
> > + goto out_lock;
> > + }
> > +
> > + installing = 1;
> > + rgn->flags |= ACPI_REGION_INSTALLING;
> > + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> space_id,
> > + acpi_region_default_handler,
> > + acpi_region_default_setup,
> > + rgn);
> > + rgn->flags &= ~ACPI_REGION_INSTALLING;
> > + if (ACPI_FAILURE(status))
> > + res = -EINVAL;
> > + else
> > + rgn->flags |= ACPI_REGION_INSTALLED;
> > +
> > +out_lock:
> > + mutex_unlock(&acpi_mutex_region);
> > + if (installing) {
> > + if (res)
> > + pr_err("Failed to install region %d\n", space_id);
> > + else
> > + pr_info("Region %d installed\n", space_id);
> > + }
> > + return res;
> > +}
> > +
> > +int acpi_register_region(acpi_adr_space_type space_id,
> > + acpi_adr_space_handler handler,
> > + acpi_adr_space_setup setup, void *context) {
> > + int res;
> > + struct acpi_region *rgn;
> > +
> > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > + return -EINVAL;
> > +
> > + rgn = &acpi_regions[space_id];
> > + if (!acpi_region_managed(rgn))
> > + return -EINVAL;
> > +
> > + res = __acpi_install_region(rgn, space_id);
> > + if (res)
> > + return res;
> > +
> > + mutex_lock(&acpi_mutex_region);
> > + if (rgn->flags & ACPI_REGION_REGISTERED) {
> > + mutex_unlock(&acpi_mutex_region);
> > + return -EBUSY;
> > + }
> > +
> > + rgn->handler = handler;
> > + rgn->setup = setup;
> > + rgn->context = context;
> > + rgn->flags |= ACPI_REGION_REGISTERED;
> > + atomic_set(&rgn->refcnt, 1);
> > + mutex_unlock(&acpi_mutex_region);
> > +
> > + pr_info("Region %d registered\n", space_id);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_register_region);
> > +
> > +void acpi_unregister_region(acpi_adr_space_type space_id) {
> > + struct acpi_region *rgn;
> > +
> > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > + return;
> > +
> > + rgn = &acpi_regions[space_id];
> > + if (!acpi_region_managed(rgn))
> > + return;
> > +
> > + mutex_lock(&acpi_mutex_region);
> > + if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
> > + mutex_unlock(&acpi_mutex_region);
> > + return;
> > + }
> > + if (rgn->flags & ACPI_REGION_UNREGISTERING) {
> > + mutex_unlock(&acpi_mutex_region);
> > + return;
>
> What about
>
> if ((rgn->flags & ACPI_REGION_UNREGISTERING)
> || !(rgn->flags & ACPI_REGION_REGISTERED)) {
> mutex_unlock(&acpi_mutex_region);
> return;
> }
>
OK.
> > + }
> > +
> > + rgn->flags |= ACPI_REGION_UNREGISTERING;
> > + rgn->handler = NULL;
> > + rgn->setup = NULL;
> > + rgn->context = NULL;
> > + mutex_unlock(&acpi_mutex_region);
> > +
> > + while (atomic_read(&rgn->refcnt) > 1)
> > + schedule_timeout_uninterruptible(usecs_to_jiffies(5));
>
> Wouldn't it be better to use a wait queue here?
Yes, I'll try.
>
> > + atomic_dec(&rgn->refcnt);
> > +
> > + mutex_lock(&acpi_mutex_region);
> > + rgn->flags &= ~(ACPI_REGION_REGISTERED |
> ACPI_REGION_UNREGISTERING);
> > + mutex_unlock(&acpi_mutex_region);
> > +
> > + pr_info("Region %d unregistered\n", space_id); }
> > +EXPORT_SYMBOL_GPL(acpi_unregister_region);
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index
> > a2c2fbb..15fad0d 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void
> > *bus) { return 0; }
> >
> > #endif /* CONFIG_ACPI */
> >
> > +int acpi_register_region(acpi_adr_space_type space_id,
> > + acpi_adr_space_handler handler,
> > + acpi_adr_space_setup setup, void *context); void
> > +acpi_unregister_region(acpi_adr_space_type space_id);
> > +
> > #endif /*__ACPI_BUS_H__*/
>
> Thanks,
> Rafael
Thanks
-Lv
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-26 0:47 ` Zheng, Lv
@ 2013-07-26 8:09 ` Zheng, Lv
2013-07-26 14:00 ` Rafael J. Wysocki
1 sibling, 0 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-26 8:09 UTC (permalink / raw)
To: Zheng, Lv, Rafael J. Wysocki
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> From: linux-acpi-owner@vger.kernel.org
> [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Friday, July 26, 2013 8:48 AM
>
>
>
> > From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> > Sent: Friday, July 26, 2013 4:27 AM
> >
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > 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 <lv.zheng@intel.com>
> > > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > > ---
> > > drivers/acpi/acpi_ipmi.c | 16 ++--
> > > drivers/acpi/osl.c | 224
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > > include/acpi/acpi_bus.h | 5 ++
> > > 3 files changed, 235 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> > > index
> > > 5f8f495..2a09156 100644
> > > --- a/drivers/acpi/acpi_ipmi.c
> > > +++ b/drivers/acpi/acpi_ipmi.c
> > > @@ -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);
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > 6ab2c35..8398e51 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
> > > static struct workqueue_struct *kacpi_notify_wq; static struct
> > > workqueue_struct *kacpi_hotplug_wq;
> > >
> > > +struct acpi_region {
> > > + unsigned long flags;
> > > +#define ACPI_REGION_DEFAULT 0x01
> > > +#define ACPI_REGION_INSTALLED 0x02
> > > +#define ACPI_REGION_REGISTERED 0x04
> > > +#define ACPI_REGION_UNREGISTERING 0x08
> > > +#define ACPI_REGION_INSTALLING 0x10
> >
> > What about (1UL << 1), (1UL << 2) etc.?
> >
> > Also please remove the #defines out of the struct definition.
>
> OK.
>
> >
> > > + /*
> > > + * NOTE: Upgrading All Region Handlers
> > > + * This flag is only used during the period where not all of the
> > > + * region handers are upgraded to the new interfaces.
> > > + */
> > > +#define ACPI_REGION_MANAGED 0x80
> > > + acpi_adr_space_handler handler;
> > > + acpi_adr_space_setup setup;
> > > + void *context;
> > > + /* Invoking references */
> > > + atomic_t refcnt;
> >
> > Actually, why don't you use krefs?
>
> If you take a look at other piece of my codes, you'll find there are two reasons:
>
> 1. I'm using while (atomic_read() > 1) to implement the objects' flushing and
> there is no kref API to do so.
> I just think it is not suitable for me to introduce such an API into kref.h and
> start another argument around kref designs in this bug fix patch. :-)
> I'll start a discussion about kref design using another thread.
> 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's kind
> of atomic_t coding style.
> If atomic_t is changed to struct kref, I will need to implement two API,
> __ipmi_dev_release() to take a struct kref as parameter and call
> ipmi_dev_release inside it.
> By not using kref, I needn't write codes to implement such API.
>
> >
> > > +};
> > > +
> > > +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);
> > > + return status;
> > > + }
> > > +
> > > + atomic_inc(&rgn->refcnt);
> > > + handler = rgn->handler;
> > > + context = rgn->context;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + status = handler(function, address, bit_width, value, context,
> > > + region_context);
> >
> > Why don't we call the handler under the mutex?
I think my reply is against this question, let me remove it up.
> It's a kind of programming style related concern.
> IMO, using locks around callback function is a buggy programming style that
> could lead to dead locks.
> Let me explain this using an example.
>
> Object A exports a register/unregister API for other objects.
> Object B calls A's register/unregister API to register/unregister B's callback.
Sorry I have to use object rather than module here as there might be several objects inside a module that have the same situation need to be handled.
> It's likely that object B will hold lock_of_B around unregister/register when
> object B is destroyed/created, the lock_of_B is likely also used inside the
> callback.
> So when object A holds the lock_of_A around the callback invocation, it leads to
> dead lock since:
> 1. the locking order for the register/unregister side will be: lock(lock_of_B),
> lock(lock_of_A) 2. the locking order for the callback side will be: lock(lock_of_A),
> lock(lock_of_B) They are in the reversed order!
I think this example is not quite correct.
There is another aspect in unregister implementation which is the intent of this patch:
No callback running/imitated after "unregister", we can call this a "flush" requirement.
Inside of the callback, lock_of_B should not be held if "flush" is required.
>
> IMO, Linux may need to introduce __callback, __api as decelerators for the
> functions, and use sparse to enforce this rule, sparse knows if a callback is
> invoked under some locks.
>
> In the case of ACPICA space_handlers, as you may know, when an ACPI
> operation region handler is invoked, there will be no lock held inside ACPICA
> (interpreter lock must be freed before executing operation region handlers).
> So the likelihood of the dead lock is pretty much high here!
I need to mention another requirement of the operation region handler.
It is required that multiple operation region handlers are executed at the same time, or the IO operations invoked by the BIOS ASL codes will be serialized.
IMO, IO operations invoked by the BIOS ASL need to be parallelized.
Thus mutex is not useful to implement a protection here.
So the mutex is unlocked before executing the handler, IMO, reference counting is useful here to meet this requirement.
> >
> > What exactly prevents context from becoming NULL before the call above?
I think my answers did not answer this question directly.
Sorry that I'm not clear what you want to ask here. Let me just try to be practical.
The code is here:
>
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (!acpi_region_callable(rgn) || !rgn->handler) {
> > > + handler = rgn->handler;
> > > + context = rgn->context;
> > > + mutex_unlock(&acpi_mutex_region);
The handler is ensured not to be NULL within the mutex lock.
Thanks for commenting.
Best regards
-Lv
> >
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static acpi_status
> > > +acpi_region_default_setup(acpi_handle handle, u32 function,
> > > + void *handler_context, void **region_context) {
> > > + acpi_adr_space_setup setup;
> > > + struct acpi_region *rgn = (struct acpi_region *)handler_context;
> > > + void *context;
> > > + acpi_status status = AE_OK;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (!acpi_region_callable(rgn) || !rgn->setup) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return status;
> > > + }
> > > +
> > > + atomic_inc(&rgn->refcnt);
> > > + setup = rgn->setup;
> > > + context = rgn->context;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + status = setup(handle, function, context, region_context);
> >
> > Can setup drop rgn->refcnt ?
>
> The reason is same as the handler, as a setup is also a callback.
>
> >
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int __acpi_install_region(struct acpi_region *rgn,
> > > + acpi_adr_space_type space_id)
> > > +{
> > > + int res = 0;
> > > + acpi_status status;
> > > + int installing = 0;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (rgn->flags & ACPI_REGION_INSTALLED)
> > > + goto out_lock;
> > > + if (rgn->flags & ACPI_REGION_INSTALLING) {
> > > + res = -EBUSY;
> > > + goto out_lock;
> > > + }
> > > +
> > > + installing = 1;
> > > + rgn->flags |= ACPI_REGION_INSTALLING;
> > > + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > space_id,
> > > + acpi_region_default_handler,
> > > + acpi_region_default_setup,
> > > + rgn);
> > > + rgn->flags &= ~ACPI_REGION_INSTALLING;
> > > + if (ACPI_FAILURE(status))
> > > + res = -EINVAL;
> > > + else
> > > + rgn->flags |= ACPI_REGION_INSTALLED;
> > > +
> > > +out_lock:
> > > + mutex_unlock(&acpi_mutex_region);
> > > + if (installing) {
> > > + if (res)
> > > + pr_err("Failed to install region %d\n", space_id);
> > > + else
> > > + pr_info("Region %d installed\n", space_id);
> > > + }
> > > + return res;
> > > +}
> > > +
> > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > + acpi_adr_space_handler handler,
> > > + acpi_adr_space_setup setup, void *context) {
> > > + int res;
> > > + struct acpi_region *rgn;
> > > +
> > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > + return -EINVAL;
> > > +
> > > + rgn = &acpi_regions[space_id];
> > > + if (!acpi_region_managed(rgn))
> > > + return -EINVAL;
> > > +
> > > + res = __acpi_install_region(rgn, space_id);
> > > + if (res)
> > > + return res;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (rgn->flags & ACPI_REGION_REGISTERED) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + rgn->handler = handler;
> > > + rgn->setup = setup;
> > > + rgn->context = context;
> > > + rgn->flags |= ACPI_REGION_REGISTERED;
> > > + atomic_set(&rgn->refcnt, 1);
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + pr_info("Region %d registered\n", space_id);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_register_region);
> > > +
> > > +void acpi_unregister_region(acpi_adr_space_type space_id) {
> > > + struct acpi_region *rgn;
> > > +
> > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > + return;
> > > +
> > > + rgn = &acpi_regions[space_id];
> > > + if (!acpi_region_managed(rgn))
> > > + return;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return;
> > > + }
> > > + if (rgn->flags & ACPI_REGION_UNREGISTERING) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return;
> >
> > What about
> >
> > if ((rgn->flags & ACPI_REGION_UNREGISTERING)
> > || !(rgn->flags & ACPI_REGION_REGISTERED)) {
> > mutex_unlock(&acpi_mutex_region);
> > return;
> > }
> >
>
> OK.
>
> > > + }
> > > +
> > > + rgn->flags |= ACPI_REGION_UNREGISTERING;
> > > + rgn->handler = NULL;
> > > + rgn->setup = NULL;
> > > + rgn->context = NULL;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + while (atomic_read(&rgn->refcnt) > 1)
> > > + schedule_timeout_uninterruptible(usecs_to_jiffies(5));
> >
> > Wouldn't it be better to use a wait queue here?
>
> Yes, I'll try.
>
> >
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + rgn->flags &= ~(ACPI_REGION_REGISTERED |
> > ACPI_REGION_UNREGISTERING);
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + pr_info("Region %d unregistered\n", space_id); }
> > > +EXPORT_SYMBOL_GPL(acpi_unregister_region);
> > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index
> > > a2c2fbb..15fad0d 100644
> > > --- a/include/acpi/acpi_bus.h
> > > +++ b/include/acpi/acpi_bus.h
> > > @@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void
> > > *bus) { return 0; }
> > >
> > > #endif /* CONFIG_ACPI */
> > >
> > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > + acpi_adr_space_handler handler,
> > > + acpi_adr_space_setup setup, void *context); void
> > > +acpi_unregister_region(acpi_adr_space_type space_id);
> > > +
> > > #endif /*__ACPI_BUS_H__*/
> >
> > Thanks,
> > Rafael
>
> Thanks
> -Lv
>
> >
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> N r y b X ǧv ^ ){.n + { i b {ay \x1dʇڙ ,j f h z \x1e w
> j:+v w j m zZ+ ݢj" ! i
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-26 0:47 ` Zheng, Lv
2013-07-26 8:09 ` Zheng, Lv
@ 2013-07-26 14:00 ` Rafael J. Wysocki
2013-07-29 1:43 ` Zheng, Lv
1 sibling, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2013-07-26 14:00 UTC (permalink / raw)
To: Zheng, Lv
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
>
> > From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> > Sent: Friday, July 26, 2013 4:27 AM
> >
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > 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 <lv.zheng@intel.com>
> > > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > > ---
> > > drivers/acpi/acpi_ipmi.c | 16 ++--
> > > drivers/acpi/osl.c | 224
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > > include/acpi/acpi_bus.h | 5 ++
> > > 3 files changed, 235 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > > 5f8f495..2a09156 100644
> > > --- a/drivers/acpi/acpi_ipmi.c
> > > +++ b/drivers/acpi/acpi_ipmi.c
> > > @@ -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);
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > 6ab2c35..8398e51 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq; static
> > > struct workqueue_struct *kacpi_notify_wq; static struct
> > > workqueue_struct *kacpi_hotplug_wq;
> > >
> > > +struct acpi_region {
> > > + unsigned long flags;
> > > +#define ACPI_REGION_DEFAULT 0x01
> > > +#define ACPI_REGION_INSTALLED 0x02
> > > +#define ACPI_REGION_REGISTERED 0x04
> > > +#define ACPI_REGION_UNREGISTERING 0x08
> > > +#define ACPI_REGION_INSTALLING 0x10
> >
> > What about (1UL << 1), (1UL << 2) etc.?
> >
> > Also please remove the #defines out of the struct definition.
>
> OK.
>
> >
> > > + /*
> > > + * NOTE: Upgrading All Region Handlers
> > > + * This flag is only used during the period where not all of the
> > > + * region handers are upgraded to the new interfaces.
> > > + */
> > > +#define ACPI_REGION_MANAGED 0x80
> > > + acpi_adr_space_handler handler;
> > > + acpi_adr_space_setup setup;
> > > + void *context;
> > > + /* Invoking references */
> > > + atomic_t refcnt;
> >
> > Actually, why don't you use krefs?
>
> If you take a look at other piece of my codes, you'll find there are two reasons:
>
> 1. I'm using while (atomic_read() > 1) to implement the objects' flushing and there is no kref API to do so.
No, there's not any, but you can read kref.refcount directly, can't you?
Moreover, it is not entirely clear to me that doing the while (atomic_read() > 1)
is actually correct.
> I just think it is not suitable for me to introduce such an API into kref.h and start another argument around kref designs in this bug fix patch. :-)
> I'll start a discussion about kref design using another thread.
You don't need to do that at all.
> 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's kind of atomic_t coding style.
> If atomic_t is changed to struct kref, I will need to implement two API, __ipmi_dev_release() to take a struct kref as parameter and call ipmi_dev_release inside it.
> By not using kref, I needn't write codes to implement such API.
I'm not following you, sorry.
Please just use krefs for reference counting, the same way as you use
struct list_head for implementing lists. This is the way everyone does
that in the kernel and that's for a reason.
Unless you do your reference counting under a lock, in which case using
atomic_t isn't necessary at all and you can use a non-atomic counter.
> > > +};
> > > +
> > > +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);
> > > + return status;
> > > + }
> > > +
> > > + atomic_inc(&rgn->refcnt);
> > > + handler = rgn->handler;
> > > + context = rgn->context;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + status = handler(function, address, bit_width, value, context,
> > > + region_context);
> >
> > Why don't we call the handler under the mutex?
> >
> > What exactly prevents context from becoming NULL before the call above?
>
> It's a kind of programming style related concern.
> IMO, using locks around callback function is a buggy programming style that could lead to dead locks.
> Let me explain this using an example.
>
> Object A exports a register/unregister API for other objects.
> Object B calls A's register/unregister API to register/unregister B's callback.
> It's likely that object B will hold lock_of_B around unregister/register when object B is destroyed/created, the lock_of_B is likely also used inside the callback.
Why is it likely to be used inside the callback? Clearly, if a callback is
executed under a lock, that lock can't be acquired by that callback.
> So when object A holds the lock_of_A around the callback invocation, it leads to dead lock since:
> 1. the locking order for the register/unregister side will be: lock(lock_of_B), lock(lock_of_A)
> 2. the locking order for the callback side will be: lock(lock_of_A), lock(lock_of_B)
> They are in the reversed order!
>
> IMO, Linux may need to introduce __callback, __api as decelerators for the functions, and use sparse to enforce this rule, sparse knows if a callback is invoked under some locks.
Oh, dear. Yes, sparse knows such things, and so what?
> In the case of ACPICA space_handlers, as you may know, when an ACPI operation region handler is invoked, there will be no lock held inside ACPICA (interpreter lock must be freed before executing operation region handlers).
> So the likelihood of the dead lock is pretty much high here!
Sorry, what are you talking about?
Please let me rephrase my question: What *practical* problems would it lead to
if we executed this particular callback under this particular mutex?
Not *theoretical* in the general theory of everything, *practical* in this
particular piece of code.
And we are talking about a *global* mutex here, not something object-specific.
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static acpi_status
> > > +acpi_region_default_setup(acpi_handle handle, u32 function,
> > > + void *handler_context, void **region_context) {
> > > + acpi_adr_space_setup setup;
> > > + struct acpi_region *rgn = (struct acpi_region *)handler_context;
> > > + void *context;
> > > + acpi_status status = AE_OK;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (!acpi_region_callable(rgn) || !rgn->setup) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return status;
> > > + }
> > > +
> > > + atomic_inc(&rgn->refcnt);
> > > + setup = rgn->setup;
> > > + context = rgn->context;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + status = setup(handle, function, context, region_context);
> >
> > Can setup drop rgn->refcnt ?
>
> The reason is same as the handler, as a setup is also a callback.
Let me rephrase: Is it legitimate for setup to modify rgn->refcnt?
If so, then why?
> >
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int __acpi_install_region(struct acpi_region *rgn,
> > > + acpi_adr_space_type space_id)
> > > +{
> > > + int res = 0;
> > > + acpi_status status;
> > > + int installing = 0;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (rgn->flags & ACPI_REGION_INSTALLED)
> > > + goto out_lock;
> > > + if (rgn->flags & ACPI_REGION_INSTALLING) {
> > > + res = -EBUSY;
> > > + goto out_lock;
> > > + }
> > > +
> > > + installing = 1;
> > > + rgn->flags |= ACPI_REGION_INSTALLING;
> > > + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > space_id,
> > > + acpi_region_default_handler,
> > > + acpi_region_default_setup,
> > > + rgn);
> > > + rgn->flags &= ~ACPI_REGION_INSTALLING;
> > > + if (ACPI_FAILURE(status))
> > > + res = -EINVAL;
> > > + else
> > > + rgn->flags |= ACPI_REGION_INSTALLED;
> > > +
> > > +out_lock:
> > > + mutex_unlock(&acpi_mutex_region);
> > > + if (installing) {
> > > + if (res)
> > > + pr_err("Failed to install region %d\n", space_id);
> > > + else
> > > + pr_info("Region %d installed\n", space_id);
> > > + }
> > > + return res;
> > > +}
> > > +
> > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > + acpi_adr_space_handler handler,
> > > + acpi_adr_space_setup setup, void *context) {
> > > + int res;
> > > + struct acpi_region *rgn;
> > > +
> > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > + return -EINVAL;
> > > +
> > > + rgn = &acpi_regions[space_id];
> > > + if (!acpi_region_managed(rgn))
> > > + return -EINVAL;
> > > +
> > > + res = __acpi_install_region(rgn, space_id);
> > > + if (res)
> > > + return res;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (rgn->flags & ACPI_REGION_REGISTERED) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + rgn->handler = handler;
> > > + rgn->setup = setup;
> > > + rgn->context = context;
> > > + rgn->flags |= ACPI_REGION_REGISTERED;
> > > + atomic_set(&rgn->refcnt, 1);
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + pr_info("Region %d registered\n", space_id);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_register_region);
> > > +
> > > +void acpi_unregister_region(acpi_adr_space_type space_id) {
> > > + struct acpi_region *rgn;
> > > +
> > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > + return;
> > > +
> > > + rgn = &acpi_regions[space_id];
> > > + if (!acpi_region_managed(rgn))
> > > + return;
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return;
> > > + }
> > > + if (rgn->flags & ACPI_REGION_UNREGISTERING) {
> > > + mutex_unlock(&acpi_mutex_region);
> > > + return;
> >
> > What about
> >
> > if ((rgn->flags & ACPI_REGION_UNREGISTERING)
> > || !(rgn->flags & ACPI_REGION_REGISTERED)) {
> > mutex_unlock(&acpi_mutex_region);
> > return;
> > }
> >
>
> OK.
>
> > > + }
> > > +
> > > + rgn->flags |= ACPI_REGION_UNREGISTERING;
> > > + rgn->handler = NULL;
> > > + rgn->setup = NULL;
> > > + rgn->context = NULL;
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + while (atomic_read(&rgn->refcnt) > 1)
> > > + schedule_timeout_uninterruptible(usecs_to_jiffies(5));
> >
> > Wouldn't it be better to use a wait queue here?
>
> Yes, I'll try.
By the way, we do we need to do that?
> > > + atomic_dec(&rgn->refcnt);
> > > +
> > > + mutex_lock(&acpi_mutex_region);
> > > + rgn->flags &= ~(ACPI_REGION_REGISTERED |
> > ACPI_REGION_UNREGISTERING);
> > > + mutex_unlock(&acpi_mutex_region);
> > > +
> > > + pr_info("Region %d unregistered\n", space_id); }
> > > +EXPORT_SYMBOL_GPL(acpi_unregister_region);
> > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index
> > > a2c2fbb..15fad0d 100644
> > > --- a/include/acpi/acpi_bus.h
> > > +++ b/include/acpi/acpi_bus.h
> > > @@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void
> > > *bus) { return 0; }
> > >
> > > #endif /* CONFIG_ACPI */
> > >
> > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > + acpi_adr_space_handler handler,
> > > + acpi_adr_space_setup setup, void *context); void
> > > +acpi_unregister_region(acpi_adr_space_type space_id);
> > > +
> > > #endif /*__ACPI_BUS_H__*/
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers
2013-07-26 14:00 ` Rafael J. Wysocki
@ 2013-07-29 1:43 ` Zheng, Lv
0 siblings, 0 replies; 18+ messages in thread
From: Zheng, Lv @ 2013-07-29 1:43 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Wysocki, Rafael J, Brown, Len, Corey Minyard, Zhao, Yakui,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
linux-acpi@vger.kernel.org,
openipmi-developer@lists.sourceforge.net
> On Friday, July 26, 2013 10:01 PM Rafael J. Wysocki wrote:
> > On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
> >
> > > On Friday, July 26, 2013 4:27 AM Rafael J. Wysocki wrote:
> > >
> > > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > > 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 <lv.zheng@intel.com>
> > > > Reviewed-by: Huang Ying <ying.huang@intel.com>
> > > > ---
> > > > drivers/acpi/acpi_ipmi.c | 16 ++--
> > > > drivers/acpi/osl.c | 224
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > include/acpi/acpi_bus.h | 5 ++
> > > > 3 files changed, 235 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > > > 5f8f495..2a09156 100644
> > > > --- a/drivers/acpi/acpi_ipmi.c
> > > > +++ b/drivers/acpi/acpi_ipmi.c
> > > > @@ -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);
> > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > > 6ab2c35..8398e51 100644
> > > > --- a/drivers/acpi/osl.c
> > > > +++ b/drivers/acpi/osl.c
> > > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
> static
> > > > struct workqueue_struct *kacpi_notify_wq; static struct
> > > > workqueue_struct *kacpi_hotplug_wq;
> > > >
> > > > +struct acpi_region {
> > > > + unsigned long flags;
> > > > +#define ACPI_REGION_DEFAULT 0x01
> > > > +#define ACPI_REGION_INSTALLED 0x02
> > > > +#define ACPI_REGION_REGISTERED 0x04
> > > > +#define ACPI_REGION_UNREGISTERING 0x08
> > > > +#define ACPI_REGION_INSTALLING 0x10
> > >
> > > What about (1UL << 1), (1UL << 2) etc.?
> > >
> > > Also please remove the #defines out of the struct definition.
> >
> > OK.
> >
> > >
> > > > + /*
> > > > + * NOTE: Upgrading All Region Handlers
> > > > + * This flag is only used during the period where not all of the
> > > > + * region handers are upgraded to the new interfaces.
> > > > + */
> > > > +#define ACPI_REGION_MANAGED 0x80
> > > > + acpi_adr_space_handler handler;
> > > > + acpi_adr_space_setup setup;
> > > > + void *context;
> > > > + /* Invoking references */
> > > > + atomic_t refcnt;
> > >
> > > Actually, why don't you use krefs?
> >
> > If you take a look at other piece of my codes, you'll find there are two
> reasons:
> >
> > 1. I'm using while (atomic_read() > 1) to implement the objects' flushing and
> there is no kref API to do so.
>
> No, there's not any, but you can read kref.refcount directly, can't you?
>
> Moreover, it is not entirely clear to me that doing the while (atomic_read() > 1)
> is actually correct.
>
> > I just think it is not suitable for me to introduce such an API into kref.h and
> start another argument around kref designs in this bug fix patch. :-)
> > I'll start a discussion about kref design using another thread.
>
> You don't need to do that at all.
>
> > 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's
> kind of atomic_t coding style.
> > If atomic_t is changed to struct kref, I will need to implement two API,
> __ipmi_dev_release() to take a struct kref as parameter and call
> ipmi_dev_release inside it.
> > By not using kref, I needn't write codes to implement such API.
>
> I'm not following you, sorry.
>
> Please just use krefs for reference counting, the same way as you use
> struct list_head for implementing lists. This is the way everyone does
> that in the kernel and that's for a reason.
>
> Unless you do your reference counting under a lock, in which case using
> atomic_t isn't necessary at all and you can use a non-atomic counter.
I'll follow your suggestion of kref.
You can find my concern 2 related stuff in the next revision.
It's trivial.
>
> > > > +};
> > > > +
> > > > +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);
> > > > + return status;
> > > > + }
> > > > +
> > > > + atomic_inc(&rgn->refcnt);
> > > > + handler = rgn->handler;
> > > > + context = rgn->context;
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > +
> > > > + status = handler(function, address, bit_width, value, context,
> > > > + region_context);
> > >
> > > Why don't we call the handler under the mutex?
> > >
> > > What exactly prevents context from becoming NULL before the call above?
> >
> > It's a kind of programming style related concern.
> > IMO, using locks around callback function is a buggy programming style that
> could lead to dead locks.
> > Let me explain this using an example.
> >
> > Object A exports a register/unregister API for other objects.
> > Object B calls A's register/unregister API to register/unregister B's callback.
> > It's likely that object B will hold lock_of_B around unregister/register when
> object B is destroyed/created, the lock_of_B is likely also used inside the
> callback.
>
> Why is it likely to be used inside the callback? Clearly, if a callback is
> executed under a lock, that lock can't be acquired by that callback.
I think this is not related to the real purpose of why we must not hold a lock in this situation.
So let's ignore this paragraph.
>
> > So when object A holds the lock_of_A around the callback invocation, it leads
> to dead lock since:
> > 1. the locking order for the register/unregister side will be: lock(lock_of_B),
> lock(lock_of_A)
> > 2. the locking order for the callback side will be: lock(lock_of_A),
> lock(lock_of_B)
> > They are in the reversed order!
> >
> > IMO, Linux may need to introduce __callback, __api as decelerators for the
> functions, and use sparse to enforce this rule, sparse knows if a callback is
> invoked under some locks.
>
> Oh, dear. Yes, sparse knows such things, and so what?
I was thinking sparse can give us warnings on __api marked function invocation where __acquire count is not 0, this might be mandatory for high quality codes.
And sparse can also give us warnings on __callback marked function invocations where __acquire count is not 0, this should be optional.
But since it is not related to our topic, let's ignore this paragraph.
>
> > In the case of ACPICA space_handlers, as you may know, when an ACPI
> operation region handler is invoked, there will be no lock held inside ACPICA
> (interpreter lock must be freed before executing operation region handlers).
> > So the likelihood of the dead lock is pretty much high here!
>
> Sorry, what are you talking about?
>
> Please let me rephrase my question: What *practical* problems would it lead
> to
> if we executed this particular callback under this particular mutex?
>
> Not *theoretical* in the general theory of everything, *practical* in this
> particular piece of code.
>
> And we are talking about a *global* mutex here, not something object-specific.
I think you have additional replies on this in another email.
Let me reply you there.
>
> > > > + atomic_dec(&rgn->refcnt);
> > > > +
> > > > + return status;
> > > > +}
> > > > +
> > > > +static acpi_status
> > > > +acpi_region_default_setup(acpi_handle handle, u32 function,
> > > > + void *handler_context, void **region_context) {
> > > > + acpi_adr_space_setup setup;
> > > > + struct acpi_region *rgn = (struct acpi_region *)handler_context;
> > > > + void *context;
> > > > + acpi_status status = AE_OK;
> > > > +
> > > > + mutex_lock(&acpi_mutex_region);
> > > > + if (!acpi_region_callable(rgn) || !rgn->setup) {
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > + return status;
> > > > + }
> > > > +
> > > > + atomic_inc(&rgn->refcnt);
> > > > + setup = rgn->setup;
> > > > + context = rgn->context;
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > +
> > > > + status = setup(handle, function, context, region_context);
> > >
> > > Can setup drop rgn->refcnt ?
> >
> > The reason is same as the handler, as a setup is also a callback.
>
> Let me rephrase: Is it legitimate for setup to modify rgn->refcnt?
> If so, then why?
Yes, the race is same as the handler.
When ACPICA is accessing the text segment of the setup function implementation, the module owns the setup function can also be unloaded as there is no lock hold before invoking setup - note that ExitInter also happens to setup invocations.
>
> > >
> > > > + atomic_dec(&rgn->refcnt);
> > > > +
> > > > + return status;
> > > > +}
> > > > +
> > > > +static int __acpi_install_region(struct acpi_region *rgn,
> > > > + acpi_adr_space_type space_id)
> > > > +{
> > > > + int res = 0;
> > > > + acpi_status status;
> > > > + int installing = 0;
> > > > +
> > > > + mutex_lock(&acpi_mutex_region);
> > > > + if (rgn->flags & ACPI_REGION_INSTALLED)
> > > > + goto out_lock;
> > > > + if (rgn->flags & ACPI_REGION_INSTALLING) {
> > > > + res = -EBUSY;
> > > > + goto out_lock;
> > > > + }
> > > > +
> > > > + installing = 1;
> > > > + rgn->flags |= ACPI_REGION_INSTALLING;
> > > > + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > > space_id,
> > > > + acpi_region_default_handler,
> > > > + acpi_region_default_setup,
> > > > + rgn);
> > > > + rgn->flags &= ~ACPI_REGION_INSTALLING;
> > > > + if (ACPI_FAILURE(status))
> > > > + res = -EINVAL;
> > > > + else
> > > > + rgn->flags |= ACPI_REGION_INSTALLED;
> > > > +
> > > > +out_lock:
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > + if (installing) {
> > > > + if (res)
> > > > + pr_err("Failed to install region %d\n", space_id);
> > > > + else
> > > > + pr_info("Region %d installed\n", space_id);
> > > > + }
> > > > + return res;
> > > > +}
> > > > +
> > > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > > + acpi_adr_space_handler handler,
> > > > + acpi_adr_space_setup setup, void *context) {
> > > > + int res;
> > > > + struct acpi_region *rgn;
> > > > +
> > > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > > + return -EINVAL;
> > > > +
> > > > + rgn = &acpi_regions[space_id];
> > > > + if (!acpi_region_managed(rgn))
> > > > + return -EINVAL;
> > > > +
> > > > + res = __acpi_install_region(rgn, space_id);
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + mutex_lock(&acpi_mutex_region);
> > > > + if (rgn->flags & ACPI_REGION_REGISTERED) {
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > + return -EBUSY;
> > > > + }
> > > > +
> > > > + rgn->handler = handler;
> > > > + rgn->setup = setup;
> > > > + rgn->context = context;
> > > > + rgn->flags |= ACPI_REGION_REGISTERED;
> > > > + atomic_set(&rgn->refcnt, 1);
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > +
> > > > + pr_info("Region %d registered\n", space_id);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(acpi_register_region);
> > > > +
> > > > +void acpi_unregister_region(acpi_adr_space_type space_id) {
> > > > + struct acpi_region *rgn;
> > > > +
> > > > + if (space_id >= ACPI_NUM_PREDEFINED_REGIONS)
> > > > + return;
> > > > +
> > > > + rgn = &acpi_regions[space_id];
> > > > + if (!acpi_region_managed(rgn))
> > > > + return;
> > > > +
> > > > + mutex_lock(&acpi_mutex_region);
> > > > + if (!(rgn->flags & ACPI_REGION_REGISTERED)) {
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > + return;
> > > > + }
> > > > + if (rgn->flags & ACPI_REGION_UNREGISTERING) {
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > + return;
> > >
> > > What about
> > >
> > > if ((rgn->flags & ACPI_REGION_UNREGISTERING)
> > > || !(rgn->flags & ACPI_REGION_REGISTERED)) {
> > > mutex_unlock(&acpi_mutex_region);
> > > return;
> > > }
> > >
> >
> > OK.
> >
> > > > + }
> > > > +
> > > > + rgn->flags |= ACPI_REGION_UNREGISTERING;
> > > > + rgn->handler = NULL;
> > > > + rgn->setup = NULL;
> > > > + rgn->context = NULL;
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > +
> > > > + while (atomic_read(&rgn->refcnt) > 1)
> > > > + schedule_timeout_uninterruptible(usecs_to_jiffies(5));
> > >
> > > Wouldn't it be better to use a wait queue here?
> >
> > Yes, I'll try.
>
> By the way, we do we need to do that?
I think you have additional replies on this in another email.
Let me reply you there.
Thanks for commenting.
Best regards
-Lv
>
> > > > + atomic_dec(&rgn->refcnt);
> > > > +
> > > > + mutex_lock(&acpi_mutex_region);
> > > > + rgn->flags &= ~(ACPI_REGION_REGISTERED |
> > > ACPI_REGION_UNREGISTERING);
> > > > + mutex_unlock(&acpi_mutex_region);
> > > > +
> > > > + pr_info("Region %d unregistered\n", space_id); }
> > > > +EXPORT_SYMBOL_GPL(acpi_unregister_region);
> > > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index
> > > > a2c2fbb..15fad0d 100644
> > > > --- a/include/acpi/acpi_bus.h
> > > > +++ b/include/acpi/acpi_bus.h
> > > > @@ -542,4 +542,9 @@ static inline int unregister_acpi_bus_type(void
> > > > *bus) { return 0; }
> > > >
> > > > #endif /* CONFIG_ACPI */
> > > >
> > > > +int acpi_register_region(acpi_adr_space_type space_id,
> > > > + acpi_adr_space_handler handler,
> > > > + acpi_adr_space_setup setup, void *context); void
> > > > +acpi_unregister_region(acpi_adr_space_type space_id);
> > > > +
> > > > #endif /*__ACPI_BUS_H__*/
>
> Thanks,
> Rafael
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 18+ messages in thread