[{"id":1771727,"web_url":"http://patchwork.ozlabs.org/comment/1771727/","msgid":"<1505903964.3026.14.camel@sipsolutions.net>","list_archive_url":null,"date":"2017-09-20T10:39:24","subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","submitter":{"id":265,"url":"http://patchwork.ozlabs.org/api/people/265/","name":"Johannes Berg","email":"johannes@sipsolutions.net"},"content":"On Wed, 2017-09-20 at 13:11 +0300, Ville Syrjala wrote:\n\n> --- a/net/mac80211/tx.c\n> +++ b/net/mac80211/tx.c\n> @@ -1770,15 +1770,21 @@ bool ieee80211_tx_prepare_skb(struct ieee80211_hw *hw,\n>  \tstruct ieee80211_tx_data tx;\n>  \tstruct sk_buff *skb2;\n>  \n> -\tif (ieee80211_tx_prepare(sdata, &tx, NULL, skb) == TX_DROP)\n> +\trcu_read_lock();\n\nThe documentation says:\n\n/**\n * ieee80211_tx_prepare_skb - prepare an 802.11 skb for transmission\n * @hw: pointer as obtained from ieee80211_alloc_hw()\n * @vif: virtual interface\n * @skb: frame to be sent from within the driver\n * @band: the band to transmit on\n * @sta: optional pointer to get the station to send the frame to\n *\n * Note: must be called under RCU lock\n */\n\nYou can't even argue that it should be the function itself doing it,\nbecause the (admittedly optional) sta pointer would otherwise not have\nproper protection after you leave the function ... You can't pass out a\nsta pointer that's RCU protected.\n\nSide note: Perhaps some annotation should be there? not sure it's\npossible - would have to be something like\n\tstruct ieee80211_sta * __rcu *sta;\n\nI guess since the outer pointer isn't protected, only the inner ...\n\n\nTherefore, this patch is wrong.\n\nI actually think the same is true for ieee80211_tx_dequeue(), but I'm\nless sure about it - the sta pointer there clearly is somehow safely\npassed in (even if it's w/o RCU, the driver can potentially make that\nsafe), but the key pointer seems unsafe in this case (as well) if\nthere's no outer RCU protection.\n\njohannes","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxx7q4gSVz9s82\n\tfor ;\n\tWed, 20 Sep 2017 20:39:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751585AbdITKja (ORCPT );\n\tWed, 20 Sep 2017 06:39:30 -0400","from s3.sipsolutions.net ([5.9.151.49]:48278 \"EHLO\n\tsipsolutions.net\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751488AbdITKj3 (ORCPT );\n\tWed, 20 Sep 2017 06:39:29 -0400","by sipsolutions.net with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89)\n\t(envelope-from )\n\tid 1ducPd-0001mf-8S; Wed, 20 Sep 2017 12:39:25 +0200"],"Message-ID":"<1505903964.3026.14.camel@sipsolutions.net>","Subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","From":"Johannes Berg ","To":"Ville Syrjala ,\n\tlinux-wireless@vger.kernel.org","Cc":"\"David S. Miller\" , netdev@vger.kernel.org","Date":"Wed, 20 Sep 2017 12:39:24 +0200","In-Reply-To":"<20170920101123.23312-1-ville.syrjala@linux.intel.com>","References":"<20170918195919.15860-1-ville.syrjala@linux.intel.com>\n\t<20170920101123.23312-1-ville.syrjala@linux.intel.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6-1 ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1771790,"web_url":"http://patchwork.ozlabs.org/comment/1771790/","msgid":"<20170920121137.GZ4914@intel.com>","list_archive_url":null,"date":"2017-09-20T12:11:37","subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","submitter":{"id":21383,"url":"http://patchwork.ozlabs.org/api/people/21383/","name":"Ville Syrjälä","email":"ville.syrjala@linux.intel.com"},"content":"On Wed, Sep 20, 2017 at 12:39:24PM +0200, Johannes Berg wrote:\n> On Wed, 2017-09-20 at 13:11 +0300, Ville Syrjala wrote:\n> \n> > --- a/net/mac80211/tx.c\n> > +++ b/net/mac80211/tx.c\n> > @@ -1770,15 +1770,21 @@ bool ieee80211_tx_prepare_skb(struct ieee80211_hw *hw,\n> >  \tstruct ieee80211_tx_data tx;\n> >  \tstruct sk_buff *skb2;\n> >  \n> > -\tif (ieee80211_tx_prepare(sdata, &tx, NULL, skb) == TX_DROP)\n> > +\trcu_read_lock();\n> \n> The documentation says:\n> \n> /**\n>  * ieee80211_tx_prepare_skb - prepare an 802.11 skb for transmission\n>  * @hw: pointer as obtained from ieee80211_alloc_hw()\n>  * @vif: virtual interface\n>  * @skb: frame to be sent from within the driver\n>  * @band: the band to transmit on\n>  * @sta: optional pointer to get the station to send the frame to\n>  *\n>  * Note: must be called under RCU lock\n>  */\n> \n> You can't even argue that it should be the function itself doing it,\n> because the (admittedly optional) sta pointer would otherwise not have\n> proper protection after you leave the function ... You can't pass out a\n> sta pointer that's RCU protected.\n\nYeah, I suppose that would need rcu_handoff+some other mechanism to\nmake sure it stays around after that.\n\n> \n> Side note: Perhaps some annotation should be there? not sure it's\n> possible - would have to be something like\n> \tstruct ieee80211_sta * __rcu *sta;\n> \n> I guess since the outer pointer isn't protected, only the inner ...\n\nI think just the fact that even the pointers in ieee80211_tx_data don't\nhave the __rcu annotation makes it rather hard to see what is really rcu\nprotected and what isn't. If every user of those pointers would have to\ndo the rcu_dereference() things would be rather more explicit.\n\n> Therefore, this patch is wrong.\n\nOK, so the problem is in ath9k then.\n\n> I actually think the same is true for ieee80211_tx_dequeue(), but I'm\n> less sure about it - the sta pointer there clearly is somehow safely\n> passed in (even if it's w/o RCU, the driver can potentially make that\n> safe), but the key pointer seems unsafe in this case (as well) if\n> there's no outer RCU protection.\n\nWell, I think this is as far as I want to dig into the matter. I can\nrespin the patch once more with just tx_dequeue() fix in there, if you\nwant (not sure you do if you think it's wrong as well). After that I'll\nleave it to someone who actually knows what they're doing with mac80211 ;)","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxzBB05mZz9t2Q\n\tfor ;\n\tWed, 20 Sep 2017 22:11:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751828AbdITMLm (ORCPT );\n\tWed, 20 Sep 2017 08:11:42 -0400","from mga05.intel.com ([192.55.52.43]:24532 \"EHLO mga05.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751660AbdITMLl (ORCPT );\n\tWed, 20 Sep 2017 08:11:41 -0400","from orsmga001.jf.intel.com ([10.7.209.18])\n\tby fmsmga105.fm.intel.com with ESMTP; 20 Sep 2017 05:11:40 -0700","from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174])\n\tby orsmga001.jf.intel.com with SMTP; 20 Sep 2017 05:11:38 -0700","by stinkbox (sSMTP sendmail emulation);\n\tWed, 20 Sep 2017 15:11:37 +0300"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,421,1500966000\"; d=\"scan'208\";a=\"1174117632\"","Date":"Wed, 20 Sep 2017 15:11:37 +0300","From":"Ville =?iso-8859-1?q?Syrj=E4l=E4?= ","To":"Johannes Berg ","Cc":"linux-wireless@vger.kernel.org,\n\t\"David S. Miller\" , netdev@vger.kernel.org","Subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","Message-ID":"<20170920121137.GZ4914@intel.com>","References":"<20170918195919.15860-1-ville.syrjala@linux.intel.com>\n\t<20170920101123.23312-1-ville.syrjala@linux.intel.com>\n\t<1505903964.3026.14.camel@sipsolutions.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1505903964.3026.14.camel@sipsolutions.net>","User-Agent":"Mutt/1.7.2 (2016-11-26)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1771792,"web_url":"http://patchwork.ozlabs.org/comment/1771792/","msgid":"<1505909866.3026.16.camel@sipsolutions.net>","list_archive_url":null,"date":"2017-09-20T12:17:46","subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","submitter":{"id":265,"url":"http://patchwork.ozlabs.org/api/people/265/","name":"Johannes Berg","email":"johannes@sipsolutions.net"},"content":"On Wed, 2017-09-20 at 15:11 +0300, Ville Syrjälä wrote:\n> \n> > I guess since the outer pointer isn't protected, only the inner ...\n> \n> I think just the fact that even the pointers in ieee80211_tx_data\n> don't have the __rcu annotation makes it rather hard to see what is\n> really rcu protected and what isn't. If every user of those pointers\n> would have to do the rcu_dereference() things would be rather more\n> explicit.\n\nIt wouldn't make sense though, because those users don't need to\nprovide the protection, and they don't need to make sure to use the\npointer in an RCU safe manner (access once etc.) since they're in code\nthat can't really go wrong... mostly.\n\n> > Therefore, this patch is wrong.\n> \n> OK, so the problem is in ath9k then.\n\nI agree.\n\n> > I actually think the same is true for ieee80211_tx_dequeue(), but \n[...]\n> Well, I think this is as far as I want to dig into the matter. I can\n> respin the patch once more with just tx_dequeue() fix in there, if\n> you want (not sure you do if you think it's wrong as well). After\n> that I'll leave it to someone who actually knows what they're doing\n> with mac80211 ;)\n\n:-)\nI think we should rather document that RCU is required for that\nfunction, I think for some usages it may be OK without but with keys\nI'm pretty sure you'll need it.\n\njohannes","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxzKH2rKXz9t2Q\n\tfor ;\n\tWed, 20 Sep 2017 22:17:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751845AbdITMRw (ORCPT );\n\tWed, 20 Sep 2017 08:17:52 -0400","from s3.sipsolutions.net ([5.9.151.49]:49686 \"EHLO\n\tsipsolutions.net\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751626AbdITMRv (ORCPT );\n\tWed, 20 Sep 2017 08:17:51 -0400","by sipsolutions.net with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89)\n\t(envelope-from )\n\tid 1dudwo-0004IM-Th; Wed, 20 Sep 2017 14:17:46 +0200"],"Message-ID":"<1505909866.3026.16.camel@sipsolutions.net>","Subject":"Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections","From":"Johannes Berg ","To":"Ville =?iso-8859-1?q?Syrj=E4l=E4?= ","Cc":"linux-wireless@vger.kernel.org,\n\t\"David S. Miller\" , netdev@vger.kernel.org","Date":"Wed, 20 Sep 2017 14:17:46 +0200","In-Reply-To":"<20170920121137.GZ4914@intel.com>","References":"<20170918195919.15860-1-ville.syrjala@linux.intel.com>\n\t<20170920101123.23312-1-ville.syrjala@linux.intel.com>\n\t<1505903964.3026.14.camel@sipsolutions.net>\n\t<20170920121137.GZ4914@intel.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6-1 ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]