[PATCH -mmotm] xfrm4: fix build when SYSCTLs are disabled

[PATCH -mmotm] xfrm4: fix build when SYSCTLs are disabled

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Fix build errors when SYSCTLs are not enabled:
(.init.text+0x5154): undefined reference to `net_ipv4_ctl_path'
(.init.text+0x5176): undefined reference to `register_net_sysctl_table'
xfrm4_policy.c:(.exit.text+0x573): undefined reference to `unregister_net_sysctl_table

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 net/ipv4/xfrm4_policy.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- mmotm-2009-0804-1422.orig/net/ipv4/xfrm4_policy.c
+++ mmotm-2009-0804-1422/net/ipv4/xfrm4_policy.c
@@ -264,6 +264,7 @@ static struct xfrm_policy_afinfo xfrm4_p
 	.fill_dst =		xfrm4_fill_dst,
 };
 
+#ifdef CONFIG_SYSCTL
 static struct ctl_table xfrm4_policy_table[] = {
 	{
 		.ctl_name       = CTL_UNNUMBERED,
@@ -277,6 +278,7 @@ static struct ctl_table xfrm4_policy_tab
 };
 
 static struct ctl_table_header *sysctl_hdr;
+#endif
 
 static void __init xfrm4_policy_init(void)
 {
@@ -285,8 +287,10 @@ static void __init xfrm4_policy_init(voi
 
 static void __exit xfrm4_policy_fini(void)
 {
+#ifdef CONFIG_SYSCTL
 	if (sysctl_hdr)
 		unregister_net_sysctl_table(sysctl_hdr);
+#endif
 	xfrm_policy_unregister_afinfo(&xfrm4_policy_afinfo);
 }
 
@@ -305,7 +309,9 @@ void __init xfrm4_init(int rt_max_size)
 	 * and start cleaning when were 1/2 full
 	 */
 	xfrm4_dst_ops.gc_thresh = rt_max_size/2;
+#ifdef CONFIG_SYSCTL
 	sysctl_hdr = register_net_sysctl_table(&init_net, net_ipv4_ctl_path,
 						xfrm4_policy_table);
+#endif
 }
 



---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH -mmotm] xfrm4: fix build when SYSCTLs are disabled

[David Miller]

From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Tue, 4 Aug 2009 18:54:30 -0700

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix build errors when SYSCTLs are not enabled:
> (.init.text+0x5154): undefined reference to `net_ipv4_ctl_path'
> (.init.text+0x5176): undefined reference to `register_net_sysctl_table'
> xfrm4_policy.c:(.exit.text+0x573): undefined reference to `unregister_net_sysctl_table
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>

Applied, thanks Randy.

The ipv6 side has the same issue, I'll fix that part up myself.

Re: mmotm 2009-08-04-14-22 uploaded

[Andrew Morton]

On Wed, 5 Aug 2009 14:39:46 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:

> Hi andrew,
> 
> I see following lockdep warning with this release:
> 
> [    0.474144] INFO: trying to register non-static key.
> [    0.474144] the code is fine but needs lockdep annotation.
> [    0.474144] turning off the locking correctness validator.
> [    0.474144] Pid: 1, comm: swapper Not tainted 2.6.31-rc5-mm1 #7
> [    0.474144] Call Trace:
> [    0.474144]  [<c1047f1e>] register_lock_class+0x58/0x241
> [    0.474144]  [<c1049ab1>] __lock_acquire+0xac/0xb73
> [    0.474144]  [<c1076eb5>] ? __alloc_pages_nodemask+0xe2/0x483
> [    0.474144]  [<c1048b64>] ? mark_lock+0x1e/0x1c7
> [    0.474144]  [<c1048b64>] ? mark_lock+0x1e/0x1c7
> [    0.474144]  [<c1048d50>] ? mark_held_locks+0x43/0x5b
> [    0.474144]  [<c10940a6>] ? kmem_cache_alloc+0xac/0x11b
> [    0.474144]  [<c104a615>] lock_acquire+0x9d/0xc0
> [    0.474144]  [<c12b8b96>] ? netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c1330feb>] _spin_lock_bh+0x20/0x2f
> [    0.474144]  [<c12b8b96>] ? netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c12b8b96>] netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c12bc3c3>] alloc_netdev_mq+0xf9/0x1a5
> [    0.474144]  [<c121f016>] ? loopback_setup+0x0/0x74
> [    0.474144]  [<c1578d49>] loopback_net_init+0x20/0x5d
> [    0.474144]  [<c12b7907>] register_pernet_operations+0x13/0x15
> [    0.474144]  [<c12b7970>] register_pernet_device+0x1f/0x47
> [    0.474144]  [<c157ee8d>] net_dev_init+0xfe/0x14d
> [    0.474144]  [<c1001137>] do_one_initcall+0x4a/0x11a
> [    0.474144]  [<c157ed8f>] ? net_dev_init+0x0/0x14d
> [    0.474144]  [<c1067e00>] ? register_irq_proc+0x64/0xa8
> [    0.474144]  [<c1067e97>] ? init_irq_proc+0x53/0x60
> [    0.474144]  [<c1557535>] kernel_init+0x129/0x17a
> [    0.474144]  [<c155740c>] ? kernel_init+0x0/0x17a
> [    0.474144]  [<c1003d47>] kernel_thread_helper+0x7/0x10

