Date: Mon, 31 Dec 2012 23:04:15 -0700 From: PseudoCylon <moonlightakkiy@yahoo.ca> To: Adrian Chadd <adrian@freebsd.org>, Monthadar Al Jaberi <monthadar@gmail.com>, Bernhard Schmidt <bschmidt@freebsd.org>, freebsd-wireless@freebsd.org Subject: Re: if_transmit() and the quirky issues with node referencing Message-ID: <CAFZ_MYJVo3_ZGgxmSV5SrR33gSQVMG-cQOjV46-YEDiBE=Vk-Q@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
> ------------------------------ > > Message: 3 > 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> > Content-Type: text/plain; charset=ISO-8859-1 > > 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. > Just to add another thing before I forget. When tearing a ba session is somehow failed, a node could still receive ampdu packets from another node which ni has already freed. I don't know exactly how that happens, yet. I will let you know when I figured it out. AK
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFZ_MYJVo3_ZGgxmSV5SrR33gSQVMG-cQOjV46-YEDiBE=Vk-Q>