Changeset View
Standalone View
sys/kern/uipc_socket.c
Show First 20 Lines • Show All 799 Lines • ▼ Show 20 Lines | if (so->so_options & SO_ACCEPTCONN) { | ||||
KASSERT((TAILQ_EMPTY(&so->so_incomp)), | KASSERT((TAILQ_EMPTY(&so->so_incomp)), | ||||
("sofree: so_incomp populated")); | ("sofree: so_incomp populated")); | ||||
} | } | ||||
SOCK_UNLOCK(so); | SOCK_UNLOCK(so); | ||||
ACCEPT_UNLOCK(); | ACCEPT_UNLOCK(); | ||||
VNET_SO_ASSERT(so); | VNET_SO_ASSERT(so); | ||||
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL) | 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) | if (pr->pr_usrreqs->pru_detach != NULL) | ||||
(*pr->pr_usrreqs->pru_detach)(so); | (*pr->pr_usrreqs->pru_detach)(so); | ||||
/* | /* | ||||
* From this point on, we assume that no other references to this | * From this point on, we assume that no other references to this | ||||
* socket exist anywhere else in the stack. Therefore, no locks need | * socket exist anywhere else in the stack. Therefore, no locks need | ||||
* to be acquired or held. | * to be acquired or held. | ||||
* | * | ||||
▲ Show 20 Lines • Show All 1,539 Lines • ▼ Show 20 Lines | |||||
{ | { | ||||
struct sockbuf *sb = &so->so_rcv; | struct sockbuf *sb = &so->so_rcv; | ||||
struct protosw *pr = so->so_proto; | struct protosw *pr = so->so_proto; | ||||
struct sockbuf asb; | struct sockbuf asb; | ||||
VNET_SO_ASSERT(so); | VNET_SO_ASSERT(so); | ||||
/* | /* | ||||
* In order to avoid calling dom_dispose with the socket buffer mutex | * In order to avoid calling dom_dispose with the socket buffer mutex | ||||
markj: This comment doesn't really apply anymore.
Now we're invoking dom_dispose without a lock held… | |||||
Not Done Inline Actions
We were before too. I don't think it's any more unsafe than it was before. The only new behavior in unp_dispose_so is (1) accessing so_pcb with sotounpcb. I don't see a lock annotation on that struct member and it seems reasonably something that might have the same lifetime as the socket itself. And, (2) setting a unp_gcflag under the UNP_LIST_LOCK. It seems safe so long as all users of dom_dispose are aware of what they're getting. In-tree, that's just Unix sockets. cem: > Now we're invoking dom_dispose without a lock held on the corresponding socket buffer, and… | |||||
Not Done Inline Actions
Hm... Do we need the SOCKBUF_LOCK or sblock to access sb_mb? Anyway, you're right, this isn't quite right... the bzero(&sb->sb_startzero) will clear sb_mb on the original so object and unp_dispose won't have much to do. Hmm. cem: > This comment doesn't really apply anymore.
Hm... Do we need the `SOCKBUF_LOCK` or `sblock`… | |||||
Not Done Inline Actionsmjg proposes just faking the entire so instead of the recv sb: https://lists.freebsd.org/pipermail/freebsd-current/2015-July/056521.html cem: mjg proposes just faking the entire `so` instead of the recv `sb`: https://lists.freebsd. | |||||
* held, and in order to generally avoid holding the lock for a long | * held, and in order to generally avoid holding the lock for a long | ||||
* time, we make a copy of the socket buffer and clear the original | * time, we make a copy of the socket buffer and clear the original | ||||
* (except locks, state). The new socket buffer copy won't have | * (except locks, state). The new socket buffer copy won't have | ||||
* initialized locks so we can only call routines that won't use or | * initialized locks so we can only call routines that won't use or | ||||
* assert those locks. | * assert those locks. | ||||
* | * | ||||
* Dislodge threads currently blocked in receive and wait to acquire | * Dislodge threads currently blocked in receive and wait to acquire | ||||
* a lock against other simultaneous readers before clearing the | * a lock against other simultaneous readers before clearing the | ||||
Show All 16 Lines | sorflush(struct socket *so) | ||||
SOCKBUF_UNLOCK(sb); | SOCKBUF_UNLOCK(sb); | ||||
sbunlock(sb); | sbunlock(sb); | ||||
/* | /* | ||||
* Dispose of special rights and flush the socket buffer. Don't call | * Dispose of special rights and flush the socket buffer. Don't call | ||||
* any unsafe routines (that rely on locks being initialized) on asb. | * any unsafe routines (that rely on locks being initialized) on asb. | ||||
*/ | */ | ||||
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL) | if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL) | ||||
(*pr->pr_domain->dom_dispose)(asb.sb_mb); | (*pr->pr_domain->dom_dispose)(so); | ||||
sbrelease_internal(&asb, so); | sbrelease_internal(&asb, so); | ||||
} | } | ||||
/* | /* | ||||
* Wrapper for Socket established helper hook. | * Wrapper for Socket established helper hook. | ||||
* Parameters: socket, context of the hook point, hook id. | * Parameters: socket, context of the hook point, hook id. | ||||
*/ | */ | ||||
static int inline | static int inline | ||||
▲ Show 20 Lines • Show All 1,301 Lines • Show Last 20 Lines |
This comment doesn't really apply anymore.
Now we're invoking dom_dispose without a lock held on the corresponding socket buffer, and that's presumably unsafe. And we probably can't just hold the buffer lock across the call to dom_dispose(), e.g. because unp_discard() does an M_WAITOK allocation.