At a guess I'd say that alloc_netdev_mq()->dev_unicast_init() is doing
netif_addr_lock_bh()->spin_lock_bh(&dev->addr_list_lock) prior to
initialising add_list_lock.

Something like this might shut it up:

--- a/net/core/dev.c~a
+++ a/net/core/dev.c
@@ -5111,7 +5111,7 @@ struct net_device *alloc_netdev_mq(int s
 	if (dev_addr_init(dev))
 		goto free_tx;
 
-	dev_unicast_init(dev);
+	__hw_addr_init(&dev->uc);
 
 	dev_net_set(dev, &init_net);
 

but it'd be better to intialise this thing earlier like:

--- a/net/core/dev.c~a
+++ a/net/core/dev.c
@@ -4730,8 +4730,6 @@ int register_netdevice(struct net_device
 	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,6 +5105,8 @@ struct net_device *alloc_netdev_mq(int s
 
 	dev = PTR_ALIGN(p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
+	spin_lock_init(&dev->addr_list_lock);
+	netdev_set_addr_lockdep_class(dev);
 
 	if (dev_addr_init(dev))
 		goto free_tx;
_

but that might break register_netdevice() for netdevs which were
allocated via other means, dunno.

I would be pointing fingers at

: commit 31278e71471399beaff9280737e52b47db4dc345
: Author:     Jiri Pirko <jpirko@redhat.com>
: AuthorDate: Wed Jun 17 01:12:19 2009 +0000
: Commit:     David S. Miller <davem@davemloft.net>
: CommitDate: Thu Jun 18 00:29:08 2009 -0700
: 
:     net: group address list and its count
    
and politely suggesting that net developers enable lockdep when testing :)

Re: mmotm 2009-08-04-14-22 uploaded

[Eric Dumazet]

Dave Young a écrit :
> Hi andrew,
> 
> I see following lockdep warning with this release:
> 
> [    0.474144] INFO: trying to register non-static key.
> [    0.474144] the code is fine but needs lockdep annotation.
> [    0.474144] turning off the locking correctness validator.
> [    0.474144] Pid: 1, comm: swapper Not tainted 2.6.31-rc5-mm1 #7
> [    0.474144] Call Trace:
> [    0.474144]  [<c1047f1e>] register_lock_class+0x58/0x241
> [    0.474144]  [<c1049ab1>] __lock_acquire+0xac/0xb73
> [    0.474144]  [<c1076eb5>] ? __alloc_pages_nodemask+0xe2/0x483
> [    0.474144]  [<c1048b64>] ? mark_lock+0x1e/0x1c7
> [    0.474144]  [<c1048b64>] ? mark_lock+0x1e/0x1c7
> [    0.474144]  [<c1048d50>] ? mark_held_locks+0x43/0x5b
> [    0.474144]  [<c10940a6>] ? kmem_cache_alloc+0xac/0x11b
> [    0.474144]  [<c104a615>] lock_acquire+0x9d/0xc0
> [    0.474144]  [<c12b8b96>] ? netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c1330feb>] _spin_lock_bh+0x20/0x2f
> [    0.474144]  [<c12b8b96>] ? netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c12b8b96>] netif_addr_lock_bh+0xd/0xf
> [    0.474144]  [<c12bc3c3>] alloc_netdev_mq+0xf9/0x1a5
> [    0.474144]  [<c121f016>] ? loopback_setup+0x0/0x74
> [    0.474144]  [<c1578d49>] loopback_net_init+0x20/0x5d
> [    0.474144]  [<c12b7907>] register_pernet_operations+0x13/0x15
> [    0.474144]  [<c12b7970>] register_pernet_device+0x1f/0x47
> [    0.474144]  [<c157ee8d>] net_dev_init+0xfe/0x14d
> [    0.474144]  [<c1001137>] do_one_initcall+0x4a/0x11a
> [    0.474144]  [<c157ed8f>] ? net_dev_init+0x0/0x14d
> [    0.474144]  [<c1067e00>] ? register_irq_proc+0x64/0xa8
> [    0.474144]  [<c1067e97>] ? init_irq_proc+0x53/0x60
> [    0.474144]  [<c1557535>] kernel_init+0x129/0x17a
> [    0.474144]  [<c155740c>] ? kernel_init+0x0/0x17a
> [    0.474144]  [<c1003d47>] kernel_thread_helper+0x7/0x10
> 
> --

Hmm, it seems addr_list_lock is not initialized at the right place...

commit a6ac65db addded a netif_addr_lock_bh() in dev_unicast_init()

We initialize dev->addr_list_lock in register_netdevice(), we should
init it earlier, right after allocation and before dev_unicast_init()

But dev->type being 0, we probably cannot call netdev_set_addr_lockdep_class()
at this point...

David, what do you think ? Is it safe to call netdev_set_addr_lockdep_class()
in register_netdevice(), after lock being used one time in dev_unicast_init() ?

Thank you

[PATCH] net: Init dev->addr_list_lock in alloc_netdev_mq()

We initialize dev->addr_list_lock in register_netdevice(), we should
init it earlier, right after allocation and before dev_unicast_init()


Reported-by: Dave Young <hidave.darkstar@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/dev.c b/net/core/dev.c
index 43e61ba..e50356b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4728,7 +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);
 
@@ -5106,6 +5105,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	dev = PTR_ALIGN(p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
 
+	spin_lock_init(&dev->addr_list_lock);
 	if (dev_addr_init(dev))
 		goto free_tx;