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

[Fan Du <fan.du@windriver.com>]

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