From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C6C82C4167B for ; Mon, 27 Nov 2023 20:26:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231980AbjK0U0C convert rfc822-to-8bit (ORCPT ); Mon, 27 Nov 2023 15:26:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50480 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229527AbjK0UZ7 (ORCPT ); Mon, 27 Nov 2023 15:25:59 -0500 Received: from lithops.sigma-star.at (lithops.sigma-star.at [195.201.40.130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7C2EBD59 for ; Mon, 27 Nov 2023 12:26:02 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by lithops.sigma-star.at (Postfix) with ESMTP id 016D96413F60; Mon, 27 Nov 2023 21:26:00 +0100 (CET) Received: from lithops.sigma-star.at ([127.0.0.1]) by localhost (lithops.sigma-star.at [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id uLRVYHk-f5Lg; Mon, 27 Nov 2023 21:25:59 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by lithops.sigma-star.at (Postfix) with ESMTP id 3013E6340E00; Mon, 27 Nov 2023 21:25:59 +0100 (CET) Received: from lithops.sigma-star.at ([127.0.0.1]) by localhost (lithops.sigma-star.at [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id K3yrBMph3TqV; Mon, 27 Nov 2023 21:25:59 +0100 (CET) Received: from lithops.sigma-star.at (lithops.sigma-star.at [195.201.40.130]) by lithops.sigma-star.at (Postfix) with ESMTP id 0CF816413F60; Mon, 27 Nov 2023 21:25:59 +0100 (CET) Date: Mon, 27 Nov 2023 21:25:58 +0100 (CET) From: Richard Weinberger To: Daniel Golle Cc: Miquel Raynal , Vignesh Raghavendra , Artem Bityutskiy , linux-mtd , linux-kernel , John Crispin Message-ID: <771902199.32600.1701116758852.JavaMail.zimbra@nod.at> In-Reply-To: <9857609999c5b7196417474938a7a09892cd1612.1701104870.git.daniel@makrotopia.org> References: <9857609999c5b7196417474938a7a09892cd1612.1701104870.git.daniel@makrotopia.org> Subject: Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Originating-IP: [195.201.40.130] X-Mailer: Zimbra 8.8.12_GA_3807 (ZimbraWebClient - FF97 (Linux)/8.8.12_GA_3809) Thread-Topic: don't decrease ubi->ref_count on detach error Thread-Index: chpmDIpsyDGT4mmuCnlwjJ5qLzrYdQ== Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- Ursprüngliche Mail ----- > Von: "Daniel Golle" > An: "richard" , "Miquel Raynal" , "Vignesh Raghavendra" , > "Artem Bityutskiy" , "linux-mtd" , "linux-kernel" > > CC: "John Crispin" > Gesendet: Montag, 27. November 2023 18:09:14 > Betreff: [PATCH] ubi: don't decrease ubi->ref_count on detach error > If attempting to detach a UBI device while it is still busy, detaching > is refused. However, the reference counter is still being decreased > despite the error. Rework detach function to only decrease the refcnt > once all conditions for detachment are met. > > Fixes: cdfa788acd13 ("UBI: prepare attach and detach functions") > Signed-off-by: Daniel Golle Good catch! Did you find this by review or while testing? > --- > drivers/mtd/ubi/build.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 7d4ff1193db6f..f47987ee9a31b 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -1099,16 +1099,16 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) > > spin_lock(&ubi_devices_lock); > put_device(&ubi->dev); > - ubi->ref_count -= 1; > - if (ubi->ref_count) { > + if (ubi->ref_count > 1) { Is there a specific reason why you have modified the check to test only for ref_count being positive? If rec_counts turns negative, due to a bug, we could still stop it here. > if (!anyway) { > spin_unlock(&ubi_devices_lock); > return -EBUSY; > } > /* This may only happen if there is a bug */ > ubi_err(ubi, "%s reference count %d, destroy anyway", > - ubi->ubi_name, ubi->ref_count); > + ubi->ubi_name, ubi->ref_count - 1); > } > + ubi->ref_count -= 1; Please add there an ubi_asert() which tests whether ref_count is really zero. ...just to be more bullet proof. Thanks, //richard