netdev - Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1387342211.19078.295.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Tue, 17 Dec 2013 20:50:11 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Fan Du <fan.du@...driver.com>
Cc:	steffen.klassert@...unet.com, davem@...emloft.net,
	netdev@...r.kernel.org
Subject: Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

On Wed, 2013-12-18 at 11:34 +0800, Fan Du wrote:
> xfrm_policy_sk_bundles, protected by net->xfrm.xfrm_policy_sk_bundle_lock
> should be put into netns xfrm structure, otherwise xfrm_policy_sk_bundles
> can be corrupted from different net namespace.
> 
> Moreover current xfrm_policy_sk_bundle_lock used in below two scenarios:
> 
> 1. xfrm_lookup(Process context)  vs  __xfrm_garbage_collect(softirq context)
> 2. xfrm_lookup(Process context)  vs  __xfrm_garbage_collect(Process context
>                                                 when SPD change or dev down) 
> 
> we can use xchg to avoid the spinlock, at the same time cover above scenarios,
> inspired by discussion in: http://marc.info/?l=linux-netdev&m=138713363113003&w=2
> 
> Signed-off-by: Fan Du <fan.du@...driver.com>
> ---
> v2:
>   Fix incorrect commit log.
> 
> ---
>  include/net/netns/xfrm.h |    2 +-
>  net/xfrm/xfrm_policy.c   |   17 +++--------------
>  2 files changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> index 1006a26..4a30b1b 100644
> --- a/include/net/netns/xfrm.h
> +++ b/include/net/netns/xfrm.h
> @@ -58,9 +58,9 @@ struct netns_xfrm {
>  	struct dst_ops		xfrm6_dst_ops;
>  #endif
>  	spinlock_t xfrm_state_lock;
> -	spinlock_t xfrm_policy_sk_bundle_lock;
>  	rwlock_t xfrm_policy_lock;
>  	struct mutex xfrm_cfg_mutex;
> +	struct dst_entry *xfrm_policy_sk_bundles;
>  };
>  
>  #endif
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index a7487f3..26d79c0 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -39,8 +39,6 @@
>  #define XFRM_QUEUE_TMO_MAX ((unsigned)(60*HZ))
>  #define XFRM_MAX_QUEUE_LEN	100
>  
> -static struct dst_entry *xfrm_policy_sk_bundles;
> -
>  static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
>  static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO]
>  						__read_mostly;
> @@ -2108,12 +2106,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
>  			}
>  
>  			dst_hold(&xdst->u.dst);
> -
> -			spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
> -			xdst->u.dst.next = xfrm_policy_sk_bundles;
> -			xfrm_policy_sk_bundles = &xdst->u.dst;
> -			spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
> -
> +			xdst->u.dst.next = xchg(&net->xfrm.xfrm_policy_sk_bundles,
> +						&xdst->u.dst);

This is not safe.

Take a look at include/linux/llist.h if you really want to avoid the
spinlock.



--
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