From 0c40f3532d947d138d5a99f5c8853de06de44fec Mon Sep 17 00:00:00 2001 From: Conrad Meyer Date: Tue, 14 Jul 2015 02:00:50 +0000 Subject: [PATCH] Fix cleanup race between unp_dispose and unp_gc 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 --- sys/kern/uipc_socket.c | 17 +++++++++-------- sys/kern/uipc_usrreq.c | 34 ++++++++++++++++++++++++++++------ sys/sys/domain.h | 3 ++- sys/sys/unpcb.h | 1 + 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index a431b4be6ace..c9d817954b97 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -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); } /* diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c index 7b2984c01780..c3a71c8fb351 100644 --- a/sys/kern/uipc_usrreq.c +++ b/sys/kern/uipc_usrreq.c @@ -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)) { diff --git a/sys/sys/domain.h b/sys/sys/domain.h index bacdad143f07..1817e7881c6f 100644 --- a/sys/sys/domain.h +++ b/sys/sys/domain.h @@ -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 */ diff --git a/sys/sys/unpcb.h b/sys/sys/unpcb.h index ba63f3044374..41fd5745bbd7 100644 --- a/sys/sys/unpcb.h +++ b/sys/sys/unpcb.h @@ -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()