[PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

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@windriver.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);
 			route = xdst->route;
 		}
 	}
@@ -2551,11 +2545,7 @@ static void __xfrm_garbage_collect(struct net *net)
 {
 	struct dst_entry *head, *next;
 
-	spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-	head = xfrm_policy_sk_bundles;
-	xfrm_policy_sk_bundles = NULL;
-	spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-
+	head = xchg(&net->xfrm.xfrm_policy_sk_bundles, NULL);
 	while (head) {
 		next = head->next;
 		dst_free(head);
@@ -2942,7 +2932,6 @@ static int __net_init xfrm_net_init(struct net *net)
 	/* Initialize the per-net locks here */
 	spin_lock_init(&net->xfrm.xfrm_state_lock);
 	rwlock_init(&net->xfrm.xfrm_policy_lock);
-	spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
 	mutex_init(&net->xfrm.xfrm_cfg_mutex);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Eric Dumazet]

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@windriver.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.

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月18日 12:50, Eric Dumazet wrote:
> 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@windriver.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.

Hi Eric

Thanks for your attention,
I'm not follow why xchg here is unsafe, could you please elaborate a bit more?


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Cong Wang]

On Wed, 18 Dec 2013 at 04:50 GMT, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-12-18 at 11:34 +0800, Fan Du wrote:
>> -
>> -			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.
>

Exactly, xfrm_policy_sk_bundles list is only traversed after deletion.

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Eric Dumazet]

On Wed, 2013-12-18 at 13:33 +0800, Fan Du wrote:

> Hi Eric
> 
> Thanks for your attention,
> I'm not follow why xchg here is unsafe, could you please elaborate a bit more?

Read include/linux/llist.h and llist_add_batch() implementation,
it will be very clear why your implementation is not safe.

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

Hi, Eric and Cong

On 2013年12月18日 12:50, Eric Dumazet wrote:
>>   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.

Here is my understanding about llist_add_batch, assume when add one single
entry into list, i.e., new_last equates new_first.

   1         do {
   2                 new->next = first = ACCESS_ONCE(head->first);
   3         } while (cmpxchg(&head->first, first, new) != first);


Caller 1:Add new1 into head                          Caller 2:Add new2 into head
      line 2: Link head into new1
                                                      line 2: Link head into new2
                                                      line 3: Make the cmpxchg, then succeed.

                                                      After this, new2 -> old_head
      line 3: Make the cmpxchg, failed, try
              again will succeed, after this
              new1 -> new2 -> old_head

So in order to not use locks, try again if cmpxchg found out someone else
has update the head. For the case involved in the patch, the problem is, after
xchg, assign the old head to the new->next will race with the delete part when
saving the head for deleting after setting the head to NULL, as the traverse
of saved head probably not see a consistent list, that's a broken one.

I think an analogy of llist_add_batch for the updating part will be ok for this:

                         struct dst_entry *first;
                         do {
                                 xdst->u.dst.next = first = ACCESS_ONCE(xfrm_policy_sk_bundles);
                         } while (cmpxchg(&xfrm_policy_sk_bundles, first, &xdst->u.dst) != first);


And the deleting part:

                         head = xchg(&net->xfrm.xfrm_policy_sk_bundles, NULL);


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Eric Dumazet]

On Thu, 2013-12-19 at 09:35 +0800, Fan Du wrote:
> Hi, Eric and Cong
> 

> Here is my understanding about llist_add_batch, assume when add one single
> entry into list, i.e., new_last equates new_first.
> 
>    1         do {
>    2                 new->next = first = ACCESS_ONCE(head->first);
>    3         } while (cmpxchg(&head->first, first, new) != first);
> 
> 
> Caller 1:Add new1 into head                          Caller 2:Add new2 into head
>       line 2: Link head into new1
>                                                       line 2: Link head into new2
>                                                       line 3: Make the cmpxchg, then succeed.
> 
>                                                       After this, new2 -> old_head
>       line 3: Make the cmpxchg, failed, try
>               again will succeed, after this
>               new1 -> new2 -> old_head
> 
> So in order to not use locks, try again if cmpxchg found out someone else
> has update the head. For the case involved in the patch, the problem is, after
> xchg, assign the old head to the new->next will race with the delete part when
> saving the head for deleting after setting the head to NULL, as the traverse
> of saved head probably not see a consistent list, that's a broken one.
> 
> I think an analogy of llist_add_batch for the updating part will be ok for this:
> 
>                          struct dst_entry *first;
>                          do {
>                                  xdst->u.dst.next = first = ACCESS_ONCE(xfrm_policy_sk_bundles);
>                          } while (cmpxchg(&xfrm_policy_sk_bundles, first, &xdst->u.dst) != first);
> 
> 
> And the deleting part:
> 
>                          head = xchg(&net->xfrm.xfrm_policy_sk_bundles, NULL);
> 
> 

