From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 30EFC29AB05 for ; Sun, 5 Jul 2026 19:58:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.47 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783281505; cv=none; b=q4VDJucyByxmzFSiC5c5cm3diDPbeOwB6Ivjwy/rZRmHWkSizuSFBEK/n6yTgoLlt93VzDwz6yScPvw04ZC4EmYpQFX2XbOdDisZT4DYpWcfyFxtA5IlEc+uAG+HKj1t66UciMPfTBw/XmPBLjYWjX2hUUgnGgrmU1g3rSbgUxs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783281505; c=relaxed/simple; bh=hehu7c3ydBHEFX5B7Vu11/GHt2p0iOqoZhhIaXw57GU=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=U8RASsGAGTUOy/Qp6s7NNMqtxFAn4J0SmzKl/v+7YVJWy/u6YVryBDR8kHraeCYGRPJYB6DX7WA4kR2S+8AliVa2wA14qrIlGBoCZJgES+OAgLvj4L2EIO2sr1RDadRpbB3hzi0RiAqADfMFPYaI4X15yVNoGR9JaNfDyH6rNb4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=nXJDn7rA; arc=none smtp.client-ip=209.85.221.47 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="nXJDn7rA" Received: by mail-wr1-f47.google.com with SMTP id ffacd0b85a97d-470174001a0so1047633f8f.0 for ; Sun, 05 Jul 2026 12:58:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1783281502; x=1783886302; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id :reply-to; bh=phsHTmLYUt1OkY46dIqbxDNcV7vGCBX/5xtjGDtARYk=; b=nXJDn7rArluRE93+m2Z4qXGHh0HmcESp4SWoP8NVmdyCXfA0HsUIIdqrjHL2tGqpIc eFlUXlNujk3g/H6A9Kd/jrltH89XsVLso6saRSHmtdz1WP2GhIRJy5fS488lJCz5FEIV EWPZfkX1/HYRLc8QJoPhAy8FyN+Rfvm39VLSEIBtwa1uu1kF8gJCDlfvfq4vJtnt4blh F2/JcHf8KNBt/PHbZqTio4/E1lVdcO/MkiI4K9rnbIIgAuxhNjdbK2EnexG8bid+Top2 S9TKLHAU3GgMYqrKq4O1USbM08Zu+55qNwiCwGZR726gHZFiy6cip/Ky3ldyasr9M5+F zEgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783281502; x=1783886302; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=phsHTmLYUt1OkY46dIqbxDNcV7vGCBX/5xtjGDtARYk=; b=jwIIZMdXp95icR3XXW85nfXxoyM80ngxBKWXX9OF7L5mIrf1jzxwtgvJn0bB/E7hHq lL9FJJAJpGgej0LbA79jdCaBLeIIiVbsQBpxTJE6jb6F6r20fNyeBMBrGokL/aElZmej PYIuTK8iJRiw8WdSAzcIFORRpR5L+6Sr4KzN9GApjiwvhBsKrviSl9d54HHbGGZqb3kD sJZ1vgJcAbHFzQF5mAqOn9ptPOZu0NyMOPqUg2UgxtBw7A9BadbE3S7dhKQaU/STwNGo BX1G65llQhuDl9AX+0G4LXzKspTBXtAbuvldPBWg+FGvMgcU3xoI0zCO2tUtk49D616H JkWw== X-Forwarded-Encrypted: i=1; AHgh+RoFp8Z5FLgTlnRgYRG9khE4uE3FBCUC/hF/Zj8MLRHAr0wvUIDsJ663gwAVVZLSR+FzYkVKKt0Ipzvv0mA=@vger.kernel.org X-Gm-Message-State: AOJu0YxcsZxGT9Cl1/JH/Cw6VYj86zFelGhTr8DZ3t1IjZC1zq14OgT6 Tgvfd7uPufuqwwn21Jd/UKOCcwFu6PqW0ol6Pszzzzea0FdnSSH7lPzj X-Gm-Gg: AfdE7clZWCttVr1Cv/3g7IMw8vVIEm6NmR9II4KByUartsHbI9hyFJslMsAuoE/buLG 6d571UjdNYPDYgrl1sE3/F5ueKZTXmrCUfKyrgaHCyYV0b6zuw047Xc4zhanxd/+jA+CsGpnsL4 Xo4+JEj0823o5l8Y8P49XdHXuBcdJBipYt8WrYP62nxyoQomxgAUbKmAVHlEHIveYi+Q6qNFb/J dWAuVV8vNYsB9C49+mGo7uOa4m3eB3AtKbFX+B2NhjQ8ENvm9AyRLERwUv3qr39eni9rEhBmoJ6 QFAqVV+NqlC/8ChOzfORqRv0H7vUzUBwUT9X+KL/TQNsyr9W2tUQTYEqTfk8ilHlJsAhOFWhsEP lO/5FrD39aBzldKtBv7n0iADLfK/qTv+kIQen5EWU2TCswSY9q0PCNjtEgk4qzzC9uJxGS1NcSB tj5iwsFc5fyIg36pK27cKNMxxc8l6xf2SXMCcs5JDotrlAAMx9n0yb+C0= X-Received: by 2002:a05:6000:2408:b0:45e:eaed:afd2 with SMTP id ffacd0b85a97d-47aa7ca8972mr9165414f8f.0.1783281502441; Sun, 05 Jul 2026 12:58:22 -0700 (PDT) Received: from Abds-MacBook-Air.local ([2a02:3037:663:8ce9:99fa:f98b:4367:d5f6]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47aa039ad21sm18380105f8f.20.2026.07.05.12.58.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 05 Jul 2026 12:58:21 -0700 (PDT) From: Abd-Alrhman Masalkhi To: yu kuai , song@kernel.org, magiclinan@didiglobal.com, xiao@kernel.org, yukuai@fygo.io Cc: linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, sashiko-bot Subject: Re: [PATCH] md: avoid modifying spares while the array is not suspended In-Reply-To: <0a7dc7e2-30af-44de-8c29-b6e27a62fa55@fygo.io> References: <20260630075640.1081634-1-abd.masalkhi@gmail.com> <0a7dc7e2-30af-44de-8c29-b6e27a62fa55@fygo.io> Date: Sun, 05 Jul 2026 21:58:12 +0200 Message-ID: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Kuai, On Mon, Jul 06, 2026 at 00:16 +0800, yu kuai wrote: > Hi, > > =E5=9C=A8 2026/6/30 15:56, Abd-Alrhman Masalkhi =E5=86=99=E9=81=93: >> remove_spares() and remove_and_add_spares() modify the array's rdev >> configuration. These operations are only safe after the array has been >> suspended. >> >> Today, md_start_sync() can call md_choose_sync_action() even when the >> array has not been suspended. As a result, md_choose_sync_action() may >> remove or replace rdevs while normal I/O is still accessing them. >> >> The race can occur as follows: >> >> raid10d Worker Normal IO >> ____________ _______________________ ______________________ >> >> raid10_write_request() >> wait_blocked_dev() >> set Blocked >> set Faulty >> Skip Faulty rdev >> rrdev->nr_pending++ >> .repl_bio =3D bio >> removeable_rdev =3D false . >> array not suspended . >> lock mddev goto err_handle >> lock mddev (wait) >> . >> update sb . >> clear Blocked . >> . >> unlock mddev . >> lock mddev (acquires) >> remove_spares() >> removeable_rdev =3D true >> >> raid10_remove_disk() >> rdev =3D replacement >> replacement =3D NULL >> rdev_dec_pending(NULL) >> unlock mddev (NULL)->nr_pending-- >> >> In this case, rdev_dec_pending() is called with a NULL pointer, >> resulting in a NULL pointer dereference when attempting to decrement >> nr_pending. >> >> Fix this by ensuring that remove_spares() and remove_and_add_spares() >> are only called after the array has been suspended, preventing >> concurrent rdev configuration changes while normal I/O is in progress. >> >> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need= reconfiguration") >> Reported-by: sashiko-bot >> Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masa= lkhi@gmail.com?part=3D3 >> Signed-off-by: Abd-Alrhman Masalkhi >> --- >> drivers/md/md.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/md/md.c b/drivers/md/md.c >> index 66a41d482e59..c85ebb59535b 100644 >> --- a/drivers/md/md.c >> +++ b/drivers/md/md.c >> @@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *m= ddev, >> return spares; >> } >>=20=20=20 >> -static bool md_choose_sync_action(struct mddev *mddev, int *spares) >> +static bool md_choose_sync_action(struct mddev *mddev, int *spares, >> + bool array_suspended) >> { >> /* Check if reshape is in progress first. */ >> if (mddev->reshape_position !=3D MaxSector) { >> @@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *= mddev, int *spares) >>=20=20=20 >> /* Check if resync is in progress. */ >> if (mddev->resync_offset < MaxSector) { >> - remove_spares(mddev, NULL); >> + if (array_suspended) >> + remove_spares(mddev, NULL); > > The problem looks real, however, I think this will cause a change that us= er will be awared, > if there are really spares that can be removed from conf, but array is no= t suspended here, > user will still expect rdev will be removed from conf automatically. > > In md_start_sync, if suspend is false, can we check again after mddev_loc= k? If suspend is > supposed to be true, we can release the lock and retry with suspend =3D t= rue. > Yes, I see, and your approach is much better. But what do you think about taking the lock first and then checking only once? >> + >> set_bit(MD_RECOVERY_SYNC, &mddev->recovery); >> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); >> clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery); >> @@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev = *mddev, int *spares) >> * also removed and re-added, to allow the personality to fail the >> * re-add. >> */ >> - *spares =3D remove_and_add_spares(mddev, NULL); >> + if (array_suspended) >> + *spares =3D remove_and_add_spares(mddev, NULL); >> + else >> + *spares =3D 0; >> + >> if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) { >> clear_bit(MD_RECOVERY_SYNC, &mddev->recovery); >> clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); >> @@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *= ws) >> * As we only add devices that are already in-sync, we can >> * activate the spares immediately. >> */ >> - remove_and_add_spares(mddev, NULL); >> + if (suspend) >> + remove_and_add_spares(mddev, NULL); >> goto not_running; >> } >>=20=20=20 >> - if (!md_choose_sync_action(mddev, &spares)) >> + if (!md_choose_sync_action(mddev, &spares, suspend)) >> goto not_running; >>=20=20=20 >> if (!mddev->pers->sync_request) > > --=20 > Thanks, > Kuai --=20 Best Regards, Abd-Alrhman