| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1335551333-6103-1-git-send-email-bpoirier@suse.de>
Date: Fri, 27 Apr 2012 14:28:52 -0400
From: Benjamin Poirier <bpoirier@...e.de>
To: netdev@...r.kernel.org
Cc: Pablo Neira Ayuso <pablo@...filter.org>,
Patrick McHardy <kaber@...sh.net>,
"David S. Miller" <davem@...emloft.net>,
Andrew Morton <akpm@...ux-foundation.org>,
Eric Dumazet <eric.dumazet@...il.com>,
Mike Frysinger <vapier@...too.org>,
Arun Sharma <asharma@...com>, netfilter-devel@...r.kernel.org,
netfilter@...r.kernel.org, coreteam@...filter.org,
linux-kernel@...r.kernel.org,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: [PATCH RFC 1/2] netfilter: conntrack: remove RCU usage in conntrack notifier
I think that the rcu usage in this code is pointless. It should either be
removed or, if it was intended to protect against something, it ought to make
that clear.
1) The code does not make use of the deferred deletion/wait for completion rcu
api (ie. synchronize_rcu(), call_rcu()).
2) It does not benefit from the barriers implied by the rcu primitives used.
The code deals with callback pointers. There's no need to order writes to the
function code (!) before writes to the function pointers here.
---
include/net/netfilter/nf_conntrack_ecache.h | 20 +++------
include/net/netns/conntrack.h | 4 +-
net/netfilter/nf_conntrack_ecache.c | 58 +++++++-------------------
3 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_ecache.h b/include/net/netfilter/nf_conntrack_ecache.h
index a88fb69..ac5b8d2 100644
--- a/include/net/netfilter/nf_conntrack_ecache.h
+++ b/include/net/netfilter/nf_conntrack_ecache.h
@@ -99,14 +99,13 @@ nf_conntrack_eventmask_report(unsigned int eventmask,
struct nf_ct_event_notifier *notify;
struct nf_conntrack_ecache *e;
- rcu_read_lock();
- notify = rcu_dereference(net->ct.nf_conntrack_event_cb);
+ notify = net->ct.nf_conntrack_event_cb;
if (notify == NULL)
- goto out_unlock;
+ return ret;
e = nf_ct_ecache_find(ct);
if (e == NULL)
- goto out_unlock;
+ return ret;
if (nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct)) {
struct nf_ct_event item = {
@@ -118,7 +117,7 @@ nf_conntrack_eventmask_report(unsigned int eventmask,
unsigned long missed = e->pid ? 0 : e->missed;
if (!((eventmask | missed) & e->ctmask))
- goto out_unlock;
+ return ret;
ret = notify->fcn(eventmask | missed, &item);
if (unlikely(ret < 0 || missed)) {
@@ -137,8 +136,6 @@ nf_conntrack_eventmask_report(unsigned int eventmask,
spin_unlock_bh(&ct->lock);
}
}
-out_unlock:
- rcu_read_unlock();
return ret;
}
@@ -178,14 +175,13 @@ nf_ct_expect_event_report(enum ip_conntrack_expect_events event,
struct nf_exp_event_notifier *notify;
struct nf_conntrack_ecache *e;
- rcu_read_lock();
- notify = rcu_dereference(net->ct.nf_expect_event_cb);
+ notify = net->ct.nf_expect_event_cb;
if (notify == NULL)
- goto out_unlock;
+ return;
e = nf_ct_ecache_find(exp->master);
if (e == NULL)
- goto out_unlock;
+ return;
if (e->expmask & (1 << event)) {
struct nf_exp_event item = {
@@ -195,8 +191,6 @@ nf_ct_expect_event_report(enum ip_conntrack_expect_events event,
};
notify->fcn(1 << event, &item);
}
-out_unlock:
- rcu_read_unlock();
}
static inline void
diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index 7a911ec..c96fd8c 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h
@@ -18,8 +18,8 @@ struct netns_ct {
struct hlist_nulls_head unconfirmed;
struct hlist_nulls_head dying;
struct ip_conntrack_stat __percpu *stat;
- struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;
- struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
+ struct nf_ct_event_notifier *nf_conntrack_event_cb;
+ struct nf_exp_event_notifier *nf_expect_event_cb;
int sysctl_events;
unsigned int sysctl_events_retry_timeout;
int sysctl_acct;
diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index b924f3a..0134009 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -38,19 +38,18 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
struct nf_ct_event item;
int ret;
- rcu_read_lock();
- notify = rcu_dereference(net->ct.nf_conntrack_event_cb);
+ notify = net->ct.nf_conntrack_event_cb;
if (notify == NULL)
- goto out_unlock;
+ return;
e = nf_ct_ecache_find(ct);
if (e == NULL)
- goto out_unlock;
+ return;
events = xchg(&e->cache, 0);
if (!nf_ct_is_confirmed(ct) || nf_ct_is_dying(ct) || !events)
- goto out_unlock;
+ return;
/* We make a copy of the missed event cache without taking
* the lock, thus we may send missed events twice. However,
@@ -58,7 +57,7 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
missed = e->missed;
if (!((events | missed) & e->ctmask))
- goto out_unlock;
+ return;
item.ct = ct;
item.pid = 0;
@@ -67,7 +66,7 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
ret = notify->fcn(events | missed, &item);
if (likely(ret >= 0 && !missed))
- goto out_unlock;
+ return;
spin_lock_bh(&ct->lock);
if (ret < 0)
@@ -75,9 +74,6 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
else
e->missed &= ~missed;
spin_unlock_bh(&ct->lock);
-
-out_unlock:
- rcu_read_unlock();
}
EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
@@ -85,21 +81,14 @@ int nf_conntrack_register_notifier(struct net *net,
struct nf_ct_event_notifier *new)
{
int ret = 0;
- struct nf_ct_event_notifier *notify;
mutex_lock(&nf_ct_ecache_mutex);
- notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
- lockdep_is_held(&nf_ct_ecache_mutex));
- if (notify != NULL) {
+ if (net->ct.nf_conntrack_event_cb != NULL)
ret = -EBUSY;
- goto out_unlock;
- }
- rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
+ else
+ net->ct.nf_conntrack_event_cb = new;
mutex_unlock(&nf_ct_ecache_mutex);
- return ret;
-out_unlock:
- mutex_unlock(&nf_ct_ecache_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
@@ -107,13 +96,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier);
void nf_conntrack_unregister_notifier(struct net *net,
struct nf_ct_event_notifier *new)
{
- struct nf_ct_event_notifier *notify;
-
mutex_lock(&nf_ct_ecache_mutex);
- notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
- lockdep_is_held(&nf_ct_ecache_mutex));
- BUG_ON(notify != new);
- RCU_INIT_POINTER(net->ct.nf_conntrack_event_cb, NULL);
+ BUG_ON(net->ct.nf_conntrack_event_cb != new);
+ net->ct.nf_conntrack_event_cb = NULL;
mutex_unlock(&nf_ct_ecache_mutex);
}
EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
@@ -122,21 +107,14 @@ int nf_ct_expect_register_notifier(struct net *net,
struct nf_exp_event_notifier *new)
{
int ret = 0;
- struct nf_exp_event_notifier *notify;
mutex_lock(&nf_ct_ecache_mutex);
- notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
- lockdep_is_held(&nf_ct_ecache_mutex));
- if (notify != NULL) {
+ if (net->ct.nf_expect_event_cb != NULL)
ret = -EBUSY;
- goto out_unlock;
- }
- rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
+ else
+ net->ct.nf_expect_event_cb = new;
mutex_unlock(&nf_ct_ecache_mutex);
- return ret;
-out_unlock:
- mutex_unlock(&nf_ct_ecache_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
@@ -144,13 +122,9 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier);
void nf_ct_expect_unregister_notifier(struct net *net,
struct nf_exp_event_notifier *new)
{
- struct nf_exp_event_notifier *notify;
-
mutex_lock(&nf_ct_ecache_mutex);
- notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
- lockdep_is_held(&nf_ct_ecache_mutex));
- BUG_ON(notify != new);
- RCU_INIT_POINTER(net->ct.nf_expect_event_cb, NULL);
+ BUG_ON(net->ct.nf_expect_event_cb != new);
+ net->ct.nf_expect_event_cb = NULL;
mutex_unlock(&nf_ct_ecache_mutex);
}
EXPORT_SYMBOL_GPL(nf_ct_expect_unregister_notifier);
--
1.7.7
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists