[BUG][AX25] Fwd: SMP with AX.25

[BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

[-- Attachment #1: Type: text/plain, Size: 66 bytes --]

Hi,

Here is attached a message I got with AX25 oopses.

Jarek P.

[-- Attachment #2: Type: message/rfc822, Size: 73123 bytes --]

[-- Attachment #2.1.1.1: Type: text/plain, Size: 599 bytes --]

Witam Jarek,

my name is Jann Traschewski, DG8NGN and I run the AMPR-Gateway and heavy
used Packet Radio Node DB0FHN on a HP Netserver Dual CPU Server. I'm in
contact with Ralf, DL5RB in WW-Convers talking about the Kernel AX.25 stuff.
Maybe you are also interested on my problems using SMP on my big machine
(see Attachment) with Kernel 2.6.24. Do you want to have an account on the
machine?

73,
Jann


--
Jann Traschewski, Drosselstr.1, D-90513 Zirndorf, Germany
Tel.: +49-911-696971, Mobile: +49-170-1045937, EMail: jann@gmx.de
Ham: DG8NGN / DB0VOX, http://www.qsl.net/db0fhn, ICQ UIN: 4130182 

[-- Attachment #2.1.1.2: db0fhn3.png --]
[-- Type: image/png, Size: 16778 bytes --]

[-- Attachment #2.1.1.3: db0fhn.png --]
[-- Type: image/png, Size: 14364 bytes --]

[-- Attachment #2.1.1.4: db0fhn2.png --]
[-- Type: image/png, Size: 17395 bytes --]

[-- Attachment #2.1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3161 bytes --]

[PATCH][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
...
> From: Jann Traschewski <jann@gmx.de>
> Subject: SMP with AX.25


[AX25] ax25_timer: use mod_timer instead of add_timer

According to one of Jann's OOPS reports it looks like
BUG_ON(timer_pending(timer)) triggers during add_timer()
in ax25_start_t1timer(). This patch changes current use
of: init_timer(), add_timer() and del_timer() to
setup_timer() with mod_timer(), which should be safer
anyway.


Reported-by: Jann Traschewski <jann@gmx.de>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 include/net/ax25.h    |    1 +
 net/ax25/af_ax25.c    |    6 +----
 net/ax25/ax25_timer.c |   60 +++++++++++++++++-------------------------------
 3 files changed, 23 insertions(+), 44 deletions(-)

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 32a57e1..3f0236f 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -416,6 +416,7 @@ extern void ax25_calculate_rtt(ax25_cb *);
 extern void ax25_disconnect(ax25_cb *, int);
 
 /* ax25_timer.c */
+extern void ax25_setup_timers(ax25_cb *);
 extern void ax25_start_heartbeat(ax25_cb *);
 extern void ax25_start_t1timer(ax25_cb *);
 extern void ax25_start_t2timer(ax25_cb *);
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 8fc64e3..94b2b1b 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -510,11 +510,7 @@ ax25_cb *ax25_create_cb(void)
 	skb_queue_head_init(&ax25->ack_queue);
 	skb_queue_head_init(&ax25->reseq_queue);
 
-	init_timer(&ax25->timer);
-	init_timer(&ax25->t1timer);
-	init_timer(&ax25->t2timer);
-	init_timer(&ax25->t3timer);
-	init_timer(&ax25->idletimer);
+	ax25_setup_timers(ax25);
 
 	ax25_fillin_cb(ax25, NULL);
 
diff --git a/net/ax25/ax25_timer.c b/net/ax25/ax25_timer.c
index 7259486..db29ea7 100644
--- a/net/ax25/ax25_timer.c
+++ b/net/ax25/ax25_timer.c
@@ -40,63 +40,45 @@ static void ax25_t2timer_expiry(unsigned long);
 static void ax25_t3timer_expiry(unsigned long);
 static void ax25_idletimer_expiry(unsigned long);
 
-void ax25_start_heartbeat(ax25_cb *ax25)
+void ax25_setup_timers(ax25_cb *ax25)
 {
-	del_timer(&ax25->timer);
-
-	ax25->timer.data     = (unsigned long)ax25;
-	ax25->timer.function = &ax25_heartbeat_expiry;
-	ax25->timer.expires  = jiffies + 5 * HZ;
+	setup_timer(&ax25->timer, ax25_heartbeat_expiry, (unsigned long)ax25);
+	setup_timer(&ax25->t1timer, ax25_t1timer_expiry, (unsigned long)ax25);
+	setup_timer(&ax25->t2timer, ax25_t2timer_expiry, (unsigned long)ax25);
+	setup_timer(&ax25->t3timer, ax25_t3timer_expiry, (unsigned long)ax25);
+	setup_timer(&ax25->idletimer, ax25_idletimer_expiry,
+		    (unsigned long)ax25);
+}
 
-	add_timer(&ax25->timer);
+void ax25_start_heartbeat(ax25_cb *ax25)
+{
+	mod_timer(&ax25->timer, jiffies + 5 * HZ);
 }
 
 void ax25_start_t1timer(ax25_cb *ax25)
 {
-	del_timer(&ax25->t1timer);
-
-	ax25->t1timer.data     = (unsigned long)ax25;
-	ax25->t1timer.function = &ax25_t1timer_expiry;
-	ax25->t1timer.expires  = jiffies + ax25->t1;
-
-	add_timer(&ax25->t1timer);
+	mod_timer(&ax25->t1timer, jiffies + ax25->t1);
 }
 
 void ax25_start_t2timer(ax25_cb *ax25)
 {
-	del_timer(&ax25->t2timer);
-
-	ax25->t2timer.data     = (unsigned long)ax25;
-	ax25->t2timer.function = &ax25_t2timer_expiry;
-	ax25->t2timer.expires  = jiffies + ax25->t2;
-
-	add_timer(&ax25->t2timer);
+	mod_timer(&ax25->t2timer, jiffies + ax25->t2);
 }
 
 void ax25_start_t3timer(ax25_cb *ax25)
 {
-	del_timer(&ax25->t3timer);
-
-	if (ax25->t3 > 0) {
-		ax25->t3timer.data     = (unsigned long)ax25;
-		ax25->t3timer.function = &ax25_t3timer_expiry;
-		ax25->t3timer.expires  = jiffies + ax25->t3;
-
-		add_timer(&ax25->t3timer);
-	}
+	if (ax25->t3 > 0)
+		mod_timer(&ax25->t3timer, jiffies + ax25->t3);
+	else
+		del_timer(&ax25->t3timer);
 }
 
 void ax25_start_idletimer(ax25_cb *ax25)
 {
-	del_timer(&ax25->idletimer);
-
-	if (ax25->idle > 0) {
-		ax25->idletimer.data     = (unsigned long)ax25;
-		ax25->idletimer.function = &ax25_idletimer_expiry;
-		ax25->idletimer.expires  = jiffies + ax25->idle;
-
-		add_timer(&ax25->idletimer);
-	}
+	if (ax25->idle > 0)
+		mod_timer(&ax25->idletimer, jiffies + ax25->idle);
+	else
+		del_timer(&ax25->idletimer);
 }
 
 void ax25_stop_heartbeat(ax25_cb *ax25)

[PATCH][AX25] ax25_ds_timer: use mod_timer instead of add_timer

[Jarek Poplawski]

On Wed, Feb 06, 2008 at 08:15:09AM +0000, Jarek Poplawski wrote:
> On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
> ...
> > From: Jann Traschewski <jann@gmx.de>
> > Subject: SMP with AX.25
...


[AX25] ax25_ds_timer: use mod_timer instead of add_timer

This patch changes current use of: init_timer(), add_timer()
and del_timer() to setup_timer() with mod_timer(), which
should be safer anyway.


Reported-by: Jann Traschewski <jann@gmx.de>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 include/net/ax25.h       |    1 +
 net/ax25/ax25_dev.c      |    2 +-
 net/ax25/ax25_ds_timer.c |   12 ++++--------
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 3f0236f..717e219 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -324,6 +324,7 @@ extern void ax25_dama_on(ax25_cb *);
 extern void ax25_dama_off(ax25_cb *);
 
 /* ax25_ds_timer.c */
+extern void ax25_ds_setup_timer(ax25_dev *);
 extern void ax25_ds_set_timer(ax25_dev *);
 extern void ax25_ds_del_timer(ax25_dev *);
 extern void ax25_ds_timer(ax25_cb *);
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 528c874..a7a0e0c 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -82,7 +82,7 @@ void ax25_dev_device_up(struct net_device *dev)
 	ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT;
 
 #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER)
-	init_timer(&ax25_dev->dama.slave_timer);
+	ax25_ds_setup_timer(ax25_dev);
 #endif
 
 	spin_lock_bh(&ax25_dev_lock);
diff --git a/net/ax25/ax25_ds_timer.c b/net/ax25/ax25_ds_timer.c
index c4e3b02..2ce79df 100644
--- a/net/ax25/ax25_ds_timer.c
+++ b/net/ax25/ax25_ds_timer.c
@@ -40,13 +40,10 @@ static void ax25_ds_timeout(unsigned long);
  *	1/10th of a second.
  */
 
-static void ax25_ds_add_timer(ax25_dev *ax25_dev)
+void ax25_ds_setup_timer(ax25_dev *ax25_dev)
 {
-	struct timer_list *t = &ax25_dev->dama.slave_timer;
-	t->data		= (unsigned long) ax25_dev;
-	t->function	= &ax25_ds_timeout;
-	t->expires	= jiffies + HZ;
-	add_timer(t);
+	setup_timer(&ax25_dev->dama.slave_timer, ax25_ds_timeout,
+		    (unsigned long)ax25_dev);
 }
 
 void ax25_ds_del_timer(ax25_dev *ax25_dev)
@@ -60,10 +57,9 @@ void ax25_ds_set_timer(ax25_dev *ax25_dev)
 	if (ax25_dev == NULL)		/* paranoia */
 		return;
 
-	del_timer(&ax25_dev->dama.slave_timer);
 	ax25_dev->dama.slave_timeout =
 		msecs_to_jiffies(ax25_dev->values[AX25_VALUES_DS_TIMEOUT]) / 10;
-	ax25_ds_add_timer(ax25_dev);
+	mod_timer(&ax25_dev->dama.slave_timer, jiffies + HZ);
 }
 
 /*

Re: [PATCH][AX25] ax25_ds_timer: use mod_timer instead of add_timer

[Jann Traschewski]

Patches from Jarek applied (incl. both "testing" patches). Machine is stable
since 2 days now.
Regards,
Jann

> -----Ursprüngliche Nachricht-----
> Von: Jarek Poplawski [mailto:jarkao2@gmail.com] 
> Gesendet: Mittwoch, 6. Februar 2008 10:14
> An: netdev@vger.kernel.org
> Cc: Ralf Baechle; Jann Traschewski; David Miller
> Betreff: [PATCH][AX25] ax25_ds_timer: use mod_timer instead 
> of add_timer
> 
> On Wed, Feb 06, 2008 at 08:15:09AM +0000, Jarek Poplawski wrote:
> > On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
> > ...
> > > From: Jann Traschewski <jann@gmx.de>
> > > Subject: SMP with AX.25
> ...
> 
> 
> [AX25] ax25_ds_timer: use mod_timer instead of add_timer
> 
> This patch changes current use of: init_timer(), add_timer() 
> and del_timer() to setup_timer() with mod_timer(), which 
> should be safer anyway.
> 
> 
> Reported-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> ---
> 
>  include/net/ax25.h       |    1 +
>  net/ax25/ax25_dev.c      |    2 +-
>  net/ax25/ax25_ds_timer.c |   12 ++++--------
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/ax25.h b/include/net/ax25.h index 
> 3f0236f..717e219 100644
> --- a/include/net/ax25.h
> +++ b/include/net/ax25.h
> @@ -324,6 +324,7 @@ extern void ax25_dama_on(ax25_cb *);  
> extern void ax25_dama_off(ax25_cb *);
>  
>  /* ax25_ds_timer.c */
> +extern void ax25_ds_setup_timer(ax25_dev *);
>  extern void ax25_ds_set_timer(ax25_dev *);  extern void 
> ax25_ds_del_timer(ax25_dev *);  extern void 
> ax25_ds_timer(ax25_cb *); diff --git a/net/ax25/ax25_dev.c 
> b/net/ax25/ax25_dev.c index 528c874..a7a0e0c 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -82,7 +82,7 @@ void ax25_dev_device_up(struct net_device *dev)
>  	ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT;
>  
>  #if defined(CONFIG_AX25_DAMA_SLAVE) || 
> defined(CONFIG_AX25_DAMA_MASTER)
> -	init_timer(&ax25_dev->dama.slave_timer);
> +	ax25_ds_setup_timer(ax25_dev);
>  #endif
>  
>  	spin_lock_bh(&ax25_dev_lock);
> diff --git a/net/ax25/ax25_ds_timer.c 
> b/net/ax25/ax25_ds_timer.c index c4e3b02..2ce79df 100644
> --- a/net/ax25/ax25_ds_timer.c
> +++ b/net/ax25/ax25_ds_timer.c
> @@ -40,13 +40,10 @@ static void ax25_ds_timeout(unsigned long);
>   *	1/10th of a second.
>   */
>  
> -static void ax25_ds_add_timer(ax25_dev *ax25_dev)
> +void ax25_ds_setup_timer(ax25_dev *ax25_dev)
>  {
> -	struct timer_list *t = &ax25_dev->dama.slave_timer;
> -	t->data		= (unsigned long) ax25_dev;
> -	t->function	= &ax25_ds_timeout;
> -	t->expires	= jiffies + HZ;
> -	add_timer(t);
> +	setup_timer(&ax25_dev->dama.slave_timer, ax25_ds_timeout,
> +		    (unsigned long)ax25_dev);
>  }
>  
>  void ax25_ds_del_timer(ax25_dev *ax25_dev) @@ -60,10 +57,9 
> @@ void ax25_ds_set_timer(ax25_dev *ax25_dev)
>  	if (ax25_dev == NULL)		/* paranoia */
>  		return;
>  
> -	del_timer(&ax25_dev->dama.slave_timer);
>  	ax25_dev->dama.slave_timeout =
>  		
> msecs_to_jiffies(ax25_dev->values[AX25_VALUES_DS_TIMEOUT]) / 10;
> -	ax25_ds_add_timer(ax25_dev);
> +	mod_timer(&ax25_dev->dama.slave_timer, jiffies + HZ);
>  }
>  
>  /*

Re: [PATCH][AX25] ax25_ds_timer: use mod_timer instead of add_timer

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 6 Feb 2008 09:14:13 +0000

> [AX25] ax25_ds_timer: use mod_timer instead of add_timer
> 
> This patch changes current use of: init_timer(), add_timer()
> and del_timer() to setup_timer() with mod_timer(), which
> should be safer anyway.
> 
> 
> Reported-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied, thanks.

Re: [PATCH][AX25] Fwd: SMP with AX.25

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 6 Feb 2008 08:15:09 +0000

> On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
> ...
> > From: Jann Traschewski <jann@gmx.de>
> > Subject: SMP with AX.25
> 
> 
> [AX25] ax25_timer: use mod_timer instead of add_timer
> 
> According to one of Jann's OOPS reports it looks like
> BUG_ON(timer_pending(timer)) triggers during add_timer()
> in ax25_start_t1timer(). This patch changes current use
> of: init_timer(), add_timer() and del_timer() to
> setup_timer() with mod_timer(), which should be safer
> anyway.
> 
> 
> Reported-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied.

[BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
...
> From: Jann Traschewski <jann@gmx.de>
> Subject: SMP with AX.25
> To: jarkao2@gmail.com

According to one of OOPSes reported by Jann softirq can break
while skb is prepared for netif_rx. The report isn't complete,
so the real reason of the later bug could be different, but
IMHO this locking in ax_bump is wrong.

I attach this patch for testing purpose only.

Jarek P.

---

 drivers/net/hamradio/mkiss.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index cfcd15a..30c9b3b 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -289,7 +289,6 @@ static void ax_bump(struct mkiss *ax)
 			*ax->rbuff &= ~0x20;
 		}
  	}
-	spin_unlock_bh(&ax->buflock);
 
 	count = ax->rcount;
 
@@ -297,17 +296,17 @@ static void ax_bump(struct mkiss *ax)
 		printk(KERN_ERR "mkiss: %s: memory squeeze, dropping packet.\n",
 		       ax->dev->name);
 		ax->stats.rx_dropped++;
+		spin_unlock_bh(&ax->buflock);
 		return;
 	}
 
-	spin_lock_bh(&ax->buflock);
 	memcpy(skb_put(skb,count), ax->rbuff, count);
-	spin_unlock_bh(&ax->buflock);
 	skb->protocol = ax25_type_trans(skb, ax->dev);
 	netif_rx(skb);
 	ax->dev->last_rx = jiffies;
 	ax->stats.rx_packets++;
 	ax->stats.rx_bytes += count;
+	spin_unlock_bh(&ax->buflock);
 }
 
 static void kiss_unesc(struct mkiss *ax, unsigned char s)

[BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

On Wed, Feb 06, 2008 at 07:45:29AM +0000, Jarek Poplawski wrote:
...
> From: Jann Traschewski <jann@gmx.de>
> Subject: SMP with AX.25

(testing patch #2)

According to some OOPSes reported by Jann it seems ax25_kick
tries to clone NULL skbs sometimes. Probably there is needed
a sock lock, but until it's checked more this patch should
help to avoid this. There are probably also cases where ax25
->paclen == 0 can happen: let's check for this.

I attach this patch for testing purpose only.

Jarek P.

---

diff -Nurp 2.6.24-/net/ax25/ax25_out.c 2.6.24+/net/ax25/ax25_out.c
--- 2.6.24-/net/ax25/ax25_out.c	2008-01-24 22:58:37.000000000 +0000
+++ 2.6.24+/net/ax25/ax25_out.c	2008-02-07 11:48:39.000000000 +0000
@@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
 	unsigned char *p;
 	int frontlen, len, fragno, ka9qfrag, first = 1;
 
+	if (paclen < 16) {
+		WARN_ON_ONCE(1);
+		kfree_skb(skb);
+		return;
+	}
+
 	if ((skb->len - 1) > paclen) {
 		if (*skb->data == AX25_P_TEXT) {
 			skb_pull(skb, 1); /* skip PID */
@@ -263,6 +269,8 @@ void ax25_kick(ax25_cb *ax25)
 	 * Dequeue the frame and copy it.
 	 */
 	skb  = skb_dequeue(&ax25->write_queue);
+	if (!skb)
+		return;
 
 	do {
 		if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {

Re: [BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

> ---------- Forwarded message ----------
> From: Jann Traschewski <jann@gmx.de>
> Date: Thu, 7 Feb 2008 06:27:38 +0100
> Subject: FW: smp kernel oops ax25 2008-02-07
> To: Jarek Poplawski <jarkao2@gmail.com>
...
> BUG: unable to handle kernel NULL pointer dereference at virtual address
> 00000065
> printing eip: c02266c7 *pde = 00000000
> Oops: 0000 [#1] SMP
> Modules linked in: netconsole ppp_deflate zlib_deflate zlib_inflate bsd_comp
> ppp_async ppp_generic slhc tun bitrev crc32 mkiss ax25 crc16 iptable_nat
> nf_nat nf_conntrack_ipv4 xt_state nf_conntrack ipt_REJECT iptable_filter
> iptable_mangle xt_MARK ipv6 ipip tunnel4 ide_cd cdrom aic7xxx
> scsi_transport_spi parport_serial parport_pc parport i2c_piix4 genrtc
> 
> Pid: 19761, comm: frmaster Not tainted (2.6.24-dg8ngn #1)
> EIP: 0060:[<c02266c7>] EFLAGS: 00010297 CPU: 1
> EIP is at skb_clone+0x3/0x4d
> EAX: 00000000 EBX: 00000000 ECX: 00000000 EDX: 00000020
> ESI: 00000008 EDI: 00000000 EBP: c0cfc8ac ESP: d3cfddc4
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> Process frmaster (pid: 19761, ti=d3cfc000 task=f7fbf570 task.ti=d3cfc000)
> Stack: c0cfc800 f8a0a8f9 c0cfc800 d3cfdf6c 00000007 00000015 c0cfc8cc
> e84492e0
>        c0cfc800 d3cfdf64 f6e6fbc0 f8a0e9d8 d3cfde60 00000000 c1d8c340
> d3cfdea0
>        c0cfc800 f5d12200 d3cfdf24 c014143d 01e7a9be 00000000 d3cfdf24
> 00000022
> Call Trace:
>  [<f8a0a8f9>] ax25_kick+0xaf/0x184 [ax25]
>  [<f8a0e9d8>] ax25_sendmsg+0x35f/0x49a [ax25]
>  [<c014143d>] __generic_file_aio_write_nolock+0x474/0x4d3
>  [<c0221214>] sock_aio_write+0xbc/0xc8
>  [<c01414f7>] generic_file_aio_write+0x5b/0xb0
>  [<c015a1d3>] do_sync_write+0xc7/0x10a
>  [<c022b643>] dev_hard_start_xmit+0x20a/0x26a
>  [<c012db6d>] autoremove_wake_function+0x0/0x35
>  [<c01253d3>] lock_timer_base+0x19/0x35
>  [<c015a960>] vfs_write+0x9e/0x10c
>  [<c015aeb7>] sys_write+0x41/0x67
>  [<c0103e4e>] syscall_call+0x7/0xb
>  =======================
> Code: 24 20 89 7c 24 08 89 44 24 04 03 aa 9c 00 00 00 89 2c 24 e8 cc 67 f9
> ff 89 44 24 54 8b 44 24 54 83 c4 3c 5b 5e 5f 5d c3 53 89 c3 <8a> 40 65 24 18
> 3c 08 75 1f 8d 8b a8 00 00 00 f6 41 65 18 75 13
> EIP: [<c02266c7>] skb_clone+0x3/0x4d SS:ESP 0068:d3cfddc4
> ---[ end trace 504af05b5c529aa1 ]---

Re: [BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

Here I resend another OOPS I got from Jann:

On Thu, Feb 07, 2008 at 05:42:42PM +0100, Jann Traschewski wrote:
...
> BUG: unable to handle kernel NULL pointer dereference at virtual address
> 00000065
> printing eip: c02266c7 *pde = 00000000
> Oops: 0000 [#1] SMP
> Modules linked in: netconsole ppp_deflate zlib_deflate zlib_inflate bsd_comp
> ppp_async ppp_generic slhc tun bitrev crc32 mkiss ax25 crc16 iptable_nat
> nf_nat nf_conntrack_ipv4 xt_state nf_conntrack ipt_REJECT iptable_filter
> iptable_mangle xt_MARK ipv6 ipip tunnel4 ide_cd cdrom aic7xxx
> scsi_transport_spi parport_serial parport_pc parport i2c_piix4 genrtc
> 
> Pid: 3035, comm: linuxnet Not tainted (2.6.24-dg8ngn #1)
> EIP: 0060:[<c02266c7>] EFLAGS: 00010202 CPU: 0
> EIP is at skb_clone+0x3/0x4d
> EAX: 00000000 EBX: 00000000 ECX: 00000001 EDX: 00000020
> ESI: 00000008 EDI: 00000000 EBP: f700a4ac ESP: f720b9ac
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> Process linuxnet (pid: 3035, ti=f720a000 task=f7c63570 task.ti=f720a000)
> Stack: f700a400 f8a0a8f9 c01254db 00000000 00000000 f700a400 f700a4cc
> f700a400 __wake_up_common+0x32/0x5c
> __wake_up+0x32/0x42
>  [<c0224e67>] sock_def_readable+0x39/0x63
>  [<c02251e1>] sock_queue_rcv_skb+0xb6/0xd1
>  [<c022b060>] netif_receive_skb+0x309/0x372
>  [<c0117f35>] dequeue_entity+0xb/0x2a
>  [<c022d42f>] process_backlog+0x5c/0xaa
>  [<c022cf21>] net_rx_action+0x8d/0x173
>  [<c012213a>] __do_softirq+0x5d/0xc1
>  [<c01221d0>] do_softirq+0x32/0x36
>  [<c01223df>] local_bh_enable_ip+0x35/0x40
>  [<c0261ebe>] udp_poll+0xc1/0xd5
>  [<c0220f2c>] sock_poll+0xc/0xe
>  [<c016495a>] do_select+0x229/0x3c9
>  [<c01650b6>] __pollwait+0x0/0xac
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c0118e73>] default_wake_function+0x0/0x8
>  [<c011756c>] __wake_up_common+0x32/0x5c
>  [<c011911c>] __wake_up+0x32/0x42
>  [<c0164da0>] core_sys_select+0x2a6/0x2c7
>  [<c011756c>] __wake_up_common+0x32/0x5c
>  [<c011911c>] __wake_up+0x32/0x42
>  [<c01d13c2>] tty_wakeup+0x4c/0x50
>  [<c01d72c6>] pty_unthrottle+0x12/0x1d
>  [<f89fc60e>] mkiss_receive_buf+0x381/0x3af [mkiss]
> sys_select+0xa4/0x187
> syscall_call+0x7/0xb
> Kernel panic - not syncing: Fatal exception in interrupt

Re: [BUG][AX25] Fwd: SMP with AX.25

[Jarek Poplawski]

On Thu, Feb 07, 2008 at 08:35:11PM +0100, Jarek Poplawski wrote:
> Here I resend another OOPS I got from Jann:
> 
> On Thu, Feb 07, 2008 at 05:42:42PM +0100, Jann Traschewski wrote:
> ...
> > BUG: unable to handle kernel NULL pointer dereference at virtual address
> > 00000065
> > printing eip: c02266c7 *pde = 00000000
> > Oops: 0000 [#1] SMP
> > Modules linked in: netconsole ppp_deflate zlib_deflate zlib_inflate bsd_comp
> > ppp_async ppp_generic slhc tun bitrev crc32 mkiss ax25 crc16 iptable_nat
> > nf_nat nf_conntrack_ipv4 xt_state nf_conntrack ipt_REJECT iptable_filter
> > iptable_mangle xt_MARK ipv6 ipip tunnel4 ide_cd cdrom aic7xxx
> > scsi_transport_spi parport_serial parport_pc parport i2c_piix4 genrtc
> > 
> > Pid: 3035, comm: linuxnet Not tainted (2.6.24-dg8ngn #1)
> > EIP: 0060:[<c02266c7>] EFLAGS: 00010202 CPU: 0
> > EIP is at skb_clone+0x3/0x4d
> > EAX: 00000000 EBX: 00000000 ECX: 00000001 EDX: 00000020
> > ESI: 00000008 EDI: 00000000 EBP: f700a4ac ESP: f720b9ac
> >  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> > Process linuxnet (pid: 3035, ti=f720a000 task=f7c63570 task.ti=f720a000)
> > Stack: f700a400 f8a0a8f9 c01254db 00000000 00000000 f700a400 f700a4cc
> > f700a400 __wake_up_common+0x32/0x5c
> > __wake_up+0x32/0x42

Jann, this report is a bit damaged here, so I could be wrong, but it
looks similar to your first reports, and I guess this skb_clone is
called from ax25_kick too. Then my today testing patch #2 should help
for this, I hope.

Jarek P.

[PATCH][AX25] mkiss: ax_bump() locking fix

[Jarek Poplawski]

Hi,

This is unchanged patch previously sent here for testing.
I think it should be applied.

Thanks,
Jarek P.

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

Subject: [PATCH][AX25] mkiss: ax_bump() locking fix

According to one of OOPSes reported by Jann softirq can break
while skb is prepared for netif_rx. The report isn't complete,
so the real reason of the later bug could be different, but
IMHO this locking break in ax_bump is unsafe and unnecessary.

Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 drivers/net/hamradio/mkiss.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index cfcd15a..30c9b3b 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -289,7 +289,6 @@ static void ax_bump(struct mkiss *ax)
 			*ax->rbuff &= ~0x20;
 		}
  	}
-	spin_unlock_bh(&ax->buflock);
 
 	count = ax->rcount;
 
@@ -297,17 +296,17 @@ static void ax_bump(struct mkiss *ax)
 		printk(KERN_ERR "mkiss: %s: memory squeeze, dropping packet.\n",
 		       ax->dev->name);
 		ax->stats.rx_dropped++;
+		spin_unlock_bh(&ax->buflock);
 		return;
 	}
 
-	spin_lock_bh(&ax->buflock);
 	memcpy(skb_put(skb,count), ax->rbuff, count);
-	spin_unlock_bh(&ax->buflock);
 	skb->protocol = ax25_type_trans(skb, ax->dev);
 	netif_rx(skb);
 	ax->dev->last_rx = jiffies;
 	ax->stats.rx_packets++;
 	ax->stats.rx_bytes += count;
+	spin_unlock_bh(&ax->buflock);
 }
 
 static void kiss_unesc(struct mkiss *ax, unsigned char s)

Re: [PATCH][AX25] mkiss: ax_bump() locking fix

[Jeff Garzik]

Jarek Poplawski wrote:
> Hi,
> 
> This is unchanged patch previously sent here for testing.
> I think it should be applied.
> 
> Thanks,
> Jarek P.
> 
> ---------------->
> 
> Subject: [PATCH][AX25] mkiss: ax_bump() locking fix
> 
> According to one of OOPSes reported by Jann softirq can break
> while skb is prepared for netif_rx. The report isn't complete,
> so the real reason of the later bug could be different, but
> IMHO this locking break in ax_bump is unsafe and unnecessary.
> 
> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> ---
> 
>  drivers/net/hamradio/mkiss.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)

applied

[PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Jarek Poplawski]

Hi,

Here is an "official" version of "testing patch #2" from this thread.
The only difference: ax25->vs is changed only after checking skb is
not NULL (plus a comment). IMHO it could be applied.

Thanks,
Jarek P.

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

Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()

According to some OOPS reports ax25_kick tries to clone NULL skbs
sometimes. It looks like a race with ax25_clear_queues(). Probably
there is no need to add more than a simple check for this yet.
Another report suggested there are probably also cases where ax25
->paclen == 0 can happen in ax25_output(); this wasn't confirmed
during testing but let's leave this debugging check for some time.


Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp 2.6.24-mm1-/net/ax25/ax25_out.c 2.6.24-mm1+/net/ax25/ax25_out.c
--- 2.6.24-mm1-/net/ax25/ax25_out.c	2008-01-24 22:58:37.000000000 +0000
+++ 2.6.24-mm1+/net/ax25/ax25_out.c	2008-02-13 10:43:50.000000000 +0000
@@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
 	unsigned char *p;
 	int frontlen, len, fragno, ka9qfrag, first = 1;
 
+	if (paclen < 16) {
+		WARN_ON_ONCE(1);
+		kfree_skb(skb);
+		return;
+	}
+
 	if ((skb->len - 1) > paclen) {
 		if (*skb->data == AX25_P_TEXT) {
 			skb_pull(skb, 1); /* skip PID */
@@ -251,8 +257,6 @@ void ax25_kick(ax25_cb *ax25)
 	if (start == end)
 		return;
 
-	ax25->vs = start;
-
 	/*
 	 * Transmit data until either we're out of data to send or
 	 * the window is full. Send a poll on the final I frame if
@@ -261,8 +265,13 @@ void ax25_kick(ax25_cb *ax25)
 
 	/*
 	 * Dequeue the frame and copy it.
+	 * Check for race with ax25_clear_queues().
 	 */
 	skb  = skb_dequeue(&ax25->write_queue);
+	if (!skb)
+		return;
+
+	ax25->vs = start;
 
 	do {
 		if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Jann Traschewski]

Applied and stable with Kernel 2.6.24.2 since 12 hours.
Regards,
Jann

> -----Ursprüngliche Nachricht-----
> Von: Jarek Poplawski [mailto:jarkao2@gmail.com] 
> Gesendet: Mittwoch, 13. Februar 2008 12:56
> An: David Miller
> Cc: Jann Traschewski; Bernard Pidoux F6BVP; Ralf Baechle; 
> netdev@vger.kernel.org
> Betreff: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
> 
> Hi,
> 
> Here is an "official" version of "testing patch #2" from this thread.
> The only difference: ax25->vs is changed only after checking 
> skb is not NULL (plus a comment). IMHO it could be applied.
> 
> Thanks,
> Jarek P.
> 
> ---------------->
> 
> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
> 
> According to some OOPS reports ax25_kick tries to clone NULL 
> skbs sometimes. It looks like a race with 
> ax25_clear_queues(). Probably there is no need to add more 
> than a simple check for this yet.
> Another report suggested there are probably also cases where ax25
> ->paclen == 0 can happen in ax25_output(); this wasn't confirmed
> during testing but let's leave this debugging check for some time.
> 
> 
> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> ---
> 
> diff -Nurp 2.6.24-mm1-/net/ax25/ax25_out.c 
> 2.6.24-mm1+/net/ax25/ax25_out.c
> --- 2.6.24-mm1-/net/ax25/ax25_out.c	2008-01-24 
> 22:58:37.000000000 +0000
> +++ 2.6.24-mm1+/net/ax25/ax25_out.c	2008-02-13 
> 10:43:50.000000000 +0000
> @@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
>  	unsigned char *p;
>  	int frontlen, len, fragno, ka9qfrag, first = 1;
>  
> +	if (paclen < 16) {
> +		WARN_ON_ONCE(1);
> +		kfree_skb(skb);
> +		return;
> +	}
> +
>  	if ((skb->len - 1) > paclen) {
>  		if (*skb->data == AX25_P_TEXT) {
>  			skb_pull(skb, 1); /* skip PID */
> @@ -251,8 +257,6 @@ void ax25_kick(ax25_cb *ax25)
>  	if (start == end)
>  		return;
>  
> -	ax25->vs = start;
> -
>  	/*
>  	 * Transmit data until either we're out of data to send or
>  	 * the window is full. Send a poll on the final I frame 
> if @@ -261,8 +265,13 @@ void ax25_kick(ax25_cb *ax25)
>  
>  	/*
>  	 * Dequeue the frame and copy it.
> +	 * Check for race with ax25_clear_queues().
>  	 */
>  	skb  = skb_dequeue(&ax25->write_queue);
> +	if (!skb)
> +		return;
> +
> +	ax25->vs = start;
>  
>  	do {
>  		if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Pidoux]

Jann Traschewski a écrit :
> Applied and stable with Kernel 2.6.24.2 since 12 hours.
> Regards,
> Jann
>
>   
>> -----Ursprüngliche Nachricht-----
>> Von: Jarek Poplawski [mailto:jarkao2@gmail.com] 
>> Gesendet: Mittwoch, 13. Februar 2008 12:56
>> An: David Miller
>> Cc: Jann Traschewski; Bernard Pidoux F6BVP; Ralf Baechle; 
>> netdev@vger.kernel.org
>> Betreff: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
>>
>> Hi,
>>
>> Here is an "official" version of "testing patch #2" from this thread.
>> The only difference: ax25->vs is changed only after checking 
>> skb is not NULL (plus a comment). IMHO it could be applied.
>>
>> Thanks,
>> Jarek P.
>>
>> ---------------->
>>
>> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
>>
>> According to some OOPS reports ax25_kick tries to clone NULL 
>> skbs sometimes. It looks like a race with 
>> ax25_clear_queues(). Probably there is no need to add more 
>> than a simple check for this yet.
>> Another report suggested there are probably also cases where ax25
>> ->paclen == 0 can happen in ax25_output(); this wasn't confirmed
>> during testing but let's leave this debugging check for some time.
>>
>>
>> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
>> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>>
>> ---
>>
>> diff -Nurp 2.6.24-mm1-/net/ax25/ax25_out.c 
>> 2.6.24-mm1+/net/ax25/ax25_out.c
>> --- 2.6.24-mm1-/net/ax25/ax25_out.c	2008-01-24 
>> 22:58:37.000000000 +0000
>> +++ 2.6.24-mm1+/net/ax25/ax25_out.c	2008-02-13 
>> 10:43:50.000000000 +0000
>> @@ -117,6 +117,12 @@ void ax25_output(ax25_cb *ax25, int pacl
>>  	unsigned char *p;
>>  	int frontlen, len, fragno, ka9qfrag, first = 1;
>>  
>> +	if (paclen < 16) {
>> +		WARN_ON_ONCE(1);
>> +		kfree_skb(skb);
>> +		return;
>> +	}
>> +
>>  	if ((skb->len - 1) > paclen) {
>>  		if (*skb->data == AX25_P_TEXT) {
>>  			skb_pull(skb, 1); /* skip PID */
>> @@ -251,8 +257,6 @@ void ax25_kick(ax25_cb *ax25)
>>  	if (start == end)
>>  		return;
>>  
>> -	ax25->vs = start;
>> -
>>  	/*
>>  	 * Transmit data until either we're out of data to send or
>>  	 * the window is full. Send a poll on the final I frame 
>> if @@ -261,8 +265,13 @@ void ax25_kick(ax25_cb *ax25)
>>  
>>  	/*
>>  	 * Dequeue the frame and copy it.
>> +	 * Check for race with ax25_clear_queues().
>>  	 */
>>  	skb  = skb_dequeue(&ax25->write_queue);
>> +	if (!skb)
>> +		return;
>> +
>> +	ax25->vs = start;
>>  
>>  	do {
>>  		if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
>>     
>
>
>
>   
Applied too and stable with kernel 2.6.24.3 since a week.

Thanks Jarek.

Bernard Pidoux

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Jarek Poplawski]

On Sun, Mar 09, 2008 at 10:02:39AM +0100, Pidoux wrote:
...
>>> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
...
> Applied too and stable with kernel 2.6.24.3 since a week.

Good to know! It's merged to 2.6.25-rc3, but maybe recommending it for
2.6.24 wouldn't be a bad idea...

Thanks Bernard,
Jarek P.

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Jann Traschewski]

Indeed. I'd like to see it in 2.6.24. My heavy used machine has no problems
with this patch since 13 days now.
Thank you,
Jann

> -----Ursprüngliche Nachricht-----
> Von: Jarek Poplawski [mailto:jarkao2@gmail.com] 
> Gesendet: Sonntag, 9. März 2008 15:31
> An: Pidoux
> Cc: Jann Traschewski; 'David Miller'; 'Ralf Baechle'; 
> netdev@vger.kernel.org
> Betreff: Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()
> 
> On Sun, Mar 09, 2008 at 10:02:39AM +0100, Pidoux wrote:
> ...
> >>> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
> ...
> > Applied too and stable with kernel 2.6.24.3 since a week.
> 
> Good to know! It's merged to 2.6.25-rc3, but maybe recommending it for
> 2.6.24 wouldn't be a bad idea...
> 
> Thanks Bernard,
> Jarek P.

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[Jarek Poplawski]

On Sun, Mar 09, 2008 at 06:34:47PM +0100, Jann Traschewski wrote:
> Indeed. I'd like to see it in 2.6.24. My heavy used machine has no problems
> with this patch since 13 days now.

I hope David or Ralf will consider this.

BTW, Jann, I guess we're waiting with this last AX.25 testing patch
for the magic 13 too?

Thanks,
Jarek P.

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 9 Mar 2008 15:30:52 +0100

> On Sun, Mar 09, 2008 at 10:02:39AM +0100, Pidoux wrote:
> ...
> >>> Subject: [AX25] ax25_out: check skb for NULL in ax25_kick()
> ...
> > Applied too and stable with kernel 2.6.24.3 since a week.
> 
> Good to know! It's merged to 2.6.25-rc3, but maybe recommending it for
> 2.6.24 wouldn't be a bad idea...

I've just queued this up for the next round of fixes I will
send to the -stable folks.

Re: [PATCH][AX25] ax25_out: check skb for NULL in ax25_kick()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 13 Feb 2008 11:56:07 +0000

> [AX25] ax25_out: check skb for NULL in ax25_kick()
> 
> According to some OOPS reports ax25_kick tries to clone NULL skbs
> sometimes. It looks like a race with ax25_clear_queues(). Probably
> there is no need to add more than a simple check for this yet.
> Another report suggested there are probably also cases where ax25
> ->paclen == 0 can happen in ax25_output(); this wasn't confirmed
> during testing but let's leave this debugging check for some time.
> 
> Reported-and-tested-by: Jann Traschewski <jann@gmx.de>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied, thanks Jarek.