| 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
| ||
|
Date: Mon, 20 Aug 2012 12:40:17 +0800
From: Fan Du <fan.du@...driver.com>
To: Eric Dumazet <eric.dumazet@...il.com>
CC: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>, <fengguang.wu@...el.com>,
Priyanka Jain <Priyanka.Jain@...escale.com>
Subject: Re: [PATCH net-next] xfrm: fix RCU bugs
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@...gle.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?
> 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@...gle.com>
> Cc: Fan Du<fan.du@...driver.com>
> Cc: Priyanka Jain<Priyanka.Jain@...escale.com>
> ---
> 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
--
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