All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Weinberger <richard@nod.at>
To: Daniel Golle <daniel@makrotopia.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 Artem Bityutskiy <Artem.Bityutskiy@nokia.com>,
	 linux-mtd <linux-mtd@lists.infradead.org>,
	 linux-kernel <linux-kernel@vger.kernel.org>,
	 John Crispin <john@phrozen.org>
Subject: Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
Date: Mon, 27 Nov 2023 21:25:58 +0100 (CET)	[thread overview]
Message-ID: <771902199.32600.1701116758852.JavaMail.zimbra@nod.at> (raw)
In-Reply-To: <9857609999c5b7196417474938a7a09892cd1612.1701104870.git.daniel@makrotopia.org>

----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <daniel@makrotopia.org>
> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>,
> "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>, "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel"
> <linux-kernel@vger.kernel.org>
> CC: "John Crispin" <john@phrozen.org>
> 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 <daniel@makrotopia.org>

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

WARNING: multiple messages have this Message-ID (diff)
From: Richard Weinberger <richard@nod.at>
To: Daniel Golle <daniel@makrotopia.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Artem Bityutskiy <Artem.Bityutskiy@nokia.com>,
	linux-mtd <linux-mtd@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	John Crispin <john@phrozen.org>
Subject: Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
Date: Mon, 27 Nov 2023 21:25:58 +0100 (CET)	[thread overview]
Message-ID: <771902199.32600.1701116758852.JavaMail.zimbra@nod.at> (raw)
In-Reply-To: <9857609999c5b7196417474938a7a09892cd1612.1701104870.git.daniel@makrotopia.org>

----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <daniel@makrotopia.org>
> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>,
> "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>, "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel"
> <linux-kernel@vger.kernel.org>
> CC: "John Crispin" <john@phrozen.org>
> 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 <daniel@makrotopia.org>

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

  reply	other threads:[~2023-11-27 20:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-27 17:09 [PATCH] ubi: don't decrease ubi->ref_count on detach error Daniel Golle
2023-11-27 17:09 ` Daniel Golle
2023-11-27 20:25 ` Richard Weinberger [this message]
2023-11-27 20:25   ` Richard Weinberger
2023-11-27 22:06   ` Daniel Golle
2023-11-27 22:06     ` Daniel Golle
2023-11-28  0:45   ` [PATCH v2] " Daniel Golle
2023-11-28  0:45     ` Daniel Golle
2023-11-28  2:18     ` Zhihao Cheng
2023-11-28  2:18       ` Zhihao Cheng
  -- strict thread matches above, loose matches on Subject: below --
2023-12-05  8:11 [PATCH] " Ryder W
2023-12-05  9:01 ` Zhihao Cheng
2023-12-05 12:23   ` Daniel Golle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=771902199.32600.1701116758852.JavaMail.zimbra@nod.at \
    --to=richard@nod.at \
    --cc=Artem.Bityutskiy@nokia.com \
    --cc=daniel@makrotopia.org \
    --cc=john@phrozen.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.