On 27/08/2019 15:10, Rafał Miłecki wrote: > On Tue, 27 Aug 2019 at 09:52, Brendan Jackman > <brendan.jackman@xxxxxxxxxxxxxxx> wrote: >> On 27/08/2019 14:34, Johannes Berg wrote: >> >> On the other hand, this now makes me worry that there could be other drivers out >> >> there that implement .tx_control_port, but do not take advantage of it to avoid >> >> the key installation race... in that case this patch could break handshakes for >> >> those drivers. On the other-other hand I don't know why you'd implement >> >> .tx_control_port except for this very reason. >> > I'm not sure how it'd break anything, it'd just behave as before? But >> > this is new, nobody really implements it yet. So I don't think we have >> > to worry about that even if I'm not sure I understand your concern. >> >> Well my fear was that there could be a driver that relies on something like: >> >> if (skb->protocol == htons(ETH_P_PAE)) >> /* explicitly disable encryption for this frame */ >> >> >> in its .ndo_start_xmit, to avoid the race condition vs key installation. But >> then it implements .tx_control_port and respects the noencrypt flag (but doesn't >> flush its TX queues in .add_key). > > FWIW, brcmfmac has brcmf_netdev_wait_pend8021x() that gets called from > .add_key and .del_key. It does nothing else than just waiting for zero > ETH_P_PAE packets queued. In .ndo_start_xmit it keeps counting queued > ETH_P_PAE packets. > > It doesn't do any encryption/.tx_control_port trickery though, so I > assume it's all safe. Ah, that's interesting. I actually considered implementing such a trick in our driver, but I could not convince myself it was a complete solution, because the EAPOL frame might not be in the ring before .add_key. (Maybe I am wrong about that). _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