The point is to _use_ the helpers, so that code review is easy.

llist.h is safe, it was extensively discussed and adopted.

If you want to get rid of the spinlock, then use llist_del_all() for the
deleting, and llist_add() for the addition.

Really, its that simple.

[PATCHv3 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月19日 10:15, Eric Dumazet wrote:
>> I think an analogy of llist_add_batch for the updating part will be ok for this:
>> >
>> >                            struct dst_entry *first;
>> >                            do {
>> >                                    xdst->u.dst.next = first = ACCESS_ONCE(xfrm_policy_sk_bundles);
>> >                            } while (cmpxchg(&xfrm_policy_sk_bundles, first,&xdst->u.dst) != first);
>> >
>> >
>> >  And the deleting part:
>> >
>> >                            head = xchg(&net->xfrm.xfrm_policy_sk_bundles, NULL);
>> >
>> >
> The point is to_use_  the helpers, so that code review is easy.
>
> llist.h is safe, it was extensively discussed and adopted.
>
> If you want to get rid of the spinlock, then use llist_del_all() for the
> deleting, and llist_add() for the addition.
>
> Really, its that simple.

Ok, I will use common api suggested by you, then locks is gone at the cost
of xfrm_dst growing up a bit, sizeof(struct llist_node).



 From b4c5fc86861abd98866111a7b1b51dc13d546a0c Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Thu, 19 Dec 2013 11:03:20 +0800
Subject: [PATCHv3 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

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 lock less list to cater to those two scenarios as suggested by
Eric Dumazet.

Signed-off-by: Fan Du <fan.du@windriver.com>
Assisted-by: Eric Dumazet <edumazet@google.com>
---
v2:
   Fix incorrect commit log.

v3:
   Drop xchg, use llist instead, adviced by Eric Dumazet.
   Build test only.
---
  include/net/netns/xfrm.h |    2 +-
  include/net/xfrm.h       |    1 +
  net/xfrm/xfrm_policy.c   |   26 ++++++--------------------
  3 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
  };

  #endif
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 59f5d0a..05296ab 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -957,6 +957,7 @@ struct xfrm_dst {
  	u32 child_mtu_cached;
  	u32 route_cookie;
  	u32 path_cookie;
+	struct llist_node       xdst_llist;
  };

  #ifdef CONFIG_XFRM
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a7487f3..fb286af 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,7 @@ 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);
-
+			llist_add(&xdst->xdst_llist, &net->xfrm.xp_sk_bundles_list);
  			route = xdst->route;
  		}
  	}
@@ -2549,18 +2542,12 @@ static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)

  static void __xfrm_garbage_collect(struct net *net)
  {
-	struct dst_entry *head, *next;
-
-	spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-	head = xfrm_policy_sk_bundles;
-	xfrm_policy_sk_bundles = NULL;
-	spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
+	struct llist_node *head;
+	struct xfrm_dst *xdst;

-	while (head) {
-		next = head->next;
-		dst_free(head);
-		head = next;
-	}
+	head = llist_del_all(&net->xfrm.xp_sk_bundles_list);
+	llist_for_each_entry(xdst, head, xdst_llist)
+		dst_free(&xdst->u.dst);
  }

  void xfrm_garbage_collect(struct net *net)
@@ -2942,7 +2929,6 @@ static int __net_init xfrm_net_init(struct net *net)
  	/* Initialize the per-net locks here */
  	spin_lock_init(&net->xfrm.xfrm_state_lock);
  	rwlock_init(&net->xfrm.xfrm_policy_lock);
-	spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
  	mutex_init(&net->xfrm.xfrm_cfg_mutex);

  	return 0;
-- 
1.7.9.5


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Eric Dumazet]

On Thu, 2013-12-19 at 11:17 +0800, Fan Du wrote:
> 

> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
>   };
> 
>   #endif
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 59f5d0a..05296ab 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -957,6 +957,7 @@ struct xfrm_dst {
>   	u32 child_mtu_cached;
>   	u32 route_cookie;
>   	u32 path_cookie;
> +	struct llist_node       xdst_llist;
>   };
> 

Hmm... Thats not very nice.

Please reuse the storage adding a llist_node in the union ?

diff --git a/include/net/dst.h b/include/net/dst.h
index 44995c13e941..3f604f47cc58 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -14,6 +14,7 @@
 #include <linux/rcupdate.h>
 #include <linux/bug.h>
 #include <linux/jiffies.h>
+#include <linux/llist.h>
 #include <net/neighbour.h>
 #include <asm/processor.h>
 
