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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 19 Dec 2013 09:35:57 +0800
From:	Fan Du <fan.du@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>,
	王聪 <xiyou.wangcong@...il.com>
CC:	<steffen.klassert@...unet.com>, <davem@...emloft.net>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCHv2 ipsec-next] xfrm: Namespacify xfrm_policy_sk_bundles

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