Skip to content

Commit

Permalink
NET: Fix locking issues in PPP, 6pack, mkiss and strip line disciplines.
Browse files Browse the repository at this point in the history 
Guido Trentalancia reports:

I am trying to use the kiss driver in the Linux kernel that is being
shipped with Fedora 10 but unfortunately I get the following oops:

mkiss: AX.25 Multikiss, Hans Albas PE1AYX
mkiss: ax0: crc mode is auto.
ADDRCONF(NETDEV_CHANGE): ax0: link becomes ready
------------[ cut here ]------------
WARNING: at kernel/softirq.c:77 __local_bh_disable+0x2f/0x83() (Not
tainted)
[...]
unloaded: microcode]
Pid: 0, comm: swapper Not tainted 2.6.27.25-170.2.72.fc10.i686 #1
 [<c042ddfb>] warn_on_slowpath+0x65/0x8b
 [<c06ab62b>] ? _spin_unlock_irqrestore+0x22/0x38
 [<c04228b4>] ? __enqueue_entity+0xe3/0xeb
 [<c042431e>] ? enqueue_entity+0x203/0x20b
 [<c0424361>] ? enqueue_task_fair+0x3b/0x3f
 [<c041f88c>] ? resched_task+0x3a/0x6e
 [<c06ab62b>] ? _spin_unlock_irqrestore+0x22/0x38
 [<c06ab4e2>] ? _spin_lock_bh+0xb/0x16
 [<c043255b>] __local_bh_disable+0x2f/0x83
 [<c04325ba>] local_bh_disable+0xb/0xd
 [<c06ab4e2>] _spin_lock_bh+0xb/0x16
 [<f8b6f600>] mkiss_receive_buf+0x2fb/0x3a6 [mkiss]
 [<c0572a30>] flush_to_ldisc+0xf7/0x198
 [<c0572b12>] tty_flip_buffer_push+0x41/0x51
 [<f89477f2>] ftdi_process_read+0x375/0x4ad [ftdi_sio]
 [<f8947a5a>] ftdi_read_bulk_callback+0x130/0x138 [ftdi_sio]
 [<c05d4bec>] usb_hcd_giveback_urb+0x63/0x93
 [<c05ea290>] uhci_giveback_urb+0xe5/0x15f
 [<c05eaabf>] uhci_scan_schedule+0x52e/0x767
 [<c05f6288>] ? psmouse_handle_byte+0xc/0xe5
 [<c054df78>] ? acpi_ev_gpe_detect+0xd6/0xe1
 [<c05ec5b0>] uhci_irq+0x110/0x125
 [<c05d4834>] usb_hcd_irq+0x40/0xa3
 [<c0465313>] handle_IRQ_event+0x2f/0x64
 [<c046642b>] handle_level_irq+0x74/0xbe
 [<c04663b7>] ? handle_level_irq+0x0/0xbe
 [<c0406e6e>] do_IRQ+0xc7/0xfe
 [<c0405668>] common_interrupt+0x28/0x30
 [<c056821a>] ? acpi_idle_enter_simple+0x162/0x19d
 [<c0617f52>] cpuidle_idle_call+0x60/0x92
 [<c0403c61>] cpu_idle+0x101/0x134
 [<c069b1ba>] rest_init+0x4e/0x50
 =======================
