From: Jiri Pirko <jpirko@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@elte.hu>, David Miller <davem@davemloft.net>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
torvalds@linux-foundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, eric.dumazet@gmail.com
Subject: Re: [PATCH] net: Fix spinlock use in alloc_netdev_mq()
Date: Thu, 6 Aug 2009 10:31:02 +0200 [thread overview]
Message-ID: <20090806083102.GA3737@psychotron.englab.brq.redhat.com> (raw)
In-Reply-To: <20090805101327.2c49cc62.akpm@linux-foundation.org>
Wed, Aug 05, 2009 at 07:13:27PM CEST, akpm@linux-foundation.org wrote:
>On Wed, 5 Aug 2009 10:47:47 +0200 Jiri Pirko <jpirko@redhat.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@redhat.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:
>
next prev parent reply other threads:[~2009-08-06 8:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-08-04 19:57 [GIT]: Networking David Miller
2009-08-05 7:02 ` Ingo Molnar
2009-08-05 7:14 ` Ingo Molnar
2009-08-05 7:16 ` [PATCH] net: Fix spinlock use in alloc_netdev_mq() Ingo Molnar
2009-08-05 8:47 ` Jiri Pirko
2009-08-05 15:37 ` David Miller
2009-08-05 17:13 ` Andrew Morton
2009-08-06 8:31 ` Jiri Pirko [this message]
2009-08-12 23:44 ` David Miller
2009-08-05 7:19 ` [GIT]: Networking Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090806083102.GA3737@psychotron.englab.brq.redhat.com \
--to=jpirko@redhat.com \
--cc=a.p.zijlstra@chello.nl \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=netdev@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).