Date: Thu, 19 Jan 2006 11:14:24 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org Cc: alc@freebsd.org, Suleiman Souhlal <ssouhlal@freebsd.org>, Kris Kennaway <kris@obsecurity.org> Subject: Re: System call munmap returning with the following locks held: Giant Message-ID: <200601191114.27075.jhb@freebsd.org> In-Reply-To: <200601190802.31914.jhb@freebsd.org> References: <20060118070549.GA617@xor.obsecurity.org> <43CEEBD4.3060604@FreeBSD.org> <200601190802.31914.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200601191114.27075.jhb>