Messages in this thread Patch in this message | | | Date | Fri, 8 Aug 1997 14:02:31 +0200 (MESZ) | From | Richard Guenther <> | Subject | Fix for binfmt_misc / Question about locking [PATCH] |
| |
Hi!
Well, I noticed, that my locking approach in binfmt_misc is the wrong thing (I used a spinlock to protect a resource, which is useless in the uniprocessor case and not nice in the SMP case). I now (temporarily?) switched to a semaphore. (Patch for this is attached below)
My question now is: Is it ok to do no locking at all, if we can sure, that only root could trigger a race condition (which could cause an oops, of cause)?? Or do we have to avoid every race condition we know of, regardless what cost it has?
Richard.
diff -u --recursive --new-file linux-2.1.48/Documentation/binfmt_misc.txt linux-2.1.48x/Documentation/binfmt_misc.txt --- linux-2.1.48/Documentation/binfmt_misc.txt Thu Aug 7 19:55:06 1997 +++ linux-2.1.48x/Documentation/binfmt_misc.txt Fri Aug 8 10:56:18 1997 @@ -9,7 +9,7 @@ with which binary. Binfmt_misc recognises the binary-type by matching some bytes at the beginning of the file with a magic byte sequence (masking out specified bits) you have supplied. Binfmt_misc can also recognise a filename extension -(aka .com) and optionally strip it off. +aka '.com' or '.exe'. To actually register a new binary type, you have to set up a string looking like :name:type:offset:magic:mask:interpreter: (where you can choose the ':' upon @@ -18,16 +18,14 @@ - 'name' is an identifier string. A new /proc file will be created with this name below /proc/sys/fs/binfmt_misc - 'type' is the type of recognition. Give 'M' for magic and 'E' for extension. - Give the corresponding lowercase letter to let binfmt_misc strip off the - filename extension. - 'offset' is the offset of the magic/mask in the file, counted in bytes. This defaults to 0 if you omit it (i.e. you write ':name:type::magic...') - 'magic' is the byte sequence binfmt_misc is matching for. The magic string may contain hex-encoded characters like \x0a or \xA4. In a shell environment you will have to write \\x0a to prevent the shell from eating your \. If you chose filename extension matching, this is the extension to be - recognised (the \x0a specials are not allowed). Extension matching is case - sensitive! + recognised (without the '.', the \x0a specials are not allowed). Extension + matching is case sensitive! - 'mask' is an (optional, defaults to all 0xff) mask. You can mask out some bits from matching by supplying a string like magic and as long as magic. The mask is anded with the byte sequence of the file. @@ -38,10 +36,14 @@ - the magic must resist in the first 128 bytes of the file, i.e. offset+size(magic) has to be less than 128 - the interpreter string may not exceed 127 characters + You may want to add the binary formats in one of your /etc/rc scripts during boot-up. Read the manual of your init program to figure out how to do this right. +New entries are appended before older ones, i.e. later added entries are +matched first! + A few examples (assumed you are in /proc/sys/fs/binfmt_misc): @@ -94,7 +96,7 @@ if [ -L "$1" ] ; then CLASS=`ls --color=no -l $1 | tr -s '\t ' ' ' | cut -d ' ' -f 11` fi -CLASSN=`basename $CLASS | sed s/\.class$//` +CLASSN=`basename $CLASS .class` CLASSP=`dirname $CLASS` FOO=$PATH diff -u --recursive --new-file linux-2.1.48/fs/binfmt_misc.c linux-2.1.48x/fs/binfmt_misc.c --- linux-2.1.48/fs/binfmt_misc.c Thu Aug 7 19:55:27 1997 +++ linux-2.1.48x/fs/binfmt_misc.c Fri Aug 8 10:45:20 1997 @@ -12,6 +12,8 @@ * 1997-05-19 cleanup * 1997-06-26 hpa: pass the real filename rather than argv[0] * 1997-06-30 minor cleanup + * 1997-07-21 use semaphore rather than spinlock which is wrong + * removed extension stripping */ #include <linux/module.h> @@ -26,15 +28,11 @@ #include <linux/string.h> #include <linux/ctype.h> #include <asm/uaccess.h> -#include <asm/spinlock.h> +#include <asm/semaphore.h> #define VERBOSE_STATUS /* undef this to save 400 bytes kernel memory */ -#ifndef MIN -#define MIN(x,y) (((x)<(y))?(x):(y)) -#endif - struct binfmt_entry { struct binfmt_entry *next; int id; @@ -50,7 +48,6 @@ #define ENTRY_ENABLED 1 /* the old binfmt_entry.enabled */ #define ENTRY_MAGIC 8 /* not filename detection */ -#define ENTRY_STRIP_EXT 32 /* strip off last filename extension */ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs); static void entry_proc_cleanup(struct binfmt_entry *e); @@ -70,7 +67,7 @@ static int free_id = 1; static int enabled = 1; -static rwlock_t entries_lock = RW_LOCK_UNLOCKED; +static struct semaphore entries_lock = MUTEX; /* @@ -80,7 +77,7 @@ { struct binfmt_entry **ep, *e; - write_lock(&entries_lock); + down(&entries_lock); ep = &entries; while (*ep && ((*ep)->id != id)) ep = &((*ep)->next); @@ -89,7 +86,7 @@ entry_proc_cleanup(e); kfree(e); } - write_unlock(&entries_lock); + up(&entries_lock); } /* @@ -99,27 +96,38 @@ { struct binfmt_entry *e; - write_lock(&entries_lock); + down(&entries_lock); while ((e = entries)) { entries = entries->next; entry_proc_cleanup(e); kfree(e); } - write_unlock(&entries_lock); + up(&entries_lock); } /* - * Find entry through id - caller has to do locking + * Find entry through id, if found, lock is hold */ static struct binfmt_entry *get_entry(int id) { struct binfmt_entry *e = entries; + down(&entries_lock); while (e && (e->id != id)) e = e->next; + if (!e) + up(&entries_lock); return e; } +/* + * Release lock for e + */ +static inline void put_entry(struct binfmt_entry *e) +{ + if (e) up(&entries_lock); +} + /* * Check if we support the binfmt @@ -160,7 +168,7 @@ struct binfmt_entry *fmt; struct dentry * dentry; char iname[128]; - char *iname_addr = iname, *p; + char *iname_addr = iname; int retval, fmt_flags = 0; MOD_INC_USE_COUNT; @@ -170,13 +178,13 @@ } /* to keep locking time low, we copy the interpreter string */ - read_lock(&entries_lock); + down(&entries_lock); if ((fmt = check_file(bprm))) { strncpy(iname, fmt->interpreter, 127); iname[127] = '\0'; fmt_flags = fmt->flags; } - read_unlock(&entries_lock); + up(&entries_lock); if (!fmt) { retval = -ENOEXEC; goto _ret; @@ -186,9 +194,6 @@ bprm->dentry = NULL; /* Build args for interpreter */ - if ((fmt_flags & ENTRY_STRIP_EXT) && - (p = strrchr(bprm->filename, '.'))) - *p = '\0'; remove_arg_zero(bprm); bprm->p = copy_strings(1, &bprm->filename, bprm->page, bprm->p, 2); bprm->argc++; @@ -288,13 +293,12 @@ e->proc_name = copyarg(&dp, &sp, &cnt, del, 0, &err); - /* we can use bit 3 and 5 of type for ext/magic and ext-strip - flag due to the nice encoding of E, M, e and m */ - if ((*sp & 0x92) || (sp[1] != del)) + /* we can use bit 3 of type for ext/magic + flag due to the nice encoding of E and M */ + if ((*sp & ~('E' | 'M')) || (sp[1] != del)) err = -EINVAL; else - e->flags = (*sp++ & (ENTRY_MAGIC | ENTRY_STRIP_EXT)) - | ENTRY_ENABLED; + e->flags = (*sp++ & (ENTRY_MAGIC | ENTRY_ENABLED)); cnt -= 2; sp++; e->offset = 0; @@ -321,10 +325,10 @@ goto _err; } - write_lock(&entries_lock); + down(&entries_lock); e->next = entries; entries = e; - write_unlock(&entries_lock); + up(&entries_lock); err = count; _err: @@ -342,17 +346,17 @@ { struct binfmt_entry *e; char *dp; - int elen, i; + int elen, i, err; MOD_INC_USE_COUNT; #ifndef VERBOSE_STATUS if (data) { - read_lock(&entries_lock); - if (!(e = get_entry((int) data))) - i = 0; - else - i = e->flags & ENTRY_ENABLED; - read_unlock(&entries_lock); + if (!(e = get_entry((int) data))) { + err = -ENOENT; + goto _err; + } + i = e->flags & ENTRY_ENABLED; + put_entry(e); } else { i = enabled; } @@ -361,10 +365,9 @@ if (!data) sprintf(page, "%s\n", (enabled ? "enabled" : "disabled")); else { - read_lock(&entries_lock); if (!(e = get_entry((int) data))) { - *page = '\0'; - goto _out; + err = -ENOENT; + goto _err; } sprintf(page, "%s\ninterpreter %s\n", (e->flags & ENTRY_ENABLED ? "enabled" : "disabled"), @@ -391,10 +394,7 @@ *dp++ = '\n'; *dp = '\0'; } - if (e->flags & ENTRY_STRIP_EXT) - sprintf(dp, "extension stripped\n"); -_out: - read_unlock(&entries_lock); + put_entry(e); } #endif @@ -403,9 +403,11 @@ elen = 0; *eof = (elen <= count) ? 1 : 0; *start = page + off; + err = elen; +_err: MOD_DEC_USE_COUNT; - return elen; + return err; } /* @@ -422,10 +424,9 @@ if (((buffer[0] == '1') || (buffer[0] == '0')) && ((count == 1) || ((count == 2) && (buffer[1] == '\n')))) { if (data) { - read_lock(&entries_lock); if ((e = get_entry((int) data))) e->flags = (e->flags & -2) | (int) (buffer[0] - '0'); - read_unlock(&entries_lock); + put_entry(e); } else { enabled = buffer[0] - '0'; }
| |