Re: [nf-next PATCH V2 4/5] netfilter: conntrack: seperate expect locking from nf_conntrack_lock
From: Florian Westphal <fw@strlen.de>
Date: 2014-02-28 15:08:58
Also in:
netfilter-devel
Jesper Dangaard Brouer [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Netfilter expectations are protected with the same lock as conntrack entries (nf_conntrack_lock). This patch split out expectations locking to use it's own lock (nf_conntrack_expect_lock).--- a/net/netfilter/nf_conntrack_helper.c +++ b/net/netfilter/nf_conntrack_helper.c@@ -258,7 +258,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i, if (help && rcu_dereference_protected( help->helper, - lockdep_is_held(&nf_conntrack_lock) + lockdep_is_held(&nf_conntrack_expect_lock) ) == me) { nf_conntrack_event(IPCT_HELPER, ct); RCU_INIT_POINTER(help->helper, NULL);
Not sure if the lockdep_is_held is correct.
quoted hunk ↗ jump to hunk
@@ -399,13 +399,14 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, int cpu; /* Get rid of expectations */ + spin_lock_bh(&nf_conntrack_expect_lock); for (i = 0; i < nf_ct_expect_hsize; i++) { hlist_for_each_entry_safe(exp, next, &net->ct.expect_hash[i], hnode) { struct nf_conn_help *help = nfct_help(exp->master); if ((rcu_dereference_protected( help->helper, - lockdep_is_held(&nf_conntrack_lock) + lockdep_is_held(&nf_conntrack_expect_lock) ) == me || exp->helper == me) && del_timer(&exp->timeout)) { nf_ct_unlink_expect(exp);@@ -413,6 +414,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, } } } + spin_unlock_bh(&nf_conntrack_expect_lock);
expect_lock is released here but
/* Get rid of expecteds, set helpers to NULL. */
for_each_possible_cpu(cpu) {will invoke unhelp() AFAIU unhelp() is safe in all cases even without nf_conntrack_expect_lock being held: * in first loop we hold nf_conntrack_expect_lock * in 2nd loop we are holding the list lock, i.e. if the ct is in the list it cannot disappear underneath * in last loop you'll hold the hashed lock for the particular hash list, so can't go away either. So I think the lockdep annotation in uhelp is incorrect and not the patch itself.