Re: Bug: devfs is sure to have the bug.

Hello,

> Hello,
> 
> 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.
> 
> 
> "dm->dm_generation" is updated with "devfs_generation" in
> 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 that we should change the lock method.
> May I have advice?
> 
> void
> devfs_populate(struct devfs_mount *dm)
> {
> 
>         sx_assert(&dm->dm_lock, SX_XLOCKED);
>         if (dm->dm_generation == devfs_generation)
>                 return;
>         while (devfs_populate_loop(dm, 0))
>                 continue;
> (***)
>         dm->dm_generation = devfs_generation;
> }
> 
> void
> devfs_create(struct cdev *dev)
> {
>         struct cdev_priv *cdp;
> 
>         mtx_assert(&devmtx, MA_OWNED);
>         cdp = cdev2priv(dev);
>         cdp->cdp_flags |= CDP_ACTIVE;
>         cdp->cdp_inode = alloc_unrl(devfs_inos);
>         dev_refl(dev);
>         TAILQ_INSERT_TAIL(&cdevp_list, cdp, cdp_list);
>         devfs_generation++;
> }
> 
> void
> devfs_destroy(struct cdev *dev)
> {
>         struct cdev_priv *cdp;
> 
>         mtx_assert(&devmtx, MA_OWNED);
>         cdp = cdev2priv(dev);
>         cdp->cdp_flags &= ~CDP_ACTIVE;
>         devfs_generation++;
> }
> 
> Thanks.

I propose the solution. May I have advice?

void
devfs_populate(struct devfs_mount *dm)
{

        sx_assert(&dm->dm_lock, SX_XLOCKED);
#if 1 /* I propose... */
        int tmp_generation;
retry:
        tmp_generation = devfs_generation;
        if (dm->dm_generation == tmp_generation)
                return;
        while (devfs_populate_loop(dm, 0))
                continue;
        if (tmp_generation != devfs_generation)
                goto retry;
        dm->dm_generation = tmp_generation;
#else /* Original */
        if (dm->dm_generation == devfs_generation)
                return;
        while (devfs_populate_loop(dm, 0))
                continue;
        dm->dm_generation = devfs_generation;
#endif
}

Thanks,
  Kohji Okuno (okuno.kohji_at_jp.panasonic.com)