On Mon, Apr 20, 2015 at 11:47 AM, Johannes Berg <[email protected]> wrote: > Hey, somebody is reviewing my patches :-) > i didn't delve into them too much, but generally they look good :)
>> > + build.key = rcu_access_pointer(sta->ptk[sta->ptk_idx]); >> > + if (!build.key) >> > + build.key = rcu_access_pointer(sdata->default_unicast_key); >> don't you need rcu_dereference here? (and you don't seem to be inside >> rcu section here) > > It's a bit complicated. > > I used to think I don't need it, but perhaps I do to avoid accessing bad > memory in this function. > > The thing is that this function is going to be called immediately > whenever those pointers change, so that the RCU handling of the fast_tx > struct itself should prevent the TX path from accessing a bad key > pointer. > > However, it seems possible that we go into this function on another CPU > for unrelated reasons, and if that CPU then stalls after getting the key > pointer but before assigning the fast_tx pointer, then it might > overwrite the assignment or clearing from the CPU processing the key > change. > > So indeed it looks like this isn't safe as is right now. > > To fix that, I think I can hold the lock longer, so that the lifetime of > the key and the fast_tx pointer are more closely correlated. If I > acquire the spinlock before checking for the key, then the CPU that > invalidates the key pointer cannot race in this way with another caller, > since the key pointer would (for this purpose) be protected by the lock. > Then either the CPU that deleted the key will have to wait (while the > key is still pretty much valid) and then will overwrite the fast_tx w/o > the key, or the other CPU will have to wait and will find the key > pointer changed/NULL already. > > Right? what do you think? sounds correct. i guess taking rcu_lock is a valid option as well (for about the same reasons). so either one of them should be good. Eliad. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
