Bug: devfs is sure to have the bug.
jh at FreeBSD.org
Fri Aug 5 16:05:03 UTC 2011
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().
> > >
> > > 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().
> > >
> > > 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 ENOENT).
> 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 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
> @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
> 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)
> while (devfs_populate_loop(dm, 0))
> - dm->dm_generation = devfs_generation;
> + dm->dm_generation = gen;
After this change dm->dm_generation may be stale although the mount
point is up to date? This is probably harmless, though.
More information about the freebsd-current