ng_l2tp: improve seq structure locking.
Cover few cases of access to seq without lock missed in 702f98951d.
There are no known bugs fixed with this change, however. With INVARIANTS
embed ng_l2tp_seq_check() into lock/unlock macros. Slightly reduce number
of locks/unlocks per packet keeping the lock between functions.
Reviewed by: mjg, markj
Differential Revision: https://reviews.freebsd.org/D31476
This commit is contained in:
Gleb Smirnoff
parent
b2954f0a8f
commit
0a76c63dd4
@ -188,10 +188,6 @@ static void ng_l2tp_seq_rack_timeout(node_p node, hook_p hook,
|
||||
static hookpriv_p ng_l2tp_find_session(priv_p privp, u_int16_t sid);
|
||||
static ng_fn_eachhook ng_l2tp_reset_session;
|
||||
|
||||
#ifdef INVARIANTS
|
||||
static void ng_l2tp_seq_check(struct l2tp_seq *seq);
|
||||
#endif
|
||||
|
||||
/* Parse type for struct ng_l2tp_seq_config. */
|
||||
static const struct ng_parse_struct_field
|
||||
ng_l2tp_seq_config_fields[] = NG_L2TP_SEQ_CONFIG_TYPE_INFO;
|
||||
@ -335,12 +331,22 @@ static struct ng_type ng_l2tp_typestruct = {
|
||||
};
|
||||
NETGRAPH_INIT(l2tp, &ng_l2tp_typestruct);
|
||||
|
||||
/* Sequence number state sanity checking */
|
||||
/* Sequence number state locking & sanity checking */
|
||||
#ifdef INVARIANTS
|
||||
#define L2TP_SEQ_CHECK(seq) ng_l2tp_seq_check(seq)
|
||||
static void ng_l2tp_seq_check(struct l2tp_seq *seq);
|
||||
#define SEQ_LOCK(seq) do { \
|
||||
mtx_lock(&(seq)->mtx); \
|
||||
ng_l2tp_seq_check(seq); \
|
||||
} while (0)
|
||||
#define SEQ_UNLOCK(seq) do { \
|
||||
ng_l2tp_seq_check(seq); \
|
||||
mtx_unlock(&(seq)->mtx); \
|
||||
} while (0)
|
||||
#else
|
||||
#define L2TP_SEQ_CHECK(x) do { } while (0)
|
||||
#define SEQ_LOCK(seq) mtx_lock(&(seq)->mtx)
|
||||
#define SEQ_UNLOCK(seq) mtx_unlock(&(seq)->mtx)
|
||||
#endif
|
||||
#define SEQ_LOCK_ASSERT(seq) mtx_assert(&(seq)->mtx, MA_OWNED)
|
||||
|
||||
/* Whether to use m_copypacket() or m_dup() */
|
||||
#define L2TP_COPY_MBUF m_copypacket
|
||||
@ -650,11 +656,10 @@ ng_l2tp_shutdown(node_p node)
|
||||
const priv_p priv = NG_NODE_PRIVATE(node);
|
||||
struct l2tp_seq *const seq = &priv->seq;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
|
||||
/* Reset sequence number state */
|
||||
SEQ_LOCK(seq);
|
||||
ng_l2tp_seq_reset(priv);
|
||||
SEQ_UNLOCK(seq);
|
||||
|
||||
/* Free private data if neither timer is running */
|
||||
ng_uncallout(&seq->rack_timer, node);
|
||||
@ -757,9 +762,6 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
int error;
|
||||
int len, plen;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
|
||||
/* If not configured, reject */
|
||||
if (!priv->conf.enabled) {
|
||||
NG_FREE_ITEM(item);
|
||||
@ -899,18 +901,20 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
if ((hdr & L2TP_HDR_CTRL) != 0) {
|
||||
struct l2tp_seq *const seq = &priv->seq;
|
||||
|
||||
SEQ_LOCK(seq);
|
||||
|
||||
/* Handle receive ack sequence number Nr */
|
||||
ng_l2tp_seq_recv_nr(priv, nr);
|
||||
|
||||
/* Discard ZLB packets */
|
||||
if (m->m_pkthdr.len == 0) {
|
||||
SEQ_UNLOCK(seq);
|
||||
priv->stats.recvZLBs++;
|
||||
NG_FREE_ITEM(item);
|
||||
NG_FREE_M(m);
|
||||
ERROUT(0);
|
||||
}
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
/*
|
||||
* If not what we expect or we are busy, drop packet and
|
||||
* send an immediate ZLB ack.
|
||||
@ -920,23 +924,16 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
priv->stats.recvDuplicates++;
|
||||
else
|
||||
priv->stats.recvOutOfOrder++;
|
||||
mtx_unlock(&seq->mtx);
|
||||
ng_l2tp_xmit_ctrl(priv, NULL, seq->ns);
|
||||
NG_FREE_ITEM(item);
|
||||
NG_FREE_M(m);
|
||||
ERROUT(0);
|
||||
}
|
||||
/*
|
||||
* Until we deliver this packet we can't receive next one as
|
||||
* we have no information for sending ack.
|
||||
*/
|
||||
seq->inproc = 1;
|
||||
mtx_unlock(&seq->mtx);
|
||||
|
||||
/* Prepend session ID to packet. */
|
||||
M_PREPEND(m, 2, M_NOWAIT);
|
||||
if (m == NULL) {
|
||||
seq->inproc = 0;
|
||||
SEQ_UNLOCK(seq);
|
||||
priv->stats.memoryFailures++;
|
||||
NG_FREE_ITEM(item);
|
||||
ERROUT(ENOBUFS);
|
||||
@ -944,10 +941,17 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
mtod(m, u_int8_t *)[0] = sid >> 8;
|
||||
mtod(m, u_int8_t *)[1] = sid & 0xff;
|
||||
|
||||
/*
|
||||
* Until we deliver this packet we can't receive next one as
|
||||
* we have no information for sending ack.
|
||||
*/
|
||||
seq->inproc = 1;
|
||||
SEQ_UNLOCK(seq);
|
||||
|
||||
/* Deliver packet to upper layers */
|
||||
NG_FWD_NEW_DATA(error, item, priv->ctrl, m);
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
SEQ_LOCK(seq);
|
||||
/* Ready to process next packet. */
|
||||
seq->inproc = 0;
|
||||
|
||||
@ -962,7 +966,7 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
NULL, 0);
|
||||
}
|
||||
}
|
||||
mtx_unlock(&seq->mtx);
|
||||
SEQ_UNLOCK(seq);
|
||||
|
||||
ERROUT(error);
|
||||
}
|
||||
@ -997,8 +1001,6 @@ ng_l2tp_rcvdata_lower(hook_p h, item_p item)
|
||||
/* Deliver data */
|
||||
NG_FWD_NEW_DATA(error, item, hook, m);
|
||||
done:
|
||||
/* Done */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
return (error);
|
||||
}
|
||||
|
||||
@ -1016,9 +1018,6 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item)
|
||||
int i;
|
||||
u_int16_t ns;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
|
||||
/* If not configured, reject */
|
||||
if (!priv->conf.enabled) {
|
||||
NG_FREE_ITEM(item);
|
||||
@ -1043,12 +1042,12 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item)
|
||||
ERROUT(EOVERFLOW);
|
||||
}
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
SEQ_LOCK(seq);
|
||||
|
||||
/* Find next empty slot in transmit queue */
|
||||
for (i = 0; i < L2TP_MAX_XWIN && seq->xwin[i] != NULL; i++);
|
||||
if (i == L2TP_MAX_XWIN) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
SEQ_UNLOCK(seq);
|
||||
priv->stats.xmitDrops++;
|
||||
m_freem(m);
|
||||
ERROUT(ENOBUFS);
|
||||
@ -1057,7 +1056,7 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item)
|
||||
|
||||
/* If peer's receive window is already full, nothing else to do */
|
||||
if (i >= seq->cwnd) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
SEQ_UNLOCK(seq);
|
||||
ERROUT(0);
|
||||
}
|
||||
|
||||
@ -1068,10 +1067,9 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item)
|
||||
|
||||
ns = seq->ns++;
|
||||
|
||||
mtx_unlock(&seq->mtx);
|
||||
|
||||
/* Copy packet */
|
||||
if ((m = L2TP_COPY_MBUF(m, M_NOWAIT)) == NULL) {
|
||||
SEQ_UNLOCK(seq);
|
||||
priv->stats.memoryFailures++;
|
||||
ERROUT(ENOBUFS);
|
||||
}
|
||||
@ -1079,8 +1077,6 @@ ng_l2tp_rcvdata_ctrl(hook_p hook, item_p item)
|
||||
/* Send packet and increment xmit sequence number */
|
||||
error = ng_l2tp_xmit_ctrl(priv, m, ns);
|
||||
done:
|
||||
/* Done */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
return (error);
|
||||
}
|
||||
|
||||
@ -1098,9 +1094,6 @@ ng_l2tp_rcvdata(hook_p hook, item_p item)
|
||||
int error;
|
||||
int i = 2;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
|
||||
/* If not configured, reject */
|
||||
if (!priv->conf.enabled) {
|
||||
NG_FREE_ITEM(item);
|
||||
@ -1161,8 +1154,6 @@ ng_l2tp_rcvdata(hook_p hook, item_p item)
|
||||
/* Send packet */
|
||||
NG_FWD_NEW_DATA(error, item, priv->lower, m);
|
||||
done:
|
||||
/* Done */
|
||||
L2TP_SEQ_CHECK(&priv->seq);
|
||||
return (error);
|
||||
}
|
||||
|
||||
@ -1204,7 +1195,6 @@ ng_l2tp_seq_init(priv_p priv)
|
||||
ng_callout_init(&seq->rack_timer);
|
||||
ng_callout_init(&seq->xack_timer);
|
||||
mtx_init(&seq->mtx, "ng_l2tp", NULL, MTX_DEF);
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
}
|
||||
|
||||
/*
|
||||
@ -1240,10 +1230,13 @@ ng_l2tp_seq_adjust(priv_p priv, const struct ng_l2tp_config *conf)
|
||||
{
|
||||
struct l2tp_seq *const seq = &priv->seq;
|
||||
u_int16_t new_wmax;
|
||||
int error = 0;
|
||||
|
||||
SEQ_LOCK(seq);
|
||||
/* If disabling node, reset state sequence number */
|
||||
if (!conf->enabled) {
|
||||
ng_l2tp_seq_reset(priv);
|
||||
SEQ_UNLOCK(seq);
|
||||
return (0);
|
||||
}
|
||||
|
||||
@ -1252,13 +1245,14 @@ ng_l2tp_seq_adjust(priv_p priv, const struct ng_l2tp_config *conf)
|
||||
if (new_wmax > L2TP_MAX_XWIN)
|
||||
new_wmax = L2TP_MAX_XWIN;
|
||||
if (new_wmax == 0)
|
||||
return (EINVAL);
|
||||
ERROUT(EINVAL);
|
||||
if (new_wmax < seq->wmax)
|
||||
return (EBUSY);
|
||||
ERROUT(EBUSY);
|
||||
seq->wmax = new_wmax;
|
||||
|
||||
/* Done */
|
||||
return (0);
|
||||
done:
|
||||
SEQ_UNLOCK(seq);
|
||||
return (error);
|
||||
}
|
||||
|
||||
/*
|
||||
@ -1271,8 +1265,7 @@ ng_l2tp_seq_reset(priv_p priv)
|
||||
hook_p hook;
|
||||
int i;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
SEQ_LOCK_ASSERT(seq);
|
||||
|
||||
/* Stop timers */
|
||||
ng_uncallout(&seq->rack_timer, priv->node);
|
||||
@ -1299,9 +1292,6 @@ ng_l2tp_seq_reset(priv_p priv)
|
||||
seq->acks = 0;
|
||||
seq->rexmits = 0;
|
||||
bzero(seq->xwin, sizeof(seq->xwin));
|
||||
|
||||
/* Done */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
}
|
||||
|
||||
/*
|
||||
@ -1316,15 +1306,12 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr)
|
||||
int i, j;
|
||||
uint16_t ns;
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
SEQ_LOCK_ASSERT(seq);
|
||||
|
||||
/* Verify peer's ACK is in range */
|
||||
if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
if ((nack = L2TP_SEQ_DIFF(nr, seq->rack)) <= 0)
|
||||
return; /* duplicate ack */
|
||||
}
|
||||
if (L2TP_SEQ_DIFF(nr, seq->ns) > 0) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
priv->stats.recvBadAcks++; /* ack for packet not sent */
|
||||
return;
|
||||
}
|
||||
@ -1375,10 +1362,8 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr)
|
||||
ng_uncallout(&seq->rack_timer, priv->node);
|
||||
|
||||
/* If transmit queue is empty, we're done for now */
|
||||
if (seq->xwin[0] == NULL) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
if (seq->xwin[0] == NULL)
|
||||
return;
|
||||
}
|
||||
|
||||
/* Start restransmit timer again */
|
||||
ng_callout(&seq->rack_timer, priv->node, NULL,
|
||||
@ -1396,8 +1381,6 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr)
|
||||
seq->ns++;
|
||||
}
|
||||
|
||||
mtx_unlock(&seq->mtx);
|
||||
|
||||
/*
|
||||
* Send prepared.
|
||||
* If there is a memory error, pretend packet was sent, as it
|
||||
@ -1407,8 +1390,10 @@ ng_l2tp_seq_recv_nr(priv_p priv, u_int16_t nr)
|
||||
struct mbuf *m;
|
||||
if ((m = L2TP_COPY_MBUF(xwin[i], M_NOWAIT)) == NULL)
|
||||
priv->stats.memoryFailures++;
|
||||
else
|
||||
else {
|
||||
ng_l2tp_xmit_ctrl(priv, m, ns);
|
||||
SEQ_LOCK(seq);
|
||||
}
|
||||
ns++;
|
||||
}
|
||||
}
|
||||
@ -1428,17 +1413,13 @@ ng_l2tp_seq_xack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
|
||||
(!callout_active(&seq->xack_timer)))
|
||||
return;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
SEQ_LOCK(seq);
|
||||
|
||||
/* Send a ZLB */
|
||||
ng_l2tp_xmit_ctrl(priv, NULL, seq->ns);
|
||||
|
||||
/* callout_deactivate() is not needed here
|
||||
as ng_uncallout() was called by ng_l2tp_xmit_ctrl() */
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
}
|
||||
|
||||
/*
|
||||
@ -1453,14 +1434,12 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
|
||||
struct mbuf *m;
|
||||
u_int delay;
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
SEQ_LOCK(seq);
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
/* Make sure callout is still active before doing anything */
|
||||
if (callout_pending(&seq->rack_timer) ||
|
||||
!callout_active(&seq->rack_timer)) {
|
||||
mtx_unlock(&seq->mtx);
|
||||
SEQ_UNLOCK(seq);
|
||||
return;
|
||||
}
|
||||
|
||||
@ -1485,41 +1464,39 @@ ng_l2tp_seq_rack_timeout(node_p node, hook_p hook, void *arg1, int arg2)
|
||||
|
||||
/* Retransmit oldest unack'd packet */
|
||||
m = L2TP_COPY_MBUF(seq->xwin[0], M_NOWAIT);
|
||||
mtx_unlock(&seq->mtx);
|
||||
if (m == NULL)
|
||||
if (m == NULL) {
|
||||
SEQ_UNLOCK(seq);
|
||||
priv->stats.memoryFailures++;
|
||||
else
|
||||
} else
|
||||
ng_l2tp_xmit_ctrl(priv, m, seq->ns++);
|
||||
|
||||
/* callout_deactivate() is not needed here
|
||||
as ng_callout() is getting called each time */
|
||||
|
||||
/* Sanity check */
|
||||
L2TP_SEQ_CHECK(seq);
|
||||
}
|
||||
|
||||
/*
|
||||
* Transmit a control stream packet, payload optional.
|
||||
* The transmit sequence number is not incremented.
|
||||
* Requires seq lock, returns unlocked.
|
||||
*/
|
||||
static int
|
||||
ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns)
|
||||
{
|
||||
struct l2tp_seq *const seq = &priv->seq;
|
||||
uint8_t *p;
|
||||
u_int16_t session_id = 0;
|
||||
uint16_t nr, session_id = 0;
|
||||
int error;
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
SEQ_LOCK_ASSERT(seq);
|
||||
|
||||
/* Stop ack timer: we're sending an ack with this packet.
|
||||
Doing this before to keep state predictable after error. */
|
||||
if (callout_active(&seq->xack_timer))
|
||||
ng_uncallout(&seq->xack_timer, priv->node);
|
||||
|
||||
seq->xack = seq->nr;
|
||||
nr = seq->xack = seq->nr;
|
||||
|
||||
mtx_unlock(&seq->mtx);
|
||||
SEQ_UNLOCK(seq);
|
||||
|
||||
/* If no mbuf passed, send an empty packet (ZLB) */
|
||||
if (m == NULL) {
|
||||
@ -1570,8 +1547,8 @@ ng_l2tp_xmit_ctrl(priv_p priv, struct mbuf *m, u_int16_t ns)
|
||||
p[7] = session_id & 0xff;
|
||||
p[8] = ns >> 8;
|
||||
p[9] = ns & 0xff;
|
||||
p[10] = seq->nr >> 8;
|
||||
p[11] = seq->nr & 0xff;
|
||||
p[10] = nr >> 8;
|
||||
p[11] = nr & 0xff;
|
||||
|
||||
/* Update sequence number info and stats */
|
||||
priv->stats.xmitPackets++;
|
||||
@ -1594,7 +1571,7 @@ ng_l2tp_seq_check(struct l2tp_seq *seq)
|
||||
|
||||
#define CHECK(p) KASSERT((p), ("%s: not: %s", __func__, #p))
|
||||
|
||||
mtx_lock(&seq->mtx);
|
||||
SEQ_LOCK_ASSERT(seq);
|
||||
|
||||
self_unack = L2TP_SEQ_DIFF(seq->nr, seq->xack);
|
||||
peer_unack = L2TP_SEQ_DIFF(seq->ns, seq->rack);
|
||||
@ -1617,8 +1594,6 @@ ng_l2tp_seq_check(struct l2tp_seq *seq)
|
||||
for ( ; i < seq->cwnd; i++) /* verify peer's recv window full */
|
||||
CHECK(seq->xwin[i] == NULL);
|
||||
|
||||
mtx_unlock(&seq->mtx);
|
||||
|
||||
#undef CHECK
|
||||
}
|
||||
#endif /* INVARIANTS */
|
||||
|
||||
Loading…
Reference in New Issue
Block a user