syncer vnode leak because of nmount() race

[Attilio Rao]

2010/6/3 Jaakko Heinonen <jh at freebsd.org>:
>
> Hi,
>
> I have been playing with devfs and stress2 recently and I was able to
> trigger a syncer vnode leak with devfs.sh. As far as I can tell it is a
> nmount(2) (initial) mount race against update mount. The code in
> vfs_domount() is protected with vfs_busy()/vfs_unbusy() but it doesn't
> protect against update mounts. The protection by Giant is defeated
> because devfs_root() may sleep due to sx_xlock(). vfs_domount() calls
> VFS_ROOT() before allocating the syncer vnode. VI_MOUNT vnode flag
> doesn't provide sufficient protection against update mounts either.

Thanks for this bug report.
I think that, luckilly, it is not a very common condition to have the
mount still in flight and get updates... :)
However, I think that the real breakage here is that the check on
mnt->mnt_syncer is done lockless and it is unsafe. It might really be
protected by the mount interlock but that's tricky because
vfs_allocate_syncvnode() sleeps. Also re-checking the condition (after
the allocation takes place) is not too simple here.
I found also this bug when rewriting the syncer and I resolved that by
using a separate flag for that (in my case it was simpler and more
beneficial actually for some other reasons, but you may do the same
thing with a mnt_kern_flag entry).
If you have no time I can work actively on the patch, even if I'm
confident, luckilly, this problem is very hard to happen in the real
life.

Additively, note that vfs_busy() here is not intended to protect
against such situations but against unmount.
Also, I'm very unsure about what Giant protection might bring here.
IMHO we might probabilly remove it at all as long as all the sleeping
point present make it completely unuseful if anything (and I don't see
a reason why it may be needed).

> PS. vfs_unbusy(9) manual page is out of date after r184554 and IMO
>    vfs_busy(9) manual page is misleading because it talks about
>    synchronizing access to a mount point.

May you be more precise on what is misleading please?

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein