Re: [PATCH] ubi: don't decrease ubi->ref_count on detach error

[Daniel Golle <daniel@makrotopia.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/