Date: Wed, 3 Aug 2011 16:50:44 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Kohji Okuno <okuno.kohji@jp.panasonic.com> Cc: freebsd-current@freebsd.org Subject: Re: Bug: devfs is sure to have the bug. Message-ID: <20110803135044.GM17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com> References: <20110803.141636.145758949453810779.okuno.kohji@jp.panasonic.com> <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--Z7URUY1CzunwZ4PW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote:
> Hello,
>=20
> > Hello,
> >=20
> > I think that devfs is sure to have the bug.
> > I found that I couldn't open "/dev/XXX" though the kernel detected XXX
> > device.
> >=20
> >=20
> > "dm->dm_generation" is updated with "devfs_generation" in
> > devfs_populate(), and the context holds only "dm->dm_lock" in
> > devfs_populate().
> >=20
> > On the other hand, "devfs_generation" is incremented in devfs_create()
> > 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 ENOEN=
T).
> >=20
> > I think that we should change the lock method.
> > May I have advice?
> >=20
> > void
> > devfs_populate(struct devfs_mount *dm)
> > {
> >=20
> > sx_assert(&dm->dm_lock, SX_XLOCKED);
> > if (dm->dm_generation =3D=3D devfs_generation)
> > return;
> > while (devfs_populate_loop(dm, 0))
> > continue;
> > (***)
> > dm->dm_generation =3D devfs_generation;
> > }
> >=20
> > void
> > devfs_create(struct cdev *dev)
> > {
> > struct cdev_priv *cdp;
> >=20
> > mtx_assert(&devmtx, MA_OWNED);
> > cdp =3D cdev2priv(dev);
> > cdp->cdp_flags |=3D CDP_ACTIVE;
> > cdp->cdp_inode =3D alloc_unrl(devfs_inos);
> > dev_refl(dev);
> > TAILQ_INSERT_TAIL(&cdevp_list, cdp, cdp_list);
> > devfs_generation++;
> > }
> >=20
> > void
> > devfs_destroy(struct cdev *dev)
> > {
> > struct cdev_priv *cdp;
> >=20
> > mtx_assert(&devmtx, MA_OWNED);
> > cdp =3D cdev2priv(dev);
> > cdp->cdp_flags &=3D ~CDP_ACTIVE;
> > devfs_generation++;
> > }
> >=20
> > Thanks.
>=20
> I propose the solution. May I have advice?
>=20
> void
> devfs_populate(struct devfs_mount *dm)
> {
>=20
> sx_assert(&dm->dm_lock, SX_XLOCKED);
> #if 1 /* I propose... */
> int tmp_generation;
> retry:
> tmp_generation =3D devfs_generation;
> if (dm->dm_generation =3D=3D tmp_generation)
> return;
> while (devfs_populate_loop(dm, 0))
> continue;
> if (tmp_generation !=3D devfs_generation)
> goto retry;
> dm->dm_generation =3D tmp_generation;
> #else /* Original */
> if (dm->dm_generation =3D=3D devfs_generation)
> return;
> while (devfs_populate_loop(dm, 0))
> continue;
> dm->dm_generation =3D devfs_generation;
> #endif
> }
I think the problem you described is real, and suggested change is right.
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.
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.
I propose the change below, consisting of your fix and also retry of
population and lookup in case generations do not match for ENOENT case.
diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c
index d72ada0..8ff9bc2 100644
--- a/sys/fs/devfs/devfs_devs.c
+++ b/sys/fs/devfs/devfs_devs.c
@@ -63,7 +63,7 @@ static MALLOC_DEFINE(M_CDEVP, "DEVFS1", "DEVFS cdev_priv =
storage");
=20
static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesystem=
");
=20
-static unsigned devfs_generation;
+unsigned devfs_generation;
SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD,
&devfs_generation, 0, "DEVFS generation number");
=20
@@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int clean=
up)
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
/*
diff --git a/sys/fs/devfs/devfs_int.h b/sys/fs/devfs/devfs_int.h
index cdc6aba..cb01ad1 100644
--- a/sys/fs/devfs/devfs_int.h
+++ b/sys/fs/devfs/devfs_int.h
@@ -71,6 +71,8 @@ struct cdev_priv {
=20
#define cdev2priv(c) member2struct(cdev_priv, cdp_c, c)
=20
+extern unsigned devfs_generation;
+
struct cdev *devfs_alloc(int);
int devfs_dev_exists(const char *);
void devfs_free(struct cdev *);
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index 955bd8b..2603caa 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -188,7 +188,7 @@ devfs_clear_cdevpriv(void)
* On success devfs_populate_vp() returns with dmp->dm_lock held.
*/
static int
-devfs_populate_vp(struct vnode *vp)
+devfs_populate_vp(struct vnode *vp, int dm_locked)
{
struct devfs_dirent *de;
struct devfs_mount *dmp;
@@ -199,7 +199,8 @@ devfs_populate_vp(struct vnode *vp)
dmp =3D VFSTODEVFS(vp->v_mount);
locked =3D VOP_ISLOCKED(vp);
=20
- sx_xlock(&dmp->dm_lock);
+ if (!dm_locked)
+ sx_xlock(&dmp->dm_lock);
DEVFS_DMP_HOLD(dmp);
=20
/* Can't call devfs_populate() with the vnode lock held. */
@@ -242,7 +243,7 @@ devfs_vptocnp(struct vop_vptocnp_args *ap)
=20
dmp =3D VFSTODEVFS(vp->v_mount);
=20
- error =3D devfs_populate_vp(vp);
+ error =3D devfs_populate_vp(vp, 0);
if (error !=3D 0)
return (error);
=20
@@ -643,7 +644,7 @@ devfs_getattr(struct vop_getattr_args *ap)
struct devfs_mount *dmp;
struct cdev *dev;
=20
- error =3D devfs_populate_vp(vp);
+ error =3D devfs_populate_vp(vp, 0);
if (error !=3D 0)
return (error);
=20
@@ -903,7 +904,7 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unloc=
k)
=20
if (cdev =3D=3D NULL)
sx_xlock(&dmp->dm_lock);
- else if (devfs_populate_vp(dvp) !=3D 0) {
+ else if (devfs_populate_vp(dvp, 0) !=3D 0) {
*dm_unlock =3D 0;
sx_xlock(&dmp->dm_lock);
if (DEVFS_DMP_DROP(dmp)) {
@@ -966,19 +967,30 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unl=
ock)
static int
devfs_lookup(struct vop_lookup_args *ap)
{
- int j;
struct devfs_mount *dmp;
- int dm_unlock;
+ int error, dm_unlock;
=20
- if (devfs_populate_vp(ap->a_dvp) !=3D 0)
+ dm_unlock =3D 0;
+retry:
+ if (devfs_populate_vp(ap->a_dvp, dm_unlock) !=3D 0)
return (ENOTDIR);
=20
dmp =3D VFSTODEVFS(ap->a_dvp->v_mount);
dm_unlock =3D 1;
- j =3D devfs_lookupx(ap, &dm_unlock);
- if (dm_unlock =3D=3D 1)
+ error =3D devfs_lookupx(ap, &dm_unlock);
+ if (error =3D=3D ENOENT) {
+ if (dm_unlock)
+ sx_assert(&dmp->dm_lock, SA_XLOCKED);
+ else {
+ sx_xlock(&dmp->dm_lock);
+ dm_unlock =3D 1;
+ }
+ if (devfs_generation !=3D dmp->dm_generation)
+ goto retry;
+ }
+ if (dm_unlock)
sx_xunlock(&dmp->dm_lock);
- return (j);
+ return (error);
}
=20
static int
@@ -1202,7 +1214,7 @@ devfs_readdir(struct vop_readdir_args *ap)
}
=20
dmp =3D VFSTODEVFS(ap->a_vp->v_mount);
- if (devfs_populate_vp(ap->a_vp) !=3D 0) {
+ if (devfs_populate_vp(ap->a_vp, 0) !=3D 0) {
if (tmp_ncookies !=3D NULL)
ap->a_ncookies =3D tmp_ncookies;
return (EIO);
@@ -1565,7 +1577,7 @@ devfs_symlink(struct vop_symlink_args *ap)
if (error)
return(error);
dmp =3D VFSTODEVFS(ap->a_dvp->v_mount);
- if (devfs_populate_vp(ap->a_dvp) !=3D 0)
+ if (devfs_populate_vp(ap->a_dvp, 0) !=3D 0)
return (ENOENT);
=20
dd =3D ap->a_dvp->v_data;
--Z7URUY1CzunwZ4PW
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk45UjQACgkQC3+MBN1Mb4hpsgCfY6WlwW7KY4ZDqYlaD4qT2LAi
5l0AoOvRtF51UCA0dkVNYz6sa28GvbEK
=XL0G
-----END PGP SIGNATURE-----
--Z7URUY1CzunwZ4PW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110803135044.GM17489>