Date: Fri, 5 Aug 2011 20:14:30 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Jaakko Heinonen <jh@freebsd.org> Cc: freebsd-current@freebsd.org, Kohji Okuno <okuno.kohji@jp.panasonic.com> Subject: Re: Bug: devfs is sure to have the bug. Message-ID: <20110805171430.GW17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110805154522.GA2054@a91-153-123-205.elisa-laajakaista.fi> References: <20110803.141636.145758949453810779.okuno.kohji@jp.panasonic.com> <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com> <20110803135044.GM17489@deviant.kiev.zoral.com.ua> <20110805154522.GA2054@a91-153-123-205.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
--T/mHeVm7rU1pEax+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Fri, Aug 05, 2011 at 06:45:22PM +0300, Jaakko Heinonen wrote:
> On 2011-08-03, Kostik Belousov wrote:
> > On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote:
> > > > devfs_populate(), and the context holds only "dm->dm_lock" in
> > > > devfs_populate().
> > > >=20
> > > > On the other hand, "devfs_generation" is incremented in devfs_creat=
e()
> > > > and devfs_destroy() the context holds only "devmtx" in devfs_create=
()
> > > > and devfs_destroy().
> > > >=20
> > > > If a context executes devfs_create() when other context is executing
> > > > (***), then "dm->dm_generation" is updated incorrect value.
> > > > As a result, we can not open the last detected device (we receive E=
NOENT).
> >=20
> > I think the problem you described is real, and suggested change is righ=
t.
> > Initially, I thought that we should work with devfs_generation as with
> > the atomic type due to unlocked access in the devfs_populate(), but then
> > convinced myself that this is not needed.
> >=20
> > But also, I think there is another half of the problem. Namely,
> > devfs_lookup() calls devfs_populate_vp(), and then does lookup with the
> > help of devfs_lookupx(). We will miss the generation update
> > happen after the drop of the dm_lock in devfs_populate_vp() to reacquire
> > the directory vnode lock.
>=20
> I don't understand this. devfs_generation is not protected with dm_lock
> in devfs_create() and devfs_destroy(). On the other hand if you mean
> that another thread calls devfs_populate() while we drop dm_lock in
> devfs_populate_vp(), isn't the mount point up to date when we re-lock
> dm_lock?
Yes, I was not quite exact in describing what I mean, and the reference
to dm_lock drop is both vague and not correct.
I am after the fact that we do allow the situation where it is externally
visible that new cdev node was successfully created before the lookup
returns ENOENT for the path of the node.
>=20
> > @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int c=
leanup)
> > void
> > devfs_populate(struct devfs_mount *dm)
> > {
> > + unsigned gen;
> > =20
> > sx_assert(&dm->dm_lock, SX_XLOCKED);
> > - if (dm->dm_generation =3D=3D devfs_generation)
> > + gen =3D devfs_generation;
> > + if (dm->dm_generation =3D=3D gen)
> > return;
> > while (devfs_populate_loop(dm, 0))
> > continue;
> > - dm->dm_generation =3D devfs_generation;
> > + dm->dm_generation =3D gen;
> > }
>=20
> After this change dm->dm_generation may be stale although the mount
> point is up to date? This is probably harmless, though.
This must be harmless, in the worst case it will cause more calls to
the populate. In fact, this even allows the dm_generation to roll backward,
which is again harmless.
--T/mHeVm7rU1pEax+
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk48JPYACgkQC3+MBN1Mb4hBKACeIeXpOd36YNL9gX20iBprVDl7
os4An0+4QuRROZYb2mLjzE0kRVaPcl7G
=QS/7
-----END PGP SIGNATURE-----
--T/mHeVm7rU1pEax+--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110805171430.GW17489>