Skip to contentSkip to conversation

3 one-liner bugfixes

PATCH In review Manfred Spraul 2 participants 4 messages May 4, 2001

Subsystems (4)
Memory management VFS MM core File locking (flock() and fcntl()/lockf())
lore
Share & tools
lore

Manfred Spraul

author
#0 lore raw

PATCH 1/1 3 one-liner bugfixes

Hi Linus,

I found a 3 small bugs:

* mm/slab.c: the offslab_limit calculation used 2 instead of sizeof(kmem_bufctl_t) [==4]. Cosmetic bug, since offslab_limit is never reached.

* expand_stack is not down_read() safe, but used in the page-in path. Fix is trivial.

* missing/wrong lock_kernel calls in fs/fcntl.c: getlk/setlk run without the big kernel lock. The ..64 function acquire the lock.

--
Manfred

mm/slab.c +1 -1
@@ -448,7 +448,7 @@
 		/* Inc off-slab bufctl limit until the ceiling is hit. */
 		if (!(OFF_SLAB(sizes->cs_cachep))) {
 			offslab_limit = sizes->cs_size-sizeof(slab_t);
-			offslab_limit /= 2;
+			offslab_limit /= sizeof(kmem_bufctl_t);
 		}
 		sprintf(name, "size-%Zd(DMA)",sizes->cs_size);
 		sizes->cs_dmacachep = kmem_cache_create(name, sizes->cs_size, 0,
include/linux/mm.h +4 -2
@@ -502,12 +502,14 @@
 {
 	unsigned long grow;
 
+	spin_lock(&vma->vm_mm->page_table_lock);
 	address &= PAGE_MASK;
 	grow = (vma->vm_start - address) >> PAGE_SHIFT;
 	if (vma->vm_end - address > current->rlim[RLIMIT_STACK].rlim_cur ||
-	    ((vma->vm_mm->total_vm + grow) << PAGE_SHIFT) > current->rlim[RLIMIT_AS].rlim_cur)
+	    ((vma->vm_mm->total_vm + grow) << PAGE_SHIFT) > current->rlim[RLIMIT_AS].rlim_cur) {
+		spin_unlock(&vma->vm_mm->page_table_lock);
 		return -ENOMEM;
-	spin_lock(&vma->vm_mm->page_table_lock);
+	}
 	vma->vm_start = address;
 	vma->vm_pgoff -= grow;
 	vma->vm_mm->total_vm += grow;
fs/fcntl.c +10 -2
@@ -254,11 +254,15 @@
 			unlock_kernel();
 			break;
 		case F_GETLK:
+			lock_kernel();
 			err = fcntl_getlk(fd, (struct flock *) arg);
+			unlock_kernel();
 			break;
 		case F_SETLK:
 		case F_SETLKW:
+			lock_kernel();
 			err = fcntl_setlk(fd, cmd, (struct flock *) arg);
+			unlock_kernel();
 			break;
 		case F_GETOWN:
 			/*

@@ -338,22 +342,26 @@
 	if (!filp)
 		goto out;
 
-	lock_kernel();
 	switch (cmd) {
 		case F_GETLK64:
+			lock_kernel();
 			err = fcntl_getlk64(fd, (struct flock64 *) arg);
+			unlock_kernel();
 			break;
 		case F_SETLK64:
+			lock_kernel();
 			err = fcntl_setlk64(fd, cmd, (struct flock64 *) arg);
+			unlock_kernel();
 			break;
 		case F_SETLKW64:
+			lock_kernel();
 			err = fcntl_setlk64(fd, cmd, (struct flock64 *) arg);
+			unlock_kernel();
 			break;
 		default:
 			err = do_fcntl(fd, cmd, arg, filp);
 			break;
 	}
-	unlock_kernel();
 	fput(filp);
 out:
 	return err;
#1 lore raw
Quoted 3 lines · Manfred Spraul

* missing/wrong lock_kernel calls in fs/fcntl.c: getlk/setlk run without the big kernel lock. The ..64 function acquire the lock.

This is wrong. The big lock (if it is needed, but I thought the current locking should be safe) should be pushed down into the point where it is needed, not at the caller..

Linus

author
#2 lore raw
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Manfred Spraul

* missing/wrong lock_kernel calls in fs/fcntl.c: getlk/setlk run without the big kernel lock. The ..64 function acquire the lock.

This is wrong. The big lock (if it is needed, but I thought the current locking should be safe) should be pushed down into the point where it is needed, not at the caller..

Ok, I've removed the locks from fs/fcntl.c and added them into fs/locks.c:

* fcntl_getlease dereferences filp->f_dentry->d_inode->i_flock. Race with multithreaded app: sys_close()->filp_close()->locks_remove_posix()
+ fcntl_getlease()
* according to Documentation/filesystems/Locking, f_op->lock() is called with the blk acquired. lock added around that call in fcntl_{get,set}lk{,64}

I've attached a new patch.

--
Manfred

// $Header$
// Kernel Version:

//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 4
//  EXTRAVERSION =
fs/fcntl.c +0 -2
@@ -338,7 +338,6 @@
 	if (!filp)
 		goto out;
 
-	lock_kernel();
 	switch (cmd) {
 		case F_GETLK64:
 			err = fcntl_getlk64(fd, (struct flock64 *) arg);

@@ -353,7 +352,6 @@
 			err = do_fcntl(fd, cmd, arg, filp);
 			break;
 	}
-	unlock_kernel();
 	fput(filp);
 out:
 	return err;
fs/locks.c +16 -3
@@ -1157,11 +1157,16 @@
 int fcntl_getlease(struct file *filp)
 {
 	struct file_lock *fl;
-	
+	int ret;
+
+	lock_kernel();
 	fl = filp->f_dentry->d_inode->i_flock;
 	if ((fl == NULL) || ((fl->fl_flags & FL_LEASE) == 0))
-		return F_UNLCK;
-	return fl->fl_type & ~F_INPROGRESS;
+		ret = F_UNLCK;
+	else
+		fl->fl_type & ~F_INPROGRESS;
+	unlock_kernel();
+	return ret;
 }
 
 /* We already had a lease on this file; just change its type */

@@ -1357,7 +1362,9 @@
 		goto out_putf;
 
 	if (filp->f_op && filp->f_op->lock) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 		else if (error == LOCK_USE_CLNT)

@@ -1481,7 +1488,9 @@
 	}
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, cmd, file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 	}

