Re: [PATCH net-next] xfrm: fix RCU bugs
From: Fan Du <hidden>
Date: 2012-08-20 04:39:08
Hi Eric Please correct me if I'm wrong about below comments. On 2012年08月19日 18:31, Eric Dumazet wrote:
From: Eric Dumazet<edumazet@google.com> This patch reverts commit 56892261ed1a (xfrm: Use rcu_dereference_bh to deference pointer protected by rcu_read_lock_bh), and fixes bugs introduced in commit 418a99ac6ad ( Replace rwlock on xfrm_policy_afinfo with rcu ) 1) We properly use RCU variant in this file, not a mix of RCU/RCU_BH 2) We must defer some writes after the synchronize_rcu() call or a reader can crash dereferencing NULL pointer.
Not exactly. net/ipv4/xfrm4_policy.c static void __exit xfrm4_policy_fini(void) -> xfrm_policy_unregister_afinfo IMHO, ip stack can never be compiled as module, so is xfrm4_policy_fini freed up after system bootup? which means xfrm4_policy_fini can never be called. so an dereferencing NULL pointer by a reader could not happen.
3) Now we use the xfrm_policy_afinfo_lock spinlock only from process context, we no longer need to block BH in xfrm_policy_register_afinfo() and xfrm_policy_unregister_afinfo()
I don't think it's related to what kinds of locks we are using. we call xfrm_policy_register_afinfo in process context, but actually what xfrm_policy_afinfo_lock protected can be used in soft irq context. that's why xx_bh is used in: e959d812 " [XFRM]: fix incorrect xfrm_policy_afinfo_lock use" Is such scenario still valid?
quoted hunk ↗ jump to hunk
4) Can use RCU_INIT_POINTER() instead of rcu_assign_pointer() in xfrm_policy_unregister_afinfo() 5) Remove a forward inline declaration (xfrm_policy_put_afinfo()), and also move xfrm_policy_get_afinfo() declaration. Signed-off-by: Eric Dumazet<edumazet@google.com> Cc: Fan Du<redacted> Cc: Priyanka Jain<redacted> --- net/xfrm/xfrm_policy.c | 76 ++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 37 deletions(-)diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6405764..e52f50f 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c@@ -48,8 +48,6 @@ static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO] static struct kmem_cache *xfrm_dst_cache __read_mostly; -static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family); -static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo); static void xfrm_init_pmtu(struct dst_entry *dst); static int stale_bundle(struct dst_entry *dst); static int xfrm_bundle_ok(struct xfrm_dst *xdst);@@ -96,6 +94,24 @@ bool xfrm_selector_match(const struct xfrm_selector *sel, const struct flowi *fl return false; } +static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family) +{ + struct xfrm_policy_afinfo *afinfo; + + if (unlikely(family>= NPROTO)) + return NULL; + rcu_read_lock(); + afinfo = rcu_dereference(xfrm_policy_afinfo[family]); + if (unlikely(!afinfo)) + rcu_read_unlock(); + return afinfo; +} + +static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo) +{ + rcu_read_unlock(); +} + static inline struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, const xfrm_address_t *saddr, const xfrm_address_t *daddr,@@ -2419,7 +2435,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) return -EINVAL; if (unlikely(afinfo->family>= NPROTO)) return -EAFNOSUPPORT; - spin_lock_bh(&xfrm_policy_afinfo_lock); + spin_lock(&xfrm_policy_afinfo_lock); if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL)) err = -ENOBUFS; else {@@ -2442,7 +2458,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) afinfo->garbage_collect = xfrm_garbage_collect_deferred; rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family], afinfo); } - spin_unlock_bh(&xfrm_policy_afinfo_lock); + spin_unlock(&xfrm_policy_afinfo_lock); rtnl_lock(); for_each_net(net) {@@ -2475,23 +2491,26 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo) return -EINVAL; if (unlikely(afinfo->family>= NPROTO)) return -EAFNOSUPPORT; - spin_lock_bh(&xfrm_policy_afinfo_lock); + spin_lock(&xfrm_policy_afinfo_lock); if (likely(xfrm_policy_afinfo[afinfo->family] != NULL)) { if (unlikely(xfrm_policy_afinfo[afinfo->family] != afinfo)) err = -EINVAL; - else { - struct dst_ops *dst_ops = afinfo->dst_ops; - rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family], - NULL); - dst_ops->kmem_cachep = NULL; - dst_ops->check = NULL; - dst_ops->negative_advice = NULL; - dst_ops->link_failure = NULL; - afinfo->garbage_collect = NULL; - } + else + RCU_INIT_POINTER(xfrm_policy_afinfo[afinfo->family], + NULL); + } + spin_unlock(&xfrm_policy_afinfo_lock); + if (!err) { + struct dst_ops *dst_ops = afinfo->dst_ops; + + synchronize_rcu(); + + dst_ops->kmem_cachep = NULL; + dst_ops->check = NULL; + dst_ops->negative_advice = NULL; + dst_ops->link_failure = NULL; + afinfo->garbage_collect = NULL; } - spin_unlock_bh(&xfrm_policy_afinfo_lock); - synchronize_rcu(); return err; } EXPORT_SYMBOL(xfrm_policy_unregister_afinfo);@@ -2500,32 +2519,15 @@ static void __net_init xfrm_dst_ops_init(struct net *net) { struct xfrm_policy_afinfo *afinfo; - rcu_read_lock_bh(); - afinfo = rcu_dereference_bh(xfrm_policy_afinfo[AF_INET]); + rcu_read_lock(); + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); if (afinfo) net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; #if IS_ENABLED(CONFIG_IPV6) - afinfo = rcu_dereference_bh(xfrm_policy_afinfo[AF_INET6]); + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); if (afinfo) net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; #endif - rcu_read_unlock_bh(); -} - -static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family) -{ - struct xfrm_policy_afinfo *afinfo; - if (unlikely(family>= NPROTO)) - return NULL; - rcu_read_lock(); - afinfo = rcu_dereference(xfrm_policy_afinfo[family]); - if (unlikely(!afinfo)) - rcu_read_unlock(); - return afinfo; -} - -static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo) -{ rcu_read_unlock(); }
-- Love each day! --fan