@@ -103,6 +104,7 @@ struct dst_entry {
 		struct rtable __rcu	*rt_next;
 		struct rt6_info		*rt6_next;
 		struct dn_route __rcu	*dn_next;
+		struct llist_node	llist;
 	};
 };
 

Re: [PATCHv3 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Eric Dumazet]

On Thu, 2013-12-19 at 11:17 +0800, Fan Du wrote:

> +	head = llist_del_all(&net->xfrm.xp_sk_bundles_list);
> +	llist_for_each_entry(xdst, head, xdst_llist)
> +		dst_free(&xdst->u.dst);
>   }

llist_for_each_entry_safe

Re: [PATCHv3 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月19日 11:44, Eric Dumazet wrote:
> On Thu, 2013-12-19 at 11:17 +0800, Fan Du wrote:
>>
>
>> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
>> index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
>>    };
>>
>>    #endif
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index 59f5d0a..05296ab 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -957,6 +957,7 @@ struct xfrm_dst {
>>    	u32 child_mtu_cached;
>>    	u32 route_cookie;
>>    	u32 path_cookie;
>> +	struct llist_node       xdst_llist;
>>    };
>>
>
> Hmm... Thats not very nice.
>
> Please reuse the storage adding a llist_node in the union ?
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 44995c13e941..3f604f47cc58 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -14,6 +14,7 @@
>   #include<linux/rcupdate.h>
>   #include<linux/bug.h>
>   #include<linux/jiffies.h>
> +#include<linux/llist.h>
>   #include<net/neighbour.h>
>   #include<asm/processor.h>
>
> @@ -103,6 +104,7 @@ struct dst_entry {
>   		struct rtable __rcu	*rt_next;
>   		struct rt6_info		*rt6_next;
>   		struct dn_route __rcu	*dn_next;

Add dst_entry with llist into a global llist head, after this
does the dst internal usage of above member corrupt below llist
if we unionize them together here?

> +		struct llist_node	llist;
>   	};
>   };



-- 
浮沉随浪只记今朝笑

--fan

[PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月19日 11:44, Eric Dumazet wrote:
> On Thu, 2013-12-19 at 11:17 +0800, Fan Du wrote:
>>
>
>> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
>> index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
>>    };
>>
>>    #endif
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index 59f5d0a..05296ab 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -957,6 +957,7 @@ struct xfrm_dst {
>>    	u32 child_mtu_cached;
>>    	u32 route_cookie;
>>    	u32 path_cookie;
>> +	struct llist_node       xdst_llist;
>>    };
>>
>
> Hmm... Thats not very nice.
>
> Please reuse the storage adding a llist_node in the union ?
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 44995c13e941..3f604f47cc58 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -14,6 +14,7 @@
>   #include<linux/rcupdate.h>
>   #include<linux/bug.h>
>   #include<linux/jiffies.h>
> +#include<linux/llist.h>
>   #include<net/neighbour.h>
>   #include<asm/processor.h>
>
> @@ -103,6 +104,7 @@ struct dst_entry {
>   		struct rtable __rcu	*rt_next;
>   		struct rt6_info		*rt6_next;
>   		struct dn_route __rcu	*dn_next;
> +		struct llist_node	llist;
>   	};
>   };

Hi, Eric

You are correct on this, have fix the patch wrt your comments.
Thank you very much.


 From 1b929e1cdea978d0c8d10d731af84969bd37004b Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Fri, 20 Dec 2013 11:23:27 +0800
Subject: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

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 lock less list to cater to those two scenarios as suggested by
Eric Dumazet.

Signed-off-by: Fan Du <fan.du@windriver.com>
Assisted-by: Eric Dumazet <edumazet@google.com>
---
v2:
   Fix incorrect commit log.

v3:
   Drop xchg, use llist instead, adviced by Eric Dumazet.

v4:
  Incorporate Eric's comments.
   -Union llist with dst_entry->next, which is used for queuing previous,
    then it's safe to do so.
   -Use llist_for_each_entry_safe for purging list.
   -Rebase with net-next, as ipsec-next got pulled.

---
  include/net/dst.h        |    5 +++++
  include/net/netns/xfrm.h |    2 +-
  net/xfrm/xfrm_policy.c   |   26 ++++++--------------------
  3 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 77eb53f..e9b81f0 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -14,6 +14,7 @@
  #include <linux/rcupdate.h>
  #include <linux/bug.h>
  #include <linux/jiffies.h>
+#include <linux/llist.h>
  #include <net/neighbour.h>
  #include <asm/processor.h>

@@ -103,6 +104,10 @@ struct dst_entry {
  		struct rtable __rcu	*rt_next;
  		struct rt6_info		*rt6_next;
  		struct dn_route __rcu	*dn_next;
+#ifdef CONFIG_XFRM		
+		/* Used to queue each policy sk dst */
+		struct llist_node       llist;
+#endif
  	};
  };

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
  };

  #endif
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a7487f3..0cc7446 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,7 @@ 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);
-
+			llist_add(&xdst->u.dst.llist, &net->xfrm.xp_sk_bundles_list);
  			route = xdst->route;
  		}
  	}
