svn commit: r285310 - head/sys/kern
Mateusz Guzik
mjguzik at gmail.com
Thu Jul 9 10:11:38 UTC 2015
On Thu, Jul 09, 2015 at 09:22:22AM +0000, Konstantin Belousov wrote:
> Author: kib
> Date: Thu Jul 9 09:22:21 2015
> New Revision: 285310
> URL: https://svnweb.freebsd.org/changeset/base/285310
>
> Log:
> Cover a race between doselwakeup() and selfdfree(). If doselwakeup()
> loop finds the selfd entry and clears its sf_si pointer, which is
> handled by selfdfree() in parallel, NULL sf_si makes selfdfree() free
> the memory. The result is the race and accesses to the freed memory.
>
> Refcount the selfd ownership. One reference is for the sf_link
> linkage, which is unconditionally dereferenced by selfdfree().
> Another reference is for sf_threads, both selfdfree() and
> doselwakeup() race to deref it, the winner unlinks and than frees the
> selfd entry.
>
> MFC after: 2 weeks
Looks like my bug introduced as a result of r273549 + r273555. It was
not MFCed, so this change does not need a MFC either.
With the lock taken unconditionally there is no window where sf_si NULL
is visible to selfdfree while doselwakeup still uses the sfp.
afair the motivation for the change was some minor contention shown by
lock profiling during buildworld/buildkernel, as sf_si comes from a mtx
pool.
So just in case, I vote for leaving stuff as it is, I'm just noting
there is no need to MFC.
>
> Modified:
> head/sys/kern/sys_generic.c
>
> Modified: head/sys/kern/sys_generic.c
> ==============================================================================
> --- head/sys/kern/sys_generic.c Thu Jul 9 07:31:40 2015 (r285309)
> +++ head/sys/kern/sys_generic.c Thu Jul 9 09:22:21 2015 (r285310)
> @@ -153,6 +153,7 @@ struct selfd {
> struct mtx *sf_mtx; /* Pointer to selinfo mtx. */
> struct seltd *sf_td; /* (k) owning seltd. */
> void *sf_cookie; /* (k) fd or pollfd. */
> + u_int sf_refs;
> };
>
> static uma_zone_t selfd_zone;
> @@ -1685,11 +1686,14 @@ selfdfree(struct seltd *stp, struct self
> STAILQ_REMOVE(&stp->st_selq, sfp, selfd, sf_link);
> if (sfp->sf_si != NULL) {
> mtx_lock(sfp->sf_mtx);
> - if (sfp->sf_si != NULL)
> + if (sfp->sf_si != NULL) {
> TAILQ_REMOVE(&sfp->sf_si->si_tdlist, sfp, sf_threads);
> + refcount_release(&sfp->sf_refs);
> + }
> mtx_unlock(sfp->sf_mtx);
> }
> - uma_zfree(selfd_zone, sfp);
> + if (refcount_release(&sfp->sf_refs))
> + uma_zfree(selfd_zone, sfp);
> }
>
> /* Drain the waiters tied to all the selfd belonging the specified selinfo. */
> @@ -1745,6 +1749,7 @@ selrecord(selector, sip)
> */
> sfp->sf_si = sip;
> sfp->sf_mtx = mtxp;
> + refcount_init(&sfp->sf_refs, 2);
> STAILQ_INSERT_TAIL(&stp->st_selq, sfp, sf_link);
> /*
> * Now that we've locked the sip, check for initialization.
> @@ -1809,6 +1814,8 @@ doselwakeup(sip, pri)
> stp->st_flags |= SELTD_PENDING;
> cv_broadcastpri(&stp->st_wait, pri);
> mtx_unlock(&stp->st_mtx);
> + if (refcount_release(&sfp->sf_refs))
> + uma_zfree(selfd_zone, sfp);
> }
> mtx_unlock(sip->si_mtx);
> }
> _______________________________________________
> svn-src-all at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"
--
Mateusz Guzik <mjguzik gmail.com>
More information about the svn-src-all
mailing list