possible circular locking

Hi,

When I killall kissattach I can see the following message.

This happens on kernel 2.6.24-rc5 already patched with the 6 previously patches I sent recently.

=======================================================
[ INFO: possible circular locking dependency detected ] 2.6.23.9 #1
-------------------------------------------------------
kissattach/2906 is trying to acquire lock:
(linkfail_lock){-+..}, at: [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]

but task is already holding lock:
(ax25_list_lock){-+..}, at: [<d8bd7c7c>] ax25_device_event+0x38/0x84
[ax25]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (ax25_list_lock){-+..}:

       [<c0130897>] __lock_acquire+0x9e9/0xbe6
       [<d8bd845c>] ax25_find_cb+0x18/0xc6 [ax25]
       [<c0130b02>] lock_acquire+0x6e/0x87
       [<d8bd845c>] ax25_find_cb+0x18/0xc6 [ax25]
       [<c02a399b>] _spin_lock_bh+0x2e/0x39
       [<d8bd845c>] ax25_find_cb+0x18/0xc6 [ax25]
       [<d8bd845c>] ax25_find_cb+0x18/0xc6 [ax25]
       [<d8bd5d57>] ax25_send_frame+0x40/0x131 [ax25]
       [<d8bed51a>] rose_send_frame+0x4a/0x5b [rose]
       [<d8bed946>] rose_link_rx_restart+0x135/0x157 [rose]
       [<c02a399b>] _spin_lock_bh+0x2e/0x39
       [<d8bee56a>] rose_route_frame+0xad/0x4f3 [rose]
       [<c0105215>] dump_trace+0x81/0x8b
       [<c012dea3>] save_trace+0x37/0x8c
       [<c012f73c>] mark_lock+0x337/0x44b
       [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
       [<d8bd471e>] ax25_protocol_function+0x30/0x34 [ax25]
       [<d8bd46fb>] ax25_protocol_function+0xd/0x34 [ax25]
       [<d8bd5271>] ax25_rx_iframe+0x2e3/0x332 [ax25]
       [<c011f839>] __mod_timer+0x89/0x93
       [<d8bd6b95>] ax25_std_frame_in+0x5b1/0x638 [ax25]
       [<d8bd4c49>] ax25_kiss_rcv+0x3cd/0x712 [ax25]
       [<c012f889>] mark_held_locks+0x39/0x53
       [<c02a3d2a>] _spin_unlock_irqrestore+0x34/0x39
       [<c024a79b>] sock_queue_rcv_skb+0xd6/0xf3
       [<c02a3879>] _read_unlock+0x14/0x1c
       [<c024a79b>] sock_queue_rcv_skb+0xd6/0xf3
       [<c025033c>] netif_receive_skb+0x22d/0x289
       [<c012fa60>] trace_hardirqs_on+0x109/0x148
       [<c02521ff>] process_backlog+0x7b/0xeb
       [<c02522c6>] net_rx_action+0x57/0xfd
       [<c011c52d>] __do_softirq+0x40/0x90
       [<c011c5a4>] do_softirq+0x27/0x3d
       [<c0106768>] do_IRQ+0x58/0x6c
       [<c0104cee>] common_interrupt+0x2e/0x40
       [<ffffffff>] 0xffffffff

-> #2 (rose_route_list_lock){-+..}:

       [<c0130897>] __lock_acquire+0x9e9/0xbe6
       [<d8bee50a>] rose_route_frame+0x4d/0x4f3 [rose]
       [<c0130b02>] lock_acquire+0x6e/0x87
       [<d8bee50a>] rose_route_frame+0x4d/0x4f3 [rose]
       [<c02a399b>] _spin_lock_bh+0x2e/0x39
       [<d8bee50a>] rose_route_frame+0x4d/0x4f3 [rose]
       [<d8bee50a>] rose_route_frame+0x4d/0x4f3 [rose]
       [<c0105215>] dump_trace+0x81/0x8b
       [<c012dea3>] save_trace+0x37/0x8c
       [<c012f73c>] mark_lock+0x337/0x44b
       [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
       [<d8bd471e>] ax25_protocol_function+0x30/0x34 [ax25]
       [<d8bd46fb>] ax25_protocol_function+0xd/0x34 [ax25]
       [<d8bd5271>] ax25_rx_iframe+0x2e3/0x332 [ax25]
       [<c011f839>] __mod_timer+0x89/0x93
       [<d8bd6b95>] ax25_std_frame_in+0x5b1/0x638 [ax25]
       [<d8bd4c49>] ax25_kiss_rcv+0x3cd/0x712 [ax25]
       [<c012f889>] mark_held_locks+0x39/0x53
       [<c02a3d2a>] _spin_unlock_irqrestore+0x34/0x39
       [<c024a79b>] sock_queue_rcv_skb+0xd6/0xf3
       [<c02a3879>] _read_unlock+0x14/0x1c
       [<c024a79b>] sock_queue_rcv_skb+0xd6/0xf3
       [<c025033c>] netif_receive_skb+0x22d/0x289
       [<c012fa60>] trace_hardirqs_on+0x109/0x148
       [<c02521ff>] process_backlog+0x7b/0xeb
       [<c02522c6>] net_rx_action+0x57/0xfd
       [<c011c52d>] __do_softirq+0x40/0x90
       [<c011c5a4>] do_softirq+0x27/0x3d
       [<c0106768>] do_IRQ+0x58/0x6c
       [<c0104cee>] common_interrupt+0x2e/0x40
       [<ffffffff>] 0xffffffff

-> #1 (rose_neigh_list_lock){-+..}:

       [<c0130897>] __lock_acquire+0x9e9/0xbe6
       [<d8bee31e>] rose_link_failed+0xe/0x44 [rose]
       [<c0130b02>] lock_acquire+0x6e/0x87
       [<d8bee31e>] rose_link_failed+0xe/0x44 [rose]
       [<d8bd7783>] ax25_t1timer_expiry+0x0/0x20 [ax25]
       [<c02a399b>] _spin_lock_bh+0x2e/0x39
       [<d8bee31e>] rose_link_failed+0xe/0x44 [rose]
       [<d8bee31e>] rose_link_failed+0xe/0x44 [rose]
       [<d8bd461a>] ax25_link_failed+0x28/0x39 [ax25]
       [<d8bd7300>] ax25_disconnect+0x34/0xbe [ax25]
       [<c011f4f3>] run_timer_softirq+0xee/0x14a
       [<c011c51e>] __do_softirq+0x31/0x90
       [<c012fa60>] trace_hardirqs_on+0x109/0x148
       [<c011c52d>] __do_softirq+0x40/0x90
       [<c011c5a4>] do_softirq+0x27/0x3d
       [<c0106768>] do_IRQ+0x58/0x6c
       [<c0104cee>] common_interrupt+0x2e/0x40
       [<d8a9163f>] acpi_processor_idle+0x262/0x3cf [processor]
       [<c0102342>] cpu_idle+0x3c/0x51
       [<c0382a0c>] start_kernel+0x272/0x277
       [<c0382323>] unknown_bootoption+0x0/0x195
       [<ffffffff>] 0xffffffff

-> #0 (linkfail_lock){-+..}:

       [<c0130780>] __lock_acquire+0x8d2/0xbe6
       [<c0130b02>] lock_acquire+0x6e/0x87
       [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
       [<c02a399b>] _spin_lock_bh+0x2e/0x39
       [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
       [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
       [<d8bd7300>] ax25_disconnect+0x34/0xbe [ax25]
       [<d8bd7c97>] ax25_device_event+0x53/0x84 [ax25]
       [<c0122670>] notifier_call_chain+0x2a/0x47
       [<c01226d3>] raw_notifier_call_chain+0x17/0x1a
       [<c0250a47>] dev_close+0x62/0x66
       [<c0250af1>] unregister_netdevice+0xa6/0x21f
       [<c0250c79>] unregister_netdev+0xf/0x15
       [<d8b4a7ad>] mkiss_close+0x63/0x7c [mkiss]
       [<c01fdd85>] release_dev+0x4f1/0x5a6
       [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
       [<c01b2226>] _atomic_dec_and_lock+0x22/0x2c
       [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
       [<c01fde41>] tty_release+0x7/0xa
       [<c015c7f7>] __fput+0xbc/0x172
       [<c015a2c6>] filp_close+0x51/0x58
       [<c0119dc3>] put_files_struct+0x5e/0xa6
       [<c011ae6e>] do_exit+0x22e/0x6d9
       [<c02a3c0d>] _spin_unlock_irq+0x20/0x23
       [<c012fa76>] trace_hardirqs_on+0x11f/0x148
       [<c011b384>] sys_exit_group+0x0/0xd
       [<c0121c36>] get_signal_to_deliver+0x3c6/0x3ea
       [<c0103475>] do_notify_resume+0x81/0x5fe
       [<c02a3d2a>] _spin_unlock_irqrestore+0x34/0x39
       [<c012fa76>] trace_hardirqs_on+0x11f/0x148
       [<c012b196>] getnstimeofday+0x2b/0xac
       [<c01b6d55>] copy_to_user+0x2f/0x46
       [<c0129ed8>] hrtimer_nanosleep+0x92/0xe5
       [<c0129b75>] hrtimer_wakeup+0x0/0x18
       [<c0129f74>] sys_nanosleep+0x49/0x59
       [<c0103ded>] work_notifysig+0x13/0x26
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

2 locks held by kissattach/2906:
#0: (rtnl_mutex){--..}, at: [<c0250c72>] unregister_netdev+0x8/0x15 #1: (ax25_list_lock){-+..}, at: [<d8bd7c7c>]

ax25_device_event+0x38/0x84 [ax25]

stack backtrace:
 [<c012efac>] print_circular_bug_tail+0x5e/0x66
 [<c0130780>] __lock_acquire+0x8d2/0xbe6
 [<c0130b02>] lock_acquire+0x6e/0x87
 [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
 [<c02a399b>] _spin_lock_bh+0x2e/0x39
 [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
 [<d8bd4603>] ax25_link_failed+0x11/0x39 [ax25]
 [<d8bd7300>] ax25_disconnect+0x34/0xbe [ax25]
 [<d8bd7c97>] ax25_device_event+0x53/0x84 [ax25]
 [<c0122670>] notifier_call_chain+0x2a/0x47
 [<c01226d3>] raw_notifier_call_chain+0x17/0x1a
 [<c0250a47>] dev_close+0x62/0x66
 [<c0250af1>] unregister_netdevice+0xa6/0x21f
 [<c0250c79>] unregister_netdev+0xf/0x15
 [<d8b4a7ad>] mkiss_close+0x63/0x7c [mkiss]
 [<c01fdd85>] release_dev+0x4f1/0x5a6
 [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
 [<c01b2226>] _atomic_dec_and_lock+0x22/0x2c
 [<c0130a4c>] __lock_acquire+0xb9e/0xbe6
 [<c01fde41>] tty_release+0x7/0xa
 [<c015c7f7>] __fput+0xbc/0x172
 [<c015a2c6>] filp_close+0x51/0x58
 [<c0119dc3>] put_files_struct+0x5e/0xa6
 [<c011ae6e>] do_exit+0x22e/0x6d9
 [<c02a3c0d>] _spin_unlock_irq+0x20/0x23
 [<c012fa76>] trace_hardirqs_on+0x11f/0x148
 [<c011b384>] sys_exit_group+0x0/0xd
 [<c0121c36>] get_signal_to_deliver+0x3c6/0x3ea
 [<c0103475>] do_notify_resume+0x81/0x5fe
 [<c02a3d2a>] _spin_unlock_irqrestore+0x34/0x39
 [<c012fa76>] trace_hardirqs_on+0x11f/0x148
 [<c012b196>] getnstimeofday+0x2b/0xac
 [<c01b6d55>] copy_to_user+0x2f/0x46
 [<c0129ed8>] hrtimer_nanosleep+0x92/0xe5
 [<c0129b75>] hrtimer_wakeup+0x0/0x18
 [<c0129f74>] sys_nanosleep+0x49/0x59
 [<c0103ded>] work_notifysig+0x13/0x26
 =======================

Bernard Pidoux

...

It seems, lockdep is warried about the different order here:

#1 (rose_neigh_list_lock){-+..}:
#3 (ax25_list_lock){-+..}:

#0 (linkfail_lock){-+..}:
#1 (rose_neigh_list_lock){-+..}:

#3 (ax25_list_lock){-+..}:
#0 (linkfail_lock){-+..}:

So, ax25_list_lock could be taken before and after linkfail_lock. I don't know if this three-thread clutch is very probable (or possible at all), but it seems this other bug nearby reported by Bernard ("[...] system impossible to reboot with linux-2.6.24-rc5") could have similar source - namely ax25_list_lock held by ax25_kill_by_device() during ax25_disconnect(). It looks like the only place which calls ax25_disconnect() this way, so I guess, it isn't necessary. But, since I don't know AX25 & ROSE at all, this should be necessarily verified by somebody who knows these things.

I attach here my very experimental proposal with breaking the lock for ax25_disconnect(), with some failsafe and debugging because of this, but, if in this special case the lock is required for some other reasons, then this patch should be dumped, of course.

Regards,
Jarek P.

WARNING:
not tested, not even compiled, needs some ack before testing!

---

diff -Nurp linux-2.6.24-rc5-/net/ax25/af_ax25.c linux-2.6.24-rc5+/net/ax25/af_ax25.c
@@ -87,10 +87,19 @@ static void ax25_kill_by_device(struct n
 		return;
 
 	spin_lock_bh(&ax25_list_lock);
+again:
 	ax25_for_each(s, node, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
+			struct hlist_node *nn = node->next;
+
 			s->ax25_dev = NULL;
+			spin_unlock_bh(&ax25_list_lock);
 			ax25_disconnect(s, ENETUNREACH);
+			spin_lock_bh(&ax25_list_lock);
+			if (nn != node->next) {
+				WARN_ON_ONCE(1);
+				goto again;
+			}
 		}
 	}
 	spin_unlock_bh(&ax25_list_lock);

Thank you Bernard for the bravery!

The loop is only for debugging: I don't know exactly what is done during this ax25_disconnect(), and how the list can change in the meantime because of some other activities. So, it's e.g. against possible list_del of this node - then some sockets would stay not disconnected.

I think, it's up to Ralf or some other ax25 expert to judge if this could be done like this. Since it's not used very often, I think this should better stay during testing: if there are no warnings
- the loop isn't repeated; otherwise it could simply take a little more time, but all sockets should be serviced, plus we know about problems.

Thanks,
Jarek P.

On Fri, Dec 28, 2007 at 09:43:07PM +0100, Pidoux wrote:
...

Thanks again Bernard.
Jarek P. too

---------------->

Subject: [ROSE][AX25] af_ax25: possible circular locking

Bernard Pidoux F6BVP reported:

...

lockdep is worried about the different order here:

#1 (rose_neigh_list_lock){-+..}:
#3 (ax25_list_lock){-+..}:

#0 (linkfail_lock){-+..}:
#1 (rose_neigh_list_lock){-+..}:

#3 (ax25_list_lock){-+..}:
#0 (linkfail_lock){-+..}:

So, ax25_list_lock could be taken before and after linkfail_lock. I don't know if this three-thread clutch is very probable (or possible at all), but it seems another bug reported by Bernard ("[...] system impossible to reboot with linux-2.6.24-rc5") could have similar source - namely ax25_list_lock held by ax25_kill_by_device() during ax25_disconnect(). It looks like the only place which calls ax25_disconnect() this way, so I guess, it isn't necessary.

This patch is breaking the lock for ax25_disconnect(), with some failsafe and debugging added to detect unforeseen problems.

---

diff -Nurp linux-2.6.24-rc5-/net/ax25/af_ax25.c linux-2.6.24-rc5+/net/ax25/af_ax25.c
@@ -87,10 +87,19 @@ static void ax25_kill_by_device(struct n
 		return;
 
 	spin_lock_bh(&ax25_list_lock);
+again:
 	ax25_for_each(s, node, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
+			struct hlist_node *nn = node->next;
+
 			s->ax25_dev = NULL;
+			spin_unlock_bh(&ax25_list_lock);
 			ax25_disconnect(s, ENETUNREACH);
+			spin_lock_bh(&ax25_list_lock);
+			if (nn != node->next) {
+				WARN_ON_ONCE(1);
+				goto again;
+			}
 		}
 	}
 	spin_unlock_bh(&ax25_list_lock);

From: Jarek Poplawski <[email protected]>
Date: Fri, 28 Dec 2007 22:48:57 +0100

I can't apply this fix, sorry.

You can't just drop this linked list lock and expect it to stay consistent like that.

Once you drop it, any thread of control can get in there and delete entries from the list.

Since we know it can happen, using a WARN_ON_ONCE(1) is not appropriate. And if it triggers it will do the wrong thing, because by branching back to "again" we can call ax25_disconnect() multiple times on the same entry which isn't right.

You'll thus need to resolve this locking conflict more properly. I know it's hard, but your current fix is worse because it adds a new known bug.

On Sat, Dec 29, 2007 at 07:14:43PM -0800, David Miller wrote:
...

The problem is 'we' don't know if it can happen... In the first message with this patch I've tried to get this information, and now it seems you are the only one with this knowledge, but of course this is more than enough for me to agree with your decision to dump this patch.

This is an equivalent of list_for_each_entry_safe(), and if such WARN_ON is ever reported this would confirm the solution is wrong. But, it seems Bernard's case was too simple to trigger this bug. Alas it was complex enough to trigger this other bug...

"again" loop should skip this entry next time: s->ax25_dev should be NULL, so not equal to ax25dev. (But of course there could be this unknown to me place, which changes this back in the meantime.)

Sorry, it seems this will've to wait until Ralf finds some time, because I really can't give this more time, considering I never used nor have any plans to use this code, and this bug could suggest there is not so many interested in this, besides Bernard, either.

Regards,
Jarek P.

From: Jarek Poplawski <[email protected]>
Date: Sun, 30 Dec 2007 15:13:23 +0100

Indeed you are right.

I'll try to look at this.

From: Jarek Poplawski <[email protected]>
Date: Sun, 30 Dec 2007 15:13:23 +0100

I've removed the warning and made the branch back to 'again' unconditional as I think this is the safest version of the change.

I'll push this upstream, thanks for fixing this Jarek.

index ecb14ee..b4725ff 100644
@@ -87,10 +87,22 @@ static void ax25_kill_by_device(struct net_device *dev)
 		return;
 
 	spin_lock_bh(&ax25_list_lock);
+again:
 	ax25_for_each(s, node, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
 			s->ax25_dev = NULL;
+			spin_unlock_bh(&ax25_list_lock);
 			ax25_disconnect(s, ENETUNREACH);
+			spin_lock_bh(&ax25_list_lock);
+
+			/* The entry could have been deleted from the
+			 * list meanwhile and thus the next pointer is
+			 * no longer valid.  Play it safe and restart
+			 * the scan.  Forward progress is ensured
+			 * because we set s->ax25_dev to NULL and we
+			 * are never passed a NULL 'dev' argument.
+			 */
+			goto again;
 		}
 	}
 	spin_unlock_bh(&ax25_list_lock);

1.5.4.rc2.84.gf85fd

...

Thanks for checking this and making safer!

Regards,
Jarek P.

I would also like to thank both Jarek and David for this patch. It is very helpful and probably safe for after killing kissattach we don't want to use attached ax25 devices.

Regards,

Bernard Pidoux

---
 include/net/rose.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index d3ab453..0cfdc0e 100644
@@ -86,7 +86,7 @@ struct rose_neigh {
        ax25_address            callsign;
        ax25_digi               *digipeat;
        ax25_cb                 *ax25;
-       struct net_device               *dev;
+       struct net_device       *dev;
        unsigned short          count;
        unsigned short          use;
        unsigned int            number;

@@ -124,7 +124,7 @@ struct rose_sock {
        ax25_address            source_digis[ROSE_MAX_DIGIS];
        ax25_address            dest_digis[ROSE_MAX_DIGIS];
        struct rose_neigh       *neighbour;
-       struct net_device               *device;
+       struct net_device       *device;
        unsigned int            lci, rand;
        unsigned char           state, condition, qbitincl, defer;
        unsigned char           cause, diagnostic;

--
1.5.3.7

Hi,

With AX25 patches applied I still get this possible circular locking message.

Regards,

Bernard P.

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.24 #3
-------------------------------------------------------
swapper/0 is trying to acquire lock:
(ax25_list_lock){-+..}, at: [<f91dd3b1>] ax25_destroy_socket+0x171/0x1f0 [ax25]

but task is already holding lock:
(slock-AF_AX25){-+..}, at: [<f91dbabc>] ax25_std_heartbeat_expiry+0x1c/0xe0 [ax25]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (slock-AF_AX25){-+..}:

       [<c014c8ca>] __lock_acquire+0xc4a/0x10d0
       [<c014cdd1>] lock_acquire+0x81/0xa0
       [<c03280e2>] _spin_lock+0x32/0x60
       [<f91dce3a>] ax25_info_show+0x24a/0x2c0 [ax25]
       [<c01a4aa1>] seq_read+0xa1/0x2a0
       [<c01c20fd>] proc_reg_read+0x5d/0x90
       [<c018a41a>] vfs_read+0xaa/0x130
       [<c018a8ed>] sys_read+0x3d/0x70
       [<c01042ae>] sysenter_past_esp+0x5f/0xa5
       [<ffffffff>] 0xffffffff

-> #0 (ax25_list_lock){-+..}:

       [<c014c6f1>] __lock_acquire+0xa71/0x10d0
       [<c014cdd1>] lock_acquire+0x81/0xa0
       [<c0328143>] _spin_lock_bh+0x33/0x60
       [<f91dd3b1>] ax25_destroy_socket+0x171/0x1f0 [ax25]
       [<f91dbaf3>] ax25_std_heartbeat_expiry+0x53/0xe0 [ax25]
       [<f91dc5ab>] ax25_heartbeat_expiry+0x1b/0x40 [ax25]
       [<c01325dd>] run_timer_softirq+0x15d/0x1c0
       [<c012e343>] __do_softirq+0x93/0x120
       [<c012e427>] do_softirq+0x57/0x60
       [<c012e868>] irq_exit+0x48/0x60
       [<c0119b3b>] smp_apic_timer_interrupt+0x5b/0x90
       [<c0104e27>] apic_timer_interrupt+0x33/0x38
       [<c0102512>] mwait_idle+0x12/0x20
       [<c0102631>] cpu_idle+0x71/0xc0
       [<c0325549>] rest_init+0x49/0x50
       [<c0427a9a>] start_kernel+0x2ea/0x370
       [<00000000>] 0x0
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

1 lock held by swapper/0:
#0: (slock-AF_AX25){-+..}, at: [<f91dbabc>] ax25_std_heartbeat_expiry+0x1c/0xe0 [ax25]

stack backtrace:
Pid: 0, comm: swapper Not tainted 2.6.24 #3
 [<c01053da>] show_trace_log_lvl+0x1a/0x30
 [<c0105e12>] show_trace+0x12/0x20
 [<c01067cc>] dump_stack+0x6c/0x80
 [<c014a32f>] print_circular_bug_tail+0x6f/0x80
 [<c014c6f1>] __lock_acquire+0xa71/0x10d0
 [<c014cdd1>] lock_acquire+0x81/0xa0
 [<c0328143>] _spin_lock_bh+0x33/0x60
 [<f91dd3b1>] ax25_destroy_socket+0x171/0x1f0 [ax25]
 [<f91dbaf3>] ax25_std_heartbeat_expiry+0x53/0xe0 [ax25]
 [<f91dc5ab>] ax25_heartbeat_expiry+0x1b/0x40 [ax25]
 [<c01325dd>] run_timer_softirq+0x15d/0x1c0
 [<c012e343>] __do_softirq+0x93/0x120
 [<c012e427>] do_softirq+0x57/0x60
 [<c012e868>] irq_exit+0x48/0x60
 [<c0119b3b>] smp_apic_timer_interrupt+0x5b/0x90
 [<c0104e27>] apic_timer_interrupt+0x33/0x38
 [<c0102512>] mwait_idle+0x12/0x20
 [<c0102631>] cpu_idle+0x71/0xc0
 [<c0325549>] rest_init+0x49/0x50
 [<c0427a9a>] start_kernel+0x2ea/0x370
 [<00000000>] 0x0
 =======================

Hi Bernard,

Could you confirm which exactly patches did you try? Is this vanilla 2.6.24 plus these two: ax25_timer and ax25_ds_timer or something more? And I'm not sure what do you mean by "still": the warning came back just after these last patches? At least these timer patches don't seem to change anything around locking?

Thanks for testing this,
Jarek P.

Hi Jarek,

Sorry, I should have been more explicit about the patches I applied.

My CPU is an Intel Core 2 duo and I compiled 2.6.24 with SMP option.

I applied 3 patches you have submitted for ax25 before I observed the possible locking.

That is :

mkiss patch

index cfcd15a..30c9b3b 100644

[AX25] ax25_timer: use mod_timer instead of add_timer

[AX25] ax25_ds_timer: use mod_timer instead of add_timer

I also applied 4 patches for rose.ko I sent to the list a while ago. But for this report I did not installed any ROSE driver nor application.

I will try your new patch version 2 and report any possible change.

Regards,

Bernard Pidoux,
F6BVP

IMHO this warning could happen earlier too...

Thanks,
Jarek P.

-------------->

Subject: [AX25] af_ax25: remove sock lock in ax25_info_show()

This lockdep warning:

...

shows that ax25_list_lock and slock-AF_AX25 are taken in different
order: ax25_info_show() takes slock (bh_lock_sock(ax25->sk)) while ax25_list_lock is held, so reversely to other functions. To fix this the sock lock should be moved to ax25_info_start(), but since it's only for reading proc info it seems this is not necessary (e.g. ax25_send_to_raw() does similar reading without this lock too). So, this patch removes this lock to avoid deadlock possibility.

---

 net/ax25/af_ax25.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

index 94b2b1b..68b5171 100644
@@ -1924,12 +1924,10 @@ static int ax25_info_show(struct seq_file *seq, void *v)
 		   ax25->paclen);
 
 	if (ax25->sk != NULL) {
-		bh_lock_sock(ax25->sk);
 		seq_printf(seq," %d %d %ld\n",
 			   atomic_read(&ax25->sk->sk_wmem_alloc),
 			   atomic_read(&ax25->sk->sk_rmem_alloc),
 			   ax25->sk->sk_socket != NULL ? SOCK_INODE(ax25->sk->sk_socket)->i_ino : 0L);
-		bh_unlock_sock(ax25->sk);
 	} else {
 		seq_puts(seq, " * * *\n");
 	}

Hi,

Here is a little bit better version, I hope.

Regards,
Jarek P.

--------------> (take 2)

Subject: [AX25] af_ax25: remove sock lock in ax25_info_show()

This lockdep warning:

...

shows that ax25_list_lock and slock-AF_AX25 are taken in different
order: ax25_info_show() takes slock (bh_lock_sock(ax25->sk)) while ax25_list_lock is held, so reversely to other functions. To fix this the sock lock should be moved to ax25_info_start(), and there would be still problem with breaking ax25_list_lock (it seems this "proper" order isn't optimal yet). But, since it's only for reading proc info it seems this is not necessary (e.g. ax25_send_to_raw() does similar reading without this lock too).

So, this patch removes sock lock to avoid deadlock possibility; there is also used sock_i_ino() function, which reads sk_socket under proper read lock. Additionally printf format of this i_ino is changed to %lu.

---

 net/ax25/af_ax25.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

index 94b2b1b..48bfcc7 100644
@@ -1924,12 +1924,10 @@ static int ax25_info_show(struct seq_file *seq, void *v)
 		   ax25->paclen);
 
 	if (ax25->sk != NULL) {
-		bh_lock_sock(ax25->sk);
-		seq_printf(seq," %d %d %ld\n",
+		seq_printf(seq, " %d %d %lu\n",
 			   atomic_read(&ax25->sk->sk_wmem_alloc),
 			   atomic_read(&ax25->sk->sk_rmem_alloc),
-			   ax25->sk->sk_socket != NULL ? SOCK_INODE(ax25->sk->sk_socket)->i_ino : 0L);
-		bh_unlock_sock(ax25->sk);
+			   sock_i_ino(ax25->sk));
 	} else {
 		seq_puts(seq, " * * *\n");
 	}

From: Jarek Poplawski <[email protected]>
Date: Sun, 10 Feb 2008 14:10:51 +0100

Applied, thanks Jarek.