@@ -2549,18 +2542,12 @@ static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)

  static void __xfrm_garbage_collect(struct net *net)
  {
-	struct dst_entry *head, *next;
+	struct llist_node *head;
+	struct dst_entry *dst, *tmp;

-	spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-	head = xfrm_policy_sk_bundles;
-	xfrm_policy_sk_bundles = NULL;
-	spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-
-	while (head) {
-		next = head->next;
-		dst_free(head);
-		head = next;
-	}
+	head = llist_del_all(&net->xfrm.xp_sk_bundles_list);
+	llist_for_each_entry_safe(dst, tmp, head, llist)
+		dst_free(dst);
  }

  void xfrm_garbage_collect(struct net *net)
@@ -2942,7 +2929,6 @@ static int __net_init xfrm_net_init(struct net *net)
  	/* Initialize the per-net locks here */
  	spin_lock_init(&net->xfrm.xfrm_state_lock);
  	rwlock_init(&net->xfrm.xfrm_policy_lock);
-	spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
  	mutex_init(&net->xfrm.xfrm_cfg_mutex);

  	return 0;
-- 
1.7.9.5


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月20日 11:34, Fan Du wrote:
>
>
> On 2013年12月19日 11:44, Eric Dumazet wrote:
>> On Thu, 2013-12-19 at 11:17 +0800, Fan Du wrote:
>>>
>>
>>> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
>>> index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
>>> };
>>>
>>> #endif
>>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>>> index 59f5d0a..05296ab 100644
>>> --- a/include/net/xfrm.h
>>> +++ b/include/net/xfrm.h
>>> @@ -957,6 +957,7 @@ struct xfrm_dst {
>>> u32 child_mtu_cached;
>>> u32 route_cookie;
>>> u32 path_cookie;
>>> + struct llist_node xdst_llist;
>>> };
>>>
>>
>> Hmm... Thats not very nice.
>>
>> Please reuse the storage adding a llist_node in the union ?
>>
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 44995c13e941..3f604f47cc58 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -14,6 +14,7 @@
>> #include<linux/rcupdate.h>
>> #include<linux/bug.h>
>> #include<linux/jiffies.h>
>> +#include<linux/llist.h>
>> #include<net/neighbour.h>
>> #include<asm/processor.h>
>>
>> @@ -103,6 +104,7 @@ struct dst_entry {
>> struct rtable __rcu *rt_next;
>> struct rt6_info *rt6_next;
>> struct dn_route __rcu *dn_next;
>> + struct llist_node llist;
>> };
>> };
>
> Hi, Eric
>
> You are correct on this, have fix the patch wrt your comments.
> Thank you very much.
>
>
>  From 1b929e1cdea978d0c8d10d731af84969bd37004b Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Fri, 20 Dec 2013 11:23:27 +0800
> Subject: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles
>
> 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.

Hi Dave

I saw this patch is marked 'Not Applicable' in patchwork.

Not sure what's the improperness in it, corruption of xfrm_policy_sk_bundles
from different netns is real problem here. If you don't like below lock less
addition to avoid original spinlock, I could drop this part if you would require
and then put xfrm_policy_sk_bundles in net.xfrm only.

Could you please comment on this?
Thanks a lot :)

