Date: Thu, 4 Aug 2011 12:50:43 +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: <20110804095043.GA17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> References: <20110803.141636.145758949453810779.okuno.kohji@jp.panasonic.com> <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com> <20110803135044.GM17489@deviant.kiev.zoral.com.ua> <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--KQOSl2CQwBKYGM8f
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote:
> Hello Kostik,
>=20
> From: Kostik Belousov <kostikbel@gmail.com>
> Subject: Re: Bug: devfs is sure to have the bug.
> Date: Wed, 3 Aug 2011 16:50:44 +0300
> > 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 propose the change below, consisting of your fix and also retry of
> > population and lookup in case generations do not match for ENOENT
> >case.
> >=20
> > 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_p=
riv storage");
> > =20
> > static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesy=
stem");
> > =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 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
> > /*
> > 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_u=
nlock)
> > =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=
_unlock)
> > 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;
>=20
> Thank you for your comment.
> But, now I'm using 8.1-RELEASE. May I have advice about 8.X ?
Do you mean a patch for the stable/8 ? I believe it is enough to
apply rev. 211628 to stable/8, then the patch I posted yesterday
should be compilable.
--KQOSl2CQwBKYGM8f
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk46a3MACgkQC3+MBN1Mb4hCDACg6RkKaszL0UO2d2dqfqZT/Ouq
I7sAoPBZ0w5jX7/wPR+H4qUXLTVfpK9z
=c1bA
-----END PGP SIGNATURE-----
--KQOSl2CQwBKYGM8f--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110804095043.GA17489>