---[ end trace b7cc8076093467ad ]---
------------[ cut here ]------------
WARNING: at kernel/softirq.c:136 _local_bh_enable_ip+0x3d/0xc4()
[...]
Pid: 0, comm: swapper Tainted: G        W 2.6.27.25-170.2.72.fc10.i686
 [<c042ddfb>] warn_on_slowpath+0x65/0x8b
 [<c06ab62b>] ? _spin_unlock_irqrestore+0x22/0x38
 [<c04228b4>] ? __enqueue_entity+0xe3/0xeb
 [<c042431e>] ? enqueue_entity+0x203/0x20b
 [<c0424361>] ? enqueue_task_fair+0x3b/0x3f
 [<c041f88c>] ? resched_task+0x3a/0x6e
 [<c06ab62b>] ? _spin_unlock_irqrestore+0x22/0x38
 [<c06ab4e2>] ? _spin_lock_bh+0xb/0x16
 [<f8b6f642>] ? mkiss_receive_buf+0x33d/0x3a6 [mkiss]
 [<c04325f9>] _local_bh_enable_ip+0x3d/0xc4
 [<c0432688>] local_bh_enable_ip+0x8/0xa
 [<c06ab54d>] _spin_unlock_bh+0x11/0x13
 [<f8b6f642>] mkiss_receive_buf+0x33d/0x3a6 [mkiss]
 [<c0572a30>] flush_to_ldisc+0xf7/0x198
 [<c0572b12>] tty_flip_buffer_push+0x41/0x51
 [<f89477f2>] ftdi_process_read+0x375/0x4ad [ftdi_sio]
 [<f8947a5a>] ftdi_read_bulk_callback+0x130/0x138 [ftdi_sio]
 [<c05d4bec>] usb_hcd_giveback_urb+0x63/0x93
 [<c05ea290>] uhci_giveback_urb+0xe5/0x15f
 [<c05eaabf>] uhci_scan_schedule+0x52e/0x767
 [<c05f6288>] ? psmouse_handle_byte+0xc/0xe5
 [<c054df78>] ? acpi_ev_gpe_detect+0xd6/0xe1
 [<c05ec5b0>] uhci_irq+0x110/0x125
 [<c05d4834>] usb_hcd_irq+0x40/0xa3
 [<c0465313>] handle_IRQ_event+0x2f/0x64
 [<c046642b>] handle_level_irq+0x74/0xbe
 [<c04663b7>] ? handle_level_irq+0x0/0xbe
 [<c0406e6e>] do_IRQ+0xc7/0xfe
 [<c0405668>] common_interrupt+0x28/0x30
 [<c056821a>] ? acpi_idle_enter_simple+0x162/0x19d
 [<c0617f52>] cpuidle_idle_call+0x60/0x92
 [<c0403c61>] cpu_idle+0x101/0x134
 [<c069b1ba>] rest_init+0x4e/0x50
 =======================
---[ end trace b7cc8076093467ad ]---
mkiss: ax0: Trying crc-smack
mkiss: ax0: Trying crc-flexnet

The issue was, that the locking code in mkiss was assuming it was only
ever being called in process or bh context.  Fixed by converting the
involved locking code to use irq-safe locks.

Review of other networking line disciplines shows that 6pack, both sync
and async PPP and STRIP have similar issues.  The ppp_async one is the
most interesting one as it sorts out half of the issue as far back as
2004 in commit http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=2996d8deaeddd01820691a872550dc0cfba0c37d

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Reported-by: Guido Trentalancia <guido@trentalancia.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Ralf Baechle authored and David S. Miller committed Jul 13, 2009
1 parent 635ecaa commit adeab1a
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 44 deletions.
10 changes: 6 additions & 4 deletions drivers/net/hamradio/6pack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,13 +398,14 @@ static DEFINE_RWLOCK(disc_data_lock);