> 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 lock less list to cater to those two scenarios as suggested by
> Eric Dumazet.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
> Assisted-by: Eric Dumazet <edumazet@google.com>
> ---
> v2:
> Fix incorrect commit log.
>
> v3:
> Drop xchg, use llist instead, adviced by Eric Dumazet.
>
> v4:
> Incorporate Eric's comments.
> -Union llist with dst_entry->next, which is used for queuing previous,
> then it's safe to do so.
> -Use llist_for_each_entry_safe for purging list.
> -Rebase with net-next, as ipsec-next got pulled.
>
> ---
> include/net/dst.h | 5 +++++
> include/net/netns/xfrm.h | 2 +-
> net/xfrm/xfrm_policy.c | 26 ++++++--------------------
> 3 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 77eb53f..e9b81f0 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -14,6 +14,7 @@
> #include <linux/rcupdate.h>
> #include <linux/bug.h>
> #include <linux/jiffies.h>
> +#include <linux/llist.h>
> #include <net/neighbour.h>
> #include <asm/processor.h>
>
> @@ -103,6 +104,10 @@ struct dst_entry {
> struct rtable __rcu *rt_next;
> struct rt6_info *rt6_next;
> struct dn_route __rcu *dn_next;
> +#ifdef CONFIG_XFRM
> + /* Used to queue each policy sk dst */
> + struct llist_node llist;
> +#endif
> };
> };
>
> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> index 1006a26..22f4f90 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 llist_head xp_sk_bundles_list;
> };
>
> #endif
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index a7487f3..0cc7446 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,7 @@ 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);
> -
> + llist_add(&xdst->u.dst.llist, &net->xfrm.xp_sk_bundles_list);
> route = xdst->route;
> }
> }
> @@ -2549,18 +2542,12 @@ static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)
>
> static void __xfrm_garbage_collect(struct net *net)
> {
> - struct dst_entry *head, *next;
> + struct llist_node *head;
> + struct dst_entry *dst, *tmp;
>
> - spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
> - head = xfrm_policy_sk_bundles;
> - xfrm_policy_sk_bundles = NULL;
> - spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
> -
> - while (head) {
> - next = head->next;
> - dst_free(head);
> - head = next;
> - }
> + head = llist_del_all(&net->xfrm.xp_sk_bundles_list);
> + llist_for_each_entry_safe(dst, tmp, head, llist)
> + dst_free(dst);
> }
>
> void xfrm_garbage_collect(struct net *net)
> @@ -2942,7 +2929,6 @@ static int __net_init xfrm_net_init(struct net *net)
> /* Initialize the per-net locks here */
> spin_lock_init(&net->xfrm.xfrm_state_lock);
> rwlock_init(&net->xfrm.xfrm_policy_lock);
> - spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
> mutex_init(&net->xfrm.xfrm_cfg_mutex);
>
> return 0;

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 24 Dec 2013 09:12:32 +0800

> I saw this patch is marked 'Not Applicable' in patchwork.

It means that it doesn't go in via my tree directly, it
"doesn't apply".

Instead, it goes into Steffen's IPSEC tree.

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月24日 13:31, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Tue, 24 Dec 2013 09:12:32 +0800
>
>> >  I saw this patch is marked 'Not Applicable' in patchwork.
> It means that it doesn't go in via my tree directly, it
> "doesn't apply".
>
> Instead, it goes into Steffen's IPSEC tree.
>

Thanks for your explanation!!!

I'm not well understand the curtsy about patchwork, and one last question,
[PATCHv4 net-next 0/8] pktgen IPsec support is tagged as 'Awaiting Upstream'
what does that mean and do I need to take further action about this patch set?


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Steffen Klassert]

On Tue, Dec 24, 2013 at 01:39:45PM +0800, Fan Du wrote:
> 
> 
> On 2013年12月24日 13:31, David Miller wrote:
> >From: Fan Du<fan.du@windriver.com>
> >Date: Tue, 24 Dec 2013 09:12:32 +0800
> >
> >>>  I saw this patch is marked 'Not Applicable' in patchwork.
> >It means that it doesn't go in via my tree directly, it
> >"doesn't apply".
> >
> >Instead, it goes into Steffen's IPSEC tree.
> >

Yes, I'll take care about this one. I'm currently traveling,
so I have not that much time for review and tests of patches.
I'll integrate all pending ipsec patches after the holidays.

> 
> Thanks for your explanation!!!
> 
> I'm not well understand the curtsy about patchwork, and one last question,
> [PATCHv4 net-next 0/8] pktgen IPsec support is tagged as 'Awaiting Upstream'
> what does that mean and do I need to take further action about this patch set?
> 

I guess that means the same, it goes into the ipsec-next tree :)

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月24日 17:50, Steffen Klassert wrote:
> On Tue, Dec 24, 2013 at 01:39:45PM +0800, Fan Du wrote:
>>
>>
>> On 2013年12月24日 13:31, David Miller wrote:
>>> From: Fan Du<fan.du@windriver.com>
>>> Date: Tue, 24 Dec 2013 09:12:32 +0800
>>>
>>>>>   I saw this patch is marked 'Not Applicable' in patchwork.
>>> It means that it doesn't go in via my tree directly, it
>>> "doesn't apply".
>>>
>>> Instead, it goes into Steffen's IPSEC tree.
>>>
>
> Yes, I'll take care about this one. I'm currently traveling,
> so I have not that much time for review and tests of patches.
> I'll integrate all pending ipsec patches after the holidays.
>
>>
>> Thanks for your explanation!!!
>>
>> I'm not well understand the curtsy about patchwork, and one last question,
>> [PATCHv4 net-next 0/8] pktgen IPsec support is tagged as 'Awaiting Upstream'
>> what does that mean and do I need to take further action about this patch set?
>>
>
> I guess that means the same, it goes into the ipsec-next tree :)

Ok, I understood, and Merry Christmas!

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Steffen Klassert]

On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
> 
> Subject: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles
> 
> 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.

