[2.3.0] Read-write locks instead semaphores on UTS structures

Message ID Pine.LNX.4.10.9905121645430.498-200000@freak.conectiva
State New, archived
Headers show
Series
  • [2.3.0] Read-write locks instead semaphores on UTS structures
Related show

Commit Message

Marcelo Tosatti May 12, 1999, 7:50 p.m. UTC
This patch implements read-write locks instead semaphores in UTS
structures as noted by Linus in kernel/sys.c.

 Linus, can you apply this? 

  - Marcelo

Comments

Jakub Jelinek May 14, 1999, 10:34 a.m. UTC | #1
On Thu, May 13, 1999 at 06:12:15PM +0100, Jan-Simon Pendry wrote:
> Marcelo Tosatti wrote:
> > 
> > This patch implements read-write locks instead semaphores in UTS
> > structures as noted by Linus in kernel/sys.c.
> > 
> >  Linus, can you apply this?
> > 
> >   - Marcelo
> > 
> >   ------------------------------------------------------------------------
> >                        Name: uts-rwlock.patch
> >    uts-rwlock.patch    Type: Plain Text (TEXT/PLAIN)
> >                    Encoding: BASE64
> 
> i don't think you want to be holding spin-locks across
> a put_user/copy_from_user etc.  put_user et al can page
> fault and go to sleep holding the spin lock.  this can
> then deadlock the system if other processes comes along
> and contend for the same lock.  if you are desperate for
> parallel performance in this area you will need to implement
> reader-writer semaphores.

Huh? His patch uses read-write locks and not normal spin-locks, so I don't
understand what you find wrong on it. For the common case (hostname is
really not changed very often) anybody can hold the read lock so there is no
contention. I wonder whether it is worth doing something with the areas
which hold the write lock, maybe for those places the uaccess stuff should
move the data from userland first into some buffer on kernel stack (it is
65bytes at most, so it does not matter that much) and only memcpy it from
there with the write_lock held. But as only root can write to it, no DOS can
happen and so I would not worry.

Just there is one more bug in the patch (besides the one I pointed out in
sparc64/solaris/): in sysctl.c's the patch will acquire write lock even when
it is not needed and that can lead into some sort of a DOS attack (a couple of user
programs, each one reading from /proc/sys/kernel/hostname into some unmapped
area in a cycle).
So I'd instead write:

static int proc_doutsstring(ctl_table *table, int write, struct file *filp,
                  void *buffer, size_t *lenp)
{
	int r;
	if (write) {
		write_lock(&uts_rwlock);
	        r=proc_dostring(table,1,filp,buffer,lenp);
		write_unlock(&uts_rwlock);
	} else {
		read_lock(&uts_rwlock);
	        r=proc_dostring(table,0,filp,buffer,lenp);
		read_unlock(&uts_rwlock);
	}
        return r;
}


Cheers,
    Jakub
Jan-Simon Pendry May 14, 1999, 2:36 p.m. UTC | #2
Jakub Jelinek wrote
> 
> Huh? His patch uses read-write locks and not normal spin-locks, so I don't
> understand what you find wrong on it.

read-write locks are a variant of spin locks.  you can deadlock
the system if you go to sleep holding holding any spin lock.
the only exception is the kernel lock which has special code
to deal with it in schedule().

ideally the spinlock code (spin_lock and read_lock et al)
would have debug versions that counted how many locks were
held (in some per-cpu data structure), and have schedule()
check that this number was zero before putting the process
to sleep.

jan-simon.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

Patch

diff -Nur linux.orig/CREDITS linux/CREDITS
--- linux.orig/CREDITS	Wed May 12 15:45:30 1999
+++ linux/CREDITS	Wed May 12 15:47:32 1999
@@ -1886,6 +1886,15 @@ 
 S: Santa Clara, California 95051
 S: USA
 
