Messages in this thread | | | Date | Mon, 27 May 2013 15:34:31 +0400 | From | Roman Gushchin <> | Subject | Re: [PATCH v2] rcu: fix a race in hlist_nulls_for_each_entry_rcu macro |
| |
On 25.05.2013 15:37, Paul E. McKenney wrote: >> 2) A problem occurs when restart_condition is true and we jump to the begin label. >> We do not recalculate (head + offsetof(head, first)) address, we just dereference >> again the OLD (head->first) pointer. So, we get a node, that WAS the first node in a >> previous time instead of current first node. That node can be dead, or, for instance, >> can be a head of another chain. > > OK, this is what I was referring to when I said that the RCU list macros > assume that the list header is static (or equivalently, appropriately > protected). > > With some_func() as written above, you would need to return some sort > of failure indication from some_func(), and have the caller refetch head. > Otherwise, as far as gcc is concerned, the value of the parameter head > is constant throughout some_func().
Personally I have nothing against this approach, but I'm not sure, if networking maintainers will adopt corresponding patch. I'll try to find it out.
> >> It is correct from gcc's point of view, since it doesn't expect the head pointer >> to change, and this address is just (head + constant offset). > > Agreed. > > How does the caller calculate the value to pass in through the argument "head"?
struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport, __be32 daddr, __be16 dport, int dif, struct udp_table *udptable) { ... unsigned int hash2, slot2, slot = udp_hashfn(net, hnum, udptable->mask); struct udp_hslot *hslot2, *hslot = &udptable->hash[slot]; int score, badness;
rcu_read_lock(); if (hslot->count > 10) { hash2 = udp4_portaddr_hash(net, daddr, hnum); slot2 = hash2 & udptable->mask; hslot2 = &udptable->hash2[slot2]; ... result = udp4_lib_lookup2(net, saddr, sport, daddr, hnum, dif, hslot2, slot2); ... }
static struct sock *udp4_lib_lookup2(struct net *net, __be32 saddr, __be16 sport, __be32 daddr, unsigned int hnum, int dif, struct udp_hslot *hslot2, unsigned int slot2) { ... udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) { ...
Thank you!
Regards, Roman
| |