From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ying Xue Subject: Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties Date: Thu, 4 Jan 2018 15:30:52 +0800 Message-ID: References: <1514802241-7896-1-git-send-email-ying.xue@windriver.com> <20180103.104850.406238717375090795.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, syzkaller-bugs@googlegroups.com, tipc-discussion@lists.sourceforge.net To: David Miller Return-path: In-Reply-To: <20180103.104850.406238717375090795.davem@davemloft.net> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tipc-discussion-bounces@lists.sourceforge.net List-Id: netdev.vger.kernel.org On 01/03/2018 11:48 PM, David Miller wrote: > As soon as you drop the RTNL lock, the media or bearer entry can be > removed from the tables. > Thanks for the review. Yes, you are right. But even if we temporarily release RTNL lock, it's still safe for us because when we set media/bearer properties in __tipc_nl_compat_doit(), tipc_nl_media_set() and tipc_nl_bearer_set() will probe media or bearer again within RTNL lock protection. > This invalidates what you do next, whether it's > tipc_nl_compat_media_set(), tipc_nl_compat_bearer_set(), etc. In fact tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set() don't really change media or bearer's properties, instead they only format the contents pointed by their "msg" parameter. > > Therefore, you have to lock down the tipc configuration state around > this entire operation, from media/bearer probe to the building of the > netlink message(s). > Sorry, we cannot hold RTNL lock in the entire operation path because TIPC now supports two different sets of netlink APIs: One set of API's execution path: genl_family_rcv_msg() tipc_nl_media_set() rtnl_lock() tipc_media_find() //set media properties genl_family_rcv_msg() tipc_nl_bearer_set() rtnl_lock() tipc_bearer_find() //set bearer properties Another set of API's execution path: genl_family_rcv_msg() tipc_nl_compat_recv() tipc_nl_compat_handle net() __tipc_nl_compat_doit net() tipc_nl_compat_link_set() tipc_nl_media_set() genl_family_rcv_msg() tipc_nl_compat_recv() tipc_nl_compat_handle net() __tipc_nl_compat_doit net() tipc_nl_compat_link_set() tipc_nl_bearer_set() As we see in above call chains, tipc_nl_media_set() and tipc_nl_bearer_set() are shared by the two sets of netlink APIs. If we hold RTNL lock from tipc_nl_compat_recv(), it means we cannot directly call tipc_nl_media_set() or tipc_nl_bearer_set() in __tipc_nl_compat_doit net(). > Either this entire code path must execute with the bearer/media entry > present, or without. If you drop the RTNL mutex in the middle, this > invariant is not held. ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot