Hey, somebody is reviewing my patches :-) > > + 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? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
