md/raid5: fix possible null-pointer dereferences in raid5_store_group_thread_cnt()
PATCH v2 1/1 md/raid5: fix possible null-pointer dereferences in raid5_store_group_thread_cnt()
The variable mddev->private is first assigned to conf and then checked:
conf = mddev->private;
if (!conf) ...If conf is NULL, then mddev->private is also NULL. In this case, null-pointer dereferences can occur when calling raid5_quiesce():
raid5_quiesce(mddev, true); raid5_quiesce(mddev, false);
since mddev->private is assigned to conf again in raid5_quiesce(), and conf is dereferenced in several places, for example:
conf->quiesce = 0; wake_up(&conf->wait_for_quiescent);
To fix this issue, the function should unlock mddev and return before invoking raid5_quiesce() when conf is NULL, following the existing pattern in raid5_change_consistency_policy().
---
v2:
* Move the NULL check and early return ahead of the first call to raid5_quiesce().
Thanks to Yu Kuai for helpful advice.
--- drivers/md/raid5.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/md/raid5.c +6 -4
@@ -7187,12 +7187,14 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len) err =mddev_suspend_and_lock (mddev);if (err)return err; + conf = mddev->private; +if (!conf) { +mddev_unlock_and_resume (mddev); +return -ENODEV ; + }raid5_quiesce (mddev, true); - conf = mddev->private; -if (!conf) - err = -ENODEV ; -else if (new != conf->worker_cnt_per_group) { +if (new != conf->worker_cnt_per_group) { old_groups = conf->worker_groups;if (old_groups)flush_workqueue (raid5_wq);
Signature · 1 line
2.43.0
Quoted 57 lines · Tuo Li
The variable
mddev->privateis first assigned to conf and then checked:conf = mddev->private; if (!conf) ...If conf is NULL, then
mddev->privateis also NULL. In this case, null-pointer dereferences can occur when callingraid5_quiesce():raid5_quiesce(mddev, true); raid5_quiesce(mddev, false);since
mddev->privateis assigned to conf again inraid5_quiesce(), and conf is dereferenced in several places, for example:conf->quiesce = 0; wake_up(&conf->wait_for_quiescent);To fix this issue, the function should unlock mddev and return before invoking
raid5_quiesce()when conf is NULL, following the existing pattern inraid5_change_consistency_policy().Fixes fa1944bbe622 ("md/raid5: Wait sync io to finish before changing group cnt")Signed-off-by Tuo Li <[email protected]>---
v2:
* Move the NULL check and early return ahead of the first call toraid5_quiesce().
Thanks to Yu Kuai for helpful advice.--- drivers/md/raid5.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)drivers/md/raid5.c +6
-5@@ -7187,12 +7187,14 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len) err =mddev_suspend_and_lock (mddev);if (err)return err; + conf = mddev->private; +if (!conf) { +mddev_unlock_and_resume (mddev); +return -ENODEV ; + }raid5_quiesce (mddev, true); - conf = mddev->private; -if (!conf) - err = -ENODEV ; -else if (new != conf->worker_cnt_per_group) { +if (new != conf->worker_cnt_per_group) { old_groups = conf->worker_groups;if (old_groups)flush_workqueue (raid5_wq); --
Quoted 3 lines
2.43.0
Thanks for the patch. It looks good to me.
Dear Tuo,
Thank you for your patch.
Am 25.12.25 um 18:03 schrieb Tuo Li:
Quoted 4 lines
The variable
mddev->privateis first assigned to conf and then checked:conf = mddev->private; if (!conf) ...
Should you resend, please align conf and if`.
Quoted 40 lines
If conf is NULL, then
mddev->privateis also NULL. In this case, null-pointer dereferences can occur when callingraid5_quiesce():raid5_quiesce(mddev, true); raid5_quiesce(mddev, false);since
mddev->privateis assigned to conf again inraid5_quiesce(), and conf is dereferenced in several places, for example:conf->quiesce = 0; wake_up(&conf->wait_for_quiescent);To fix this issue, the function should unlock mddev and return before invoking
raid5_quiesce()when conf is NULL, following the existing pattern inraid5_change_consistency_policy().Fixes fa1944bbe622 ("md/raid5: Wait sync io to finish before changing group cnt")Signed-off-by Tuo Li <[email protected]>---
v2:
* Move the NULL check and early return ahead of the first call toraid5_quiesce().
Thanks to Yu Kuai for helpful advice.--- drivers/md/raid5.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)drivers/md/raid5.c +5
-0@@ -7187,12 +7187,14 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len) err =mddev_suspend_and_lock (mddev);if (err)return err; + conf = mddev->private; +if (!conf) { +mddev_unlock_and_resume (mddev); +return -ENODEV ; + }
drivers/md/raid5.c raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len) · line 7200
Quoted 10 lines
drivers/md/raid5.c +1
-4raid5_quiesce (mddev, true); - conf = mddev->private; -if (!conf) - err = -ENODEV ; -else if (new != conf->worker_cnt_per_group) { +if (new != conf->worker_cnt_per_group) { old_groups = conf->worker_groups;if (old_groups)flush_workqueue (raid5_wq);
Kind regards,
Paul
Yu Kuai
AppliedQuoted 23 lines · Tuo Li
The variable
mddev->privateis first assigned to conf and then checked:conf = mddev->private; if (!conf) ...If conf is NULL, then
mddev->privateis also NULL. In this case, null-pointer dereferences can occur when callingraid5_quiesce():raid5_quiesce(mddev, true); raid5_quiesce(mddev, false);since
mddev->privateis assigned to conf again inraid5_quiesce(), and conf is dereferenced in several places, for example:conf->quiesce = 0; wake_up(&conf->wait_for_quiescent);To fix this issue, the function should unlock mddev and return before invoking
raid5_quiesce()when conf is NULL, following the existing pattern inraid5_change_consistency_policy().Fixes fa1944bbe622 ("md/raid5: Wait sync io to finish before changing group cnt")Signed-off-by Tuo Li<[email protected]>
Quoted 8 lines
---
v2:
* Move the NULL check and early return ahead of the first call toraid5_quiesce().
Thanks to Yu Kuai for helpful advice.--- drivers/md/raid5.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Applied to md-6.19
Signature · 2 lines
Thansk, Kuai