netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: don't try to ops lock uninitialized devs
@ 2025-04-15 15:15 Jakub Kicinski
  2025-04-15 17:35 ` Stanislav Fomichev
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Jakub Kicinski @ 2025-04-15 15:15 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, Jakub Kicinski,
	syzbot+de1c7d68a10e3f123bdd, sdf, kuniyu

We need to be careful when operating on dev while in rtnl_create_link().
Some devices (vxlan) initialize netdev_ops in ->newlink, so later on.
Avoid using netdev_lock_ops(), the device isn't registered so we
cannot legally call its ops or generate any notifications for it.

netdev_ops_assert_locked_or_invisible() is safe to use, it checks
registration status first.

Reported-by: syzbot+de1c7d68a10e3f123bdd@syzkaller.appspotmail.com
Fixes: 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: sdf@fomichev.me
CC: kuniyu@amazon.com

I wasn't sure whether Kuniyuki is going to send this or he's waiting
for me to send.. so let me send and get this off my tracking list :)
---
 net/core/dev.c       | 2 ++
 net/core/rtnetlink.c | 5 +----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 03d20a98f8b7..c5e15701cfb3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1572,6 +1572,8 @@ EXPORT_SYMBOL(netdev_features_change);
 
 void netif_state_change(struct net_device *dev)
 {
+	netdev_ops_assert_locked_or_invisible(dev);
+
 	if (dev->flags & IFF_UP) {
 		struct netdev_notifier_change_info change_info = {
 			.info.dev = dev,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 38526210b8fd..bb624fc6ca8a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3677,11 +3677,8 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 				nla_len(tb[IFLA_BROADCAST]));
 	if (tb[IFLA_TXQLEN])
 		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
-	if (tb[IFLA_OPERSTATE]) {
-		netdev_lock_ops(dev);
+	if (tb[IFLA_OPERSTATE])
 		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
-		netdev_unlock_ops(dev);
-	}
 	if (tb[IFLA_LINKMODE])
 		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
 	if (tb[IFLA_GROUP])
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: don't try to ops lock uninitialized devs
  2025-04-15 15:15 [PATCH net] net: don't try to ops lock uninitialized devs Jakub Kicinski
@ 2025-04-15 17:35 ` Stanislav Fomichev
  2025-04-15 22:08 ` Kuniyuki Iwashima
  2025-04-17  1:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2025-04-15 17:35 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	syzbot+de1c7d68a10e3f123bdd, sdf, kuniyu

On 04/15, Jakub Kicinski wrote:
> We need to be careful when operating on dev while in rtnl_create_link().
> Some devices (vxlan) initialize netdev_ops in ->newlink, so later on.
> Avoid using netdev_lock_ops(), the device isn't registered so we
> cannot legally call its ops or generate any notifications for it.
> 
> netdev_ops_assert_locked_or_invisible() is safe to use, it checks
> registration status first.
> 
> Reported-by: syzbot+de1c7d68a10e3f123bdd@syzkaller.appspotmail.com
> Fixes: 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: don't try to ops lock uninitialized devs
  2025-04-15 15:15 [PATCH net] net: don't try to ops lock uninitialized devs Jakub Kicinski
  2025-04-15 17:35 ` Stanislav Fomichev
@ 2025-04-15 22:08 ` Kuniyuki Iwashima
  2025-04-17  1:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Kuniyuki Iwashima @ 2025-04-15 22:08 UTC (permalink / raw)
  To: kuba
  Cc: andrew+netdev, davem, edumazet, horms, kuniyu, netdev, pabeni,
	sdf, syzbot+de1c7d68a10e3f123bdd

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 15 Apr 2025 08:15:52 -0700
> We need to be careful when operating on dev while in rtnl_create_link().
> Some devices (vxlan) initialize netdev_ops in ->newlink, so later on.
> Avoid using netdev_lock_ops(), the device isn't registered so we
> cannot legally call its ops or generate any notifications for it.
> 
> netdev_ops_assert_locked_or_invisible() is safe to use, it checks
> registration status first.
> 
> Reported-by: syzbot+de1c7d68a10e3f123bdd@syzkaller.appspotmail.com
> Fixes: 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: sdf@fomichev.me
> CC: kuniyu@amazon.com
> 
> I wasn't sure whether Kuniyuki is going to send this or he's waiting
> for me to send.. so let me send and get this off my tracking list :)

wondering the same, thanks!

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: don't try to ops lock uninitialized devs
  2025-04-15 15:15 [PATCH net] net: don't try to ops lock uninitialized devs Jakub Kicinski
  2025-04-15 17:35 ` Stanislav Fomichev
  2025-04-15 22:08 ` Kuniyuki Iwashima
@ 2025-04-17  1:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-04-17  1:40 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	syzbot+de1c7d68a10e3f123bdd, sdf, kuniyu

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 15 Apr 2025 08:15:52 -0700 you wrote:
> We need to be careful when operating on dev while in rtnl_create_link().
> Some devices (vxlan) initialize netdev_ops in ->newlink, so later on.
> Avoid using netdev_lock_ops(), the device isn't registered so we
> cannot legally call its ops or generate any notifications for it.
> 
> netdev_ops_assert_locked_or_invisible() is safe to use, it checks
> registration status first.
> 
> [...]

Here is the summary with links:
  - [net] net: don't try to ops lock uninitialized devs
    https://git.kernel.org/netdev/net/c/4798cfa2097f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-04-17  1:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-15 15:15 [PATCH net] net: don't try to ops lock uninitialized devs Jakub Kicinski
2025-04-15 17:35 ` Stanislav Fomichev
2025-04-15 22:08 ` Kuniyuki Iwashima
2025-04-17  1:40 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).