LinuxLists.cc - [CHECKER] 56 potential lock/unlock bugs in 2.5.8

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Thu, 11 Jul 2002, Dawson Engler wrote:
> ############################################################
> # 2.5.8 specific errors
>
> #
> ---------------------------------------------------------
> [BUG] it seems like one.
> /u2/engler/mc/oses/linux/2.5.8/mm/shmem.c:554:shmem_getpage_locked: ERROR:A_B:506:554:Did not reverse 'spin_lock' [COUNTER=spin_lock:506] [fit=3] [fit_fn=2] [fn_ex=5] [fn_counter=1] [ex=5619] [counter=272] [z = 1.34804760770983] [fn-z = -1.31122013621437]
>
> entry = shmem_alloc_entry (info, idx);
> if (IS_ERR(entry))
> return (void *)entry;
>
> Start --->
> spin_lock (&info->lock);
>
> ... DELETED 42 lines ...
>
> goto wait_retry;
>
> error = move_from_swap_cache(page, idx, mapping);
> if (error < 0) {
> UnlockPage(page);
> Error --->
> return ERR_PTR(error);
> }
>
> swap_free(*entry);

This one is easy:

Index: mm/shmem.c
===================================================================
RCS file: /var/cvs/thunder-2.5/mm/shmem.c,v
retrieving revision 1.3
diff -p -u -r1.3 shmem.c
--- mm/shmem.c 6 Jul 2002 18:17:44 -0000 1.3
+++ mm/shmem.c 11 Jul 2002 21:47:22 -0000
@@ -607,6 +607,7 @@ repeat:
if (error < 0) {
unlock_page(page);
page_cache_release(page);
+ spin_unlock (&info->lock);
return ERR_PTR(error);
}

> ---------------------------------------------------------
> [BUG]
> /u2/engler/mc/oses/linux/2.5.8/drivers/mtd/chips/cfi_cmdset_0001.c:782:do_write_buffer: ERROR:A_B:700:782:Did not reverse 'spin_lock' [COUNTER=spin_lock:700] [fit=3] [fit_fn=1] [fn_ex=5] [fn_counter=1] [ex=5619] [counter=272] [z = 1.34804760770983] [fn-z = -1.31122013621437]
> /* Let's determine this according to the interleave only once */
> status_OK = CMD(0x80);
>
> timeo = jiffies + HZ;
> retry:
> Start --->
> spin_lock_bh(chip->mutex);
>
> ... DELETED 76 lines ...
>
> map->write16 (map, *((__u16*)buf)++, adr+z);
> } else if (cfi_buswidth_is_4()) {
> map->write32 (map, *((__u32*)buf)++, adr+z);
> } else {
> DISABLE_VPP(map);
> Error --->
> return -EINVAL;
> }
> }
> /* GO GO GO */

This one, too:

Index: drivers/mtd/chips/cfi_cmdset_0001.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/mtd/chips/cfi_cmdset_0001.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 cfi_cmdset_0001.c
--- drivers/mtd/chips/cfi_cmdset_0001.c 21 Jun 2002 22:17:29 -0000
1.1.1.1
+++ drivers/mtd/chips/cfi_cmdset_0001.c 11 Jul 2002 21:52:35 -0000
@@ -779,6 +779,7 @@ static inline int do_write_buffer(struct
map->write32 (map, *((__u32*)buf)++, adr+z);
} else {
DISABLE_VPP(map);
+ spin_unlock_bh(chip->mutex);
return -EINVAL;
}
}

I'm on the rest.

Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------

2002-07-11 22:16:34

by Oliver Neukum

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Am Donnerstag, 11. Juli 2002 23:35 schrieb Dawson Engler:
> Hi All,
>
> enclosed is an error log for a checker that warns when a lock/disable
> was not paired with an unlock/enable. These errors could be tricky,
> and they only got a quick inspection, so treat the reports as potential
> rather than guaranteed bugs.
>
> Run over 2.5.8 it found 56 potential errors.
>
> Dawson

Hi,

I checked the USB ones. Two current bugs were found. In one other case
has been heavily updated. In one case the cker was wrong, it seems to have
trouble with goto and the other cases had already been fixed.

Regards
Oliver

2002-07-11 22:29:44

by Andrew Grover

[permalink] [raw]

Subject: RE: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

> From: Dawson Engler [mailto:[email protected]]
> ---------------------------------------------------------
> [BUG] all other case arms call __sti(); however, it may be
> that safe_halt
> does something weird.
> /u2/engler/mc/oses/linux/2.5.8/drivers/acpi/acpi_processor.c:5

#define safe_halt() __asm__ __volatile__("sti; hlt": : :"memory")

I think we're OK for now. I doubt safe_halt will change, but if it does I'll
fix this issue.

Regards -- Andy

2002-07-11 22:31:22

by Andi Kleen

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Dawson Engler <[email protected]> writes:

> [BUG] recheck: seems unlikely, though it does seem that the path is valid.
> /u2/engler/mc/oses/linux/2.5.8/net/ipv6/tcp_ipv6.c:206:tcp_v6_get_port: ERROR:A_B:112:206:Did not reverse 'spin_lock' [COUNTER=spin_lock:112] [fit=3] [fit_fn=11] [fn_ex=2] [fn_counter=1] [ex=5619] [counter=272] [z = 1.34804760770983] [fn-z = -2.25170500701057]
> rover = tcp_port_rover;
> do { rover++;
> if ((rover < low) || (rover > high))
> rover = low;
> head = &tcp_bhash[tcp_bhashfn(rover)];
> Start --->
> spin_lock(&head->lock);
>

} while (--remaining > 0);
tcp_port_rover = rover;
spin_unlock(&tcp_portalloc_lock);

/* Exhausted local port range during search? */
ret = 1;
if (remaining <= 0)
goto fail;

the goto can only hit when the lock hasn't been taken, so not unlocking it
is correct. It just rechecks the loop end condition, but your tool probably
doesn't know that. The wonders of structured programming :-)

-Andi

2002-07-11 23:11:45

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

Here is the whole set.

A copy can be found at
<URL:http://luckynet.dynu.com/~thunder/patches/checker.patch>
The ones I didn't handle can be found at
<URL:http://luckynet.dynu.com/~thunder/patches/checker.text>