I'm ok with this patch, but I wonder where we use these cached socket
bundles. After a quick look I see where we add and where we delete
them, but I can't see how we use these cached bundles. 

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Tue, 24 Dec 2013 13:39:45 +0800

> 
> 
> On 2013年12月24日 13:31, David Miller wrote:
>> From: Fan Du<fan.du@windriver.com>
>> Date: Tue, 24 Dec 2013 09:12:32 +0800
>>
>>> >  I saw this patch is marked 'Not Applicable' in patchwork.
>> It means that it doesn't go in via my tree directly, it
>> "doesn't apply".
>>
>> Instead, it goes into Steffen's IPSEC tree.
>>
> 
> Thanks for your explanation!!!
> 
> I'm not well understand the curtsy about patchwork, and one last
> question,
> [PATCHv4 net-next 0/8] pktgen IPsec support is tagged as 'Awaiting
> Upstream'
> what does that mean and do I need to take further action about this
> patch set?

It means the same thing, that I expect to receive this change via
upstream, which is Steffen's IPSEC tree.

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

ccing Timo

On 2013年12月24日 18:35, Steffen Klassert wrote:
> On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
>>
>> Subject: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles
>>
>> 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.
>
> I'm ok with this patch, but I wonder where we use these cached socket
> bundles. After a quick look I see where we add and where we delete
> them, but I can't see how we use these cached bundles.

Interesting

The per socket bundles is introduced by Timo in commit 80c802f3
("xfrm: cache bundles instead of policies for outgoing flows")

But one fundamental question is why not use existing flow cache
for per socket bundles as well? then no need to create such per
sock xdst for every packet, and also share the same flow cache
flush mechanism.

My first impression is it can be done this way, I'm going to head
this way unless turn out otherwise.

So Timo ?

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Timo Teras]

On Wed, 25 Dec 2013 14:40:36 +0800
Fan Du <fan.du@windriver.com> wrote:

> ccing Timo
> 
> On 2013年12月24日 18:35, Steffen Klassert wrote:
> > On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
> >>
> >> Subject: [PATCHv4 net-next] xfrm: Namespacify
> >> xfrm_policy_sk_bundles
> >>
> >> 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.
> >
> > I'm ok with this patch, but I wonder where we use these cached
> > socket bundles. After a quick look I see where we add and where we
> > delete them, but I can't see how we use these cached bundles.
> 
> Interesting
> 
> The per socket bundles is introduced by Timo in commit 80c802f3
> ("xfrm: cache bundles instead of policies for outgoing flows")

Those existed even before. I just did systematic transformation of the
caching code to work on bundle level instead of policy level.

> But one fundamental question is why not use existing flow cache
> for per socket bundles as well? then no need to create such per
> sock xdst for every packet, and also share the same flow cache
> flush mechanism.

It was needed when the flow cache cached policies. They explicitly
needed to check the socket for per-socket policy. So it made no sense
to have anything socket related in the cache.

> My first impression is it can be done this way, I'm going to head
> this way unless turn out otherwise.

I think it could converted. You'll still need to look up the per-socket
policies. But if caching the bundles in flow cache simplifies overall
code it sounds like a good thing to do.

- Timo

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2013年12月25日 16:11, Timo Teras wrote:
> On Wed, 25 Dec 2013 14:40:36 +0800
> Fan Du<fan.du@windriver.com>  wrote:
>
>> >  ccing Timo
>> >
>> >  On 2013年12月24日 18:35, Steffen Klassert wrote:
>>> >  >  On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
>>>> >  >>
>>>> >  >>  Subject: [PATCHv4 net-next] xfrm: Namespacify
>>>> >  >>  xfrm_policy_sk_bundles
>>>> >  >>
>>>> >  >>  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.
>>> >  >
>>> >  >  I'm ok with this patch, but I wonder where we use these cached
>>> >  >  socket bundles. After a quick look I see where we add and where we
>>> >  >  delete them, but I can't see how we use these cached bundles.
>> >
>> >  Interesting
>> >
>> >  The per socket bundles is introduced by Timo in commit 80c802f3
>> >  ("xfrm: cache bundles instead of policies for outgoing flows")
> Those existed even before. I just did systematic transformation of the
> caching code to work on bundle level instead of policy level.

Apologizes and thanks for your quick reply :)

>> >  But one fundamental question is why not use existing flow cache
>> >  for per socket bundles as well? then no need to create such per
>> >  sock xdst for every packet, and also share the same flow cache
>> >  flush mechanism.
> It was needed when the flow cache cached policies. They explicitly
> needed to check the socket for per-socket policy. So it made no sense
> to have anything socket related in the cache.

I understand your concern.

per sk bundles could be distinguished by putting per sk policy pointer into
struct flow_cache_entry, and then compare sk policy between cached policy
against with sk policy.

