[PATCH AUTOSEL 6.19-6.6] md-cluster: fix NULL pointer dereference in process_metadata_update
Sasha Levin <[email protected]>
| Newsgroups | gmane.linux.kernel.stable,gmane.linux.raid |
|---|---|
| Message-ID | <[email protected]> |
From: Jiasheng Jiang <[email protected]> [ Upstream commit f150e753cb8dd756085f46e86f2c35ce472e0a3c ] The function process_metadata_update() blindly dereferences the 'thread' pointer (acquired via rcu_dereference_protected) within the wait_event() macro. While the code comment states "daemon thread must exist", there is a valid race condition window during the MD array startup sequence (md_run): 1. bitmap_load() is called, which invokes md_cluster_ops->join(). 2. join() starts the "cluster_recv" thread (recv_daemon). 3. At this point, recv_daemon is active and processing messages. 4. However, mddev->thread (the main MD thread) is not initialized until later in md_run(). If a METADATA_UPDATED message is received from a remote node during this specific window, process_metadata_update() will be called while mddev->thread is still NULL, leading to a kernel panic. To fix this, we must validate the 'thread' pointer. If it is NULL, we release the held lock (no_new_dev_lockres) and return early, safely ignoring the update request as the array is not yet fully ready to process it. Link: https://lore.kernel.org/linux-raid/[email protected] Signed-off-by: Jiasheng Jiang <[email protected]> Signed-off-by: Yu Kuai <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: Now I have a comprehensive understanding of the issue. Let me summarize my analysis. --- ## Detailed Analysis ### 1. Commit Message Analysis The commit fixes a **NULL pointer dereference** in `process_metadata_update()` in `drivers/md/md-cluster.c`. The subject explicitly says "fix NULL pointer dereference" -- a strong indicator for stable. ### 2. Code Change Analysis The vulnerable code is at line 552-556 (before the fix): ```552:556:drivers/md/md-cluster.c /* daemaon thread must exist */ thread = rcu_dereference_protected(mddev->thread, true); wait_event(thread->wqueue, (got_lock = mddev_trylock(mddev)) || test_bit(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state)); ``` The code obtains `mddev->thread` via `rcu_dereference_protected()` and **immediately dereferences `thread->wqueue`** without any NULL check. If `thread` is NULL, this is a guaranteed kernel panic. **Critical comparison**: All other uses of `mddev->thread` in `md- cluster.c` (lines 352, 468, 571, 726, 1079) go through `md_wakeup_thread()`, which has a **built-in NULL check**: ```8520:8531:drivers/md/md.c void __md_wakeup_thread(struct md_thread __rcu *thread) { struct md_thread *t; t = rcu_dereference(thread); if (t) { pr_debug("md: waking up MD thread %s.\n", t->tsk->comm); set_bit(THREAD_WAKEUP, &t->flags); if (wq_has_sleeper(&t->wqueue)) wake_up(&t->wqueue); } } ``` So `process_metadata_update()` is the **only location** in the file that directly dereferences `mddev->thread` without safety. ### 3. The Race Condition The vulnerability was introduced in commit `0ba959774e939` ("md-cluster: use sync way to handle METADATA_UPDATED msg", 2017, v4.12). The author of that commit was aware of the `thread->wqueue` dependency -- they even wrote a follow-up commit `48df498daf62e` ("md: move bitmap_destroy to the beginning of __md_stop") that explicitly states: > "process_metadata_update is depended on mddev->thread->wqueue" > "clustered raid could possible hang if array received a METADATA_UPDATED msg after array unregistered mddev->thread" This follow-up only addressed the **shutdown ordering** (moving `bitmap_destroy` before `mddev_detach`), but did NOT add a NULL safety check for the startup/error paths. The race window during startup: - `md_run()` calls `pers->run()` which sets `mddev->thread` - Then `md_bitmap_create()` -> `join()` creates recv_thread - Then `bitmap_load()` -> `load_bitmaps()` enables message processing While the normal ordering seems safe, there are scenarios involving: - Error paths during bitmap creation where `mddev_detach()` is called (NULLing `mddev->thread`) while the recv_thread may still have work pending - Edge cases in `dm-raid` which has a different bitmap_load timing - Future code changes that could affect the ordering ### 4. The Fix The fix adds a simple NULL check: ```diff thread = rcu_dereference_protected(mddev->thread, true); + if (!thread) { + pr_warn("md-cluster: Received metadata update but MD thread is not ready\n"); + dlm_unlock_sync(cinfo->no_new_dev_lockres); + return; + } ``` The fix properly: - Checks for NULL before dereferencing `thread->wqueue` - Releases the DLM lock (`no_new_dev_lockres`) acquired earlier in the function (avoids deadlock on early return) - Logs a warning for debugging - Returns early, safely skipping the update (the array isn't fully ready anyway) - Removes the incorrect "daemaon" typo comment ### 5. Scope and Risk Assessment - **Lines changed**: +6/-1, single file - **Risk**: Near zero. The check only triggers when `thread` is NULL (abnormal case). Normal operation is completely unaffected. - **Subsystem**: MD RAID (clustered), mature subsystem present since v4.12 - **Could break something**: No. This is purely defensive -- adding a safety check that only activates in the error scenario. ### 6. User Impact - **Who is affected**: Users of clustered MD RAID (enterprise/SAN environments) - **Severity if triggered**: Kernel panic/oops (NULL pointer dereference) - **Affected stable trees**: All versions since v4.12 (5.4, 5.10, 5.15, 6.1, 6.6, 6.12, etc.) ### 7. Stable Criteria Checklist - **Obviously correct and tested**: Yes, trivially correct NULL check with proper cleanup - **Fixes a real bug**: Yes, NULL pointer dereference leading to kernel panic - **Important issue**: Yes, kernel crash - **Small and contained**: Yes, 6-line change in one function in one file - **No new features**: Correct - **Clean backport**: The fix should apply cleanly to all stable trees since the code hasn't materially changed since v4.12 **YES** drivers/md/md-cluster.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c index 11f1e91d387d8..896279988dfd5 100644 --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -549,8 +549,13 @@ static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR); - /* daemaon thread must exist */ thread = rcu_dereference_protected(mddev->thread, true); + if (!thread) { + pr_warn("md-cluster: Received metadata update but MD thread is not ready\n"); + dlm_unlock_sync(cinfo->no_new_dev_lockres); + return; + } + wait_event(thread->wqueue, (got_lock = mddev_trylock(mddev)) || test_bit(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state)); -- 2.51.0