From: Corey Minyard <co...@minyard.net>
[ Upstream commit 6bd0eb6d759b9a22c5509ea04e19c2e8407ba418 ]
It was possible for the SSIF thread to stop and quit before the
kthread_stop() call because ssif->stopping was set before the
stop. So only exit the SSIF thread is kthread_should_stop()
returns true.
There is no need to wake the thread, as the wait will be interrupted
by kthread_stop().
Signed-off-by: Corey Minyard <cminy...@mvista.com>
Signed-off-by: Sasha Levin <sas...@kernel.org>
---
Based on my analysis of the commit and the current kernel code, here's
my assessment: **YES** This commit should be backported to stable kernel
trees. ## Detailed Analysis: ### Nature of the Bug Fix: The commit fixes
a **race condition during shutdown** in the IPMI SSIF driver. This is a
critical synchronization bug that can lead to system instability during
driver cleanup. ### Specific Race Condition: Looking at the code
changes, the race occurs between: 1. **Thread termination logic**: In
`ipmi_ssif_thread()` at line 484, there was a check for
`ssif_info->stopping` that would break out of the thread loop 2.
**Shutdown sequence**: In `shutdown_ssif()` at line 1270,
`ssif_info->stopping` is set to `true`, followed by
`complete(&ssif_info->wake_thread)` and then `kthread_stop()` ### The
Race Window: The problematic sequence was: 1. `shutdown_ssif()` sets
`ssif_info->stopping = true` (line 1270) 2. `shutdown_ssif()` calls
`complete(&ssif_info->wake_thread)` (line 1274) 3. The thread wakes up,
sees `stopping = true`, and exits the loop (line 484-485) 4. The thread
function returns and the thread terminates 5. `shutdown_ssif()` calls
`kthread_stop()` (line 1275) on an already-dead thread ### The Fix: The
commit removes the `ssif_info->stopping` check from the thread loop
(lines 484-485 removed) and the unnecessary
`complete(&ssif_info->wake_thread)` call from shutdown (line 1274
removed). Now: 1. The thread **only** exits when `kthread_should_stop()`
returns true (line 478) 2. `kthread_stop()` will properly signal the
thread to stop and wait for it 3. The
`wait_for_completion_interruptible()` call will be interrupted by
`kthread_stop()` ### Why This Should Be Backported: 1. **Critical Bug**:
Race conditions during shutdown can lead to kernel crashes, memory
corruption, or system hangs during reboot/module unload 2. **Minimal
Risk**: The changes are very small and surgical: - Removes 2 lines from
thread logic - Removes 1 line from shutdown logic - No new functionality
introduced - No architectural changes 3. **Follows Stable Tree
Criteria**: - Important bugfix (race condition) - Small, contained
change - Low regression risk - Affects critical kernel subsystem (IPMI)
4. **Similar Pattern**: Looking at the historical commits, commit #4
("Remove rtc_us_timer") and commit #5 ("Rename idle state and check")
both had `Cc: sta...@vger.kernel.org` tags and were marked as
backportable, showing this maintainer's pattern of marking important
SSIF fixes for stable backporting 5. **Production Impact**: IPMI is
commonly used in server environments where system stability during
shutdown/reboot is critical 6. **Clean Fix**: The fix aligns with proper
kernel thread management practices - relying on the standard
`kthread_should_stop()` mechanism rather than custom stopping flags The
commit addresses a real concurrency bug with a clean, minimal fix that
follows kernel best practices for thread management. The risk of
regression is extremely low while the benefit of preventing shutdown
races is significant.
drivers/char/ipmi/ipmi_ssif.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 0b45b07dec22c..5bf038e620c75 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -481,8 +481,6 @@ static int ipmi_ssif_thread(void *data)
/* Wait for something to do */
result = wait_for_completion_interruptible(
&ssif_info->wake_thread);
- if (ssif_info->stopping)
- break;
if (result == -ERESTARTSYS)
continue;
init_completion(&ssif_info->wake_thread);
@@ -1270,10 +1268,8 @@ static void shutdown_ssif(void *send_info)
ssif_info->stopping = true;
timer_delete_sync(&ssif_info->watch_timer);
timer_delete_sync(&ssif_info->retry_timer);
- if (ssif_info->thread) {
- complete(&ssif_info->wake_thread);
+ if (ssif_info->thread)
kthread_stop(ssif_info->thread);
- }
}
static void ssif_remove(struct i2c_client *client)
--
2.39.5
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
