Re: [PATCH] [UBI] 1/4 UBI volume notifications - UBI changes

[Artem Bityutskiy <dedekind@infradead.org>]

On Wed, 2008-12-10 at 22:50 +0300, dmitry pervushin wrote:
> On Tue, 2008-12-09 at 12:36 +0200, Artem Bityutskiy wrote:
> 
> Thanks for your review, my the only comment is inlined below.
> 
> [skipped]
> > <snip>
> > > +/**
> > >   * ubi_get_device - get UBI device.
> > >   * @ubi_num: UBI device number
> > >   *
> > > @@ -842,6 +870,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
> > >  			goto out_detach;
> > >  	}
> > >  
> > > +	/* when processing uif_init, we already might want to open the volume */
> > > +	ubi_devices[ubi_num] = ubi;
> > >  	err = uif_init(ubi);
> > >  	if (err)
> > >  		goto out_nofree;
> > 
> > I do not understand this change. The point is to prevent anyone from
> > opening the volume before it is completely initialized. What you do -
> > you allow the volume to be opened while it is in the middle of
> > initialization, which is wrong. E.g., what if the initialization fails
> > at some point?
> > 
> > And this change does not seem to be relevant to this patch.
> This change is absolutely needed :)
> Well, the sequence of steps is as follows:
> 1. uif_init calls ubi_add_volume
> 2. ubi_add_volume notifies everyone about volume adding
> 3. (successful exit is not interested to us)
> 4. in case of errors reported by uif_init ubi_attach_mtd_dev calls
> ubi_kill_volumes
> 5. ubi_kill_volumes calls ubi_free_volume, which notifies everyone about
> volume deleting.

What may happen is

1. You make the UBI device visible by doing 'ubi_devices[ubi_num] =
ubi'.
2. You call 'uif_init()' which starts adding volumes. Suppose it added N
volumes out of M (M > N).
3. Some other task opens the UBI device, opens volume L (L <= N), and
starts utilizing it. E.g., it might mounted by UBIFS.
4. 'uif_init()' fails to add volume S (S > N <= M), and all resources,
including the opened volume L will be freed, and the system is in
trouble.

> In current version, "notifies about volume adding" corresponds to
> ubi_create_gluebi and "...deleting" means ubi_destory_gluebi"
> 
> Does this sound right? OK, then I am replacing ubi_create_gluebi with
> notification, notification function tries to open new (just appeared)
> volume... and fails, because ubi_open_volume tries to ubi_get_device.
> It, in turn checks the "ubi_devices[ubi_num]" which is not filled yet.

I think the solution is to call notifiers _after_ _everything_ is
successfully initialized and has no chances to fail anymore, i.e., at
the very end of 'ubi_attach_mtd_dev()' function.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)