And I also notice flow cache is global across different namespaces, but flow
cache flush is doing a per-cpu(also global) operation, that's not fair for
slim netns as compared with fat netns which floods flow cache. Maybe it's
time to make flow cache also name space aware.

Let's see what Steffen think about it.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Steffen Klassert]

On Wed, Dec 25, 2013 at 04:44:26PM +0800, Fan Du wrote:
> 
> 
> On 2013年12月25日 16:11, Timo Teras wrote:
> >On Wed, 25 Dec 2013 14:40:36 +0800
> >Fan Du<fan.du@windriver.com>  wrote:
> >
> >>>  ccing Timo
> >>>
> >>>  On 2013年12月24日 18:35, Steffen Klassert wrote:
> >>>>  >  On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
> >>>>>  >>
> >>>>>  >>  Subject: [PATCHv4 net-next] xfrm: Namespacify
> >>>>>  >>  xfrm_policy_sk_bundles
> >>>>>  >>
> >>>>>  >>  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.
> >>>>  >
> >>>>  >  I'm ok with this patch, but I wonder where we use these cached
> >>>>  >  socket bundles. After a quick look I see where we add and where we
> >>>>  >  delete them, but I can't see how we use these cached bundles.
> >>>
> >>>  Interesting
> >>>
> >>>  The per socket bundles is introduced by Timo in commit 80c802f3
> >>>  ("xfrm: cache bundles instead of policies for outgoing flows")
> >Those existed even before. I just did systematic transformation of the
> >caching code to work on bundle level instead of policy level.
> 
> Apologizes and thanks for your quick reply :)
> 
> >>>  But one fundamental question is why not use existing flow cache
> >>>  for per socket bundles as well? then no need to create such per
> >>>  sock xdst for every packet, and also share the same flow cache
> >>>  flush mechanism.
> >It was needed when the flow cache cached policies. They explicitly
> >needed to check the socket for per-socket policy. So it made no sense
> >to have anything socket related in the cache.
> 
> I understand your concern.
> 
> per sk bundles could be distinguished by putting per sk policy pointer into
> struct flow_cache_entry, and then compare sk policy between cached policy
> against with sk policy.

Most protocols cache the used routes at the sockets, so I'm not sure if
we really need to cache them in xfrm too.

Given the fact that we don't use these cached socket policy bundles,
it would be already an improvement if we would simply remove this caching.
All we are doing here is wasting memory.

> 
> And I also notice flow cache is global across different namespaces, but flow
> cache flush is doing a per-cpu(also global) operation, that's not fair for
> slim netns as compared with fat netns which floods flow cache. Maybe it's
> time to make flow cache also name space aware.

Yes, making the flow cache namespace aware would be a good thing.

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2014年01月06日 18:35, Steffen Klassert wrote:
> On Wed, Dec 25, 2013 at 04:44:26PM +0800, Fan Du wrote:
>>
>>
>> On 2013年12月25日 16:11, Timo Teras wrote:
>>> On Wed, 25 Dec 2013 14:40:36 +0800
>>> Fan Du<fan.du@windriver.com>   wrote:
>>>
>>>>>   ccing Timo
>>>>>
>>>>>   On 2013年12月24日 18:35, Steffen Klassert wrote:
>>>>>>   >   On Fri, Dec 20, 2013 at 11:34:41AM +0800, Fan Du wrote:
>>>>>>>   >>
>>>>>>>   >>   Subject: [PATCHv4 net-next] xfrm: Namespacify
>>>>>>>   >>   xfrm_policy_sk_bundles
>>>>>>>   >>
>>>>>>>   >>   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.
>>>>>>   >
>>>>>>   >   I'm ok with this patch, but I wonder where we use these cached
>>>>>>   >   socket bundles. After a quick look I see where we add and where we
>>>>>>   >   delete them, but I can't see how we use these cached bundles.
>>>>>
>>>>>   Interesting
>>>>>
>>>>>   The per socket bundles is introduced by Timo in commit 80c802f3
>>>>>   ("xfrm: cache bundles instead of policies for outgoing flows")
>>> Those existed even before. I just did systematic transformation of the
>>> caching code to work on bundle level instead of policy level.
>>
>> Apologizes and thanks for your quick reply :)
>>
>>>>>   But one fundamental question is why not use existing flow cache
>>>>>   for per socket bundles as well? then no need to create such per
>>>>>   sock xdst for every packet, and also share the same flow cache
>>>>>   flush mechanism.
>>> It was needed when the flow cache cached policies. They explicitly
>>> needed to check the socket for per-socket policy. So it made no sense
>>> to have anything socket related in the cache.
>>
>> I understand your concern.
>>
>> per sk bundles could be distinguished by putting per sk policy pointer into
>> struct flow_cache_entry, and then compare sk policy between cached policy
>> against with sk policy.