Index: mm/shmem.c
===================================================================
RCS file: /var/cvs/thunder-2.5/mm/shmem.c,v
retrieving revision 1.3
diff -p -u -r1.3 shmem.c
--- mm/shmem.c 6 Jul 2002 18:17:44 -0000 1.3
+++ mm/shmem.c 11 Jul 2002 21:47:22 -0000
@@ -607,6 +607,7 @@ repeat:
if (error < 0) {
unlock_page(page);
page_cache_release(page);
+ spin_unlock (&info->lock);
return ERR_PTR(error);
}
Index: drivers/mtd/chips/cfi_cmdset_0001.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/mtd/chips/cfi_cmdset_0001.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 cfi_cmdset_0001.c
--- drivers/mtd/chips/cfi_cmdset_0001.c 21 Jun 2002 22:17:29 -0000
1.1.1.1
+++ drivers/mtd/chips/cfi_cmdset_0001.c 11 Jul 2002 21:52:35 -0000
@@ -779,6 +779,7 @@ static inline int do_write_buffer(struct
map->write32 (map, *((__u32*)buf)++, adr+z);
} else {
DISABLE_VPP(map);
+ spin_unlock_bh(chip->mutex);
return -EINVAL;
}
}
Index: drivers/usb/class/printer.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/usb/class/printer.c,v
retrieving revision 1.2
diff -p -u -r1.2 printer.c
--- drivers/usb/class/printer.c 6 Jul 2002 18:17:14 -0000 1.2
+++ drivers/usb/class/printer.c 11 Jul 2002 21:56:49 -0000
@@ -654,8 +654,12 @@ static ssize_t usblp_write(struct file *
usblp->writeurb->transfer_buffer_length = (count - writecount) < USBLP_BUF_SIZE ?
(count - writecount) : USBLP_BUF_SIZE;

- if (copy_from_user(usblp->writeurb->transfer_buffer, buffer + writecount,
- usblp->writeurb->transfer_buffer_length)) return -EFAULT;
+ if (copy_from_user(usblp->writeurb->transfer_buffer,
+ buffer + writecount,
+ usblp->writeurb->transfer_buffer_length)) {
+ up (&usblp->sem);
+ return -EFAULT;
+ }

usblp->writeurb->dev = usblp->dev;
usblp->wcomplete = 0;
Index: sound/core/pcm_lib.c
===================================================================
RCS file: /var/cvs/thunder-2.5/sound/core/pcm_lib.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 pcm_lib.c
--- sound/core/pcm_lib.c 20 Jun 2002 22:53:51 -0000 1.1.1.1
+++ sound/core/pcm_lib.c 11 Jul 2002 22:00:26 -0000
@@ -1883,7 +1883,7 @@ static snd_pcm_sframes_t snd_pcm_lib_wri
frames = cont;
if (frames == 0 && runtime->status->state == SNDRV_PCM_STATE_PAUSED) {
err = -EPIPE;
- goto _end;
+ goto _end_unlock;
}
snd_assert(frames != 0,
spin_unlock_irq(&runtime->lock);
Index: fs/jfs/jfs_imap.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/jfs/jfs_imap.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 jfs_imap.c
--- fs/jfs/jfs_imap.c 20 Jun 2002 22:53:46 -0000 1.1.1.1
+++ fs/jfs/jfs_imap.c 11 Jul 2002 22:03:17 -0000
@@ -1453,6 +1453,7 @@ int diAlloc(struct inode *pip, boolean_t
iagno = INOTOIAG(inum);
if ((rc = diIAGRead(imap, iagno, &mp))) {
IREAD_UNLOCK(ipimap);
+ AG_UNLOCK(imap, agno);
return (rc);
}
iagp = (iag_t *) mp->data;
Index: drivers/net/tokenring/smctr.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/net/tokenring/smctr.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 smctr.c
--- drivers/net/tokenring/smctr.c 19 Jun 2002 02:11:55 -0000 1.1.1.1
+++ drivers/net/tokenring/smctr.c 11 Jul 2002 22:11:01 -0000
@@ -4582,6 +4582,7 @@ static int smctr_rx_frame(struct net_dev
break;
}

+ sti();
return (err);
}

Index: fs/hpfs/dir.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/hpfs/dir.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 dir.c
--- fs/hpfs/dir.c 19 Jun 2002 02:11:50 -0000 1.1.1.1
+++ fs/hpfs/dir.c 11 Jul 2002 22:12:53 -0000
@@ -211,7 +211,9 @@ struct dentry *hpfs_lookup(struct inode

lock_kernel();
if ((err = hpfs_chk_name((char *)name, &len))) {
- if (err == -ENAMETOOLONG) return ERR_PTR(-ENAMETOOLONG);
+ if (err == -ENAMETOOLONG) {
+ return ERR_PTR(-ENAMETOOLONG);
+ }
goto end_add;
}

Index: sound/pci/rme9652/rme9652.c
===================================================================
RCS file: /var/cvs/thunder-2.5/sound/pci/rme9652/rme9652.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 rme9652.c
--- sound/pci/rme9652/rme9652.c 20 Jun 2002 22:53:51 -0000 1.1.1.1
+++ sound/pci/rme9652/rme9652.c 11 Jul 2002 22:15:24 -0000
@@ -523,6 +523,7 @@ static int rme9652_set_rate(rme9652_t *r
rate = RME9652_DS | RME9652_freq;
break;
default:
+ spin_unlock_irq(&rme9652->lock);
return -EINVAL;
}

Index: drivers/message/i2o/i2o_core.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/message/i2o/i2o_core.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 i2o_core.c
--- drivers/message/i2o/i2o_core.c 19 Jun 2002 02:11:56 -0000 1.1.1.1
+++ drivers/message/i2o/i2o_core.c 11 Jul 2002 22:22:10 -0000
@@ -726,6 +726,7 @@ int i2o_claim_device(struct i2o_device *
I2O_CLAIM_PRIMARY))
{
d->owner = NULL;
+ up(&i2o_configuration_lock);
return -EBUSY;
}
up(&i2o_configuration_lock);
Index: sound/pci/es1968.c
===================================================================
RCS file: /var/cvs/thunder-2.5/sound/pci/es1968.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 es1968.c
--- sound/pci/es1968.c 20 Jun 2002 22:53:55 -0000 1.1.1.1
+++ sound/pci/es1968.c 11 Jul 2002 22:24:04 -0000
@@ -1446,8 +1446,10 @@ static esm_memory_t *snd_es1968_new_memo
__found:
if (buf->size > size) {
esm_memory_t *chunk = kmalloc(sizeof(*chunk), GFP_KERNEL);
- if (chunk == NULL)
+ if (chunk == NULL) {
+ up(&chip->memory_mutex);
return NULL;
+ }
chunk->size = buf->size - size;
chunk->buf = buf->buf + size;
chunk->addr = buf->addr + size;
Index: sound/oss/es1371.c
===================================================================
RCS file: /var/cvs/thunder-2.5/sound/oss/es1371.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 es1371.c
--- sound/oss/es1371.c 19 Jun 2002 02:11:49 -0000 1.1.1.1
+++ sound/oss/es1371.c 11 Jul 2002 22:32:23 -0000
@@ -1345,7 +1345,7 @@ static ssize_t es1371_read(struct file *
return -EFAULT;
down(&s->sem);
if (!s->dma_adc.ready && (ret = prog_dmabuf_adc(s)))
- goto out2;
+ goto out;

add_wait_queue(&s->dma_adc.wait, &wait);
while (count > 0) {
@@ -1423,8 +1423,10 @@ static ssize_t es1371_write(struct file
if (!access_ok(VERIFY_READ, buffer, count))
return -EFAULT;
down(&s->sem);
- if (!s->dma_dac2.ready && (ret = prog_dmabuf_dac2(s)))
+ if (!s->dma_dac2.ready && (ret = prog_dmabuf_dac2(s))) {
+ up(&s->sem);
goto out3;
+ }
ret = 0;
add_wait_queue(&s->dma_dac2.wait, &wait);
while (count > 0) {
Index: net/irda/ircomm/ircomm_core.c
===================================================================
RCS file: /var/cvs/thunder-2.5/net/irda/ircomm/ircomm_core.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 ircomm_core.c
--- net/irda/ircomm/ircomm_core.c 20 Jun 2002 22:53:41 -0000 1.1.1.1
+++ net/irda/ircomm/ircomm_core.c 11 Jul 2002 22:34:37 -0000
@@ -536,6 +536,7 @@ int ircomm_proc_read(char *buf, char **s
self = (struct ircomm_cb *) hashbin_get_next(ircomm);
}
restore_flags(flags);
+ sti();

return len;
}
Index: fs/affs/namei.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/affs/namei.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 namei.c
--- fs/affs/namei.c 19 Jun 2002 02:11:51 -0000 1.1.1.1
+++ fs/affs/namei.c 11 Jul 2002 22:36:41 -0000
@@ -345,10 +345,14 @@ affs_rmdir(struct inode *dir, struct den
lock_kernel();

/* WTF??? */
+ res = -ENOENT;
+
if (!dentry->d_inode)
- return -ENOENT;
+ goto out_unlock;

res = affs_remove_header(dentry);
+
+ out_unlock:
unlock_kernel();
return res;
}
Index: fs/intermezzo/file.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/intermezzo/file.c,v
retrieving revision 1.2
diff -p -u -r1.2 file.c
--- fs/intermezzo/file.c 23 Jun 2002 01:17:59 -0000 1.2
+++ fs/intermezzo/file.c 11 Jul 2002 22:38:24 -0000
@@ -299,12 +299,13 @@ static void presto_apply_write_policy(st
if ( presto_get_permit(file->f_dentry->d_inode) < 0 ) {
EXIT;
/* we must be disconnected, not to worry */
- return;
+ unlock_kernel();
+ return;
}
error = presto_journal_close
(&rec, fset, file, file->f_dentry, &new_file_ver);
presto_put_permit(file->f_dentry->d_inode);
- unlock_kernel();
+ unlock_kernel();
if ( error ) {
printk("presto_close: cannot journal close\n");
/* XXX these errors are really bad */
Index: fs/intermezzo/vfs.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/intermezzo/vfs.c,v
retrieving revision 1.2
diff -p -u -r1.2 vfs.c
--- fs/intermezzo/vfs.c 23 Jun 2002 01:18:00 -0000 1.2
+++ fs/intermezzo/vfs.c 11 Jul 2002 22:41:42 -0000
@@ -1948,6 +1948,7 @@ again: /* look the named file or a pare
error = presto_walk(tmp, &nd);
if ( error && error != -ENOENT ) {
EXIT;
+ unlock_kernel();
return error;
}
if (error == -ENOENT)
@@ -2049,6 +2050,7 @@ int lento_close(unsigned int fd, struct
error = filp_close(filp, files);
} else {
EXIT;
+ unlock_kernel();
return error;
}

Index: drivers/ieee1394/dv1394.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/ieee1394/dv1394.c,v
retrieving revision 1.2
diff -p -u -r1.2 dv1394.c
--- drivers/ieee1394/dv1394.c 22 Jun 2002 01:13:34 -0000 1.2
+++ drivers/ieee1394/dv1394.c 11 Jul 2002 22:46:09 -0000
@@ -2627,6 +2627,7 @@ dv1394_devfs_find( char *name)
}
}
}
+ spin_unlock(&dv1394_devfs_lock);
return NULL;
}

Index: sound/pci/ali5451/ali5451.c
===================================================================
RCS file: /var/cvs/thunder-2.5/sound/pci/ali5451/ali5451.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 ali5451.c
--- sound/pci/ali5451/ali5451.c 19 Jun 2002 02:11:58 -0000 1.1.1.1
+++ sound/pci/ali5451/ali5451.c 11 Jul 2002 22:49:43 -0000
@@ -1440,8 +1440,10 @@ static int snd_ali_capture_prepare(snd_p

unsigned int rate;

- if (codec->revision != ALI_5451_V02)
+ if (codec->revision != ALI_5451_V02) {
+ spin_lock_irqsave(&codec->reg_lock, flags);
return -1;
+ }
rate = snd_ali_get_spdif_in_rate(codec);
if (rate == 0) {
snd_printk("ali_capture_preapre: spdif rate detect err!\n");
Index: drivers/media/video/cpia_pp.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/media/video/cpia_pp.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 cpia_pp.c
--- drivers/media/video/cpia_pp.c 21 Jun 2002 02:28:37 -0000 1.1.1.1
+++ drivers/media/video/cpia_pp.c 11 Jul 2002 22:51:26 -0000
@@ -616,6 +616,7 @@ static void cpia_pp_detach (struct parpo
break;
}
}
+ spin_unlock( &cam_list_lock_pp );
}

static void cpia_pp_attach (struct parport *port)
Index: drivers/usb/media/usbvideo.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/usb/media/usbvideo.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 usbvideo.c
--- drivers/usb/media/usbvideo.c 19 Jun 2002 02:11:55 -0000 1.1.1.1
+++ drivers/usb/media/usbvideo.c 11 Jul 2002 22:54:54 -0000
@@ -1096,6 +1096,7 @@ uvd_t *usbvideo_AllocateDevice(usbvideo_
if (uvd->sbuf[i].urb == NULL) {
err("usb_alloc_urb(%d.) failed.", FRAMES_PER_DESC);
uvd->uvd_used = 0;
+ up(&uvd->lock);
uvd = NULL;
goto allocate_done;
}
@@ -1112,8 +1113,8 @@ uvd_t *usbvideo_AllocateDevice(usbvideo_
* The client is free to overwrite those because we
* return control to the client's probe function right now.
*/
-allocate_done:
up (&uvd->lock);
+ allocate_done:
usbvideo_ClientDecModCount(uvd);
return uvd;
}
Index: drivers/i2c/i2c-core.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/i2c/i2c-core.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 i2c-core.c
--- drivers/i2c/i2c-core.c 21 Jun 2002 22:17:01 -0000 1.1.1.1
+++ drivers/i2c/i2c-core.c 11 Jul 2002 23:01:13 -0000
@@ -231,6 +231,7 @@ int i2c_del_adapter(struct i2c_adapter *
printk(KERN_WARNING "i2c-core.o: can't detach adapter %s "
"while detaching driver %s: driver not "
"detached!",adap->name,drivers[j]->name);
+ ADAP_UNLOCK();
goto ERROR1;
}
DRV_UNLOCK();
@@ -364,6 +365,7 @@ int i2c_del_driver(struct i2c_driver *dr
"not unloaded!",driver->name,
adap->name);
ADAP_UNLOCK();
+ DRV_UNLOCK();
return res;
}
} else {
@@ -388,6 +390,7 @@ int i2c_del_driver(struct i2c_driver *dr
client->addr,
adap->name);
ADAP_UNLOCK();
+ DRV_UNLOCK();
return res;
}
}
Index: drivers/net/irda/ali-ircc.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/net/irda/ali-ircc.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 ali-ircc.c
--- drivers/net/irda/ali-ircc.c 20 Jun 2002 22:53:50 -0000 1.1.1.1
+++ drivers/net/irda/ali-ircc.c 11 Jul 2002 23:03:07 -0000
@@ -2027,11 +2027,11 @@ static int ali_ircc_net_ioctl(struct net
ASSERT(self != NULL, return -1;);

IRDA_DEBUG(2, __FUNCTION__ "(), %s, (cmd=0x%X)\n", dev->name, cmd);
-
+
/* Disable interrupts & save flags */
save_flags(flags);
- cli();
-
+ cli();
+
switch (cmd) {
case SIOCSBANDWIDTH: /* Set bandwidth */
IRDA_DEBUG(1, __FUNCTION__ "(), SIOCSBANDWIDTH\n");
@@ -2040,8 +2040,10 @@ static int ali_ircc_net_ioctl(struct net
* speed, so we still must allow for speed change within
* interrupt context.
*/
- if (!in_interrupt() && !capable(CAP_NET_ADMIN))
+ if (!in_interrupt() && !capable(CAP_NET_ADMIN)) {
+ sti();
return -EPERM;
+ }

ali_ircc_change_speed(self, irq->ifr_baudrate);
break;
Index: drivers/char/rio/riointr.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/char/rio/riointr.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 riointr.c
--- drivers/char/rio/riointr.c 19 Jun 2002 02:11:44 -0000 1.1.1.1
+++ drivers/char/rio/riointr.c 11 Jul 2002 23:04:53 -0000
@@ -155,8 +155,8 @@ struct rio_info * p;
RIOServiceHost(p, HostP, 'p' );
rio_spin_lock( &HostP->HostLock);
HostP->InIntr = 0;
- rio_spin_unlock (&HostP->HostLock);
}
+ rio_spin_unlock (&HostP->HostLock);
}
rio_spin_unlock (&p->RIOIntrSem);
}
Index: drivers/usb/media/pwc-if.c
===================================================================
RCS file: /var/cvs/thunder-2.5/drivers/usb/media/pwc-if.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 pwc-if.c
--- drivers/usb/media/pwc-if.c 19 Jun 2002 02:11:57 -0000 1.1.1.1
+++ drivers/usb/media/pwc-if.c 11 Jul 2002 23:07:01 -0000
@@ -1756,19 +1756,23 @@ static void usb_pwc_disconnect(struct us
pdev = (struct pwc_device *)ptr;
if (pdev == NULL) {
Err("pwc_disconnect() Called without private pointer.\n");
+ unlock_kernel();
return;
}
if (pdev->udev == NULL) {
Err("pwc_disconnect() already called for %p\n", pdev);
+ unlock_kernel();
return;
}
if (pdev->udev != udev) {
Err("pwc_disconnect() Woops: pointer mismatch udev/pdev.\n");
+ unlock_kernel();
return;
}
#ifdef PWC_MAGIC
if (pdev->magic != PWC_MAGIC) {
Err("pwc_disconnect() Magic number failed. Consult your scrolls and try again.\n");
+ unlock_kernel();
return;
}
#endif
Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------

2002-07-11 23:23:23

by Dawson Engler

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

> > From: Dawson Engler [mailto:[email protected]]
> > ---------------------------------------------------------
> > [BUG] all other case arms call __sti(); however, it may be
> > that safe_halt
> > does something weird.
> > /u2/engler/mc/oses/linux/2.5.8/drivers/acpi/acpi_processor.c:5
>
> #define safe_halt() __asm__ __volatile__("sti; hlt": : :"memory")
>
> I think we're OK for now. I doubt safe_halt will change, but if it does I'll
> fix this issue.

ah. missed the sti. thanks!

2002-07-11 23:32:10

by Andreas Dilger

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Jul 11, 2002 17:14 -0600, Thunder from the hill wrote:
> --- fs/hpfs/dir.c 19 Jun 2002 02:11:50 -0000 1.1.1.1
> +++ fs/hpfs/dir.c 11 Jul 2002 22:12:53 -0000
> @@ -211,7 +211,9 @@ struct dentry *hpfs_lookup(struct inode
>
> lock_kernel();
> if ((err = hpfs_chk_name((char *)name, &len))) {
> - if (err == -ENAMETOOLONG) return ERR_PTR(-ENAMETOOLONG);
> + if (err == -ENAMETOOLONG) {
> + return ERR_PTR(-ENAMETOOLONG);
> + }
> goto end_add;
> }

So, how does adding braces and a linefeed fix the locking problem here?
;-)

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

2002-07-11 23:39:39

by Chris Wright

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

* Thunder from the hill ([email protected]) wrote:
>
> Index: fs/hpfs/dir.c
> ===================================================================
> RCS file: /var/cvs/thunder-2.5/fs/hpfs/dir.c,v
> retrieving revision 1.1.1.1
> diff -p -u -r1.1.1.1 dir.c
> --- fs/hpfs/dir.c 19 Jun 2002 02:11:50 -0000 1.1.1.1
> +++ fs/hpfs/dir.c 11 Jul 2002 22:12:53 -0000
> @@ -211,7 +211,9 @@ struct dentry *hpfs_lookup(struct inode
>
> lock_kernel();
> if ((err = hpfs_chk_name((char *)name, &len))) {
> - if (err == -ENAMETOOLONG) return ERR_PTR(-ENAMETOOLONG);
> + if (err == -ENAMETOOLONG) {
> + return ERR_PTR(-ENAMETOOLONG);
> + }
> goto end_add;
> }

This does not fix the problem.
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net

2002-07-11 23:43:05

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Thu, 11 Jul 2002, Andreas Dilger wrote:
> So, how does adding braces and a linefeed fix the locking problem here?
> ;-)

I did add the contents, they were just not added physically. I have them
somewhere in my mind...

Index: fs/hpfs/dir.c
===================================================================
RCS file: /var/cvs/thunder-2.5/fs/hpfs/dir.c,v
retrieving revision 1.1
diff -p -u -r1.1 dir.c
--- fs/hpfs/dir.c 19 Jun 2002 02:11:50 -0000 1.1
+++ fs/hpfs/dir.c 11 Jul 2002 23:44:58 -0000 mind
@@ -211,7 +211,10 @@ struct dentry *hpfs_lookup(struct inode

lock_kernel();
if ((err = hpfs_chk_name((char *)name, &len))) {
- if (err == -ENAMETOOLONG) return ERR_PTR(-ENAMETOOLONG);
+ if (err == -ENAMETOOLONG) {
+ unlock_kernel();
+ return ERR_PTR(-ENAMETOOLONG);
+ }
goto end_add;
}

Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------

2002-07-12 00:21:10

by Andrew Morton

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Thunder from the hill wrote:
>
> Hi,
>
> Here is the whole set.
>

It's fair enough for a fix I guess. But careful readers will
have observed that a goodly portion of these bugs are directly
due to the poor programming practice of putting more than one
return statement in a C function.

-

2002-07-12 01:41:59

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Em Thu, Jul 11, 2002 at 05:21:50PM -0700, Andrew Morton escreveu:
> Thunder from the hill wrote:
> > Here is the whole set.

> It's fair enough for a fix I guess. But careful readers will
> have observed that a goodly portion of these bugs are directly
> due to the poor programming practice of putting more than one
> return statement in a C function.

woohooo!

"gotos considered !harmful"

8)

<asbestos suit>
- Arnaldo (that will continue putting more gotos in the kernel (hopefully)
where they make sense, _clarifying_ the code and making it
more maintainable 8) )
</asbestos suit>

2002-07-12 13:15:15

by David Woodhouse

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

[email protected] said:
> /u2/engler/mc/oses/linux/2.5.8/drivers/mtd/chips/cfi_cmdset_0001.c:782:
> do_write_buffer: ERROR:A_B:700:782:Did not reverse 'spin_lock'
> [COUNTER=spin_lock:700] [fit=3] [fit_fn=1] [fn_ex=5] [fn_counter=1]
> [ex=5619] [counter=272] [z = 1.34804760770983] [fn-z =
> -1.31122013621437]

That one can't ever actually happen -- it's effectively a default case in a
switch statement which can't ever be reached because we'd never get that far
unless one of the real cases is going to be taken. I think I'll replace the
return statement with panic("The world is broken");

--
dwmw2

2002-07-12 13:24:18

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, David Woodhouse wrote:
> That one can't ever actually happen -- it's effectively a default case in a
> switch statement which can't ever be reached because we'd never get that far
> unless one of the real cases is going to be taken. I think I'll replace the
> return statement with panic("The world is broken");

But don't forget to unlock_kernel() before ;-)

Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------

2002-07-12 17:38:02

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Thu, 11 Jul 2002, Thunder from the hill wrote:

> --- fs/affs/namei.c 19 Jun 2002 02:11:51 -0000 1.1.1.1
> +++ fs/affs/namei.c 11 Jul 2002 22:36:41 -0000
> @@ -345,10 +345,14 @@ affs_rmdir(struct inode *dir, struct den
> lock_kernel();
>
> /* WTF??? */
> + res = -ENOENT;
> +
> if (!dentry->d_inode)
> - return -ENOENT;
> + goto out_unlock;
>
> res = affs_remove_header(dentry);
> +
> + out_unlock:
> unlock_kernel();
> return res;
> }

Please drop this patch, it's impossible to hit this problem and I have a
better patch for this.

bye, Roman

2002-07-12 17:51:59

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Roman Zippel wrote:
> Please drop this patch, it's impossible to hit this problem and I have a
> better patch for this.

You mean

static inline int affs_rmdir(struct inode *dir, struct dentry *dentry)
{
int res;
lock_kernel();
res = affs_remove_header(dentry);
unlock_kernel();
return res;
}

Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------

2002-07-12 18:02:20

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Fri, Jul 12, 2002 at 07:40:35PM +0200, Roman Zippel wrote:

> Please drop this patch, it's impossible to hit this problem and I have a
> better patch for this.

(whilst on the subject of affs, and whilst I remember..)
btw, affs has been failing fsx runs again for the last few kernels.
truncating to largest ever: 0x13e76
domapwrite: mmap: Invalid argument
LOG DUMP (4 total operations):
1(1 mod 256): TRUNCATE UP from 0x0 to 0x13e76
2(2 mod 256): WRITE 0x17098 thru 0x26857 (0xf7c0 bytes) HOLE
3(3 mod 256): READ 0xc73e thru 0x1b801 (0xf0c4 bytes)
4(4 mod 256): MAPWRITE 0x32e00 thru 0x331fc (0x3fd bytes)
fsx: save_buffer: short write, 0x30ba8 bytes instead of 0x331fd

This is on an affs image mounted over loopback.
Before/After copies of the image available on request..

Dave

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-12 18:29:41

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Dave Jones wrote:

> (whilst on the subject of affs, and whilst I remember..)
> btw, affs has been failing fsx runs again for the last few kernels.
> truncating to largest ever: 0x13e76
> domapwrite: mmap: Invalid argument
> LOG DUMP (4 total operations):
> 1(1 mod 256): TRUNCATE UP from 0x0 to 0x13e76
> 2(2 mod 256): WRITE 0x17098 thru 0x26857 (0xf7c0 bytes) HOLE
> 3(3 mod 256): READ 0xc73e thru 0x1b801 (0xf0c4 bytes)
> 4(4 mod 256): MAPWRITE 0x32e00 thru 0x331fc (0x3fd bytes)
> fsx: save_buffer: short write, 0x30ba8 bytes instead of 0x331fd

Which last few kernels? Was it a ffs or an ofs image? For ofs images you
have to call fsx with "-W -R" to disable mmap operations.

bye, Roman

2002-07-12 18:34:52

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Fri, Jul 12, 2002 at 08:32:20PM +0200, Roman Zippel wrote:
> Which last few kernels? Was it a ffs or an ofs image? For ofs images you
> have to call fsx with "-W -R" to disable mmap operations.

OFS afaik. Has this always been the case ? I'm sure I ran fsx without
disabling mmap before on this image, and it used to pass.

Second bad news, with the -W -R options, it goes splat in an
even more dramatic way.

Dave.

Unable to handle kernel NULL pointer dereference at virtual address 00000008
c01f91a7
*pde = 00000000
Oops: 0000
CPU: 0
EIP: 0010:[<c01f91a7>] Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010296
eax: c01f9198 ebx: 000006a0 ecx: cff6bea8 edx: 00000000
esi: c133acc0 edi: cd352524 ebp: cefb6c00 esp: cd0efe9c
ds: 0018 es: 0018 ss: 0018
Process fsx (pid: 908, stackpage=cd0ef000)
Stack: 000006a0 c133acc0 cd352524 cefb6c00 cd352524 c01f9a26 00000000 c133acc0 000006a0 000006a0 000186a0 00000000 cd352474 00000000 00000000 cd352474 00000000 000000cc c0123f01 00000002 c0123f60 cd352474 00000048 cd0eff74 Call Trace: [<c01f9a26>] [<c0123f01>] [<c0123f60>] [<c0144eab>] [<c01f7bdf>]
[<c0144fe0>] [<c0131669>] [<c0131907>] [<c0106b73>]
Code: 8b 42 08 31 d2 8b 48 08 8b 74 24 1c 8b 46 18 a9 08 00 00 00

>>EIP; c01f91a7 <affs_prepare_write_ofs+f/fc> <=====

>>eax; c01f9198 <affs_prepare_write_ofs+0/fc>
>>ebx; 000006a0 Before first symbol
>>ecx; cff6bea8 <END_OF_CODE+fabb96c/????>
>>esi; c133acc0 <END_OF_CODE+e8a784/????>
>>edi; cd352524 <END_OF_CODE+cea1fe8/????>
>>ebp; cefb6c00 <END_OF_CODE+eb066c4/????>
>>esp; cd0efe9c <END_OF_CODE+cc3f960/????>

Trace; c01f9a26 <affs_truncate+a6/375>
Trace; c0123f01 <vmtruncate+9d/124>
Trace; c0123f60 <vmtruncate+fc/124>
Trace; c0144eab <inode_setattr+23/b0>
Trace; c01f7bdf <affs_notify_change+77/94>
Trace; c0144fe0 <notify_change+5c/dc>
Trace; c0131669 <do_truncate+4d/64>
Trace; c0131907 <sys_ftruncate+107/11c>
Trace; c0106b73 <system_call+33/40>

Code; c01f91a7 <affs_prepare_write_ofs+f/fc>
00000000 <_EIP>:
Code; c01f91a7 <affs_prepare_write_ofs+f/fc> <=====
0: 8b 42 08 mov 0x8(%edx),%eax <=====
Code; c01f91aa <affs_prepare_write_ofs+12/fc>
3: 31 d2 xor %edx,%edx
Code; c01f91ac <affs_prepare_write_ofs+14/fc>
5: 8b 48 08 mov 0x8(%eax),%ecx
Code; c01f91af <affs_prepare_write_ofs+17/fc>
8: 8b 74 24 1c mov 0x1c(%esp,1),%esi
Code; c01f91b3 <affs_prepare_write_ofs+1b/fc>
c: 8b 46 18 mov 0x18(%esi),%eax
Code; c01f91b6 <affs_prepare_write_ofs+1e/fc>
f: a9 08 00 00 00 test $0x8,%eax

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-12 18:34:41

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Thunder from the hill wrote:

> You mean
>
> static inline int affs_rmdir(struct inode *dir, struct dentry *dentry)
> {
> int res;
> lock_kernel();
> res = affs_remove_header(dentry);
> unlock_kernel();
> return res;
> }

lock_kernel isn't required here. Yesterday I went through affs and removed
the BKL completely.

bye, Roman

2002-07-12 20:31:44

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Dave Jones wrote:

> > Which last few kernels? Was it a ffs or an ofs image? For ofs images you
> > have to call fsx with "-W -R" to disable mmap operations.
>
> OFS afaik. Has this always been the case ? I'm sure I ran fsx without
> disabling mmap before on this image, and it used to pass.

ofs never supported mmap.

> Second bad news, with the -W -R options, it goes splat in an
> even more dramatic way.

Which kernel version? It looks like a bug which already has been fixed
quite some time ago.

bye, Roman

2002-07-12 20:45:34

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Fri, Jul 12, 2002 at 10:34:29PM +0200, Roman Zippel wrote:

> > OFS afaik. Has this always been the case ? I'm sure I ran fsx without
> > disabling mmap before on this image, and it used to pass.
> ofs never supported mmap.

Interesting. My old testing must have been with an ffs image I guess.

> > Second bad news, with the -W -R options, it goes splat in an
> > even more dramatic way.
> Which kernel version? It looks like a bug which already has been fixed
> quite some time ago.

2.5.25-dj1. I expect the same problem to exist in mainline if its
AFFS specific, as I currently have no patches in that area.

Unfortunatly I've not time right now to test mainline, as I've
more important things to do before I catch a flight. If it's
still happening when I get back in a week or so, I'll try again.

Dave

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-12 21:27:21

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Dave Jones wrote:

> > Which kernel version? It looks like a bug which already has been fixed
> > quite some time ago.
>
> 2.5.25-dj1. I expect the same problem to exist in mainline if its
> AFFS specific, as I currently have no patches in that area.

I'm just testing it with 2.4.18 under uml and it runs happily. The 2.4 and
2.5 are basically identical, so it's really strange. What I can see from
the disassembly it must be an old affs version.

bye, Roman

2002-07-12 21:36:01

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Fri, Jul 12, 2002 at 11:30:06PM +0200, Roman Zippel wrote:

> I'm just testing it with 2.4.18 under uml and it runs happily. The 2.4 and
> 2.5 are basically identical, so it's really strange. What I can see from
> the disassembly it must be an old affs version.

You mean the disk image is an old version ?
There's a gzip'd copy at http://www.codemonkey.org.uk/cruft/EMPTY-AFFS.ADF.gz
if you're curious..

if 2.4/2.5 AFFS is in sync as you say (which iirc it is), then
I fail to see how you think the 2.5.25 disassembly is an old version.

Dave.

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-12 21:53:22

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Fri, 12 Jul 2002, Dave Jones wrote:

> if 2.4/2.5 AFFS is in sync as you say (which iirc it is), then
> I fail to see how you think the 2.5.25 disassembly is an old version.

The last disassembled instruction is the PageUptodate() test, which was
moved in later versions and can't be that early in the function for a
recent version, but the disassembly is exactly what I would expect from
an old affs version.

bye, Roman

2002-07-12 22:12:11

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

On Fri, Jul 12, 2002 at 11:56:07PM +0200, Roman Zippel wrote:

> > if 2.4/2.5 AFFS is in sync as you say (which iirc it is), then
> > I fail to see how you think the 2.5.25 disassembly is an old version.
> The last disassembled instruction is the PageUptodate() test, which was
> moved in later versions and can't be that early in the function for a
> recent version, but the disassembly is exactly what I would expect from
> an old affs version.

Hmm, interesting. Time to test the possibility of a ccache bug I think.
How old exactly out of curiosity ?

Dave

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-12 22:31:55

[permalink] [raw]

Subject: Re: [CHECKER] 56 potential lock/unlock bugs in 2.5.8

Hi,

On Sat, 13 Jul 2002, Dave Jones wrote:

> Hmm, interesting. Time to test the possibility of a ccache bug I think.
> How old exactly out of curiosity ?

Since 2.5.15 (about two months ago).

bye, Roman

2002-07-12 22:39:01