From 9930702cfebb24acf6c000b55541239095447e47 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 17 May 2023 16:54:12 +0800 Subject: [PATCH 01/10] ipmi_watchdog: Fix read syscall not responding to signals during sleep Read syscall cannot response to sigals when data_to_read remains at 0 and the while loop cannot break. Check signal_pending in the loop. Reported-by: Zhen Ni Message-Id: <20230517085412.367022-1-zhen.ni@easystack.cn> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_watchdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index 0d4a8dcacfd4b..9a459257489f0 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -802,7 +802,7 @@ static ssize_t ipmi_read(struct file *file, init_waitqueue_entry(&wait, current); add_wait_queue(&read_q, &wait); - while (!data_to_read) { + while (!data_to_read && !signal_pending(current)) { set_current_state(TASK_INTERRUPTIBLE); spin_unlock_irq(&ipmi_read_lock); schedule(); From e64c82b8064174a23434094d13a142254c138099 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= Date: Thu, 25 May 2023 22:40:21 +0200 Subject: [PATCH 02/10] ipmi: Switch i2c drivers back to use .probe() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new() call-back type"), all drivers being converted to .probe_new() and then 03c835f498b5 ("i2c: Switch .probe() to not take an id parameter") convert back to (the new) .probe() to be able to eventually drop .probe_new() from struct i2c_driver. Signed-off-by: Uwe Kleine-König Message-Id: <20230525204021.696858-1-u.kleine-koenig@pengutronix.de> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmb_dev_int.c | 2 +- drivers/char/ipmi/ipmi_ipmb.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 2 +- drivers/char/ipmi/ssif_bmc.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c index a0e9e80d92eeb..49100845fcb7b 100644 --- a/drivers/char/ipmi/ipmb_dev_int.c +++ b/drivers/char/ipmi/ipmb_dev_int.c @@ -366,7 +366,7 @@ static struct i2c_driver ipmb_driver = { .name = "ipmb-dev", .acpi_match_table = ACPI_PTR(acpi_ipmb_id), }, - .probe_new = ipmb_probe, + .probe = ipmb_probe, .remove = ipmb_remove, .id_table = ipmb_id, }; diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c index 3f1c9f1573e78..4e335832fc264 100644 --- a/drivers/char/ipmi/ipmi_ipmb.c +++ b/drivers/char/ipmi/ipmi_ipmb.c @@ -572,7 +572,7 @@ static struct i2c_driver ipmi_ipmb_driver = { .name = DEVICE_NAME, .of_match_table = of_ipmi_ipmb_match, }, - .probe_new = ipmi_ipmb_probe, + .probe = ipmi_ipmb_probe, .remove = ipmi_ipmb_remove, .id_table = ipmi_ipmb_id, }; diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 3b921c78ba083..c6c9bcf6bf55c 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -2054,7 +2054,7 @@ static struct i2c_driver ssif_i2c_driver = { .driver = { .name = DEVICE_NAME }, - .probe_new = ssif_probe, + .probe = ssif_probe, .remove = ssif_remove, .alert = ssif_alert, .id_table = ssif_id, diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c index caee848261e9f..56346fb328727 100644 --- a/drivers/char/ipmi/ssif_bmc.c +++ b/drivers/char/ipmi/ssif_bmc.c @@ -860,7 +860,7 @@ static struct i2c_driver ssif_bmc_driver = { .name = DEVICE_NAME, .of_match_table = ssif_bmc_match, }, - .probe_new = ssif_bmc_probe, + .probe = ssif_bmc_probe, .remove = ssif_bmc_remove, .id_table = ssif_bmc_id, }; From 02210d52641a2f0d35daf67d7bad2801baeb72da Mon Sep 17 00:00:00 2001 From: Krzysztof Kozlowski Date: Fri, 9 Jun 2023 16:07:29 +0200 Subject: [PATCH 03/10] dt-bindings: ipmi: aspeed,ast2400-kcs-bmc: drop unneeded quotes Cleanup bindings dropping unneeded quotes. Once all these are fixed, checking for this can be enabled in yamllint. Signed-off-by: Krzysztof Kozlowski Message-Id: <20230609140729.64799-1-krzysztof.kozlowski@linaro.org> Acked-by: Andrew Jeffery Signed-off-by: Corey Minyard --- .../devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml index 4ff6fabfcb309..129e32c4c7741 100644 --- a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml +++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml @@ -41,7 +41,7 @@ properties: - description: STR register aspeed,lpc-io-reg: - $ref: '/schemas/types.yaml#/definitions/uint32-array' + $ref: /schemas/types.yaml#/definitions/uint32-array minItems: 1 maxItems: 2 description: | @@ -50,7 +50,7 @@ properties: status address may be optionally provided. aspeed,lpc-interrupts: - $ref: "/schemas/types.yaml#/definitions/uint32-array" + $ref: /schemas/types.yaml#/definitions/uint32-array minItems: 2 maxItems: 2 description: | @@ -63,12 +63,12 @@ properties: kcs_chan: deprecated: true - $ref: '/schemas/types.yaml#/definitions/uint32' + $ref: /schemas/types.yaml#/definitions/uint32 description: The LPC channel number in the controller kcs_addr: deprecated: true - $ref: '/schemas/types.yaml#/definitions/uint32' + $ref: /schemas/types.yaml#/definitions/uint32 description: The host CPU IO map address required: From c5586d0f711e9744d0cade39b0c4a2d116a333ca Mon Sep 17 00:00:00 2001 From: Jiasheng Jiang Date: Mon, 19 Jun 2023 17:28:02 +0800 Subject: [PATCH 04/10] ipmi:ssif: Add check for kstrdup Add check for the return value of kstrdup() and return the error if it fails in order to avoid NULL pointer dereference. Fixes: c4436c9149c5 ("ipmi_ssif: avoid registering duplicate ssif interface") Signed-off-by: Jiasheng Jiang Message-Id: <20230619092802.35384-1-jiasheng@iscas.ac.cn> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_ssif.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index c6c9bcf6bf55c..07cfde579cfb9 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1600,6 +1600,11 @@ static int ssif_add_infos(struct i2c_client *client) info->addr_src = SI_ACPI; info->client = client; info->adapter_name = kstrdup(client->adapter->name, GFP_KERNEL); + if (!info->adapter_name) { + kfree(info); + return -ENOMEM; + } + info->binfo.addr = client->addr; list_add_tail(&info->link, &ssif_infos); return 0; From b8d72e32e1453d37ee5c8a219f24e7eeadc471ef Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Mon, 19 Jun 2023 11:43:33 -0500 Subject: [PATCH 05/10] ipmi:ssif: Fix a memory leak when scanning for an adapter The adapter scan ssif_info_find() sets info->adapter_name if the adapter info came from SMBIOS, as it's not set in that case. However, this function can be called more than once, and it will leak the adapter name if it had already been set. So check for NULL before setting it. Fixes: c4436c9149c5 ("ipmi_ssif: avoid registering duplicate ssif interface") Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_ssif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 07cfde579cfb9..3d21c39e2060e 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1400,7 +1400,7 @@ static struct ssif_addr_info *ssif_info_find(unsigned short addr, restart: list_for_each_entry(info, &ssif_infos, link) { if (info->binfo.addr == addr) { - if (info->addr_src == SI_SMBIOS) + if (info->addr_src == SI_SMBIOS && !info->adapter_name) info->adapter_name = kstrdup(adapter_name, GFP_KERNEL); From 392fa3a3abdb03105c8767b7fb176bc8793349f5 Mon Sep 17 00:00:00 2001 From: Ivan Orlov Date: Tue, 20 Jun 2023 16:37:02 +0200 Subject: [PATCH 06/10] ipmi: make ipmi_class a static const structure Now that the driver core allows for struct class to be in read-only memory, move the ipmi_class structure to be declared at build time placing it into read-only memory, instead of having to be dynamically allocated at boot time. Cc: Corey Minyard Cc: openipmi-developer@lists.sourceforge.net Suggested-by: Greg Kroah-Hartman Signed-off-by: Ivan Orlov Signed-off-by: Greg Kroah-Hartman Message-Id: <20230620143701.577657-2-gregkh@linuxfoundation.org> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_devintf.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c index 73e5a9e28f851..332082e02ea54 100644 --- a/drivers/char/ipmi/ipmi_devintf.c +++ b/drivers/char/ipmi/ipmi_devintf.c @@ -807,7 +807,9 @@ struct ipmi_reg_list { static LIST_HEAD(reg_list); static DEFINE_MUTEX(reg_list_mutex); -static struct class *ipmi_class; +static const struct class ipmi_class = { + .name = "ipmi", +}; static void ipmi_new_smi(int if_num, struct device *device) { @@ -822,7 +824,7 @@ static void ipmi_new_smi(int if_num, struct device *device) entry->dev = dev; mutex_lock(®_list_mutex); - device_create(ipmi_class, device, dev, NULL, "ipmi%d", if_num); + device_create(&ipmi_class, device, dev, NULL, "ipmi%d", if_num); list_add(&entry->link, ®_list); mutex_unlock(®_list_mutex); } @@ -840,7 +842,7 @@ static void ipmi_smi_gone(int if_num) break; } } - device_destroy(ipmi_class, dev); + device_destroy(&ipmi_class, dev); mutex_unlock(®_list_mutex); } @@ -860,15 +862,13 @@ static int __init init_ipmi_devintf(void) pr_info("ipmi device interface\n"); - ipmi_class = class_create("ipmi"); - if (IS_ERR(ipmi_class)) { - pr_err("ipmi: can't register device class\n"); - return PTR_ERR(ipmi_class); - } + rv = class_register(&ipmi_class); + if (rv) + return rv; rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops); if (rv < 0) { - class_destroy(ipmi_class); + class_unregister(&ipmi_class); pr_err("ipmi: can't get major %d\n", ipmi_major); return rv; } @@ -880,7 +880,7 @@ static int __init init_ipmi_devintf(void) rv = ipmi_smi_watcher_register(&smi_watcher); if (rv) { unregister_chrdev(ipmi_major, DEVICE_NAME); - class_destroy(ipmi_class); + class_unregister(&ipmi_class); pr_warn("ipmi: can't register smi watcher\n"); return rv; } @@ -895,11 +895,11 @@ static void __exit cleanup_ipmi(void) mutex_lock(®_list_mutex); list_for_each_entry_safe(entry, entry2, ®_list, link) { list_del(&entry->link); - device_destroy(ipmi_class, entry->dev); + device_destroy(&ipmi_class, entry->dev); kfree(entry); } mutex_unlock(®_list_mutex); - class_destroy(ipmi_class); + class_unregister(&ipmi_class); ipmi_smi_watcher_unregister(&smi_watcher); unregister_chrdev(ipmi_major, DEVICE_NAME); } From e87443a5f68da7bacefe933c90453ae7215263b3 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Tue, 20 Jun 2023 09:59:53 -0500 Subject: [PATCH 07/10] ipmi: Change request_module to request_module_nowait When probing for an ACPI-specified IPMI device, the code would request that the acpi_ipmi module be loaded ACPI operations through IPMI can be performed. This could happen through module load context, for instance, if an I2C module is loaded that caused the IPMI interface to be probed. This is not allowed because a synchronous module load in this context can result in an deadlock, and I was getting a warning: [ 23.967853] WARNING: CPU: 0 PID: 21 at kernel/module/kmod.c:144 __request_module+0x1de/0x2d0 [ 23.968852] Modules linked in: i2c_i801 ipmi_ssif The IPMI driver is not dependent on acpi_ipmi, so just change the called to request_module_nowait to make the load asynchronous. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_platform.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c index 505cc978c97a0..70f73911457b5 100644 --- a/drivers/char/ipmi/ipmi_si_platform.c +++ b/drivers/char/ipmi/ipmi_si_platform.c @@ -381,7 +381,7 @@ static int acpi_ipmi_probe(struct platform_device *pdev) dev_info(dev, "%pR regsize %d spacing %d irq %d\n", res, io.regsize, io.regspacing, io.irq); - request_module("acpi_ipmi"); + request_module_nowait("acpi_ipmi"); return ipmi_si_add_smi(&io); } diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 3d21c39e2060e..df8dd50b4cbed 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1439,7 +1439,7 @@ static bool check_acpi(struct ssif_info *ssif_info, struct device *dev) if (acpi_handle) { ssif_info->addr_source = SI_ACPI; ssif_info->addr_info.acpi_info.acpi_handle = acpi_handle; - request_module("acpi_ipmi"); + request_module_nowait("acpi_ipmi"); return true; } #endif From 6cf1a126de2992b4efe1c3c4d398f8de4aed6e3f Mon Sep 17 00:00:00 2001 From: Yi Yang Date: Thu, 29 Jun 2023 20:33:28 +0800 Subject: [PATCH 08/10] ipmi_si: fix a memleak in try_smi_init() Kmemleak reported the following leak info in try_smi_init(): unreferenced object 0xffff00018ecf9400 (size 1024): comm "modprobe", pid 2707763, jiffies 4300851415 (age 773.308s) backtrace: [<000000004ca5b312>] __kmalloc+0x4b8/0x7b0 [<00000000953b1072>] try_smi_init+0x148/0x5dc [ipmi_si] [<000000006460d325>] 0xffff800081b10148 [<0000000039206ea5>] do_one_initcall+0x64/0x2a4 [<00000000601399ce>] do_init_module+0x50/0x300 [<000000003c12ba3c>] load_module+0x7a8/0x9e0 [<00000000c246fffe>] __se_sys_init_module+0x104/0x180 [<00000000eea99093>] __arm64_sys_init_module+0x24/0x30 [<0000000021b1ef87>] el0_svc_common.constprop.0+0x94/0x250 [<0000000070f4f8b7>] do_el0_svc+0x48/0xe0 [<000000005a05337f>] el0_svc+0x24/0x3c [<000000005eb248d6>] el0_sync_handler+0x160/0x164 [<0000000030a59039>] el0_sync+0x160/0x180 The problem was that when an error occurred before handlers registration and after allocating `new_smi->si_sm`, the variable wouldn't be freed in the error handling afterwards since `shutdown_smi()` hadn't been registered yet. Fix it by adding a `kfree()` in the error handling path in `try_smi_init()`. Cc: stable@vger.kernel.org # 4.19+ Fixes: 7960f18a5647 ("ipmi_si: Convert over to a shutdown handler") Signed-off-by: Yi Yang Co-developed-by: GONG, Ruiqi Signed-off-by: GONG, Ruiqi Message-Id: <20230629123328.2402075-1-gongruiqi@huaweicloud.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_intf.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index abddd7e43a9a6..5cd031f3fc970 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2082,6 +2082,11 @@ static int try_smi_init(struct smi_info *new_smi) new_smi->io.io_cleanup = NULL; } + if (rv && new_smi->si_sm) { + kfree(new_smi->si_sm); + new_smi->si_sm = NULL; + } + return rv; } From b02bb79eee074f07acdfde540f2d4fe2a04471d8 Mon Sep 17 00:00:00 2001 From: Chengfeng Ye Date: Tue, 27 Jun 2023 15:24:49 +0000 Subject: [PATCH 09/10] ipmi: fix potential deadlock on &kcs_bmc->lock As kcs_bmc_handle_event() is executed inside both a timer and a hardirq, it should disable irq before lock acquisition otherwise deadlock could happen if the timmer is preemtped by the irq. Possible deadlock scenario: aspeed_kcs_check_obe() (timer) -> kcs_bmc_handle_event() -> spin_lock(&kcs_bmc->lock) -> aspeed_kcs_irq() -> kcs_bmc_handle_event() -> spin_lock(&kcs_bmc->lock) (deadlock here) This flaw was found using an experimental static analysis tool we are developing for irq-related deadlock. The tentative patch fix the potential deadlock by spin_lock_irqsave() Signed-off-by: Chengfeng Ye Message-Id: <20230627152449.36093-1-dg573847474@gmail.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/kcs_bmc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c index 03d02a848f3a2..8b1161d5194ae 100644 --- a/drivers/char/ipmi/kcs_bmc.c +++ b/drivers/char/ipmi/kcs_bmc.c @@ -56,12 +56,13 @@ irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc) { struct kcs_bmc_client *client; irqreturn_t rc = IRQ_NONE; + unsigned long flags; - spin_lock(&kcs_bmc->lock); + spin_lock_irqsave(&kcs_bmc->lock, flags); client = kcs_bmc->client; if (client) rc = client->ops->event(client); - spin_unlock(&kcs_bmc->lock); + spin_unlock_irqrestore(&kcs_bmc->lock, flags); return rc; } From d40f09c1a23024f0e550d9423f4d389672e1dfaf Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Wed, 9 Aug 2023 21:05:17 +0000 Subject: [PATCH 10/10] ipmi_si: fix -Wvoid-pointer-to-enum-cast warning With W=1 we see the following warning: | drivers/char/ipmi/ipmi_si_platform.c:272:15: error: \ | cast to smaller integer type 'enum si_type' from \ | 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast] | 272 | io.si_type = (enum si_type) match->data; | | ^~~~~~~~~~~~~~~~~~~~~~~~~~ This is due to the fact that the `si_type` enum members are int-width and a cast from pointer-width down to int will cause truncation and possible data loss. Although in this case `si_type` has only a few enumerated fields and thus there is likely no data loss occurring. Nonetheless, this patch is necessary to the goal of promoting this warning out of W=1. Link: https://github.com/ClangBuiltLinux/linux/issues/1902 Link: https://lore.kernel.org/llvm/202308081000.tTL1ElTr-lkp@intel.com/ Reported-by: kernel test robot Signed-off-by: Justin Stitt Message-Id: <20230809-cbl-1902-v1-1-92def12d1dea@google.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_si_platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c index 70f73911457b5..c3d8ac7873baa 100644 --- a/drivers/char/ipmi/ipmi_si_platform.c +++ b/drivers/char/ipmi/ipmi_si_platform.c @@ -269,7 +269,7 @@ static int of_ipmi_probe(struct platform_device *pdev) } memset(&io, 0, sizeof(io)); - io.si_type = (enum si_type) match->data; + io.si_type = (unsigned long) match->data; io.addr_source = SI_DEVICETREE; io.irq_setup = ipmi_std_irq_setup;