linux-kernel - Re: [PATCH] md: do not delete safemode_timer in mddev

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: <33185f65-b657-b15b-ffa8-e35319fa0a5f@huaweicloud.com>
Date: Thu, 9 May 2024 09:34:15 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: linan666@...weicloud.com, song@...nel.org
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
 yi.zhang@...wei.com, houtao1@...wei.com, yangerkun@...wei.co,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] md: do not delete safemode_timer in mddev_suspend

在 2024/05/08 17:20, linan666@...weicloud.com 写道:
> From: Li Nan <linan122@...wei.com>
> 
> The deletion of safemode_timer in mddev_suspend() is redundant and
> potentially harmful now. If timer is about to be woken up but gets
> deleted, 'in_sync' will remain 0 until the next write, causing array
> to stay in the 'active' state instead of transitioning to 'clean'.
> 
> Commit 0d9f4f135eb6 ("MD: Add del_timer_sync to mddev_suspend (fix
> nasty panic))" introduced this deletion for dm, because if timer fired
> after dm is destroyed, the resource which the timer depends on might
> have been freed.
> 
> However, commit 0dd84b319352 ("md: call __md_stop_writes in md_stop")
> added __md_stop_writes() to md_stop(), which is called before freeing
> resource. Timer is deleted in __md_stop_writes(), and the origin issue
> is resolved. Therefore, delete safemode_timer can be removed safely now.
> 
> Signed-off-by: Li Nan <linan122@...wei.com>
> ---
>   drivers/md/md.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index aff9118ff697..09c55d9a2c54 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -479,7 +479,6 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
>   	 */
>   	WRITE_ONCE(mddev->suspended, mddev->suspended + 1);
>   
> -	del_timer_sync(&mddev->safemode_timer);

I don't understand why time is deleted here before, it's right based on
git log, commit 0d9f4f135eb6 add this to fix panic for dm-raid, and it's
not necessary now.

LGTM, feel free to add:

Reviewed-by: Yu Kuai <yukuai3@...wei.com>

However, since this behaviour is introduced since 2012, does anybody
really care about array status is 'active' instead of 'clean' while
there is no IO after suspend?

Thanks,
Kuai

>   	/* restrict memory reclaim I/O during raid array is suspend */
>   	mddev->noio_flag = memalloc_noio_save();
>   
>