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

Rework locking, that I have introduced recently, since it was incorrect:

Browse Source 
First, mutexed callouts are incompatible with netgraph nodes, because
  netgraph(4) can guarantee that the function will be called with mutex
  held.

  Second, nodes should not send data to their neighbor holding their
  mutex. A node does not know what stack can it enter sending data in
  some direction. May be executing will encounter a place to sleep.

New locking:

  - ng_pptpgre_recv() and ng_pptpgre_xmit() must be entered with mutex held.
  - ng_pptpgre_recv() and ng_pptpgre_xmit() unlock mutex before
    sending data and then return unlocked.
  - callout routines acquire mutex themselves.
This commit is contained in:
glebius 2005-09-08 14:26:23 +00:00
parent 8f3eb2a425
commit 0743267ea6
1 changed files with 54 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

89
sys/netgraph/ng_pptpgre.c
View File

@ -286,8 +286,8 @@ ng_pptpgre_constructor(node_p node)
/* Initialize state */ /* Initialize state */
mtx_init(&priv->mtx, "ng_pptp", NULL, MTX_DEF); mtx_init(&priv->mtx, "ng_pptp", NULL, MTX_DEF);
ng_callout_init_mtx(&priv->ackp.sackTimer, &priv->mtx); ng_callout_init(&priv->ackp.sackTimer);
ng_callout_init_mtx(&priv->ackp.rackTimer, &priv->mtx); ng_callout_init(&priv->ackp.rackTimer);
/* Done */ /* Done */
return (0); return (0);
@ -409,7 +409,7 @@ ng_pptpgre_rcvdata(hook_p hook, item_p item)
else else
panic("%s: weird hook", __func__); panic("%s: weird hook", __func__);
mtx_unlock(&priv->mtx); mtx_assert(&priv->mtx, MA_NOTOWNED);
return (rval); return (rval);
} }
@ -475,6 +475,8 @@ ng_pptpgre_xmit(node_p node, item_p item)
int grelen, error; int grelen, error;
struct mbuf *m; struct mbuf *m;
mtx_assert(&priv->mtx, MA_OWNED);
if (item) { if (item) {
NGI_GET_M(item, m); NGI_GET_M(item, m);
} else { } else {
@ -489,18 +491,14 @@ ng_pptpgre_xmit(node_p node, item_p item)
if ((u_int32_t)PPTP_SEQ_DIFF(priv->xmitSeq, if ((u_int32_t)PPTP_SEQ_DIFF(priv->xmitSeq,
priv->recvAck) >= a->xmitWin) { priv->recvAck) >= a->xmitWin) {
priv->stats.xmitDrops++; priv->stats.xmitDrops++;
NG_FREE_M(m); ERROUT(ENOBUFS);
NG_FREE_ITEM(item);
return (ENOBUFS);
} }
} }
/* Sanity check frame length */ /* Sanity check frame length */
if (m != NULL && m->m_pkthdr.len > PPTP_MAX_PAYLOAD) { if (m != NULL && m->m_pkthdr.len > PPTP_MAX_PAYLOAD) {
priv->stats.xmitTooBig++; priv->stats.xmitTooBig++;
NG_FREE_M(m); ERROUT(EMSGSIZE);
NG_FREE_ITEM(item);
return (EMSGSIZE);
} }
} else { } else {
priv->stats.xmitLoneAcks++; priv->stats.xmitLoneAcks++;
@ -536,9 +534,7 @@ ng_pptpgre_xmit(node_p node, item_p item)
MGETHDR(m, M_DONTWAIT, MT_DATA); MGETHDR(m, M_DONTWAIT, MT_DATA);
if (m == NULL) { if (m == NULL) {
priv->stats.memoryFailures++; priv->stats.memoryFailures++;
if (item) ERROUT(ENOBUFS);
NG_FREE_ITEM(item);
return (ENOBUFS);
} }
m->m_len = m->m_pkthdr.len = grelen; m->m_len = m->m_pkthdr.len = grelen;
m->m_pkthdr.rcvif = NULL; m->m_pkthdr.rcvif = NULL;
@ -547,9 +543,7 @@ ng_pptpgre_xmit(node_p node, item_p item)
if (m == NULL || (m->m_len < grelen if (m == NULL || (m->m_len < grelen
&& (m = m_pullup(m, grelen)) == NULL)) { && (m = m_pullup(m, grelen)) == NULL)) {
priv->stats.memoryFailures++; priv->stats.memoryFailures++;
if (item) ERROUT(ENOBUFS);
NG_FREE_ITEM(item);
return (ENOBUFS);
} }
} }
bcopy(gre, mtod(m, u_char *), grelen); bcopy(gre, mtod(m, u_char *), grelen);
@ -558,6 +552,15 @@ ng_pptpgre_xmit(node_p node, item_p item)
priv->stats.xmitPackets++; priv->stats.xmitPackets++;
priv->stats.xmitOctets += m->m_pkthdr.len; priv->stats.xmitOctets += m->m_pkthdr.len;
/*
* XXX: we should reset timer only after an item has been sent
* successfully.
*/
if (gre->hasSeq && priv->xmitSeq == priv->recvAck + 1)
ng_pptpgre_start_recv_ack_timer(node);
mtx_unlock(&priv->mtx);
/* Deliver packet */ /* Deliver packet */
if (item) { if (item) {
NG_FWD_NEW_DATA(error, item, priv->lower, m); NG_FWD_NEW_DATA(error, item, priv->lower, m);
@ -565,10 +568,13 @@ ng_pptpgre_xmit(node_p node, item_p item)
NG_SEND_DATA_ONLY(error, priv->lower, m); NG_SEND_DATA_ONLY(error, priv->lower, m);
} }
return (error);
/* Start receive ACK timer if data was sent and not already running */ done:
if (error == 0 && gre->hasSeq && priv->xmitSeq == priv->recvAck + 1) mtx_unlock(&priv->mtx);
ng_pptpgre_start_recv_ack_timer(node); NG_FREE_M(m);
if (item)
NG_FREE_ITEM(item);
return (error); return (error);
} }
@ -585,6 +591,8 @@ ng_pptpgre_recv(node_p node, item_p item)
int error = 0; int error = 0;
struct mbuf *m; struct mbuf *m;
mtx_assert(&priv->mtx, MA_OWNED);
NGI_GET_M(item, m); NGI_GET_M(item, m);
/* Update stats */ /* Update stats */
priv->stats.recvPackets++; priv->stats.recvPackets++;
@ -593,26 +601,21 @@ ng_pptpgre_recv(node_p node, item_p item)
/* Sanity check packet length */ /* Sanity check packet length */
if (m->m_pkthdr.len < sizeof(*ip) + sizeof(*gre)) { if (m->m_pkthdr.len < sizeof(*ip) + sizeof(*gre)) {
priv->stats.recvRunts++; priv->stats.recvRunts++;
bad: ERROUT(EINVAL);
NG_FREE_M(m);
NG_FREE_ITEM(item);
return (EINVAL);
} }
/* Safely pull up the complete IP+GRE headers */ /* Safely pull up the complete IP+GRE headers */
if (m->m_len < sizeof(*ip) + sizeof(*gre) if (m->m_len < sizeof(*ip) + sizeof(*gre)
&& (m = m_pullup(m, sizeof(*ip) + sizeof(*gre))) == NULL) { && (m = m_pullup(m, sizeof(*ip) + sizeof(*gre))) == NULL) {
priv->stats.memoryFailures++; priv->stats.memoryFailures++;
NG_FREE_ITEM(item); ERROUT(ENOBUFS);
return (ENOBUFS);
} }
ip = mtod(m, const struct ip *); ip = mtod(m, const struct ip *);
iphlen = ip->ip_hl << 2; iphlen = ip->ip_hl << 2;
if (m->m_len < iphlen + sizeof(*gre)) { if (m->m_len < iphlen + sizeof(*gre)) {
if ((m = m_pullup(m, iphlen + sizeof(*gre))) == NULL) { if ((m = m_pullup(m, iphlen + sizeof(*gre))) == NULL) {
priv->stats.memoryFailures++; priv->stats.memoryFailures++;
NG_FREE_ITEM(item); ERROUT(ENOBUFS);
return (ENOBUFS);
} }
ip = mtod(m, const struct ip *); ip = mtod(m, const struct ip *);
} }
@ -620,13 +623,12 @@ ng_pptpgre_recv(node_p node, item_p item)
grelen = sizeof(*gre) + sizeof(u_int32_t) * (gre->hasSeq + gre->hasAck); grelen = sizeof(*gre) + sizeof(u_int32_t) * (gre->hasSeq + gre->hasAck);
if (m->m_pkthdr.len < iphlen + grelen) { if (m->m_pkthdr.len < iphlen + grelen) {
priv->stats.recvRunts++; priv->stats.recvRunts++;
goto bad; ERROUT(EINVAL);
} }
if (m->m_len < iphlen + grelen) { if (m->m_len < iphlen + grelen) {
if ((m = m_pullup(m, iphlen + grelen)) == NULL) { if ((m = m_pullup(m, iphlen + grelen)) == NULL) {
priv->stats.memoryFailures++; priv->stats.memoryFailures++;
NG_FREE_ITEM(item); ERROUT(ENOBUFS);
return (ENOBUFS);
} }
ip = mtod(m, const struct ip *); ip = mtod(m, const struct ip *);
gre = (const struct greheader *)((const u_char *)ip + iphlen); gre = (const struct greheader *)((const u_char *)ip + iphlen);
@ -637,16 +639,16 @@ ng_pptpgre_recv(node_p node, item_p item)
- (iphlen + grelen + gre->hasSeq * (u_int16_t)ntohs(gre->length)); - (iphlen + grelen + gre->hasSeq * (u_int16_t)ntohs(gre->length));
if (extralen < 0) { if (extralen < 0) {
priv->stats.recvBadGRE++; priv->stats.recvBadGRE++;
goto bad; ERROUT(EINVAL);
} }
if ((ntohl(*((const u_int32_t *)gre)) & PPTP_INIT_MASK) if ((ntohl(*((const u_int32_t *)gre)) & PPTP_INIT_MASK)
!= PPTP_INIT_VALUE) { != PPTP_INIT_VALUE) {
priv->stats.recvBadGRE++; priv->stats.recvBadGRE++;
goto bad; ERROUT(EINVAL);
} }
if (ntohs(gre->cid) != priv->conf.cid) { if (ntohs(gre->cid) != priv->conf.cid) {
priv->stats.recvBadCID++; priv->stats.recvBadCID++;
goto bad; ERROUT(EINVAL);
} }
/* Look for peer ack */ /* Look for peer ack */
@ -711,7 +713,7 @@ ng_pptpgre_recv(node_p node, item_p item)
priv->stats.recvDuplicates++; priv->stats.recvDuplicates++;
else else
priv->stats.recvOutOfOrder++; priv->stats.recvOutOfOrder++;
goto bad; /* out-of-order or dup */ ERROUT(EINVAL);
} }
priv->recvSeq = seq; priv->recvSeq = seq;
@ -723,9 +725,10 @@ ng_pptpgre_recv(node_p node, item_p item)
maxWait = (a->rtt >> 2); maxWait = (a->rtt >> 2);
/* If delayed ACK is disabled, send it now */ /* If delayed ACK is disabled, send it now */
if (!priv->conf.enableDelayedAck) /* ack now */ if (!priv->conf.enableDelayedAck) { /* ack now */
ng_pptpgre_xmit(node, NULL); ng_pptpgre_xmit(node, NULL);
else { /* ack later */ mtx_lock(&priv->mtx);
} else { /* ack later */
if (maxWait < PPTP_MIN_ACK_DELAY) if (maxWait < PPTP_MIN_ACK_DELAY)
maxWait = PPTP_MIN_ACK_DELAY; maxWait = PPTP_MIN_ACK_DELAY;
if (maxWait > PPTP_MAX_ACK_DELAY) if (maxWait > PPTP_MAX_ACK_DELAY)
@ -739,13 +742,22 @@ ng_pptpgre_recv(node_p node, item_p item)
if (extralen > 0) if (extralen > 0)
m_adj(m, -extralen); m_adj(m, -extralen);
mtx_unlock(&priv->mtx);
/* Deliver frame to upper layers */ /* Deliver frame to upper layers */
NG_FWD_NEW_DATA(error, item, priv->upper, m); NG_FWD_NEW_DATA(error, item, priv->upper, m);
} else { } else {
priv->stats.recvLoneAcks++; priv->stats.recvLoneAcks++;
mtx_unlock(&priv->mtx);
NG_FREE_ITEM(item); NG_FREE_ITEM(item);
NG_FREE_M(m); /* no data to deliver */ NG_FREE_M(m); /* no data to deliver */
} }
return (error);
done:
mtx_unlock(&priv->mtx);
NG_FREE_ITEM(item);
NG_FREE_M(m);
return (error); return (error);
} }
@ -811,6 +823,7 @@ ng_pptpgre_recv_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
const priv_p priv = NG_NODE_PRIVATE(node); const priv_p priv = NG_NODE_PRIVATE(node);
struct ng_pptpgre_ackp *const a = &priv->ackp; struct ng_pptpgre_ackp *const a = &priv->ackp;
mtx_lock(&priv->mtx);
/* Update adaptive timeout stuff */ /* Update adaptive timeout stuff */
priv->stats.recvAckTimeouts++; priv->stats.recvAckTimeouts++;
@ -832,6 +845,8 @@ ng_pptpgre_recv_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
priv->recvAck = priv->xmitSeq; /* pretend we got the ack */ priv->recvAck = priv->xmitSeq; /* pretend we got the ack */
a->xmitWin = (a->xmitWin + 1) / 2; /* shrink transmit window */ a->xmitWin = (a->xmitWin + 1) / 2; /* shrink transmit window */
a->winAck = priv->recvAck + a->xmitWin; /* reset win expand time */ a->winAck = priv->recvAck + a->xmitWin; /* reset win expand time */
mtx_unlock(&priv->mtx);
} }
/* /*
@ -872,8 +887,12 @@ ng_pptpgre_stop_send_ack_timer(node_p node)
static void static void
ng_pptpgre_send_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2) ng_pptpgre_send_ack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
{ {
const priv_p priv = NG_NODE_PRIVATE(node);
mtx_lock(&priv->mtx);
/* Send a frame with an ack but no payload */ /* Send a frame with an ack but no payload */
ng_pptpgre_xmit(node, NULL); ng_pptpgre_xmit(node, NULL);
mtx_assert(&priv->mtx, MA_NOTOWNED);
} }
/************************************************************************* /*************************************************************************