+N: Marcelo W. Tosatti
+E: marcelo@conectiva.com.br
+W: http://lie-br.conectiva.com.br/~marcelo/
+D: Miscellaneous kernel hacker
+D: Cyclom 2X driver hacker
+S: R. Prof. Rubens Elke Braga, 558 - Parolin
+S: 80220-320 Curitiba - Parana
+S: Brazil
+
 N: Stefan Traby
 E: stefan@quant-x.com
 D: Minor Alpha kernel hacks
diff -Nur linux.orig/arch/alpha/kernel/osf_sys.c linux/arch/alpha/kernel/osf_sys.c
--- linux.orig/arch/alpha/kernel/osf_sys.c	Wed May 12 15:46:58 1999
+++ linux/arch/alpha/kernel/osf_sys.c	Wed May 12 15:47:33 1999
@@ -545,7 +545,7 @@ 
 {
 	int error;
 
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	error = -EFAULT;
 	if (copy_to_user(name + 0, system_utsname.sysname, 32))
 		goto out;
@@ -560,7 +560,7 @@ 
 
 	error = 0;
 out:
-	up(&uts_sem);	
+	read_unlock(&uts_rwlock);
 	return error;
 }
 
@@ -619,13 +619,13 @@ 
 	if (namelen > 32)
 		len = 32;
 
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	for (i = 0; i < len; ++i) {
 		__put_user(system_utsname.domainname[i], name + i);
 		if (system_utsname.domainname[i] == '\0')
 			break;
 	}
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 out:
 	unlock_kernel();
 	return error;
@@ -859,7 +859,7 @@ 
 		goto out;
 	}
 	
-	down(&uts_sem);
+	read_lock(&uts_rwlock)
 	res = sysinfo_table[offset];
 	len = strlen(res)+1;
 	if (len > count)
@@ -868,7 +868,7 @@ 
 		err = -EFAULT;
 	else
 		err = 0;
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 out:
 	unlock_kernel();
 	return err;
diff -Nur linux.orig/arch/arm/kernel/sys_arm.c linux/arch/arm/kernel/sys_arm.c
--- linux.orig/arch/arm/kernel/sys_arm.c	Wed May 12 15:47:13 1999
+++ linux/arch/arm/kernel/sys_arm.c	Wed May 12 15:47:33 1999
@@ -321,9 +321,9 @@ 
 
 	if(!name)
 		return -EFAULT;
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	err=copy_to_user (name, &system_utsname, sizeof (*name));
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);	
 	return err?-EFAULT:0;
 }
 
@@ -344,7 +344,7 @@ 
 	if (!access_ok(VERIFY_WRITE,name,sizeof(struct oldold_utsname)))
 		return -EFAULT;
 
-	down(&uts_sem);
+	read_lock(&uts_rwlock);	
 	
 	error = __copy_to_user(&name->sysname,&system_utsname.sysname,__OLD_UTS_LEN);
 	error -= __put_user(0,name->sysname+__OLD_UTS_LEN);
@@ -357,7 +357,7 @@ 
 	error -= __copy_to_user(&name->machine,&system_utsname.machine,__OLD_UTS_LEN);
 	error -= __put_user(0,name->machine+__OLD_UTS_LEN);
 	
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	
 	error = error ? -EFAULT : 0;
 
diff -Nur linux.orig/arch/i386/kernel/sys_i386.c linux/arch/i386/kernel/sys_i386.c
--- linux.orig/arch/i386/kernel/sys_i386.c	Wed May 12 15:46:49 1999
+++ linux/arch/i386/kernel/sys_i386.c	Wed May 12 15:47:34 1999
@@ -206,9 +206,9 @@ 
 	int err;
 	if (!name)
 		return -EFAULT;
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	err=copy_to_user(name, &system_utsname, sizeof (*name));
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return err?-EFAULT:0;
 }
 
@@ -221,7 +221,7 @@ 
 	if (!access_ok(VERIFY_WRITE,name,sizeof(struct oldold_utsname)))
 		return -EFAULT;
   