Yes, I tested sk policy with udp, when transmit, dst will be cached into sk
by sk_dst_set. Let's leave current implementation as it is.

Please kindly review if there is any concern about v4.

> Most protocols cache the used routes at the sockets, so I'm not sure if
> we really need to cache them in xfrm too.
>
> Given the fact that we don't use these cached socket policy bundles,
> it would be already an improvement if we would simply remove this caching.
> All we are doing here is wasting memory.
>>
>> And I also notice flow cache is global across different namespaces, but flow
>> cache flush is doing a per-cpu(also global) operation, that's not fair for
>> slim netns as compared with fat netns which floods flow cache. Maybe it's
>> time to make flow cache also name space aware.
>
> Yes, making the flow cache namespace aware would be a good thing.
>

I will give it a try :)

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Steffen Klassert]

On Tue, Jan 07, 2014 at 10:43:38AM +0800, Fan Du wrote:
> 
> Yes, I tested sk policy with udp, when transmit, dst will be cached into sk
> by sk_dst_set. Let's leave current implementation as it is.
> 
> Please kindly review if there is any concern about v4.

Why do you want to keep the current implementation? We don't
use the cached bundles. I'd remove this caching with the patch
below during the next development cycle if nobody has a good
reason why we should keep it.


Subject: [PATCH RFC] xfrm: Remove caching of xfrm_policy_sk_bundles

We currently cache socket policy bundles at xfrm_policy_sk_bundles.
These cached bundles are never used. Instead we create and cache
a new one whenever xfrm_lookup() is called on a socket policy.

Most protocols cache the used routes to the socket, so let's
remove the unused caching of socket policy bundles in xfrm.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/netns/xfrm.h |    1 -
 net/xfrm/xfrm_policy.c   |   28 ----------------------------
 2 files changed, 29 deletions(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 1006a26..cf65269 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -58,7 +58,6 @@ 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;
 };
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a7487f3..1c79ca8 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;
@@ -2107,13 +2105,6 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 				goto no_transform;
 			}
 
-			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);
-
 			route = xdst->route;
 		}
 	}
@@ -2547,33 +2538,15 @@ static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)
 	return dst;
 }
 
-static void __xfrm_garbage_collect(struct net *net)
-{
-	struct dst_entry *head, *next;
-
-	spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-	head = xfrm_policy_sk_bundles;
-	xfrm_policy_sk_bundles = NULL;
-	spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-
-	while (head) {
-		next = head->next;
-		dst_free(head);
-		head = next;
-	}
-}
-
 void xfrm_garbage_collect(struct net *net)
 {
 	flow_cache_flush();
-	__xfrm_garbage_collect(net);
 }
 EXPORT_SYMBOL(xfrm_garbage_collect);
 
 static void xfrm_garbage_collect_deferred(struct net *net)
 {
 	flow_cache_flush_deferred();
-	__xfrm_garbage_collect(net);
 }
 
 static void xfrm_init_pmtu(struct dst_entry *dst)
@@ -2942,7 +2915,6 @@ static int __net_init xfrm_net_init(struct net *net)
 	/* Initialize the per-net locks here */
 	spin_lock_init(&net->xfrm.xfrm_state_lock);
 	rwlock_init(&net->xfrm.xfrm_policy_lock);
-	spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
 	mutex_init(&net->xfrm.xfrm_cfg_mutex);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles

[Fan Du]

On 2014年01月09日 20:38, Steffen Klassert wrote:
> On Tue, Jan 07, 2014 at 10:43:38AM +0800, Fan Du wrote:
>> >
>> >  Yes, I tested sk policy with udp, when transmit, dst will be cached into sk
>> >  by sk_dst_set. Let's leave current implementation as it is.
>> >
>> >  Please kindly review if there is any concern about v4.
> Why do you want to keep the current implementation? We don't
> use the cached bundles. I'd remove this caching with the patch
> below during the next development cycle if nobody has a good
> reason why we should keep it.
>
>
> Subject: [PATCH RFC] xfrm: Remove caching of xfrm_policy_sk_bundles
>
> We currently cache socket policy bundles at xfrm_policy_sk_bundles.
> These cached bundles are never used. Instead we create and cache
> a new one whenever xfrm_lookup() is called on a socket policy.
>
> Most protocols cache the used routes to the socket, so let's
> remove the unused caching of socket policy bundles in xfrm.

Honestly speaking, I cannot think of any other obvious reason
to retain sk bundle cache for what I know about XFRM so far, though
this mysterious ancient code still puzzles me.

Acked-by: Fan Du <fan.du@windriver.com>

> Signed-off-by: Steffen Klassert<steffen.klassert@secunet.com>

-- 
浮沉随浪只记今朝笑

--fan