d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix cleanup race between unp_dispose and unp_gc

Browse Source 
unp_dispose and unp_gc could race to teardown the same mbuf chains, which
can lead to dereferencing freed filedesc pointers.

This patch adds an IGNORE_RIGHTS flag on unpcbs marking the unpcb's RIGHTS
as invalid/freed. The flag is protected by UNP_LIST_LOCK.

To serialize against unp_gc, unp_dispose needs the socket object. Change the
dom_dispose() KPI to take a socket object instead of an mbuf chain directly.

PR:		194264
Differential Revision:	https://reviews.freebsd.org/D3044
Reviewed by:	mjg (earlier version)
Approved by:	markj (mentor)
Obtained from:	mjg
MFC after:	1 month
Sponsored by:	EMC / Isilon Storage Division
This commit is contained in:
cem 2015-07-14 02:00:50 +00:00
parent 05a0acba04
commit 576619e564
4 changed files with 40 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
sys/kern/uipc_socket.c
View File

@ -805,7 +805,7 @@ sofree(struct socket *so)
VNET_SO_ASSERT(so);
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL)
(*pr->pr_domain->dom_dispose)(so->so_rcv.sb_mb);
(*pr->pr_domain->dom_dispose)(so);
if (pr->pr_usrreqs->pru_detach != NULL)
(*pr->pr_usrreqs->pru_detach)(so);
@ -2356,7 +2356,7 @@ sorflush(struct socket *so)
{
struct sockbuf *sb = &so->so_rcv;
struct protosw *pr = so->so_proto;
struct sockbuf asb;
struct socket aso;
VNET_SO_ASSERT(so);
@ -2381,8 +2381,9 @@ sorflush(struct socket *so)
* and mutex data unchanged.
*/
SOCKBUF_LOCK(sb);
bzero(&asb, offsetof(struct sockbuf, sb_startzero));
bcopy(&sb->sb_startzero, &asb.sb_startzero,
bzero(&aso, sizeof(aso));
aso.so_pcb = so->so_pcb;
bcopy(&sb->sb_startzero, &aso.so_rcv.sb_startzero,
sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
bzero(&sb->sb_startzero,
sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
@ -2390,12 +2391,12 @@ sorflush(struct socket *so)
sbunlock(sb);
/*
* Dispose of special rights and flush the socket buffer. Don't call
* any unsafe routines (that rely on locks being initialized) on asb.
* Dispose of special rights and flush the copied socket. Don't call
* any unsafe routines (that rely on locks being initialized) on aso.
*/
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL)
(*pr->pr_domain->dom_dispose)(asb.sb_mb);
sbrelease_internal(&asb, so);
(*pr->pr_domain->dom_dispose)(&aso);
sbrelease_internal(&aso.so_rcv, so);
}
/*

34
sys/kern/uipc_usrreq.c
View File

@ -278,6 +278,7 @@ static int unp_connectat(int, struct socket *, struct sockaddr *,
static int unp_connect2(struct socket *so, struct socket *so2, int);
static void unp_disconnect(struct unpcb *unp, struct unpcb *unp2);
static void unp_dispose(struct mbuf *);
static void unp_dispose_so(struct socket *so);
static void unp_shutdown(struct unpcb *);
static void unp_drop(struct unpcb *, int);
static void unp_gc(__unused void *, int);
@ -334,7 +335,7 @@ static struct domain localdomain = {
.dom_name = "local",
.dom_init = unp_init,
.dom_externalize = unp_externalize,
.dom_dispose = unp_dispose,
.dom_dispose = unp_dispose_so,
.dom_protosw = localsw,
.dom_protoswNPROTOSW = &localsw[sizeof(localsw)/sizeof(localsw[0])]
};
@ -2216,15 +2217,19 @@ unp_gc_process(struct unpcb *unp)
* Mark all sockets we reference with RIGHTS.
*/
so = unp->unp_socket;
SOCKBUF_LOCK(&so->so_rcv);
unp_scan(so->so_rcv.sb_mb, unp_accessable);
SOCKBUF_UNLOCK(&so->so_rcv);
if ((unp->unp_gcflag & UNPGC_IGNORE_RIGHTS) == 0) {
SOCKBUF_LOCK(&so->so_rcv);
unp_scan(so->so_rcv.sb_mb, unp_accessable);
SOCKBUF_UNLOCK(&so->so_rcv);
}
/*
* Mark all sockets in our accept queue.
*/
ACCEPT_LOCK();
TAILQ_FOREACH(soa, &so->so_comp, so_list) {
if ((sotounpcb(soa)->unp_gcflag & UNPGC_IGNORE_RIGHTS) != 0)
continue;
SOCKBUF_LOCK(&soa->so_rcv);
unp_scan(soa->so_rcv.sb_mb, unp_accessable);
SOCKBUF_UNLOCK(&soa->so_rcv);
@ -2254,11 +2259,13 @@ unp_gc(__unused void *arg, int pending)
unp_taskcount++;
UNP_LIST_LOCK();
/*
* First clear all gc flags from previous runs.
* First clear all gc flags from previous runs, apart from
* UNPGC_IGNORE_RIGHTS.
*/
for (head = heads; *head != NULL; head++)
LIST_FOREACH(unp, *head, unp_link)
unp->unp_gcflag = 0;
unp->unp_gcflag =
(unp->unp_gcflag & UNPGC_IGNORE_RIGHTS);
/*
* Scan marking all reachable sockets with UNPGC_REF. Once a socket
@ -2335,6 +2342,21 @@ unp_dispose(struct mbuf *m)
unp_scan(m, unp_freerights);
}
/*
* Synchronize against unp_gc, which can trip over data as we are freeing it.
*/
static void
unp_dispose_so(struct socket *so)
{
struct unpcb *unp;
unp = sotounpcb(so);
UNP_LIST_LOCK();
unp->unp_gcflag |= UNPGC_IGNORE_RIGHTS;
UNP_LIST_UNLOCK();
unp_dispose(so->so_rcv.sb_mb);
}
static void
unp_scan(struct mbuf *m0, void (*op)(struct filedescent **, int))
{

3
sys/sys/domain.h
View File

@ -42,6 +42,7 @@
*/
struct mbuf;
struct ifnet;
struct socket;
struct domain {
int dom_family; /* AF_xxx */
@ -53,7 +54,7 @@ struct domain {
int (*dom_externalize) /* externalize access rights */
(struct mbuf *, struct mbuf **, int);
void (*dom_dispose) /* dispose of internalized rights */
(struct mbuf *);
(struct socket *);
struct protosw *dom_protosw, *dom_protoswNPROTOSW;
struct domain *dom_next;
int (*dom_rtattach) /* initialize routing table */

1
sys/sys/unpcb.h
View File

@ -106,6 +106,7 @@ struct unpcb {
#define UNPGC_REF 0x1 /* unpcb has external ref. */
#define UNPGC_DEAD 0x2 /* unpcb might be dead. */
#define UNPGC_SCANNED 0x4 /* Has been scanned. */
#define UNPGC_IGNORE_RIGHTS 0x8 /* Attached rights are freed */
/*
* These flags are used to handle non-atomicity in connect() and bind()