Module Name: src
Committed By: rmind
Date: Sun Feb 1 22:41:22 UTC 2015
Modified Files:
src/sys/net/npf: npf_conn.c
Log Message:
- npf_conn_establish: remove a rare race condition when we might destroy a
connection when it is still referenced by another thread.
- npf_conn_destroy: remove the backwards entry using the saved key, PR/49488.
- Sprinkle some asserts.
To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sys/net/npf/npf_conn.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/npf/npf_conn.c
diff -u src/sys/net/npf/npf_conn.c:1.14 src/sys/net/npf/npf_conn.c:1.15
--- src/sys/net/npf/npf_conn.c:1.14 Sat Dec 20 16:19:43 2014
+++ src/sys/net/npf/npf_conn.c Sun Feb 1 22:41:22 2015
@@ -1,7 +1,7 @@
-/* $NetBSD: npf_conn.c,v 1.14 2014/12/20 16:19:43 rmind Exp $ */
+/* $NetBSD: npf_conn.c,v 1.15 2015/02/01 22:41:22 rmind Exp $ */
/*-
- * Copyright (c) 2014 Mindaugas Rasiukevicius <rmind at netbsd org>
+ * Copyright (c) 2014-2015 Mindaugas Rasiukevicius <rmind at netbsd org>
* Copyright (c) 2010-2014 The NetBSD Foundation, Inc.
* All rights reserved.
*
@@ -99,7 +99,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_conn.c,v 1.14 2014/12/20 16:19:43 rmind Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_conn.c,v 1.15 2015/02/01 22:41:22 rmind Exp $");
#include <sys/param.h>
#include <sys/types.h>
@@ -369,7 +369,6 @@ npf_conn_lookup(const npf_cache_t *npc,
/* Check if connection is active and not expired. */
flags = con->c_flags;
ok = (flags & (CONN_ACTIVE | CONN_EXPIRE)) == CONN_ACTIVE;
-
if (__predict_false(!ok)) {
atomic_dec_uint(&con->c_refcnt);
return NULL;
@@ -453,6 +452,7 @@ npf_conn_establish(npf_cache_t *npc, int
{
const nbuf_t *nbuf = npc->npc_nbuf;
npf_conn_t *con;
+ int error = 0;
KASSERT(!nbuf_flag_p(nbuf, NBUF_DATAREF_RESET));
@@ -468,16 +468,16 @@ npf_conn_establish(npf_cache_t *npc, int
NPF_PRINTF(("NPF: create conn %p\n", con));
npf_stats_inc(NPF_STAT_CONN_CREATE);
- /* Reference count and flags (indicate direction). */
mutex_init(&con->c_lock, MUTEX_DEFAULT, IPL_SOFTNET);
con->c_flags = (di & PFIL_ALL);
- con->c_refcnt = 1;
+ con->c_refcnt = 0;
con->c_rproc = NULL;
con->c_nat = NULL;
- /* Initialize protocol state. */
+ /* Initialize the protocol state. */
if (!npf_state_init(npc, &con->c_state)) {
- goto err;
+ npf_conn_destroy(con);
+ return NULL;
}
KASSERT(npf_iscached(npc, NPC_IP46));
@@ -488,45 +488,65 @@ npf_conn_establish(npf_cache_t *npc, int
* Construct "forwards" and "backwards" keys. Also, set the
* interface ID for this connection (unless it is global).
*/
- if (!npf_conn_conkey(npc, fw, true)) {
- goto err;
- }
- if (!npf_conn_conkey(npc, bk, false)) {
- goto err;
+ if (!npf_conn_conkey(npc, fw, true) ||
+ !npf_conn_conkey(npc, bk, false)) {
+ npf_conn_destroy(con);
+ return NULL;
}
fw->ck_backptr = bk->ck_backptr = con;
con->c_ifid = per_if ? nbuf->nb_ifid : 0;
con->c_proto = npc->npc_proto;
- /* Set last activity time for a new connection. */
+ /*
+ * Set last activity time for a new connection and acquire
+ * a reference for the caller before we make it visible.
+ */
getnanouptime(&con->c_atime);
+ con->c_refcnt = 1;
/*
* Insert both keys (entries representing directions) of the
- * connection. At this point, it becomes visible.
+ * connection. At this point it becomes visible, but we activate
+ * the connection later.
*/
+ mutex_enter(&con->c_lock);
if (!npf_conndb_insert(conn_db, fw, con)) {
+ error = EISCONN;
goto err;
}
if (!npf_conndb_insert(conn_db, bk, con)) {
- /* We have hit the duplicate. */
- npf_conndb_remove(conn_db, fw);
- npf_stats_inc(NPF_STAT_RACE_CONN);
+ npf_conn_t *ret __diagused;
+ ret = npf_conndb_remove(conn_db, fw);
+ KASSERT(ret == con);
+ error = EISCONN;
goto err;
}
+err:
+ /*
+ * If we have hit the duplicate: mark the connection as expired
+ * and let the G/C thread to take care of it. We cannot do it
+ * here since there might be references acquired already.
+ */
+ if (error) {
+ const u_int dflags = CONN_REMOVED | CONN_EXPIRE;
+ atomic_or_uint(&con->c_flags, dflags);
+ npf_stats_inc(NPF_STAT_RACE_CONN);
+ } else {
+ NPF_PRINTF(("NPF: establish conn %p\n", con));
+ }
/* Finally, insert into the connection list. */
- NPF_PRINTF(("NPF: establish conn %p\n", con));
npf_conndb_enqueue(conn_db, con);
- return con;
-err:
- npf_conn_destroy(con);
- return NULL;
+ mutex_exit(&con->c_lock);
+
+ return error ? NULL : con;
}
static void
npf_conn_destroy(npf_conn_t *con)
{
+ KASSERT(con->c_refcnt == 0);
+
if (con->c_nat) {
/* Release any NAT structures. */
npf_nat_destroy(con->c_nat);
@@ -582,6 +602,8 @@ npf_conn_setnat(const npf_cache_t *npc,
mutex_exit(&con->c_lock);
return EINVAL;
}
+ KASSERT((con->c_flags & CONN_REMOVED) == 0);
+
if (__predict_false(con->c_nat != NULL)) {
/* Race with a duplicate packet. */
mutex_exit(&con->c_lock);
@@ -590,7 +612,7 @@ npf_conn_setnat(const npf_cache_t *npc,
}
/* Remove the "backwards" entry. */
- ret = npf_conndb_remove(conn_db, &key);
+ ret = npf_conndb_remove(conn_db, &con->c_back_entry);
KASSERT(ret == con);
/* Set the source/destination IDs to the translation values. */
@@ -606,7 +628,9 @@ npf_conn_setnat(const npf_cache_t *npc,
* Race: we have hit the duplicate, remove the "forwards"
* entry and expire our connection; it is no longer valid.
*/
- (void)npf_conndb_remove(conn_db, &con->c_forw_entry);
+ ret = npf_conndb_remove(conn_db, &con->c_forw_entry);
+ KASSERT(ret == con);
+
atomic_or_uint(&con->c_flags, CONN_REMOVED | CONN_EXPIRE);
mutex_exit(&con->c_lock);
