netdev - Re: [PATCH net-next 2/3] udp_tunnel: remove rtnl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aC4FK0fmUoaXYt4k@mini-arch>
Date: Wed, 21 May 2025 09:54:03 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, skalluru@...vell.com, manishc@...vell.com,
	andrew+netdev@...n.ch, michael.chan@...adcom.com,
	pavan.chebbi@...adcom.com, ajit.khaparde@...adcom.com,
	sriharsha.basavapatna@...adcom.com, somnath.kotur@...adcom.com,
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
	tariqt@...dia.com, saeedm@...dia.com, louis.peens@...igine.com,
	shshaikh@...vell.com, GR-Linux-NIC-Dev@...vell.com,
	ecree.xilinx@...il.com, horms@...nel.org, dsahern@...nel.org,
	ruanjinjie@...wei.com, mheib@...hat.com,
	linux-kernel@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
	linux-rdma@...r.kernel.org, oss-drivers@...igine.com,
	linux-net-drivers@....com, leon@...nel.org
Subject: Re: [PATCH net-next 2/3] udp_tunnel: remove rtnl_lock dependency

On 05/21, Jakub Kicinski wrote:
> On Tue, 20 May 2025 13:36:13 -0700 Stanislav Fomichev wrote:
> > Drivers that are using ops lock and don't depend on RTNL lock
> > still need to manage it because udp_tunnel's RTNL dependency.
> > Introduce new udp_tunnel_nic_lock and use it instead of
> > rtnl_lock. Drop non-UDP_TUNNEL_NIC_INFO_MAY_SLEEP mode from
> > udp_tunnel infra (udp_tunnel_nic_device_sync_work needs to
> > grab udp_tunnel_nic_lock mutex and might sleep).
> 
> There is a netdevsim-based test for this that needs to be fixed up.

Oh, I did not see that one, let me try to find and run it.

> > diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> > index 2df3b8344eb5..7f5537fdf2c9 100644
> > --- a/include/net/udp_tunnel.h
> > +++ b/include/net/udp_tunnel.h
> > @@ -221,19 +221,17 @@ static inline void udp_tunnel_encap_enable(struct sock *sk)
> >  #define UDP_TUNNEL_NIC_MAX_TABLES	4
> >  
> >  enum udp_tunnel_nic_info_flags {
> > -	/* Device callbacks may sleep */
> > -	UDP_TUNNEL_NIC_INFO_MAY_SLEEP	= BIT(0),
> 
> Could we use a different lock for sleeping and non-sleeping drivers?

We can probably do it if we reorder the locks (as you ask/suggest
below). Overall, I'm not sure I understand why we want to have two
paths here. If we can do everything via work queue, why have a separate
path for the non-sleepable callback? (more code -> more bugs)

> > @@ -554,11 +543,11 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev)
> >  	struct udp_tunnel_nic *utn;
> >  	unsigned int i, j;
> >  
> > -	ASSERT_RTNL();
> > +	mutex_lock(&udp_tunnel_nic_lock);
> >  
> >  	utn = dev->udp_tunnel_nic;
> 
> utn and info's lifetimes are tied to the lifetime of the device
> I think their existence can remain protected by the external locks

SG, will move the lock down a bit.

> >  	if (!utn)
> > -		return;
> > +		goto unlock;
> >  
> >  	utn->need_sync = false;
> >  	for (i = 0; i < utn->n_tables; i++)
> 
> > -	rtnl_lock();
> > +	mutex_lock(&udp_tunnel_nic_lock);
> >  	utn->work_pending = 0;
> >  	__udp_tunnel_nic_device_sync(utn->dev, utn);
> >  
> > -	if (utn->need_replay)
> > +	if (utn->need_replay) {
> > +		rtnl_lock();
> >  		udp_tunnel_nic_replay(utn->dev, utn);
> > -	rtnl_unlock();
> > +		rtnl_unlock();
> > +	}
> > +	mutex_unlock(&udp_tunnel_nic_lock);
> >  }
> 
> What's the lock ordering between the new lock and rtnl lock?

>From ops-locked, we'll get: ops->tunnel_lock (__udp_tunnel_nic_reset_ntf)
>From non-ops locked, we'll get: rtnl->tunnel_lock

I see your point, we need to do rtnl->tunnel_lock here as well.

> BTW the lock could live in utn, right? We can't use the instance
> lock because of sharing, but we could put the lock in utn?

I was thinking that there is some global state besides udp_tunnel_nic,
but I don't see any, will move the lock, thanks!