netdev - Re: [PATCH] net: Fix spinlock use in alloc_netdev

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]

Date:	Thu, 6 Aug 2009 10:31:02 +0200
From:	Jiri Pirko <jpirko@...hat.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Ingo Molnar <mingo@...e.hu>, David Miller <davem@...emloft.net>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	torvalds@...ux-foundation.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH] net: Fix spinlock use in alloc_netdev_mq()

Wed, Aug 05, 2009 at 07:13:27PM CEST, akpm@...ux-foundation.org wrote:
>On Wed, 5 Aug 2009 10:47:47 +0200 Jiri Pirko <jpirko@...hat.com> wrote:
>
>> >it's using an zero-initialized spinlock. This is a side-effect of:
>> >
>> >        dev_unicast_init(dev);
>> >
>> >in alloc_netdev_mq() making use of dev->addr_list_lock.
>> >
>> >The device has just been allocated freshly, it's not accessible
>> >anywhere yet so no locking is needed at all - in fact it's wrong
>> >to lock it here (the lock isnt initialized yet).
>> 
>> Yes this looks like the right approach. Sorry for this bug :(
>
>Really?
>
>> >--- a/net/core/dev.c
>> >+++ b/net/core/dev.c
>> >@@ -4007,9 +4007,7 @@ static void dev_unicast_flush(struct net_device *dev)
>> > 
>> > static void dev_unicast_init(struct net_device *dev)
>> > {
>> >-	netif_addr_lock_bh(dev);
>> > 	__hw_addr_init(&dev->uc);
>> >-	netif_addr_unlock_bh(dev);
>> > }
>
>This means that the net_device is still floating around for quite a
>long time with an uninitialised spinlock, so it will be quite easy for
>the same problem to reoccur as the code evolves.
>
>It would be more robust were we to initialise that lock close to the
>netdev's allocation site.

Hmm, I see your point here. Eric previously posted patch which moved spin lock
init into alloc_netdev_mq(). But he was worried about having it here and
netdev_set_addr_lockdep_class() in register_netdevice() (because before
dev_unicast_init() dev->type is not set). So how about the following patch?

[PATCH net-2.6] net: move address lists spinlock closer to alloc and do unicast_init locking

Move spin_lock_init(), netdev_set_addr_lockdep_class() and dev_unicast_init()
right after setup is called from alloc_netdev_mq(). In that moment dev->type is
initialized. List is not needed to be initialized earlier. Also restore
previously removed locking in dev_unicast_init().

Signed-off-by: Jiri Pirko <jpirko@...hat.com>

diff --git a/net/core/dev.c b/net/core/dev.c
index 6a94475..916a6d0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4007,7 +4007,9 @@ static void dev_unicast_flush(struct net_device *dev)
 
 static void dev_unicast_init(struct net_device *dev)
 {
+	netif_addr_lock_bh(dev);
 	__hw_addr_init(&dev->uc);
+	netif_addr_unlock_bh(dev);
 }
 
 
@@ -4726,8 +4728,6 @@ int register_netdevice(struct net_device *dev)
 	BUG_ON(dev->reg_state != NETREG_UNINITIALIZED);
 	BUG_ON(!net);
 
-	spin_lock_init(&dev->addr_list_lock);
-	netdev_set_addr_lockdep_class(dev);
 	netdev_init_queue_locks(dev);
 
 	dev->iflink = -1;
@@ -5107,8 +5107,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	if (dev_addr_init(dev))
 		goto free_tx;
 
-	dev_unicast_init(dev);
-
 	dev_net_set(dev, &init_net);
 
 	dev->_tx = tx;
@@ -5123,6 +5121,11 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	dev->priv_flags = IFF_XMIT_DST_RELEASE;
 	setup(dev);
 	strcpy(dev->name, name);
+
+	spin_lock_init(&dev->addr_list_lock);
+	netdev_set_addr_lockdep_class(dev);
+	dev_unicast_init(dev);
+
 	return dev;
 
 free_tx:
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html