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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3624f83b-cfb3-9b1a-8cc8-8caffba7a786@huaweicloud.com>
Date: Mon, 1 Sep 2025 10:16:36 +0800
From: Li Nan <linan666@...weicloud.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, Li Nan <linan666@...weicloud.com>
Cc: song@...nel.org, yukuai3@...wei.com, linux-raid@...r.kernel.org,
 linux-kernel@...r.kernel.org, yangerkun@...wei.com, yi.zhang@...wei.com
Subject: Re: [PATCH] md: ensure consistent action state in md_do_sync



在 2025/8/30 17:51, Paul Menzel 写道:
> Dear Nan,
> 
> 
> Thank you for your patch.
> 
> Am 30.08.25 um 11:05 schrieb linan666@...weicloud.com:
>> From: Li Nan <linan122@...wei.com>
>>
>> The 'mddev->recovery' flags can change during md_do_sync(), leading to
>> inconsistencies. For example, starting with MD_RECOVERY_RECOVER and
>> ending with MD_RECOVERY_SYNC can cause incorrect offset updates.
> 
> Can you give a concrete example?
> 

T1					T2
md_do_sync
  action = ACTION_RECOVER
					(write sysfs)
					action_store
					 set MD_RECOVERY_SYNC
  [ do recovery ]
  update resync_offset

The corresponding code is:
```
         if (!test_bit(MD_RECOVERY_CHECK, &mddev->recovery) &&
             mddev->curr_resync > MD_RESYNC_ACTIVE) {
                 if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) {	->SYNC 
is set, But what we do is recovery
                         if (test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
                                 if (mddev->curr_resync >= 
mddev->resync_offset) {
                                         pr_debug("md: checkpointing %s of 
%s.\n",
                                                  desc, mdname(mddev));
                                         if (test_bit(MD_RECOVERY_ERROR,
                                                 &mddev->recovery))
                                                 mddev->resync_offset =
 
mddev->curr_resync_completed;
                                         else
                                                 mddev->resync_offset =
                                                         mddev->curr_resync;
                                 }
```

>> To avoid this, use the 'action' determined at the beginning of the
>> function instead of repeatedly checking 'mddev->recovery'.
> 
> Do you have a reproducer?
> 

I don't have a reproducer because reproducing it requires modifying the
kernel. The approximate steps are:

- Modify the kernel to add a delay before the above check.
- Trigger recovery by removing and adding disks.
- After recovery completes, write to the sysfs interface at the delay point
to set the sync flag.

> Add a Fixes: tag?
> 

I will add it in v2.

>> Signed-off-by: Li Nan <linan122@...wei.com>
>> ---
>>   drivers/md/md.c | 21 +++++++++------------
>>   1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 6828a569e819..67cda9b64c87 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9516,7 +9516,7 @@ void md_do_sync(struct md_thread *thread)
>>           skipped = 0;
>> -        if (!test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
>> +        if (action != ACTION_RESHAPE &&
>>               ((mddev->curr_resync > mddev->curr_resync_completed &&
>>                 (mddev->curr_resync - mddev->curr_resync_completed)
>>                 > (max_sectors >> 4)) ||
>> @@ -9529,8 +9529,7 @@ void md_do_sync(struct md_thread *thread)
>>               wait_event(mddev->recovery_wait,
>>                      atomic_read(&mddev->recovery_active) == 0);
>>               mddev->curr_resync_completed = j;
>> -            if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) &&
>> -                j > mddev->resync_offset)
>> +            if (action == ACTION_RESYNC && j > mddev->resync_offset)
>>                   mddev->resync_offset = j;
>>               update_time = jiffies;
>>               set_bit(MD_SB_CHANGE_CLEAN, &mddev->sb_flags);
>> @@ -9646,7 +9645,7 @@ void md_do_sync(struct md_thread *thread)
>>       blk_finish_plug(&plug);
>>       wait_event(mddev->recovery_wait, 
>> !atomic_read(&mddev->recovery_active));
>> -    if (!test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
>> +    if (action != ACTION_RESHAPE &&
>>           !test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
>>           mddev->curr_resync >= MD_RESYNC_ACTIVE) {
>>           mddev->curr_resync_completed = mddev->curr_resync;
>> @@ -9654,9 +9653,8 @@ void md_do_sync(struct md_thread *thread)
>>       }
>>       mddev->pers->sync_request(mddev, max_sectors, max_sectors, &skipped);
>> -    if (!test_bit(MD_RECOVERY_CHECK, &mddev->recovery) &&
>> -        mddev->curr_resync > MD_RESYNC_ACTIVE) {
>> -        if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) {
>> +    if (action != ACTION_CHECK && mddev->curr_resync > MD_RESYNC_ACTIVE) {
>> +        if (action == ACTION_RESYNC) {
>>               if (test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
>>                   if (mddev->curr_resync >= mddev->resync_offset) {
>>                       pr_debug("md: checkpointing %s of %s.\n",
>> @@ -9674,8 +9672,7 @@ void md_do_sync(struct md_thread *thread)
>>           } else {
>>               if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery))
>>                   mddev->curr_resync = MaxSector;
>> -            if (!test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
>> -                test_bit(MD_RECOVERY_RECOVER, &mddev->recovery)) {
>> +            if (action == ACTION_RECOVER) {
> 
> What about `MD_RECOVERY_RESHAPE`?
> 
>>                   rcu_read_lock();
>>                   rdev_for_each_rcu(rdev, mddev)
>>                       if (mddev->delta_disks >= 0 &&
>> @@ -9692,7 +9689,7 @@ void md_do_sync(struct md_thread *thread)
>>       set_mask_bits(&mddev->sb_flags, 0,
>>                 BIT(MD_SB_CHANGE_PENDING) | BIT(MD_SB_CHANGE_DEVS));
>> -    if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
>> +    if (action == ACTION_RESHAPE &&
>>               !test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
>>               mddev->delta_disks > 0 &&
>>               mddev->pers->finish_reshape &&
>> @@ -9709,10 +9706,10 @@ void md_do_sync(struct md_thread *thread)
>>       spin_lock(&mddev->lock);
>>       if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
>>           /* We completed so min/max setting can be forgotten if used. */
>> -        if (test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))
>> +        if (action == ACTION_REPAIR)
>>               mddev->resync_min = 0;
>>           mddev->resync_max = MaxSector;
>> -    } else if (test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))
>> +    } else if (action == ACTION_REPAIR)
>>           mddev->resync_min = mddev->curr_resync_completed;
>>       set_bit(MD_RECOVERY_DONE, &mddev->recovery);
>>       mddev->curr_resync = MD_RESYNC_NONE;
> 
> I have not fully grogged yet, what the consequence of a mismatch between 
> `action`, set at the beginning, and changed flags in `&mddev->recovery` are.
> 
> 
> Kind regards,
> 
> Paul
> 
> .

-- 
Thanks,
Nan


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