Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 4 Nov 2008 18:59:51 +0200
From:      Kostik Belousov <kostikbel@gmail.com>
To:        Scott Long <scottl@samsco.org>
Cc:        svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-7@freebsd.org
Subject:   Re: svn commit: r184641 - in stable/7/sys: . kern
Message-ID:  <20081104165951.GQ18100@deviant.kiev.zoral.com.ua>
In-Reply-To: <49107254.9070307@samsco.org>
References:  <200811041556.mA4FuijN001109@svn.freebsd.org> <49107254.9070307@samsco.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--sUOf/0EjaVk9Y/9c
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Nov 04, 2008 at 09:03:32AM -0700, Scott Long wrote:
> In stable branches, and especially during release cycles, would it be=20
> possible to annotate whether changes like this fix known panics or=20
> user-visible bugs?
I thought that description of the change made it obvious.
Access to the partially initialized structure is sure reason for a bad
behaviour, panic in this particular case.

It is slightly more involved in this case, because other thread was
able to overwrite pointer to fully initialized structure put by current
thread. This is what prevented by vnode interlock region.

>=20
> Scott
>=20
>=20
> Konstantin Belousov wrote:
> >Author: kib
> >Date: Tue Nov  4 15:56:44 2008
> >New Revision: 184641
> >URL: http://svn.freebsd.org/changeset/base/184641
> >
> >Log:
> >  MFC r184409:
> >  Protect check for v_pollinfo =3D=3D NULL and assignment of the newly=
=20
> >  allocated
> >  vpollinfo with vnode interlock. Fully initialize vpollinfo before putt=
ing
> >  pointer to it into vp->v_pollinfo.
> > =20
> >  Approved by:	re (kensmith)
> >
> >Modified:
> >  stable/7/sys/   (props changed)
> >  stable/7/sys/kern/vfs_subr.c
> >
> >Modified: stable/7/sys/kern/vfs_subr.c
> >=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> >--- stable/7/sys/kern/vfs_subr.c	Tue Nov  4 15:47:06 2008 (r184640)
> >+++ stable/7/sys/kern/vfs_subr.c	Tue Nov  4 15:56:44 2008 (r184641)
> >@@ -109,7 +109,7 @@ static void	vgonel(struct vnode *);
> > static void	vfs_knllock(void *arg);
> > static void	vfs_knlunlock(void *arg);
> > static int	vfs_knllocked(void *arg);
> >-
> >+static void	destroy_vpollinfo(struct vpollinfo *vi);
> >=20
> > /*
> >  * Enable Giant pushdown based on whether or not the vm is mpsafe in th=
is
> >@@ -815,11 +815,8 @@ vdestroy(struct vnode *vp)
> > #ifdef MAC
> > 	mac_destroy_vnode(vp);
> > #endif
> >-	if (vp->v_pollinfo !=3D NULL) {
> >-		knlist_destroy(&vp->v_pollinfo->vpi_selinfo.si_note);
> >-		mtx_destroy(&vp->v_pollinfo->vpi_lock);
> >-		uma_zfree(vnodepoll_zone, vp->v_pollinfo);
> >-	}
> >+	if (vp->v_pollinfo !=3D NULL)
> >+		destroy_vpollinfo(vp->v_pollinfo);
> > #ifdef INVARIANTS
> > 	/* XXX Elsewhere we can detect an already freed vnode via NULL v_op.=
=20
> > 	*/
> > 	vp->v_op =3D NULL;
> >@@ -3050,6 +3047,14 @@ vbusy(struct vnode *vp)
> > 	mtx_unlock(&vnode_free_list_mtx);
> > }
> >=20
> >+static void
> >+destroy_vpollinfo(struct vpollinfo *vi)
> >+{
> >+	knlist_destroy(&vi->vpi_selinfo.si_note);
> >+	mtx_destroy(&vi->vpi_lock);
> >+	uma_zfree(vnodepoll_zone, vi);
> >+}
> >+
> > /*
> >  * Initalize per-vnode helper structure to hold poll-related state.
> >  */
> >@@ -3058,15 +3063,20 @@ v_addpollinfo(struct vnode *vp)
> > {
> > 	struct vpollinfo *vi;
> >=20
> >+	if (vp->v_pollinfo !=3D NULL)
> >+		return;
> > 	vi =3D uma_zalloc(vnodepoll_zone, M_WAITOK);
> >+	mtx_init(&vi->vpi_lock, "vnode pollinfo", NULL, MTX_DEF);
> >+	knlist_init(&vi->vpi_selinfo.si_note, vp, vfs_knllock,
> >+	    vfs_knlunlock, vfs_knllocked);
> >+	VI_LOCK(vp);
> > 	if (vp->v_pollinfo !=3D NULL) {
> >-		uma_zfree(vnodepoll_zone, vi);
> >+		VI_UNLOCK(vp);
> >+		destroy_vpollinfo(vi);
> > 		return;
> > 	}
> > 	vp->v_pollinfo =3D vi;
> >-	mtx_init(&vp->v_pollinfo->vpi_lock, "vnode pollinfo", NULL, MTX_DEF);
> >-	knlist_init(&vp->v_pollinfo->vpi_selinfo.si_note, vp, vfs_knllock,
> >-	    vfs_knlunlock, vfs_knllocked);
> >+	VI_UNLOCK(vp);
> > }
> >=20
> > /*
> >@@ -3081,8 +3091,7 @@ int
> > vn_pollrecord(struct vnode *vp, struct thread *td, int events)
> > {
> >=20
> >-	if (vp->v_pollinfo =3D=3D NULL)
> >-		v_addpollinfo(vp);
> >+	v_addpollinfo(vp);
> > 	mtx_lock(&vp->v_pollinfo->vpi_lock);
> > 	if (vp->v_pollinfo->vpi_revents & events) {
> > 		/*
> >@@ -3917,8 +3926,7 @@ vfs_kqfilter(struct vop_kqfilter_args *a
> >=20
> > 	kn->kn_hook =3D (caddr_t)vp;
> >=20
> >-	if (vp->v_pollinfo =3D=3D NULL)
> >-		v_addpollinfo(vp);
> >+	v_addpollinfo(vp);
> > 	if (vp->v_pollinfo =3D=3D NULL)
> > 		return (ENOMEM);
> > 	knl =3D &vp->v_pollinfo->vpi_selinfo.si_note;

--sUOf/0EjaVk9Y/9c
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)

iEYEARECAAYFAkkQf4cACgkQC3+MBN1Mb4h9sQCgsy6wmhViv3AMrezd1PIXE5zw
Vg4An1N13nuyG3CkPIEyrWnFZuSz/TWt
=qPC0
-----END PGP SIGNATURE-----

--sUOf/0EjaVk9Y/9c--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081104165951.GQ18100>