-  	down(&uts_sem);
+	read_lock(&uts_rwlock);	
 	
 	error = __copy_to_user(&name->sysname,&system_utsname.sysname,__OLD_UTS_LEN);
 	error |= __put_user(0,name->sysname+__OLD_UTS_LEN);
@@ -234,7 +234,7 @@ 
 	error |= __copy_to_user(&name->machine,&system_utsname.machine,__OLD_UTS_LEN);
 	error |= __put_user(0,name->machine+__OLD_UTS_LEN);
 	
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	
 	error = error ? -EFAULT : 0;
 
diff -Nur linux.orig/arch/sparc/kernel/sys_sparc.c linux/arch/sparc/kernel/sys_sparc.c
--- linux.orig/arch/sparc/kernel/sys_sparc.c	Wed May 12 15:47:05 1999
+++ linux/arch/sparc/kernel/sys_sparc.c	Wed May 12 15:47:34 1999
@@ -344,8 +344,8 @@ 
  	int nlen;
  	int err = -EFAULT;
  	
- 	down(&uts_sem);
- 	
+	read_lock(&uts_rwlock); 
+	
 	nlen = strlen(system_utsname.domainname) + 1;
 
 	if (nlen < len)
@@ -356,7 +356,7 @@ 
 		goto done;
 	err = 0;
 done:
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return err;
 }
 
diff -Nur linux.orig/arch/sparc/kernel/sys_sunos.c linux/arch/sparc/kernel/sys_sunos.c
--- linux.orig/arch/sparc/kernel/sys_sunos.c	Wed May 12 15:47:06 1999
+++ linux/arch/sparc/kernel/sys_sunos.c	Wed May 12 15:47:34 1999
@@ -584,7 +584,7 @@ 
 asmlinkage int sunos_uname(struct sunos_utsname *name)
 {
 	int ret;
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	ret = copy_to_user(&name->sname[0], &system_utsname.sysname[0], sizeof(name->sname) - 1);
 	if (!ret) {
 		ret |= __copy_to_user(&name->nname[0], &system_utsname.nodename[0], sizeof(name->nname) - 1);
@@ -593,7 +593,7 @@ 
 		ret |= __copy_to_user(&name->ver[0], &system_utsname.version[0], sizeof(name->ver) - 1);
 		ret |= __copy_to_user(&name->mach[0], &system_utsname.machine[0], sizeof(name->mach) - 1);
 	}
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return ret;
 }
 
diff -Nur linux.orig/arch/sparc64/kernel/sys_sparc.c linux/arch/sparc64/kernel/sys_sparc.c
--- linux.orig/arch/sparc64/kernel/sys_sparc.c	Wed May 12 15:47:13 1999
+++ linux/arch/sparc64/kernel/sys_sparc.c	Wed May 12 15:47:34 1999
@@ -237,8 +237,8 @@ 
         int nlen;
 	int err = -EFAULT;
 
- 	down(&uts_sem);
- 	
+	read_lock(&uts_rwlock); 
+	
 	nlen = strlen(system_utsname.domainname) + 1;
 
         if (nlen < len)
@@ -249,7 +249,7 @@ 
 		goto done;
 	err = 0;
 done:
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return err;
 }
 
diff -Nur linux.orig/arch/sparc64/kernel/sys_sunos32.c linux/arch/sparc64/kernel/sys_sunos32.c
--- linux.orig/arch/sparc64/kernel/sys_sunos32.c	Wed May 12 15:47:13 1999
+++ linux/arch/sparc64/kernel/sys_sunos32.c	Wed May 12 15:47:34 1999
@@ -546,14 +546,14 @@ 
 {
 	int ret;
 
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	ret = copy_to_user(&name->sname[0], &system_utsname.sysname[0], sizeof(name->sname) - 1);
 	ret |= copy_to_user(&name->nname[0], &system_utsname.nodename[0], sizeof(name->nname) - 1);
 	ret |= put_user('\0', &name->nname[8]);
 	ret |= copy_to_user(&name->rel[0], &system_utsname.release[0], sizeof(name->rel) - 1);
 	ret |= copy_to_user(&name->ver[0], &system_utsname.version[0], sizeof(name->ver) - 1);
 	ret |= copy_to_user(&name->mach[0], &system_utsname.machine[0], sizeof(name->mach) - 1);
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return ret;
 }
 
