* [PATCH] ubi: don't decrease ubi->ref_count on detach error
@ 2023-12-05 8:11 Ryder W
2023-12-05 9:01 ` Zhihao Cheng
0 siblings, 1 reply; 6+ messages in thread
From: Ryder W @ 2023-12-05 8:11 UTC (permalink / raw)
To: linux-mtd@lists.infradead.org, Zhihao Cheng,
daniel@makrotopia.org
> > Fixes: cdfa788acd13 ("UBI: prepare attach and detach functions")
> > Signed-off-by: Daniel Golle <daniel at makrotopia.org>
> > ---
> > 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) {
> > 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;
> > ubi_devices[ubi_num] = NULL;
> > spin_unlock(&ubi_devices_lock);
In the last code of ubi_detach_mtd_dev, the line "ubi->ref_count -= 1" after the line "put_device(&ubi->dev)" is just to decrease ubi->ref_count, which is increased from calling put_device. I don't understand why it should be moved after the sanity check of ubi->ref_count. It may introduce some more critical bug.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
2023-12-05 8:11 [PATCH] ubi: don't decrease ubi->ref_count on detach error Ryder W
@ 2023-12-05 9:01 ` Zhihao Cheng
2023-12-05 12:23 ` Daniel Golle
0 siblings, 1 reply; 6+ messages in thread
From: Zhihao Cheng @ 2023-12-05 9:01 UTC (permalink / raw)
To: Ryder W, linux-mtd@lists.infradead.org, daniel@makrotopia.org
在 2023/12/5 16:11, Ryder W 写道:
>>> Fixes: cdfa788acd13 ("UBI: prepare attach and detach functions")
>>> Signed-off-by: Daniel Golle <daniel at makrotopia.org>
>>> ---
>>> 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) {
>>> 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;
>>> ubi_devices[ubi_num] = NULL;
>>> spin_unlock(&ubi_devices_lock);
>
> In the last code of ubi_detach_mtd_dev, the line "ubi->ref_count -= 1" after the line "put_device(&ubi->dev)" is just to decrease ubi->ref_count, which is increased from calling put_device. I don't understand why it should be moved after the sanity check of ubi->ref_count. It may introduce some more critical bug.
>
>
Thanks for reminding, you are right. I think I missed the code of
'ubi->ref_count' increasing in ubi_get_device, so decreasing the
'ubi->ref_count' unconditionally is right. In most cases, the
'ubi->ref_count' is zero(unless ubifs is mounted).
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
2023-12-05 9:01 ` Zhihao Cheng
@ 2023-12-05 12:23 ` Daniel Golle
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Golle @ 2023-12-05 12:23 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: Ryder W, linux-mtd@lists.infradead.org
On Tue, Dec 05, 2023 at 05:01:36PM +0800, Zhihao Cheng wrote:
> 在 2023/12/5 16:11, Ryder W 写道:
> > > > Fixes: cdfa788acd13 ("UBI: prepare attach and detach functions")
> > > > Signed-off-by: Daniel Golle <daniel at makrotopia.org>
> > > > ---
> > > > 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) {
> > > > 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;
> > > > ubi_devices[ubi_num] = NULL;
> > > > spin_unlock(&ubi_devices_lock);
> >
> > In the last code of ubi_detach_mtd_dev, the line "ubi->ref_count -= 1" after the line "put_device(&ubi->dev)" is just to decrease ubi->ref_count, which is increased from calling put_device. I don't understand why it should be moved after the sanity check of ubi->ref_count. It may introduce some more critical bug.
> >
> >
>
> Thanks for reminding, you are right. I think I missed the code of
> 'ubi->ref_count' increasing in ubi_get_device, so decreasing the
> 'ubi->ref_count' unconditionally is right. In most cases, the
> 'ubi->ref_count' is zero(unless ubifs is mounted).
Sorry for the noise, I agree, the current code is correct and ref_count
indeed needs to be decreased unconditionally as it has just been
increased by calling ubi_get_device() in the lines above.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ubi: don't decrease ubi->ref_count on detach error
@ 2023-11-27 17:09 Daniel Golle
2023-11-27 20:25 ` Richard Weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Golle @ 2023-11-27 17:09 UTC (permalink / raw)
To: Richard Weinberger, Miquel Raynal, Vignesh Raghavendra,
Artem Bityutskiy, linux-mtd, linux-kernel
Cc: John Crispin
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>
---
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) {
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;
ubi_devices[ubi_num] = NULL;
spin_unlock(&ubi_devices_lock);
--
2.43.0
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
2023-11-27 17:09 Daniel Golle
@ 2023-11-27 20:25 ` Richard Weinberger
2023-11-27 22:06 ` Daniel Golle
0 siblings, 1 reply; 6+ messages in thread
From: Richard Weinberger @ 2023-11-27 20:25 UTC (permalink / raw)
To: Daniel Golle
Cc: Miquel Raynal, Vignesh Raghavendra, Artem Bityutskiy, linux-mtd,
linux-kernel, John Crispin
----- 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/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error
2023-11-27 20:25 ` Richard Weinberger
@ 2023-11-27 22:06 ` Daniel Golle
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Golle @ 2023-11-27 22:06 UTC (permalink / raw)
To: Richard Weinberger
Cc: Miquel Raynal, Vignesh Raghavendra, Artem Bityutskiy, linux-mtd,
linux-kernel, John Crispin
Hi Richard,
On Mon, Nov 27, 2023 at 09:25:58PM +0100, Richard Weinberger wrote:
> > 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?
I was working on simplifying the NVMEM-on-UBI code which includes
attaching UBI via MTD notifiers. You and others had rightously
criticized the sketchy situation of the 'remove' handler which has now
lead me to rework that part of my patches, which made me end up looking
at the ref_count logic and error path at some point it popped into my
eyes that this can't be right.
>
> > ---
> > 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?
My idea was to really change only what I meant to change and make
that change the least intrusive possible.
> If rec_counts turns negative, due to a bug, we could still stop it here.
... here and in every other pleace where we touch it?
Adding new sanity checks to the code probably doesn't hurt but goes
beyond the scope of fixing this very bug, so I'll only do it there for
now.
>
> > 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.
That makes sense, now that it became clear that ref_count wasn't
trustable for more than a decade, let's better make sure it is now.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-05 12:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-05 8:11 [PATCH] ubi: don't decrease ubi->ref_count on detach error Ryder W
2023-12-05 9:01 ` Zhihao Cheng
2023-12-05 12:23 ` Daniel Golle
-- strict thread matches above, loose matches on Subject: below --
2023-11-27 17:09 Daniel Golle
2023-11-27 20:25 ` Richard Weinberger
2023-11-27 22:06 ` Daniel Golle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox