From owner-freebsd-wireless@FreeBSD.ORG Fri Dec 28 06:24:42 2012 Return-Path: Delivered-To: freebsd-wireless@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 6072021D; Fri, 28 Dec 2012 06:24:42 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 64EFB8FC0A; Fri, 28 Dec 2012 06:24:41 +0000 (UTC) Received: by mail-wg0-f50.google.com with SMTP id es5so4737115wgb.29 for ; Thu, 27 Dec 2012 22:24:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:date:x-google-sender-auth:message-id:subject :from:to:content-type; bh=ZR2R1GCckFbqbI/Q+/cUXAtfUdpnfioLkv48KAwY2R4=; b=kVRUPOe1YXZ+mUpCWX9JjNwSUJcSfOYsKwOsdm5T7biRS9fkcxArG/V1QIPdmHdCiN eGpw7s09y+fDNvi+RxTIg6c/BCjHGrDl3WZW3ZL6dpIwN/joVRMp2S1++jqvsJcGOhTV cX7vJi5Beg4Me+UI7J3KD5YXGpUXjbU00AATNL5zFu7Gf9wOtbZU5jW3RAQycXfgMt5E Q/qzKa05Qp/AeLgTPZHGsIJ9mNKdIIhIk1L+0qaKDXUl3ycduSZzV65BDGPAOMcoPL9J ZVAfPty8B/VdBvUKcvadr0+1l8o2EpC/QM/LPbYabaLrThLhho9my5VXw0zOSaogWPyi Lyyw== MIME-Version: 1.0 Received: by 10.194.83.36 with SMTP id n4mr52007139wjy.59.1356675873796; Thu, 27 Dec 2012 22:24:33 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.217.57.9 with HTTP; Thu, 27 Dec 2012 22:24:33 -0800 (PST) Date: Thu, 27 Dec 2012 22:24:33 -0800 X-Google-Sender-Auth: 2VOoaJQMusXV71lYq2EBLCd-jfQ Message-ID: Subject: .. if_transmit() and the quirky issues with node referencing From: Adrian Chadd To: Monthadar Al Jaberi , Bernhard Schmidt , freebsd-wireless@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 X-BeenThere: freebsd-wireless@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussions of 802.11 stack, tools device driver development." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Dec 2012 06:24:42 -0000 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