diff -Nur linux.orig/arch/sparc64/solaris/misc.c linux/arch/sparc64/solaris/misc.c
--- linux.orig/arch/sparc64/solaris/misc.c	Wed May 12 15:47:13 1999
+++ linux/arch/sparc64/solaris/misc.c	Wed May 12 15:47:34 1999
@@ -261,7 +261,7 @@ 
 asmlinkage int solaris_utsname(u32 buf)
 {
 	/* Why should we not lie a bit? */
-	down(&uts_sem);
+	write_lock(&uts_rwlock); 
 	set_utsfield(((struct sol_utsname *)A(buf))->sysname, 
 			"SunOS", 0, 0);
 	set_utsfield(((struct sol_utsname *)A(buf))->nodename, 
@@ -272,7 +272,7 @@ 
 			"Generic", 0, 0);
 	set_utsfield(((struct sol_utsname *)A(buf))->machine, 
 			machine(), 0, 0);
-	up(&uts_sem);
+	write_unlock(&uts_rwlock);
 	return 0;
 }
 
diff -Nur linux.orig/drivers/sound/soundcard.c linux/drivers/sound/soundcard.c
--- linux.orig/drivers/sound/soundcard.c	Wed May 12 15:46:37 1999
+++ linux/drivers/sound/soundcard.c	Wed May 12 15:47:34 1999
@@ -17,8 +17,11 @@ 
  *                   which should disappear in the near future)
  *
  * Rob Riggs		Added persistent DMA buffers support (1998/10/17)
+ *
+ * Marcelo Tosatti : used rw locks for reading utsname structures instead of
+ * semaphores
  */
-
+ 
 #include <linux/config.h>
 
 #include "sound_config.h"
@@ -163,7 +166,7 @@ 
 #define MODULEPROCSTRING "Driver compiled into kernel"
 #endif
 
-	down(&uts_sem);	
+	read_lock(&uts_rwlock);	
 
 	len = sprintf(buffer, "OSS/Free:" SOUND_VERSION_STRING "\n"
 		      "Load type: " MODULEPROCSTRING "\n"
@@ -171,7 +174,7 @@ 
 		      "Config options: %x\n\nInstalled drivers: \n", 
 		      system_utsname.sysname, system_utsname.nodename, system_utsname.release, 
 		      system_utsname.version, system_utsname.machine, SELECTED_SOUND_OPTIONS);
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	
 	for (i = 0; (i < num_sound_drivers) && (pos <= offset + length); i++) {
 		if (!sound_drivers[i].card_type)
diff -Nur linux.orig/include/linux/utsname.h linux/include/linux/utsname.h
--- linux.orig/include/linux/utsname.h	Wed May 12 15:45:30 1999
+++ linux/include/linux/utsname.h	Wed May 12 15:47:34 1999
@@ -1,3 +1,5 @@ 
+#include <asm/spinlock.h>
+
 #ifndef _LINUX_UTSNAME_H
 #define _LINUX_UTSNAME_H
 
@@ -32,5 +34,5 @@ 
 
 extern struct new_utsname system_utsname;
 
-extern struct semaphore uts_sem;
+extern rwlock_t uts_rwlock;
 #endif
diff -Nur linux.orig/kernel/ksyms.c linux/kernel/ksyms.c
--- linux.orig/kernel/ksyms.c	Wed May 12 15:45:29 1999
+++ linux/kernel/ksyms.c	Wed May 12 15:48:37 1999
@@ -326,8 +326,8 @@ 
 EXPORT_SYMBOL(bdevname);
 EXPORT_SYMBOL(cdevname);
 EXPORT_SYMBOL(simple_strtoul);
+EXPORT_SYMBOL(uts_rwlock); /* UTS read-write lock */
 EXPORT_SYMBOL(system_utsname);	/* UTS data */
-EXPORT_SYMBOL(uts_sem);		/* UTS semaphore */
 EXPORT_SYMBOL(sys_call_table);
 EXPORT_SYMBOL(machine_restart);
 EXPORT_SYMBOL(machine_halt);
diff -Nur linux.orig/kernel/sys.c linux/kernel/sys.c
--- linux.orig/kernel/sys.c	Wed May 12 15:45:29 1999
+++ linux/kernel/sys.c	Wed May 12 15:47:35 1999
@@ -2,6 +2,8 @@ 
  *  linux/kernel/sys.c
  *
  *  Copyright (C) 1991, 1992  Linus Torvalds
+ *  Modified: replaced uts_sem semaphore with uts_rwlock read-write lock.
+ *  Marcelo Tosatti <marcelo@conectiva.com.br> 
  */
 
 #include <linux/mm.h>
@@ -807,21 +809,16 @@ 
 	return 1;
 }
 
