| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <d8ba0843-cbeb-4a91-b658-34b21e8b36fe@fnnas.com>
Date: Thu, 30 Oct 2025 10:11:40 +0800
From: "Yu Kuai" <yukuai@...as.com>
To: "Kenta Akagi" <k@...l.me>, "Song Liu" <song@...nel.org>,
"Shaohua Li" <shli@...com>, "Mariusz Tkaczyk" <mtkaczyk@...nel.org>,
"Guoqing Jiang" <jgq516@...il.com>
Cc: <linux-raid@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 02/16] md: serialize md_error()
Hi,
在 2025/10/27 23:04, Kenta Akagi 写道:
> Serialize the md_error() function in preparation for the introduction of
> a conditional md_error() in a subsequent commit. The conditional
> md_error() is intended to prevent unintentional setting of MD_BROKEN
> during RAID1/10 failfast handling.
>
> To enhance the failfast bio error handler, it must verify that the
> affected rdev is not the last working device before marking it as
> faulty. Without serialization, a race condition can occur if multiple
> failfast bios attempt to call the error handler concurrently:
>
> failfast bio1 failfast bio2
> --- ---
> md_cond_error(md,rdev1,bio) md_cond_error(md,rdev2,bio)
> if(!is_degraded(md)) if(!is_degraded(md))
> raid1_error(md,rdev1) raid1_error(md,rdev2)
> spin_lock(md)
> set_faulty(rdev1)
> spin_unlock(md)
> spin_lock(md)
> set_faulty(rdev2)
> set_broken(md)
> spin_unlock(md)
>
> This can unintentionally cause the array to stop in situations where the
> 'Last' rdev should not be marked as Faulty.
>
> This commit serializes the md_error() function for all RAID
> personalities to avoid this race condition. Future commits will
> introduce a conditional md_error() specifically for failfast bio
> handling.
>
> Serialization is applied to both the standard and conditional md_error()
> for the following reasons:
>
> - Both use the same error-handling mechanism, so it's clearer to
> serialize them consistently.
> - The md_error() path is cold, meaning serialization has no performance
> impact.
>
> Signed-off-by: Kenta Akagi <k@...l.me>
> ---
> drivers/md/md-linear.c | 1 +
> drivers/md/md.c | 10 +++++++++-
> drivers/md/md.h | 1 +
> drivers/md/raid0.c | 1 +
> drivers/md/raid1.c | 6 +-----
> drivers/md/raid10.c | 9 ++-------
> drivers/md/raid5.c | 4 +---
> 7 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> index 7033d982d377..0f6893e4b9f5 100644
> --- a/drivers/md/md-linear.c
> +++ b/drivers/md/md-linear.c
> @@ -298,6 +298,7 @@ static void linear_status(struct seq_file *seq, struct mddev *mddev)
> }
>
> static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
This is fine, however, I personally prefer lockdep_assert_held(). :)
Otherwise LGTM.
Thanks,
Kuai
> {
> if (!test_and_set_bit(MD_BROKEN, &mddev->flags)) {
> char *md_name = mdname(mddev);
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d667580e3125..4ad9cb0ac98c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8444,7 +8444,8 @@ void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **threadp)
> }
> EXPORT_SYMBOL(md_unregister_thread);
>
> -void md_error(struct mddev *mddev, struct md_rdev *rdev)
> +void _md_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
> {
> if (!rdev || test_bit(Faulty, &rdev->flags))
> return;
> @@ -8469,6 +8470,13 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
> queue_work(md_misc_wq, &mddev->event_work);
> md_new_event();
> }
> +
> +void md_error(struct mddev *mddev, struct md_rdev *rdev)
> +{
> + spin_lock(&mddev->device_lock);
> + _md_error(mddev, rdev);
> + spin_unlock(&mddev->device_lock);
> +}
> EXPORT_SYMBOL(md_error);
>
> /* seq_file implementation /proc/mdstat */
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 64ac22edf372..c982598cbf97 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -913,6 +913,7 @@ extern void md_write_start(struct mddev *mddev, struct bio *bi);
> extern void md_write_inc(struct mddev *mddev, struct bio *bi);
> extern void md_write_end(struct mddev *mddev);
> extern void md_done_sync(struct mddev *mddev, int blocks, int ok);
> +void _md_error(struct mddev *mddev, struct md_rdev *rdev);
> extern void md_error(struct mddev *mddev, struct md_rdev *rdev);
> extern void md_finish_reshape(struct mddev *mddev);
> void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index e443e478645a..8cf3caf9defd 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -625,6 +625,7 @@ static void raid0_status(struct seq_file *seq, struct mddev *mddev)
> }
>
> static void raid0_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
> {
> if (!test_and_set_bit(MD_BROKEN, &mddev->flags)) {
> char *md_name = mdname(mddev);
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7924d5ee189d..202e510f73a4 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1749,11 +1749,9 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
> * &mddev->fail_last_dev is off.
> */
> static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
> {
> struct r1conf *conf = mddev->private;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&conf->mddev->device_lock, flags);
>
> if (test_bit(In_sync, &rdev->flags) &&
> (conf->raid_disks - mddev->degraded) == 1) {
> @@ -1761,7 +1759,6 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
>
> if (!mddev->fail_last_dev) {
> conf->recovery_disabled = mddev->recovery_disabled;
> - spin_unlock_irqrestore(&conf->mddev->device_lock, flags);
> return;
> }
> }
> @@ -1769,7 +1766,6 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> if (test_and_clear_bit(In_sync, &rdev->flags))
> mddev->degraded++;
> set_bit(Faulty, &rdev->flags);
> - spin_unlock_irqrestore(&conf->mddev->device_lock, flags);
> /*
> * if recovery is running, make sure it aborts.
> */
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 57c887070df3..25c0ab09807b 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1993,19 +1993,15 @@ static int enough(struct r10conf *conf, int ignore)
> * &mddev->fail_last_dev is off.
> */
> static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
> {
> struct r10conf *conf = mddev->private;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&conf->mddev->device_lock, flags);
>
> if (test_bit(In_sync, &rdev->flags) && !enough(conf, rdev->raid_disk)) {
> set_bit(MD_BROKEN, &mddev->flags);
>
> - if (!mddev->fail_last_dev) {
> - spin_unlock_irqrestore(&conf->mddev->device_lock, flags);
> + if (!mddev->fail_last_dev)
> return;
> - }
> }
> if (test_and_clear_bit(In_sync, &rdev->flags))
> mddev->degraded++;
> @@ -2015,7 +2011,6 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
> set_bit(Faulty, &rdev->flags);
> set_mask_bits(&mddev->sb_flags, 0,
> BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
> - spin_unlock_irqrestore(&conf->mddev->device_lock, flags);
> pr_crit("md/raid10:%s: Disk failure on %pg, disabling device.\n"
> "md/raid10:%s: Operation continuing on %d devices.\n",
> mdname(mddev), rdev->bdev,
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 3350dcf9cab6..d1372b1bc405 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2905,15 +2905,14 @@ static void raid5_end_write_request(struct bio *bi)
> }
>
> static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
> + __must_hold(&mddev->device_lock)
> {
> struct r5conf *conf = mddev->private;
> - unsigned long flags;
> pr_debug("raid456: error called\n");
>
> pr_crit("md/raid:%s: Disk failure on %pg, disabling device.\n",
> mdname(mddev), rdev->bdev);
>
> - spin_lock_irqsave(&conf->mddev->device_lock, flags);
> set_bit(Faulty, &rdev->flags);
> clear_bit(In_sync, &rdev->flags);
> mddev->degraded = raid5_calc_degraded(conf);
> @@ -2929,7 +2928,6 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
> mdname(mddev), conf->raid_disks - mddev->degraded);
> }
>
> - spin_unlock_irqrestore(&conf->mddev->device_lock, flags);
> set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>
> set_bit(Blocked, &rdev->flags);
Powered by blists - more mailing lists