Re: CVS commit: src/sys/ufs/ufs

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Tue, Sep 22, 2009 at 09:23:05PM +0300, Antti Kantee wrote:
> On Tue Sep 22 2009 at 20:06:40 +0200, Manuel Bouyer wrote:
> > On Sun, Sep 20, 2009 at 08:23:38PM +0300, Antti Kantee wrote:
> > > > In ufs_ihashget(), vget() can return a vnode that has been vclean'ed 
> > > > because
> > > > vget() can sleep. After vget returns, check that vp is still connected 
> > > > with
> > > > ip, and that ip still points to the inode we want. This fix the NULL
> > > > pointer dereference in ufs_fhtovp() I've been seeing on a NFS server.
> > > 
> > > Um, hold the phone.  The whole point of vget() is to provide race-free
> > > access to the weak vnode reference held by the file system.  Are you
> > > saying this does not hold anymore?
> > 
> > It depends on what you mean with "race-free". If you mean that the
> > vnode returned by vget() can't be recygled, I think this is true.
> > If you mean that vget() can't return a clean vnode then this is false:
> > vget() can sleep in vn_lock(), and it releases the v_interlock mutex before
> > sleeping. While sleeping vclean() can VOP_RECLAIM() the vnode, even
> > if v_usecount is > 1.
> 
> What is the practical difference of "cleaned" and "recycled" for the
> file system driver?

From what I understand the vnode is not on the free list yet so it can't
be reused for something else.

> 
> If there is a race in vfs and XLOCK is not used properly, I think that
> should be investigated and fixed instead of patching file systems here
> and there.

I don't know how XLOCK is supposed to be used (and I even less know how
to change things without creating deadlocks). As I see it XLOCK is set
while the vnode is being cleaned, not before or after cleaning it, so
XLOCK is not going to avoid this race anyway.

I asked for help on tech-kern about this but didn't get any reply ...

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--