list corruption on removal of snd_seq_dummy

list corruption on removal of snd_seq_dummy

[Dave Jones]

If you apply the debugging patch below (based on one from -mm)
and rmmod snd_seq_dummy, you get this spew:
(based on a 2.6.17rc6 kernel, but likely still there in todays -git)

The code in question is doing..

        __list_add(&deleted_list,
               client->ports_list_head.prev,
               client->ports_list_head.next);

which looks fishy, as those two elements aren't going to be consecutive,
as __list_add expects.

		Dave

List corruption. next->prev should be f76896e8, but was f70acbcc
------------[ cut here ]------------
kernel BUG at include/linux/list.h:58!
invalid opcode: 0000 [#1]
SMP
last sysfs file: /devices/pci0000:00/0000:00:1f.3/i2c-0/0-002e/pwm3
Modules linked in: lm85 hwmon_vid hwmon i2c_isa ipv6 nls_utf8 loop snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss matroxfb_base matroxfb_DAC1064 snd_mixer_oss matroxfb_accel matroxfb_Ti3026 matroxfb_g450 g450_pll matroxfb_misc snd_pcm 3w_9xxx e1000 snd_timer snd i2c_i801 soundcore i2c_core snd_page_alloc ext3 jbd 3w_xxxx ata_piix libata sd_mod scsi_mod
CPU:    1
EIP:    0060:[<f8a1a56d>]    Not tainted VLI
EFLAGS: 00010092   (2.6.16-1.2232_FC6 #1)
EIP is at snd_seq_delete_all_ports+0x57/0x167 [snd_seq]
eax: 00000044   ebx: f76896e8   ecx: c06c77d0   edx: 00000096
esi: f76896e8   edi: f70acbcc   ebp: f70acb50   esp: f772cf08
ds: 007b   es: 007b   ss: 0068
Process rmmod (pid: 6274, threadinfo=f772c000 task=df26f550)
Stack: f8a1b83b f76896e8 f70acbcc f70acbe4 f70acbd4 00000282 22222222 22222222
       f70acb50 00000020 00000000 f772c000 f8a1521e f70acb50 f8a152e2 c043df66
       f772c000 c0445bf0 f70acb50 f8a1778b f89b4e00 c043e177 5f646e73 5f716573
Call Trace:
 <f8a1521e> seq_free_client1+0x8/0x90 [snd_seq]  <f8a152e2> seq_free_client+0x3c/0x78 [snd_seq]
 <c043df66> __try_stop_module+0x0/0x44  <c0445bf0> stop_machine_run+0x2e/0x34
 <f8a1778b> snd_seq_delete_kernel_client+0x1a/0x2c [snd_seq]  <c043e177> sys_delete_module+0x192/0x1bb
 <c060f020> do_page_fault+0x235/0x5ba  <c045ab3f> do_munmap+0x196/0x1af
 <c0403e1f> syscall_call+0x7/0xb
Code: 24 14 39 7d 7c 74 67 8b 5d 7c 8b b5 80 00 00 00 8b 43 04 39 f0 74 1c 89 74 24 04 89 44 24 08 c7 04 24 3b b8 a1 f8 e8 e7 ab a0 c7 <0f> 0b 3a 00 26 b8 a1 f8 8b 06 39 d8 74 1c 89 5c 24 04 89 44 24
EIP: [<f8a1a56d>] snd_seq_delete_all_ports+0x57/0x167 [snd_seq] SS:ESP 0068:f772cf08
 <3>BUG: sleeping function called from invalid context at include/linux/rwsem.h:43
in_atomic():0, irqs_disabled():1
 <c042ffa2> blocking_notifier_call_chain+0x18/0x4b  <c0427387> do_exit+0x19/0x7a5
 <c053a76b> do_unblank_screen+0x2a/0x127  <c040549a> die+0x2a5/0x2ca
 <c0405b30> do_invalid_op+0x0/0xab  <c0405bd2> do_invalid_op+0xa2/0xab
 <f8a1a56d> snd_seq_delete_all_ports+0x57/0x167 [snd_seq]  <c041e25f> find_busiest_group+0xfc/0x29c
 <c060dfd0> _spin_unlock_irq+0x5/0x7  <c04390ef> debug_mutex_add_waiter+0x97/0xa9
 <f8a1a531> snd_seq_delete_all_ports+0x1b/0x167 [snd_seq]  <c04049b7> error_code+0x4f/0x54
 <f8a1a56d> snd_seq_delete_all_ports+0x57/0x167 [snd_seq]  <f8a1521e> seq_free_client1+0x8/0x90 [snd_seq]
 <f8a152e2> seq_free_client+0x3c/0x78 [snd_seq]  <c043df66> __try_stop_module+0x0/0x44
 <c0445bf0> stop_machine_run+0x2e/0x34  <f8a1778b> snd_seq_delete_kernel_client+0x1a/0x2c [snd_seq]
 <c043e177> sys_delete_module+0x192/0x1bb  <c060f020> do_page_fault+0x235/0x5ba
 <c045ab3f> do_munmap+0x196/0x1af  <c0403e1f> syscall_call+0x7/0xb
BUG: rmmod/6274, lock held at task exit time!
 [f8a21fa0] {register_mutex}
.. held by:             rmmod: 6274 [df26f550, 117]
... acquired at:               seq_free_client+0x10/0x78 [snd_seq]



--- linux-2.6.12/include/linux/list.h~	2005-08-08 15:34:50.000000000 -0400
+++ linux-2.6.12/include/linux/list.h	2005-08-08 15:35:22.000000000 -0400
@@ -5,7 +5,9 @@
 
 #include <linux/stddef.h>
 #include <linux/prefetch.h>
+#include <linux/kernel.h>
 #include <asm/system.h>
+#include <asm/bug.h>
 
 /*
  * These are non-NULL pointers that will result in page faults
@@ -52,6 +52,16 @@ static inline void __list_add(struct lis
 			      struct list_head *prev,
 			      struct list_head *next)
 {
+	if (next->prev != prev) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+				prev, next->prev);
+		BUG();
+	}
+	if (prev->next != next) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+				next, prev->next);
+		BUG();
+	}
 	next->prev = new;
 	new->next = next;
 	new->prev = prev;
@@ -162,6 +162,16 @@ static inline void __list_del(struct lis
  */
 static inline void list_del(struct list_head *entry)
 {
+	if (entry->prev->next != entry) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+				entry, entry->prev->next);
+		BUG();
+	}
+	if (entry->next->prev != entry) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+				entry, entry->next->prev);
+		BUG();
+	}
 	__list_del(entry->prev, entry->next);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;


