Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[Ying Xue <ying.xue@windriver.com>]

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