Re: System call munmap returning with the following locks held: Giant

On Thursday 19 January 2006 08:02, John Baldwin wrote:
> On Wednesday 18 January 2006 08:31 pm, Suleiman Souhlal wrote:
> > Hi,
> >
> > John Baldwin wrote:
> > > I sent this to you on IRC, but for the archives, here's a possible fix.
> > > It looks like vm_object_deallocate() never unlocks Giant if it locks
> > > it, and the leak would only happen if mpsafevfs=0 or you are using a
> > > non-safe filesystem:
> >
> > The real problem is that vm_object_deallocate() doesn't expect the
> > object's type to change if it sees it's a vnode, when it's not holding
> > the object lock:
> >      /*
> >       * In general, the object should be locked when working with
> >       * its type.  In this case, in order to maintain proper lock
> >       * ordering, an exception is possible because a vnode-backed
> >       * object never changes its type.
> >       */
> >       vfslocked = 0;
> >       if (object->type == OBJT_VNODE) {
> >           struct vnode *vp = (struct vnode *) object->handle;
> >           vfslocked = VFS_LOCK_GIANT(vp->v_mount);
> >       }
> >       VM_OBJECT_LOCK(object);
> >       if (object->type == OBJT_VNODE) {
> >           vm_object_vndeallocate(object);
> >           VFS_UNLOCK_GIANT(vfslocked);
> >           return;
> >        }
> >
> > The comment is actually wrong, and the object's type can change to
> > OBJT_DEAD when the corresponing vnode gets freed, so maybe you might
> > want to change it.
>
> Well, that's not the cause of Kris' panic at all (the function really is
> not ever dropping Giant).  If the object does change to OBJT_DEAD after
> Giant is acquired then some of the MPASS()'s I added might fail I think. 
> I'm not sure if that's all that has to be done to fix the problem you are
> concerned about.

Actually, if the object's type isn't guaranteed to be stable by teh caller 
somehow, then the VFS_LOCK_GIANT part itself is racey.  Really fixing it 
would be something ugly like this:

	vfslocked = 0;
restart:
	VM_OBJECT_LOCK(object);
	if (object->type == OBJT_VNODE) {
		struct vnode *vp = (struct vnode *)object->handle;

		if (VFS_NEEDSGIANT(vp->v_mount) {
			VM_OBJECT_UNLOCK(object);
			mtx_lock(&Giant);
			vfslocked = 1;
			goto restart;
		} else
			VFS_UNLOCK_GIANT(vfslocked);
	} else
		VFS_UNLOCK_GIANT(vfslocked);
	...

Are you really sure the object's type can change or does the caller of 
vm_object_deallocate() hold some sort of reference or what not that prevents 
the type from changing?

-- 
John Baldwin <jhb_at_FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org