Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933543AbaDVR07 (ORCPT ); Tue, 22 Apr 2014 13:26:59 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:53822 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932873AbaDVR04 (ORCPT ); Tue, 22 Apr 2014 13:26:56 -0400 Message-ID: <1398187596.7767.60.camel@deadeye.wl.decadent.org.uk> Subject: Re: [PATCH 1/1] net: Add rtnl_lock for netif_device_attach/detach From: Ben Hutchings To: "Li, Zhen-Hua" Cc: "David S. Miller" , Eric Dumazet , Veaceslav Falico , Nicolas Dichtel , Jiri Pirko , stephen hemminger , Jerry Chu , Sathya Perla , Subbu Seetharaman , Ajit Khaparde , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 22 Apr 2014 18:26:36 +0100 In-Reply-To: <1397632082-18453-1-git-send-email-zhen-hual@hp.com> References: <1397632082-18453-1-git-send-email-zhen-hual@hp.com> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-Se94WuL3tJsfIamJns3f" X-Mailer: Evolution 3.8.5-2+b3 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 192.168.4.249 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-Se94WuL3tJsfIamJns3f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-04-16 at 15:08 +0800, Li, Zhen-Hua wrote: > From: "Li, Zhen-Hua" >=20 > As netif_running is called in netif_device_attach/detach. There should be > rtnl_lock/unlock called, to avoid dev stat change during netif_device_att= ach > and detach being called. > I checked NIC some drivers, some of them have netif_device_attach/detach > called between rtnl_lock/unlock, while some drivers do not. >=20 > This patch is tring to find a generic way to fix this for all NIC drivers= . I don't think you can generically use the RTNL lock for this. > Signed-off-by: Li, Zhen-Hua > --- > net/core/dev.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) >=20 > diff --git a/net/core/dev.c b/net/core/dev.c > index 5b3042e..795bbc5 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2190,10 +2190,19 @@ EXPORT_SYMBOL(__dev_kfree_skb_any); > */ > void netif_device_detach(struct net_device *dev) > { > + /** > + * As netif_running is called , rtnl_lock and unlock are needed to > + * avoid __LINK_STATE_START bit changes during this function call. > + */ > + int need_unlock; > + > + need_unlock =3D rtnl_trylock(); It is never correct to use trylock and then continue even if it fails. I think you're trying to simulate a reentrant mutex but this will fail if *any* task already has the mutex. Furthermore it is currently allowed and useful to call these functions from atomic context (transmit or completion path) where it is not possible to hold the mutex. > if (test_and_clear_bit(__LINK_STATE_PRESENT, &dev->state) && > netif_running(dev)) { > netif_tx_stop_all_queues(dev); > } > + if (need_unlock) > + rtnl_unlock(); > } > EXPORT_SYMBOL(netif_device_detach); For netif_device_detach(), I wonder whether it is necessary to check netif_running(). What are we trying to avoid? > @@ -2205,11 +2214,20 @@ EXPORT_SYMBOL(netif_device_detach); > */ > void netif_device_attach(struct net_device *dev) > { > + /** > + * As netif_running is called , rtnl_lock and unlock are needed t= o > + * avoid __LINK_STATE_START bit changes during this function call= . > + */ > + int need_unlock; > + > + need_unlock =3D rtnl_trylock(); > if (!test_and_set_bit(__LINK_STATE_PRESENT, &dev->state) && > netif_running(dev)) { > netif_tx_wake_all_queues(dev); > __netdev_watchdog_up(dev); > } > + if (need_unlock) > + rtnl_unlock(); > } > EXPORT_SYMBOL(netif_device_attach); I do see a problem if netif_device_detach() races with dev_deactivate_many(), which is being mitigated but not avoided by the test of netif_running(). I think a proper solution is going to involve changing dev_deactivate_many() as well, removing the use of netif_running(), and possible using cmpxchg() to atomically manipulate multiple bits of dev->state. Ben. --=20 Ben Hutchings Beware of programmers who carry screwdrivers. - Leonard Brandwein --=-Se94WuL3tJsfIamJns3f Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIVAwUAU1amTOe/yOyVhhEJAQrXrRAAqSYBXPHgTHzE6cxzl9I7n15xvysC3Bkd puhc8nFji00rY/RKt50QWQNpAKCbHby6UQl5OEYFIH6s9GLORlkCTUgmdKT/zv0i Fhvf6FcjW+AwhpKrZBp3Hjaej4Lrt70Ioeq1tEEe2D+MQKzBgZgjYJvRfdfhAYCw WGOz3kbuE71YRJOhMSLUbhWFXMb+f2SwGptDniFFxN7Eo7eWtejZXH11QDRB9di+ pOCMSrtCY/Ue/ljFhhJ2eoD4cYVqK0pRr6uxiXOsLRxZWAxRRzgGa9vt4UiD3f5e Oysd6PULOE3duAnXxN/cyRtguF1CJ/zK4gpkaoe/czBpX5Lr3oDGlIkz/kLrU5y6 k/v39pptUHlgxrXp8kX5ToxP0ItP735+hSXBM/+dSpny9LEBxHiWjLXX5tFMM1Qg ri80t57KASXi0+kvOtYFu9y0TPOZ44njI74sYHMegcl7u9b8AmOmMGGwxsq0RmYJ q3KGNDgulTYI0N1wTdSYc61DEyfNDhd88d2fC/bh+K2t8I+VHgq5WWDwnB3Dx731 E37PRl+7VDf5TtMaA8wDf7E4Sl5MR6fL6mSiVKgoY8mGLpgbvvhjWByHJRGtxXdj VK10iHYcwqn83TT1MIQNBd5d8XDMHPO50sh1AnQ5fc9HGzZ0hW4LhTx2sgA37179 2jJDlBkg1IE= =KXDx -----END PGP SIGNATURE----- --=-Se94WuL3tJsfIamJns3f-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/