-- 
http://www.codemonkey.org.uk

Re: list corruption on removal of snd_seq_dummy

[Takashi Iwai]

At Thu, 22 Jun 2006 23:15:26 -0400,
Dave Jones wrote:
> 
> If you apply the debugging patch below (based on one from -mm)
> and rmmod snd_seq_dummy, you get this spew:
> (based on a 2.6.17rc6 kernel, but likely still there in todays -git)
> 
> The code in question is doing..
> 
>         __list_add(&deleted_list,
>                client->ports_list_head.prev,
>                client->ports_list_head.next);
> 
> which looks fishy, as those two elements aren't going to be consecutive,
> as __list_add expects.

I think the code behaves correctly but probably misusing __list_add().
It movies the whole entries from an existing list_head A
(clients->ports_list_head) to a new list_head B (deleted_list).
The above is exapnded:

	A->next->prev = B;
	B->next = A->next;
	B->prev = A->prev;
	A->prev->next = B;


Any better way to achieve it using standard macros?


thanks,

Takashi

> 
> 		Dave
> 
> List corruption. next->prev should be f76896e8, but was f70acbcc
> ------------[ cut here ]------------
> kernel BUG at include/linux/list.h:58!
> invalid opcode: 0000 [#1]
> SMP
> last sysfs file: /devices/pci0000:00/0000:00:1f.3/i2c-0/0-002e/pwm3
> Modules linked in: lm85 hwmon_vid hwmon i2c_isa ipv6 nls_utf8 loop snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss matroxfb_base matroxfb_DAC1064 snd_mixer_oss matroxfb_accel matroxfb_Ti3026 matroxfb_g450 g450_pll matroxfb_misc snd_pcm 3w_9xxx e1000 snd_timer snd i2c_i801 soundcore i2c_core snd_page_alloc ext3 jbd 3w_xxxx ata_piix libata sd_mod scsi_mod
> CPU:    1
> EIP:    0060:[<f8a1a56d>]    Not tainted VLI
> EFLAGS: 00010092   (2.6.16-1.2232_FC6 #1)
> EIP is at snd_seq_delete_all_ports+0x57/0x167 [snd_seq]
> eax: 00000044   ebx: f76896e8   ecx: c06c77d0   edx: 00000096
> esi: f76896e8   edi: f70acbcc   ebp: f70acb50   esp: f772cf08
> ds: 007b   es: 007b   ss: 0068
> Process rmmod (pid: 6274, threadinfo=f772c000 task=df26f550)
> Stack: f8a1b83b f76896e8 f70acbcc f70acbe4 f70acbd4 00000282 22222222 22222222
>        f70acb50 00000020 00000000 f772c000 f8a1521e f70acb50 f8a152e2 c043df66
>        f772c000 c0445bf0 f70acb50 f8a1778b f89b4e00 c043e177 5f646e73 5f716573
> Call Trace:
>  <f8a1521e> seq_free_client1+0x8/0x90 [snd_seq]  <f8a152e2> seq_free_client+0x3c/0x78 [snd_seq]
>  <c043df66> __try_stop_module+0x0/0x44  <c0445bf0> stop_machine_run+0x2e/0x34
>  <f8a1778b> snd_seq_delete_kernel_client+0x1a/0x2c [snd_seq]  <c043e177> sys_delete_module+0x192/0x1bb
>  <c060f020> do_page_fault+0x235/0x5ba  <c045ab3f> do_munmap+0x196/0x1af
>  <c0403e1f> syscall_call+0x7/0xb
> Code: 24 14 39 7d 7c 74 67 8b 5d 7c 8b b5 80 00 00 00 8b 43 04 39 f0 74 1c 89 74 24 04 89 44 24 08 c7 04 24 3b b8 a1 f8 e8 e7 ab a0 c7 <0f> 0b 3a 00 26 b8 a1 f8 8b 06 39 d8 74 1c 89 5c 24 04 89 44 24
> EIP: [<f8a1a56d>] snd_seq_delete_all_ports+0x57/0x167 [snd_seq] SS:ESP 0068:f772cf08
>  <3>BUG: sleeping function called from invalid context at include/linux/rwsem.h:43
> in_atomic():0, irqs_disabled():1
>  <c042ffa2> blocking_notifier_call_chain+0x18/0x4b  <c0427387> do_exit+0x19/0x7a5
>  <c053a76b> do_unblank_screen+0x2a/0x127  <c040549a> die+0x2a5/0x2ca
>  <c0405b30> do_invalid_op+0x0/0xab  <c0405bd2> do_invalid_op+0xa2/0xab
>  <f8a1a56d> snd_seq_delete_all_ports+0x57/0x167 [snd_seq]  <c041e25f> find_busiest_group+0xfc/0x29c
>  <c060dfd0> _spin_unlock_irq+0x5/0x7  <c04390ef> debug_mutex_add_waiter+0x97/0xa9
>  <f8a1a531> snd_seq_delete_all_ports+0x1b/0x167 [snd_seq]  <c04049b7> error_code+0x4f/0x54
>  <f8a1a56d> snd_seq_delete_all_ports+0x57/0x167 [snd_seq]  <f8a1521e> seq_free_client1+0x8/0x90 [snd_seq]
>  <f8a152e2> seq_free_client+0x3c/0x78 [snd_seq]  <c043df66> __try_stop_module+0x0/0x44
>  <c0445bf0> stop_machine_run+0x2e/0x34  <f8a1778b> snd_seq_delete_kernel_client+0x1a/0x2c [snd_seq]
>  <c043e177> sys_delete_module+0x192/0x1bb  <c060f020> do_page_fault+0x235/0x5ba
>  <c045ab3f> do_munmap+0x196/0x1af  <c0403e1f> syscall_call+0x7/0xb
> BUG: rmmod/6274, lock held at task exit time!
>  [f8a21fa0] {register_mutex}
> .. held by:             rmmod: 6274 [df26f550, 117]
> ... acquired at:               seq_free_client+0x10/0x78 [snd_seq]
> 
> 
> 
> --- linux-2.6.12/include/linux/list.h~	2005-08-08 15:34:50.000000000 -0400
> +++ linux-2.6.12/include/linux/list.h	2005-08-08 15:35:22.000000000 -0400
> @@ -5,7 +5,9 @@
>  
>  #include <linux/stddef.h>
>  #include <linux/prefetch.h>
> +#include <linux/kernel.h>
>  #include <asm/system.h>
> +#include <asm/bug.h>
>  
>  /*
>   * These are non-NULL pointers that will result in page faults
> @@ -52,6 +52,16 @@ static inline void __list_add(struct lis
>  			      struct list_head *prev,
>  			      struct list_head *next)
>  {
> +	if (next->prev != prev) {
> +		printk("List corruption. next->prev should be %p, but was %p\n",
> +				prev, next->prev);
> +		BUG();
> +	}
> +	if (prev->next != next) {
> +		printk("List corruption. prev->next should be %p, but was %p\n",
> +				next, prev->next);
> +		BUG();
> +	}
>  	next->prev = new;
>  	new->next = next;
>  	new->prev = prev;
> @@ -162,6 +162,16 @@ static inline void __list_del(struct lis
>   */
>  static inline void list_del(struct list_head *entry)
>  {
> +	if (entry->prev->next != entry) {
> +		printk("List corruption. prev->next should be %p, but was %p\n",
> +				entry, entry->prev->next);
> +		BUG();
> +	}
> +	if (entry->next->prev != entry) {
> +		printk("List corruption. next->prev should be %p, but was %p\n",
> +				entry, entry->next->prev);
> +		BUG();
> +	}
>  	__list_del(entry->prev, entry->next);
>  	entry->next = LIST_POISON1;
>  	entry->prev = LIST_POISON2;
> 
> 
> -- 
> http://www.codemonkey.org.uk
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: list corruption on removal of snd_seq_dummy

[Dave Jones]

On Fri, Jun 23, 2006 at 12:47:46PM +0200, Takashi Iwai wrote:
 
 > > The code in question is doing..
 > > 
 > >         __list_add(&deleted_list,
 > >                client->ports_list_head.prev,
 > >                client->ports_list_head.next);
 > > 
 > > which looks fishy, as those two elements aren't going to be consecutive,
 > > as __list_add expects.
 > 
 > I think the code behaves correctly but probably misusing __list_add().
 > It movies the whole entries from an existing list_head A
 > (clients->ports_list_head) to a new list_head B (deleted_list).
 > The above is exapnded:
 > 
 > 	A->next->prev = B;
 > 	B->next = A->next;
 > 	B->prev = A->prev;
 > 	A->prev->next = B;
 > 
 > Any better way to achieve it using standard macros?

Why can't you just list_move() the elements ?

		Dave

-- 
http://www.codemonkey.org.uk

Re: list corruption on removal of snd_seq_dummy

[Takashi Iwai]

At Mon, 26 Jun 2006 23:27:38 -0400,
Dave Jones wrote:
> 
> On Fri, Jun 23, 2006 at 12:47:46PM +0200, Takashi Iwai wrote:
>  
>  > > The code in question is doing..
>  > > 
>  > >         __list_add(&deleted_list,
>  > >                client->ports_list_head.prev,
>  > >                client->ports_list_head.next);
>  > > 
>  > > which looks fishy, as those two elements aren't going to be consecutive,
>  > > as __list_add expects.
>  > 
>  > I think the code behaves correctly but probably misusing __list_add().
>  > It movies the whole entries from an existing list_head A
>  > (clients->ports_list_head) to a new list_head B (deleted_list).
>  > The above is exapnded:
>  > 
>  > 	A->next->prev = B;
>  > 	B->next = A->next;
>  > 	B->prev = A->prev;
>  > 	A->prev->next = B;
>  > 
>  > Any better way to achieve it using standard macros?
> 
> Why can't you just list_move() the elements ?

No, list_move() can't move the whole elements without loop.

A solution is

	list_add(B, A);
	list_del_init(A);

(although this introduces a bit more code :)


Takashi

Re: list corruption on removal of snd_seq_dummy

[Chuck Ebbert]

In-Reply-To: <s5hejxb7xrb.wl%tiwai@suse.de>

On Tue, 27 Jun 2006 11:28:56 +0200, Takashi Iwai wrote:

> >  > > The code in question is doing..
> >  > > 
> >  > >         __list_add(&deleted_list,
> >  > >                client->ports_list_head.prev,
> >  > >                client->ports_list_head.next);
> >  > > 
> >  > > which looks fishy, as those two elements aren't going to be consecutive,
> >  > > as __list_add expects.
> >  > 
> >  > I think the code behaves correctly but probably misusing __list_add().
> >  > It movies the whole entries from an existing list_head A
> >  > (clients->ports_list_head) to a new list_head B (deleted_list).
> >  > The above is exapnded:
> >  > 
> >  >  A->next->prev = B;
> >  >  B->next = A->next;
> >  >  B->prev = A->prev;
> >  >  A->prev->next = B;
> >  > 
> >  > Any better way to achieve it using standard macros?
> > 
> > Why can't you just list_move() the elements ?
> 
> No, list_move() can't move the whole elements without loop.
> 
> A solution is
> 
>       list_add(B, A);
>       list_del_init(A);
> 
> (although this introduces a bit more code :)

Shouldn't it be like this?

        ports_list_first = client->ports_list_head.next;
        list_del_init(client->ports_list_head);
        list_splice(ports_list_first, &deleted_list);

-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: list corruption on removal of snd_seq_dummy

[Takashi Iwai]

At Tue, 27 Jun 2006 08:05:01 -0400,
Chuck Ebbert wrote:
> 
> In-Reply-To: <s5hejxb7xrb.wl%tiwai@suse.de>
> 
> On Tue, 27 Jun 2006 11:28:56 +0200, Takashi Iwai wrote:
> 
> > >  > > The code in question is doing..
> > >  > > 
> > >  > >         __list_add(&deleted_list,
> > >  > >                client->ports_list_head.prev,
> > >  > >                client->ports_list_head.next);
> > >  > > 
> > >  > > which looks fishy, as those two elements aren't going to be consecutive,
> > >  > > as __list_add expects.
> > >  > 
> > >  > I think the code behaves correctly but probably misusing __list_add().
> > >  > It movies the whole entries from an existing list_head A
> > >  > (clients->ports_list_head) to a new list_head B (deleted_list).
> > >  > The above is exapnded:
> > >  > 
> > >  >  A->next->prev = B;
> > >  >  B->next = A->next;
> > >  >  B->prev = A->prev;
> > >  >  A->prev->next = B;
> > >  > 
> > >  > Any better way to achieve it using standard macros?
> > > 
> > > Why can't you just list_move() the elements ?
> > 
> > No, list_move() can't move the whole elements without loop.
> > 
> > A solution is
> > 
> >       list_add(B, A);
> >       list_del_init(A);
> > 
> > (although this introduces a bit more code :)
> 
> Shouldn't it be like this?
> 
>         ports_list_first = client->ports_list_head.next;
>         list_del_init(client->ports_list_head);
>         list_splice(ports_list_first, &deleted_list);

This requires INIT_LIST_HEAD(&deleted_list) first, so obviously
a longer code :)


Takashi

Re: list corruption on removal of snd_seq_dummy

[Dave Jones]

On Tue, Jun 27, 2006 at 02:38:39PM +0200, Takashi Iwai wrote:
 > > > No, list_move() can't move the whole elements without loop.
 > > > 
 > > > A solution is
 > > > 
 > > >       list_add(B, A);
 > > >       list_del_init(A);
 > > > 
 > > > (although this introduces a bit more code :)
 > > 
 > > Shouldn't it be like this?
 > > 
 > >         ports_list_first = client->ports_list_head.next;
 > >         list_del_init(client->ports_list_head);
 > >         list_splice(ports_list_first, &deleted_list);
 > 
 > This requires INIT_LIST_HEAD(&deleted_list) first, so obviously
 > a longer code :)

This is hardly a speed/size critical function. I'd go for readability
over cute hacks any day.

		Dave

-- 
http://www.codemonkey.org.uk

Re: list corruption on removal of snd_seq_dummy

[Takashi Iwai]

At Tue, 27 Jun 2006 12:58:19 -0400,
Dave Jones wrote:
> 
> On Tue, Jun 27, 2006 at 02:38:39PM +0200, Takashi Iwai wrote:
>  > > > No, list_move() can't move the whole elements without loop.
>  > > > 
>  > > > A solution is
>  > > > 
>  > > >       list_add(B, A);
>  > > >       list_del_init(A);
>  > > > 
>  > > > (although this introduces a bit more code :)
>  > > 
>  > > Shouldn't it be like this?
>  > > 
>  > >         ports_list_first = client->ports_list_head.next;
>  > >         list_del_init(client->ports_list_head);
>  > >         list_splice(ports_list_first, &deleted_list);
>  > 
>  > This requires INIT_LIST_HEAD(&deleted_list) first, so obviously
>  > a longer code :)
> 
> This is hardly a speed/size critical function. I'd go for readability
> over cute hacks any day.

Yeah, of course.
The code was already fixed on ALSA tree to use standard macros.


Takashi