Date: Thu, 27 Dec 2012 22:24:33 -0800 From: Adrian Chadd <adrian@freebsd.org> To: Monthadar Al Jaberi <monthadar@gmail.com>, Bernhard Schmidt <bschmidt@freebsd.org>, freebsd-wireless@freebsd.org Subject: .. if_transmit() and the quirky issues with node referencing Message-ID: <CAJ-Vmom6jNNiy16h7NP3PBaj1LjaAGqTvanMkegZLsYpXTJCbw@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
So I finally figured out my problem with if_transmit()'ifying the ath(4) code. (Yes, this means I have TX fragmentation working with ath(4), as well as some non-trivial performance improvements in TX TCP tests.) In quick summary: net80211's call to the driver will decrement the node ref if the driver if_transmit() call fails. So I went looking for instances of where if_transmit() is called without correct error checking. One is in ieee80211_hwmp.c . It pops frames off of the ageq and calls if_transmit(). It should deref the node ref the same way that the ageq code does. The other is in ieee80211_hostap.c - my fault. It uses the psq; I should deref the node ref the same way that the psq code does. This just highlights some of the rather quirky corners of net80211: * if_transmit() can be called on the driver facing interface (ie, the driver ifp) or the vap ifp. * for driver ifp, the frame should be encapsulated and there must be a node reference held/given when it's called. If if_transmit() fails here, it must deref the node. * for non-encapsulated frames (eg perhaps some stuff in the ageq or one of the psqs), if_transmit() will be called on the VAP interface. * .. now, when if_transmit() is called on the vap interface, the mbuf m_pkthdr.recvif will point to the vap ifp; * .. when if_transmit() is called on the physical interface, the mbuf m_pkthdr.recvif will point to the node reference. This is all a bit kooky and very prone to people making mistakes, especially when mbufs may be pushed up, down and throughout all kinds of weird places. I also bet the re-entrant parts of the codebase (ie stuff that uses the ageq, psq, wds .. maybe the mesh stuff) could do with a bit of a re-review. In any case, what this means is: * we need to be really, really careful with how we route mbufs through net80211 here, aiee! * if a frame has M_ENCAP tagged and it's pushed into an ageq or psq, it _must_ have the TX node ref held; .. thus, if we raw construct an encapsulated frame (locally in net80211, or in a driver, or via ieee80211_output, etc) then when we set M_ENCAP, we must hold the TX node reference and we must set recvif correctly! What I'd really like to do here: * re-review all the if_transmit() error handling - make sure that if it fails, we correctly handle dereferencing the node or things will leak; * make "get/set recvif from ifp" and "get/set recvif from node" as methods; * have those methods check whether M_ENCAP is correctly set/cleared, and complain loudly if we ever try to get the wrong reference for the given situation (eg, if we try to read the node reference from an mbuf, but M_ENCAP isn't set, then complain); * .. figure out some alternate way to store the node reference (mbuf tags?) and start migrating the code that way. Anyway, at least I've been down this rabbit hole and have higher 11n throughput _and_ legacy TX fragmentation working (at least for ath(4).) I'll post a patch tomorrow night and start thoroughly testing the error corner cases. Thanks, Adrian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmom6jNNiy16h7NP3PBaj1LjaAGqTvanMkegZLsYpXTJCbw>