From owner-freebsd-wireless@FreeBSD.ORG Tue Jan 1 06:04:23 2013 Return-Path: Delivered-To: freebsd-wireless@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A0B6F648 for ; Tue, 1 Jan 2013 06:04:23 +0000 (UTC) (envelope-from moonlightakkiy@yahoo.ca) Received: from nm17.bullet.mail.bf1.yahoo.com (nm17.bullet.mail.bf1.yahoo.com [98.139.212.176]) by mx1.freebsd.org (Postfix) with SMTP id 43F1F8FC12 for ; Tue, 1 Jan 2013 06:04:23 +0000 (UTC) Received: from [98.139.212.148] by nm17.bullet.mail.bf1.yahoo.com with NNFMP; 01 Jan 2013 06:04:15 -0000 Received: from [98.139.211.196] by tm5.bullet.mail.bf1.yahoo.com with NNFMP; 01 Jan 2013 06:04:15 -0000 Received: from [127.0.0.1] by smtp205.mail.bf1.yahoo.com with NNFMP; 01 Jan 2013 06:04:15 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.ca; s=s1024; t=1357020255; bh=gkB+R2idn78ATEyqOHZhfWx8bsx+cjFZpEj3xlJDUpE=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:Received:MIME-Version:Received:Received:Date:Message-ID:Subject:From:To:Content-Type; b=BfwR3uxXl4Nje0mSCQGNF8CkjrpjYN/CPFH6NDK7ZKTgUrleGmnfd2TmTR37+r3DCnu/Z5iomKdvOsd/Z68FndPZUykSvIJIJLg45yRpNZSQ8WWE9scsS9+0rWUbqyrRDcOVWltQQ6Ib/77TVwroW5wNUk+oMu2VZv4gQ/k7Eaw= X-Yahoo-Newman-Id: 918663.5339.bm@smtp205.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: L2AjET0VM1kvUTWQqLwwT56fnC5rIbNau_aUf601ffP_2_J 1oyvEPpo_eD7KfepSgVGTBlzzUrXKttXoFKFg_2BuSBLGakfQ2IYiQqyrOyw mw7yPPeWCR8tThKVUc0tBLjq598c1e9KFhMAn_kanquxIGndT_A3UJXjGgJZ 8D9Upvww0H3jQKIbdC0azoG.kFjLSQyWS.macd6SWYWgca0GvUBilBqVfT2D iyEGpRTpFvSQSJaupwLTIYJpbAcNOZIszUjuzOV9SADGX8FdyqkHzQ3cWVPe PHuOsMxtAzce0_CPSYnPBSUD8FONEuW8WhguKO6vNDiSFakOCmWCl1YdZZlK RiYPOpYpfFzU0EpQfN2K5Opl5NnYaBYUJ7I7xDBENNKlGz1pZ.Mxz1sEAqVZ CGRP3T6lx9eltTBOrFSyB0g6h5VyTmJFQtqATYKjLvydbpik_inTOuiF28Ei MCaJpIVpWLnQrEYjj4cE- X-Yahoo-SMTP: Xr6qjFWswBAEmd20sAvB4Q3keqXvXsIH9TjJ Received: from mail-vc0-f173.google.com (moonlightakkiy@209.85.220.173 with plain) by smtp205.mail.bf1.yahoo.com with SMTP; 31 Dec 2012 22:04:15 -0800 PST Received: by mail-vc0-f173.google.com with SMTP id f13so13289320vcb.4 for ; Mon, 31 Dec 2012 22:04:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.220.115.19 with SMTP id g19mr67015145vcq.69.1357020255347; Mon, 31 Dec 2012 22:04:15 -0800 (PST) Received: by 10.58.182.72 with HTTP; Mon, 31 Dec 2012 22:04:15 -0800 (PST) Date: Mon, 31 Dec 2012 23:04:15 -0700 Message-ID: Subject: Re: if_transmit() and the quirky issues with node referencing From: PseudoCylon To: Adrian Chadd , 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: Tue, 01 Jan 2013 06:04:23 -0000 > ------------------------------ > > Message: 3 > Date: Thu, 27 Dec 2012 22:24:33 -0800 > From: Adrian Chadd > To: Monthadar Al Jaberi , Bernhard Schmidt > , freebsd-wireless@freebsd.org > Subject: .. if_transmit() and the quirky issues with node referencing > Message-ID: > > 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