@@ -1522,7 +1531,9 @@
 		goto out_putf;
 
 	if (filp->f_op && filp->f_op->lock) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 		else if (error == LOCK_USE_CLNT)

@@ -1619,7 +1630,9 @@
 	}
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, cmd, file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 	}
author
#3 lore raw
In reply to Linus Torvalds

Review

Quoted 3 lines · Manfred Spraul
(quoted diff) +2 -0
+       else
+               fl->fl_type & ~F_INPROGRESS;

               ^^^^^^

Review

Quoted 3 lines
(quoted diff) +2 -0
+       unlock_kernel();
+       return ret;
 }

The last patch was incorrect. Corrected version attached.

--
Manfred

// $Header$
// Kernel Version:

//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 4
//  EXTRAVERSION =
fs/fcntl.c +0 -2
@@ -338,7 +338,6 @@
 	if (!filp)
 		goto out;
 
-	lock_kernel();
 	switch (cmd) {
 		case F_GETLK64:
 			err = fcntl_getlk64(fd, (struct flock64 *) arg);

@@ -353,7 +352,6 @@
 			err = do_fcntl(fd, cmd, arg, filp);
 			break;
 	}
-	unlock_kernel();
 	fput(filp);
 out:
 	return err;
fs/locks.c +16 -3
@@ -1157,11 +1157,16 @@
 int fcntl_getlease(struct file *filp)
 {
 	struct file_lock *fl;
-	
+	int ret;
+
+	lock_kernel();
 	fl = filp->f_dentry->d_inode->i_flock;
 	if ((fl == NULL) || ((fl->fl_flags & FL_LEASE) == 0))
-		return F_UNLCK;
-	return fl->fl_type & ~F_INPROGRESS;
+		ret = F_UNLCK;
+	else
+		ret = fl->fl_type & ~F_INPROGRESS;
+	unlock_kernel();
+	return ret;
 }
 
 /* We already had a lease on this file; just change its type */

@@ -1357,7 +1362,9 @@
 		goto out_putf;
 
 	if (filp->f_op && filp->f_op->lock) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 		else if (error == LOCK_USE_CLNT)

@@ -1481,7 +1488,9 @@
 	}
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, cmd, file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 	}

@@ -1522,7 +1531,9 @@
 		goto out_putf;
 
 	if (filp->f_op && filp->f_op->lock) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 		else if (error == LOCK_USE_CLNT)

@@ -1619,7 +1630,9 @@
 	}
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
+		lock_kernel();
 		error = filp->f_op->lock(filp, cmd, file_lock);
+		unlock_kernel();
 		if (error < 0)
 			goto out_putf;
 	}