Re: [PATCH net v2] ipmr: Fix access to mfc_cache_list without lock held

[David Ahern <dsahern@kernel.org>]

On 11/8/24 7:08 AM, Breno Leitao wrote:
> Accessing `mr_table->mfc_cache_list` is protected by an RCU lock. In the
> following code flow, the RCU read lock is not held, causing the
> following error when `RCU_PROVE` is not held. The same problem might
> show up in the IPv6 code path.
> 
> 	6.12.0-rc5-kbuilder-01145-gbac17284bdcb #33 Tainted: G            E    N
> 	-----------------------------
> 	net/ipv4/ipmr_base.c:313 RCU-list traversed in non-reader section!!
> 
> 	rcu_scheduler_active = 2, debug_locks = 1
> 		   2 locks held by RetransmitAggre/3519:
> 		    #0: ffff88816188c6c0 (nlk_cb_mutex-ROUTE){+.+.}-{3:3}, at: __netlink_dump_start+0x8a/0x290
> 		    #1: ffffffff83fcf7a8 (rtnl_mutex){+.+.}-{3:3}, at: rtnl_dumpit+0x6b/0x90
> 
> 	stack backtrace:
> 		    lockdep_rcu_suspicious
> 		    mr_table_dump
> 		    ipmr_rtm_dumproute
> 		    rtnl_dump_all
> 		    rtnl_dumpit
> 		    netlink_dump
> 		    __netlink_dump_start
> 		    rtnetlink_rcv_msg
> 		    netlink_rcv_skb
> 		    netlink_unicast
> 		    netlink_sendmsg
> 
> This is not a problem per see, since the RTNL lock is held here, so, it
> is safe to iterate in the list without the RCU read lock, as suggested
> by Eric.
> 
> To alleviate the concern, modify the code to use
> list_for_each_entry_rcu() with the RTNL-held argument.
> 
> The annotation will raise an error only if RTNL or RCU read lock are
> missing during iteration, signaling a legitimate problem, otherwise it
> will avoid this false positive.
> 
> This will solve the IPv6 case as well, since ip6mr_rtm_dumproute() calls
> this function as well.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changes in v2:
> - Instead of getting an RCU read lock, rely on rtnl mutex (Eric)
> - Link to v1: https://lore.kernel.org/r/20241107-ipmr_rcu-v1-1-ad0cba8dffed@debian.org
> - Still sending it against `net`, so, since this warning is annoying
> ---
>  net/ipv4/ipmr_base.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>