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';
}