Date: Thu, 04 Aug 2011 18:56:00 +0900 (JST) From: Kohji Okuno <okuno.kohji@jp.panasonic.com> To: kostikbel@gmail.com Cc: freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com Subject: Re: Bug: devfs is sure to have the bug. Message-ID: <20110804.185600.366306193153346256.okuno.kohji@jp.panasonic.com> In-Reply-To: <20110804095043.GA17489@deviant.kiev.zoral.com.ua> References: <20110803135044.GM17489@deviant.kiev.zoral.com.ua> <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> <20110804095043.GA17489@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello Kostik,
> On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote:
>> Hello Kostik,
>>
>> 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 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");
>> >
>> > static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesystem");
>> >
>> > -static unsigned devfs_generation;
>> > +unsigned devfs_generation;
>> > SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD,
>> > &devfs_generation, 0, "DEVFS generation number");
>> >
>> > @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
>> > void
>> > devfs_populate(struct devfs_mount *dm)
>> > {
>> > + unsigned gen;
>> >
>> > sx_assert(&dm->dm_lock, SX_XLOCKED);
>> > - if (dm->dm_generation == devfs_generation)
>> > + gen = devfs_generation;
>> > + if (dm->dm_generation == gen)
>> > return;
>> > while (devfs_populate_loop(dm, 0))
>> > continue;
>> > - dm->dm_generation = devfs_generation;
>> > + dm->dm_generation = gen;
>> > }
>> >
>> > /*
>> > 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 {
>> >
>> > #define cdev2priv(c) member2struct(cdev_priv, cdp_c, c)
>> >
>> > +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 = VFSTODEVFS(vp->v_mount);
>> > locked = VOP_ISLOCKED(vp);
>> >
>> > - sx_xlock(&dmp->dm_lock);
>> > + if (!dm_locked)
>> > + sx_xlock(&dmp->dm_lock);
>> > DEVFS_DMP_HOLD(dmp);
>> >
>> > /* Can't call devfs_populate() with the vnode lock held. */
>> > @@ -242,7 +243,7 @@ devfs_vptocnp(struct vop_vptocnp_args *ap)
>> >
>> > dmp = VFSTODEVFS(vp->v_mount);
>> >
>> > - error = devfs_populate_vp(vp);
>> > + error = devfs_populate_vp(vp, 0);
>> > if (error != 0)
>> > return (error);
>> >
>> > @@ -643,7 +644,7 @@ devfs_getattr(struct vop_getattr_args *ap)
>> > struct devfs_mount *dmp;
>> > struct cdev *dev;
>> >
>> > - error = devfs_populate_vp(vp);
>> > + error = devfs_populate_vp(vp, 0);
>> > if (error != 0)
>> > return (error);
>> >
>> > @@ -903,7 +904,7 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock)
>> >
>> > if (cdev == NULL)
>> > sx_xlock(&dmp->dm_lock);
>> > - else if (devfs_populate_vp(dvp) != 0) {
>> > + else if (devfs_populate_vp(dvp, 0) != 0) {
>> > *dm_unlock = 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;
>> >
>> > - if (devfs_populate_vp(ap->a_dvp) != 0)
>> > + dm_unlock = 0;
>> > +retry:
>> > + if (devfs_populate_vp(ap->a_dvp, dm_unlock) != 0)
>> > return (ENOTDIR);
>> >
>> > dmp = VFSTODEVFS(ap->a_dvp->v_mount);
>> > dm_unlock = 1;
>> > - j = devfs_lookupx(ap, &dm_unlock);
>> > - if (dm_unlock == 1)
>> > + error = devfs_lookupx(ap, &dm_unlock);
>> > + if (error == ENOENT) {
>> > + if (dm_unlock)
>> > + sx_assert(&dmp->dm_lock, SA_XLOCKED);
>> > + else {
>> > + sx_xlock(&dmp->dm_lock);
>> > + dm_unlock = 1;
>> > + }
>> > + if (devfs_generation != dmp->dm_generation)
>> > + goto retry;
>> > + }
>> > + if (dm_unlock)
>> > sx_xunlock(&dmp->dm_lock);
>> > - return (j);
>> > + return (error);
>> > }
>> >
>> > static int
>> > @@ -1202,7 +1214,7 @@ devfs_readdir(struct vop_readdir_args *ap)
>> > }
>> >
>> > dmp = VFSTODEVFS(ap->a_vp->v_mount);
>> > - if (devfs_populate_vp(ap->a_vp) != 0) {
>> > + if (devfs_populate_vp(ap->a_vp, 0) != 0) {
>> > if (tmp_ncookies != NULL)
>> > ap->a_ncookies = tmp_ncookies;
>> > return (EIO);
>> > @@ -1565,7 +1577,7 @@ devfs_symlink(struct vop_symlink_args *ap)
>> > if (error)
>> > return(error);
>> > dmp = VFSTODEVFS(ap->a_dvp->v_mount);
>> > - if (devfs_populate_vp(ap->a_dvp) != 0)
>> > + if (devfs_populate_vp(ap->a_dvp, 0) != 0)
>> > return (ENOENT);
>> >
>> > dd = ap->a_dvp->v_data;
>>
>> 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.
I'm sorry.
I need a patch for 8.1-RELEASE. Could you propose patch?
Thanks,
Kohji Okuno
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110804.185600.366306193153346256.okuno.kohji>