static struct sixpack *sp_get(struct tty_struct *tty)
{
unsigned long flags;
struct sixpack *sp;

read_lock(&disc_data_lock);
read_lock_irqsave(&disc_data_lock, flags);
sp = tty->disc_data;
if (sp)
atomic_inc(&sp->refcnt);
read_unlock(&disc_data_lock);
read_unlock_irqrestore(&disc_data_lock, flags);

return sp;
}
Expand Down Expand Up @@ -688,12 +689,13 @@ static int sixpack_open(struct tty_struct *tty)
*/
static void sixpack_close(struct tty_struct *tty)
{
unsigned long flags;
struct sixpack *sp;

write_lock(&disc_data_lock);
write_lock_irqsave(&disc_data_lock, flags);
sp = tty->disc_data;
tty->disc_data = NULL;
write_unlock(&disc_data_lock);
write_unlock_irqrestore(&disc_data_lock, flags);
if (!sp)
return;

Expand Down
41 changes: 24 additions & 17 deletions drivers/net/hamradio/mkiss.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,15 +244,16 @@ static int kiss_esc_crc(unsigned char *s, unsigned char *d, unsigned short crc,
/* Send one completely decapsulated AX.25 packet to the AX.25 layer. */
static void ax_bump(struct mkiss *ax)
{
unsigned long flags;
struct sk_buff *skb;
int count;

spin_lock_bh(&ax->buflock);
spin_lock_irqsave(&ax->buflock, flags);
if (ax->rbuff[0] > 0x0f) {
if (ax->rbuff[0] & 0x80) {
if (check_crc_16(ax->rbuff, ax->rcount) < 0) {
ax->dev->stats.rx_errors++;
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);

return;
}
Expand All @@ -267,7 +268,7 @@ static void ax_bump(struct mkiss *ax)
} else if (ax->rbuff[0] & 0x20) {
if (check_crc_flex(ax->rbuff, ax->rcount) < 0) {
ax->dev->stats.rx_errors++;
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
return;
}
if (ax->crcmode != CRC_MODE_FLEX && ax->crcauto) {
Expand All @@ -294,7 +295,7 @@ static void ax_bump(struct mkiss *ax)
printk(KERN_ERR "mkiss: %s: memory squeeze, dropping packet.\n",
ax->dev->name);
ax->dev->stats.rx_dropped++;
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
return;
}

Expand All @@ -303,11 +304,13 @@ static void ax_bump(struct mkiss *ax)
netif_rx(skb);
ax->dev->stats.rx_packets++;
ax->dev->stats.rx_bytes += count;
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
}

static void kiss_unesc(struct mkiss *ax, unsigned char s)
{
unsigned long flags;

switch (s) {
case END:
/* drop keeptest bit = VSV */
Expand All @@ -334,18 +337,18 @@ static void kiss_unesc(struct mkiss *ax, unsigned char s)
break;
}

spin_lock_bh(&ax->buflock);
spin_lock_irqsave(&ax->buflock, flags);
if (!test_bit(AXF_ERROR, &ax->flags)) {
if (ax->rcount < ax->buffsize) {
ax->rbuff[ax->rcount++] = s;
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
return;
}

ax->dev->stats.rx_over_errors++;
set_bit(AXF_ERROR, &ax->flags);
}
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
}

static int ax_set_mac_address(struct net_device *dev, void *addr)
Expand All @@ -367,6 +370,7 @@ static void ax_changedmtu(struct mkiss *ax)
{
struct net_device *dev = ax->dev;
unsigned char *xbuff, *rbuff, *oxbuff, *orbuff;
unsigned long flags;
int len;

len = dev->mtu * 2;
Expand All @@ -392,7 +396,7 @@ static void ax_changedmtu(struct mkiss *ax)
return;
}

spin_lock_bh(&ax->buflock);
spin_lock_irqsave(&ax->buflock, flags);

oxbuff = ax->xbuff;
ax->xbuff = xbuff;
Expand Down Expand Up @@ -423,7 +427,7 @@ static void ax_changedmtu(struct mkiss *ax)
ax->mtu = dev->mtu + 73;
ax->buffsize = len;

spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);

kfree(oxbuff);
kfree(orbuff);
Expand All @@ -433,6 +437,7 @@ static void ax_changedmtu(struct mkiss *ax)
static void ax_encaps(struct net_device *dev, unsigned char *icp, int len)
{
struct mkiss *ax = netdev_priv(dev);
unsigned long flags;
unsigned char *p;
int actual, count;

Expand All @@ -449,7 +454,7 @@ static void ax_encaps(struct net_device *dev, unsigned char *icp, int len)

p = icp;

spin_lock_bh(&ax->buflock);
spin_lock_irqsave(&ax->buflock, flags);
if ((*p & 0x0f) != 0) {
/* Configuration Command (kissparms(1).
* Protocol spec says: never append CRC.
Expand Down Expand Up @@ -479,7 +484,7 @@ static void ax_encaps(struct net_device *dev, unsigned char *icp, int len)
ax->crcauto = (cmd ? 0 : 1);
printk(KERN_INFO "mkiss: %s: crc mode %s %d\n", ax->dev->name, (len) ? "set to" : "is", cmd);
}
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);
netif_start_queue(dev);

return;
Expand Down Expand Up @@ -512,7 +517,7 @@ static void ax_encaps(struct net_device *dev, unsigned char *icp, int len)
count = kiss_esc(p, (unsigned char *)ax->xbuff, len);
}
}
spin_unlock_bh(&ax->buflock);
spin_unlock_irqrestore(&ax->buflock, flags);

set_bit(TTY_DO_WRITE_WAKEUP, &ax->tty->flags);
actual = ax->tty->ops->write(ax->tty, ax->xbuff, count);
Expand Down Expand Up @@ -704,13 +709,14 @@ static DEFINE_RWLOCK(disc_data_lock);

static struct mkiss *mkiss_get(struct tty_struct *tty)
{
unsigned long flags;
struct mkiss *ax;

read_lock(&disc_data_lock);
read_lock_irqsave(&disc_data_lock, flags);
ax = tty->disc_data;
if (ax)
atomic_inc(&ax->refcnt);
read_unlock(&disc_data_lock);
read_unlock_irqrestore(&disc_data_lock, flags);

return ax;
}
Expand Down Expand Up @@ -809,12 +815,13 @@ static int mkiss_open(struct tty_struct *tty)

static void mkiss_close(struct tty_struct *tty)
{
unsigned long flags;
struct mkiss *ax;

write_lock(&disc_data_lock);
write_lock_irqsave(&disc_data_lock, flags);
ax = tty->disc_data;
tty->disc_data = NULL;
write_unlock(&disc_data_lock);
write_unlock_irqrestore(&disc_data_lock, flags);

if (!ax)
return;
Expand Down
11 changes: 7 additions & 4 deletions drivers/net/ppp_async.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,15 @@ static DEFINE_RWLOCK(disc_data_lock);

static struct asyncppp *ap_get(struct tty_struct *tty)
{
unsigned long flags;
struct asyncppp *ap;

read_lock(&disc_data_lock);
read_lock_irqsave(&disc_data_lock, flags);
ap = tty->disc_data;
if (ap != NULL)
atomic_inc(&ap->refcnt);
read_unlock(&disc_data_lock);
read_unlock_irqrestore(&disc_data_lock, flags);

return ap;
}

Expand Down Expand Up @@ -215,12 +217,13 @@ ppp_asynctty_open(struct tty_struct *tty)
static void
ppp_asynctty_close(struct tty_struct *tty)
{
unsigned long flags;
struct asyncppp *ap;

write_lock_irq(&disc_data_lock);
write_lock_irqsave(&disc_data_lock, flags);
ap = tty->disc_data;
tty->disc_data = NULL;
write_unlock_irq(&disc_data_lock);
write_unlock_irqrestore(&disc_data_lock, flags);
if (!ap)
return;

Expand Down
11 changes: 7 additions & 4 deletions drivers/net/ppp_synctty.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,15 @@ static DEFINE_RWLOCK(disc_data_lock);

static struct syncppp *sp_get(struct tty_struct *tty)
{
unsigned long flags;
struct syncppp *ap;

read_lock(&disc_data_lock);
read_lock_irqsave(&disc_data_lock, flags);
ap = tty->disc_data;
if (ap != NULL)
atomic_inc(&ap->refcnt);
read_unlock(&disc_data_lock);
read_unlock_irqrestore(&disc_data_lock, flags);

return ap;
}

Expand Down Expand Up @@ -262,12 +264,13 @@ ppp_sync_open(struct tty_struct *tty)
static void
ppp_sync_close(struct tty_struct *tty)
{
unsigned long flags;
struct syncppp *ap;

write_lock_irq(&disc_data_lock);
write_lock_irqsave(&disc_data_lock, flags);
ap = tty->disc_data;
tty->disc_data = NULL;
write_unlock_irq(&disc_data_lock);
write_unlock_irqrestore(&disc_data_lock, flags);
if (!ap)
return;

Expand Down
Loading

0 comments on commit adeab1a

Please sign in to comment.