-/*
- * This should really be a blocking read-write lock
- * rather than a semaphore. Anybody want to implement
- * one?
- */
-struct semaphore uts_sem = MUTEX;
+rwlock_t uts_rwlock;
 
 asmlinkage int sys_newuname(struct new_utsname * name)
 {
 	int errno = 0;
 
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	if (copy_to_user(name,&system_utsname,sizeof *name))
 		errno = -EFAULT;
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return errno;
 }
 
@@ -833,13 +830,13 @@ 
 		return -EPERM;
 	if (len < 0 || len > __NEW_UTS_LEN)
 		return -EINVAL;
-	down(&uts_sem);
+	write_lock(&uts_rwlock);	
 	errno = -EFAULT;
 	if (!copy_from_user(system_utsname.nodename, name, len)) {
 		system_utsname.nodename[len] = 0;
 		errno = 0;
 	}
-	up(&uts_sem);
+	write_unlock(&uts_rwlock);
 	return errno;
 }
 
@@ -849,14 +846,14 @@ 
 
 	if (len < 0)
 		return -EINVAL;
-	down(&uts_sem);
+	read_lock(&uts_rwlock);
 	i = 1 + strlen(system_utsname.nodename);
 	if (i > len)
 		i = len;
 	errno = 0;
 	if (copy_to_user(name, system_utsname.nodename, i))
 		errno = -EFAULT;
-	up(&uts_sem);
+	read_unlock(&uts_rwlock);
 	return errno;
 }
 
@@ -873,13 +870,13 @@ 
 	if (len < 0 || len > __NEW_UTS_LEN)
 		return -EINVAL;
 
-	down(&uts_sem);
+	write_lock(&uts_rwlock);
 	errno = -EFAULT;
 	if (!copy_from_user(system_utsname.domainname, name, len)) {
 		errno = 0;
 		system_utsname.domainname[len] = 0;
 	}
-	up(&uts_sem);
+	write_unlock(&uts_rwlock);
 	return errno;
 }
 
diff -Nur linux.orig/kernel/sysctl.c linux/kernel/sysctl.c
--- linux.orig/kernel/sysctl.c	Wed May 12 15:45:30 1999
+++ linux/kernel/sysctl.c	Wed May 12 15:47:35 1999
@@ -678,9 +678,9 @@ 
 		  void *buffer, size_t *lenp)
 {
 	int r;
-	down(&uts_sem);
+	write_lock(&uts_rwlock);
 	r=proc_dostring(table,write,filp,buffer,lenp);
-	up(&uts_sem);
+	write_unlock(&uts_rwlock);
 	return r;
 }