[PATCH] add a stub by which a module can bind to the AFS syscall

[PATCH] add a stub by which a module can bind to the AFS syscall

[David Howells]

Hi Linus,

This patch makes it possible for a module to bind safely to the AFS syscall,
without having to modify the syscall table directly.

David

diff -uNr -x'*.o' -x'.*' -xTAGS linux-2.5.67/include/linux/syscallstub.h linux-2.5.67-afs/include/linux/syscallstub.h
--- linux-2.5.67/include/linux/syscallstub.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.67-afs/include/linux/syscallstub.h	2003-04-07 13:17:38.000000000 +0100
@@ -0,0 +1,28 @@
+/* syscallstub.h: system call stub management
+ *
+ * Copyright (C) 2003 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _LINUX_SYSCALLSTUB_H
+#define _LINUX_SYSCALLSTUB_H
+
+#include <linux/module.h>
+
+/*
+ * AFS system call stub management
+ */
+typedef int (*afs_syscall_handler_t)(long cmd, va_list va);
+
+extern int register_afs_syscall_handler(afs_syscall_handler_t handler, struct module *owner);
+extern void unregister_afs_syscall_handler(afs_syscall_handler_t handler, struct module *owner);
+
+extern asmlinkage int sys_afs(long cmd, ...);
+
+
+#endif /* _LINUX_SYSCALLSTUB_H */
diff -uNr -x'*.o' -x'.*' -xTAGS linux-2.5.67/kernel/Makefile linux-2.5.67-afs/kernel/Makefile
--- linux-2.5.67/kernel/Makefile	2003-03-05 03:29:02.000000000 +0000
+++ linux-2.5.67-afs/kernel/Makefile	2003-04-07 13:16:52.000000000 +0100
@@ -6,7 +6,8 @@
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o futex.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    syscallstub.o
 
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o
 obj-$(CONFIG_SMP) += cpu.o
diff -uNr -x'*.o' -x'.*' -xTAGS linux-2.5.67/kernel/syscallstub.c linux-2.5.67-afs/kernel/syscallstub.c
--- linux-2.5.67/kernel/syscallstub.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.67-afs/kernel/syscallstub.c	2003-04-30 14:35:11.000000000 +0100
@@ -0,0 +1,88 @@
+/* syscallstub.c: module providable syscall stub management
+ *
+ * Copyright (C) 2003 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/syscallstub.h>
+#include <linux/spinlock.h>
+#include <linux/errno.h>
+
+/*
+ * AFS system call stub management
+ */
+static afs_syscall_handler_t	afs_syscall_handler;
+static struct module		*afs_syscall_owner;
+static rwlock_t			afs_syscall_handler_lock = RW_LOCK_UNLOCKED;
+
+int register_afs_syscall_handler(afs_syscall_handler_t handler, struct module *owner)
+{
+	int ret;
+
+	if (!handler || !owner)
+		return -EINVAL;
+
+	ret = -EEXIST;
+	write_lock(&afs_syscall_handler_lock);
+
+	if (!afs_syscall_handler) {
+		afs_syscall_handler	= handler;
+		afs_syscall_owner	= owner;
+		ret = 0;
+	}
+
+	write_unlock(&afs_syscall_handler_lock);
+
+	return ret;
+}
+
+EXPORT_SYMBOL(register_afs_syscall_handler);
+
+void unregister_afs_syscall_handler(afs_syscall_handler_t handler, struct module *owner)
+{
+	write_lock(&afs_syscall_handler_lock);
+
+	if (afs_syscall_handler==handler && afs_syscall_owner==owner) {
+		afs_syscall_handler	= NULL;
+		afs_syscall_owner	= NULL;
+	}
+	else {
+		printk("module %s tried to free AFS syscall which it did not own\n",
+		       owner->name);
+	}
+
+	write_unlock(&afs_syscall_handler_lock);
+}
+
+EXPORT_SYMBOL(unregister_afs_syscall_handler);
+
+asmlinkage int sys_afs(long cmd, ...)
+{
+	afs_syscall_handler_t handler;
+	struct module *owner;
+	va_list va;
+	int ret;
+
+	ret = -ENOSYS;
+	read_lock(&afs_syscall_handler_lock);
+	handler = afs_syscall_handler;
+	owner = afs_syscall_owner;
+	if (handler && try_module_get(owner))
+			ret = 0;
+	read_unlock(&afs_syscall_handler_lock);
+
+	if (ret<0)
+		return ret;
+
+	va_start(va,cmd);
+	ret = handler(cmd,va);
+	va_end(va);
+
+	module_put(owner);
+	return ret;
+}
+
diff -uNr -x'*.o' -x'.*' -xTAGS linux-2.5.67/arch/i386/kernel/entry.S linux-2.5.67-afs/arch/i386/kernel/entry.S
--- linux-2.5.67/arch/i386/kernel/entry.S	2003-03-28 11:37:52.000000000 +0000
+++ linux-2.5.67-afs/arch/i386/kernel/entry.S	2003-04-30 14:38:32.000000000 +0100
@@ -721,7 +721,7 @@
 	.long sys_bdflush
 	.long sys_sysfs		/* 135 */
 	.long sys_personality
-	.long sys_ni_syscall	/* reserved for afs_syscall */
+	.long sys_afs
 	.long sys_setfsuid16
 	.long sys_setfsgid16
 	.long sys_llseek	/* 140 */

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 02:44:54PM +0100, David Howells wrote:
> 
> Hi Linus,
> 
> This patch makes it possible for a module to bind safely to the AFS syscall,
> without having to modify the syscall table directly.

Umm, you're adding a handler so that a module can register a variadic
multiplexer syscall??  I think you need to rething your design..

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Jeff Garzik]

On Wed, Apr 30, 2003 at 03:02:11PM +0100, Christoph Hellwig wrote:
> On Wed, Apr 30, 2003 at 02:44:54PM +0100, David Howells wrote:
> > 
> > Hi Linus,
> > 
> > This patch makes it possible for a module to bind safely to the AFS syscall,
> > without having to modify the syscall table directly.
> 
> Umm, you're adding a handler so that a module can register a variadic
> multiplexer syscall??  I think you need to rething your design..

It's better than the alternative, having OpenAFS patch the system
call table itself... ;-)

	Jeff

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 10:46:38AM -0400, Jeff Garzik wrote:
> It's better than the alternative, having OpenAFS patch the system
> call table itself... ;-)

That's already taken care of with David's first patch.  There's no
reason we can't have an afsctlfs like the nfsctlfs.  But anyway,
please list the prototypes and usages of the subcalls here, maybe
some of them are generally usefull.

This kind of "I need a stuff fr a random syscall multiplexer"
requests are silly. APIs need review or you'll get the syssgi syndrome
really soon..

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[chas williams]

In message <20030430150211.A7024@infradead.org>,Christoph Hellwig writes:
>Umm, you're adding a handler so that a module can register a variadic
>multiplexer syscall??  I think you need to rething your design..

i dont believe it needs to be a variadic (the afs syscall will/uses the
first params).  however, the syscall interface for afs predates linux.
changing to a different interface just for linux kernels seems somewhat
costly (programmers time) when this existing interface has been well
tested and debugged on several other operating systems.

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 10:57:22AM -0400, chas williams wrote:
> i dont believe it needs to be a variadic (the afs syscall will/uses the
> first params).  however, the syscall interface for afs predates linux.
> changing to a different interface just for linux kernels seems somewhat
> costly (programmers time) when this existing interface has been well
> tested and debugged on several other operating systems.

We need to repeat a mistake others did has never been a valid
argument in linux devlopment..  Anyway, it's really hard to judge about
this before seeing the actual implementation instead of just saying
here's a stub I need.

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[chas williams]

In message <20030430160239.A8956@infradead.org>,Christoph Hellwig writes:
>We need to repeat a mistake others did has never been a valid
>argument in linux devlopment..  Anyway, it's really hard to judge about
>this before seeing the actual implementation instead of just saying
>here's a stub I need.

at the time afs was written it wasnt a mistake.  syscall was the only
(easy) way into the kernel from user space.  adding multiple syscalls
would have just been completely painful.  as for examples, pioctl() --
the user space of the afs syscall -- is a bit like syssgi() i am afraid:

venus/fs.c:     code = pioctl(0, VIOC_GETCELLSTATUS, &blob, 1);
venus/fs.c:    code = pioctl(0, VIOC_SETRXKCRYPT, &blob, 1);
vlserver/sascnvldb.c:   code = pioctl(ti->data, VIOCGETVOLSTAT, &blob, 1);
auth/ktc_nt.c:  code = pioctl(0, VIOCNEWGETTOK, &iob, 0);
auth/ktc_nt.c:  code = pioctl(0, VIOCDELTOK, &iob, 0);
package/package.c:  code = pioctl(0, VIOC_AFS_SYSNAME, &data, 1);
venus/up.c:          code = pioctl(file1, _VICEIOCTL(2), &blob, 1);

in reality, very few things other than afs are going to want to use
the afs syscall (arla might be a possible user).

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 11:13:23AM -0400, chas williams wrote:
> at the time afs was written it wasnt a mistake.

Oh yes, it was.  The same mistake as the even earlier SysV IPC mess.

> syscall was the only
> (easy) way into the kernel from user space.  adding multiple syscalls
> would have just been completely painful.  as for examples, pioctl() --
> the user space of the afs syscall -- is a bit like syssgi() i am afraid:
> 
> venus/fs.c:     code = pioctl(0, VIOC_GETCELLSTATUS, &blob, 1);
> venus/fs.c:    code = pioctl(0, VIOC_SETRXKCRYPT, &blob, 1);
> vlserver/sascnvldb.c:   code = pioctl(ti->data, VIOCGETVOLSTAT, &blob, 1);
> auth/ktc_nt.c:  code = pioctl(0, VIOCNEWGETTOK, &iob, 0);
> auth/ktc_nt.c:  code = pioctl(0, VIOCDELTOK, &iob, 0);
> package/package.c:  code = pioctl(0, VIOC_AFS_SYSNAME, &data, 1);
> venus/up.c:          code = pioctl(file1, _VICEIOCTL(2), &blob, 1);
> 
> in reality, very few things other than afs are going to want to use
> the afs syscall (arla might be a possible user).

So fix the AFS code up to use a routine for each subcall that
can still map to pioctl for !linux.  After that we can continue the
discussion on how these calls are best implemented on linux.

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[chas williams]

In message <20030430162739.A9255@infradead.org>,Christoph Hellwig writes:
>So fix the AFS code up to use a routine for each subcall that
>can still map to pioctl for !linux.  After that we can continue the
>discussion on how these calls are best implemented on linux.

because time is precious its quite a bit easier to fix one spot in 
the linux kernel than to fix a hundred or so in the afs code.

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 11:33:57AM -0400, chas williams wrote:
> In message <20030430162739.A9255@infradead.org>,Christoph Hellwig writes:
> >So fix the AFS code up to use a routine for each subcall that
> >can still map to pioctl for !linux.  After that we can continue the
> >discussion on how these calls are best implemented on linux.
> 
> because time is precious its quite a bit easier to fix one spot in 
> the linux kernel than to fix a hundred or so in the afs code.

It might be easier but it's not the correct fix.  I've we had done what's
easier all the time Linux would look like SCO Unix or IRIX now..

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Wed, 2003-04-30 at 17:33, chas williams wrote:
> In message <20030430162739.A9255@infradead.org>,Christoph Hellwig writes:
> >So fix the AFS code up to use a routine for each subcall that
> >can still map to pioctl for !linux.  After that we can continue the
> >discussion on how these calls are best implemented on linux.
> 
> because time is precious its quite a bit easier to fix one spot in 
> the linux kernel than to fix a hundred or so in the afs code.

ehm no.
There is no valid AFS code using this since there is no syscall yet ;)

And it IS more work for linux, since 32 bit emulation on 64 bit machines
will be a MAJOR pain if everything is this opaque.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Anton Blanchard]

> Oh yes, it was.  The same mistake as the even earlier SysV IPC mess.

And multiplexed syscalls are a pain for people like me who have to write
32/64 bit translations.

Anton

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[viro]

On Wed, Apr 30, 2003 at 11:13:23AM -0400, chas williams wrote:
> at the time afs was written it wasnt a mistake.  syscall was the only
> (easy) way into the kernel from user space.  adding multiple syscalls
> would have just been completely painful.  as for examples, pioctl() --
> the user space of the afs syscall -- is a bit like syssgi() i am afraid:
> 
> venus/fs.c:     code = pioctl(0, VIOC_GETCELLSTATUS, &blob, 1);
> venus/fs.c:    code = pioctl(0, VIOC_SETRXKCRYPT, &blob, 1);
> vlserver/sascnvldb.c:   code = pioctl(ti->data, VIOCGETVOLSTAT, &blob, 1);
> auth/ktc_nt.c:  code = pioctl(0, VIOCNEWGETTOK, &iob, 0);
> auth/ktc_nt.c:  code = pioctl(0, VIOCDELTOK, &iob, 0);
> package/package.c:  code = pioctl(0, VIOC_AFS_SYSNAME, &data, 1);
> venus/up.c:          code = pioctl(file1, _VICEIOCTL(2), &blob, 1);
> 
> in reality, very few things other than afs are going to want to use
> the afs syscall (arla might be a possible user).

Which means only one thing - changing that API will affect very few
things.

Let's keep the kernel side sane.  We don't have to mess with multiplexors
and even if we decide to use them, we will be better off by having decoder
outside of AFS proper.  Again, take a look at interaction between userland
and knfsd.  Right now we have a sane interface (IO on nfsctl files) and
we have a wrapper (sys_nfsctl) that does decode/open/write/read/close.

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[David Howells]

Hi Alex,

> Which means only one thing - changing that API will affect very few
> things.
> 
> Let's keep the kernel side sane.  We don't have to mess with multiplexors
> and even if we decide to use them, we will be better off by having decoder
> outside of AFS proper.  Again, take a look at interaction between userland
> and knfsd.  Right now we have a sane interface (IO on nfsctl files) and
> we have a wrapper (sys_nfsctl) that does decode/open/write/read/close.

As you suggested on IRC, almost all pioctls can be emulated by either:

 (1) doing a decode/mount/open/write/read/close on a file on a special
     internal filesystem if an inode is not required,

or:

 (2) translating the call to get/set/lget/lsetxattr calls if an inode is
     required.

However, there's always an exception. One of the pioctls requires three
things:

 (a) a dentry and inode,
 (b) an input buffer with a filename in it,
 (c) an output buffer for holding a fair amount of data.

This can't be done easily with (1) because of (a), and it can't be done
atomically as (2) because two separate calls would have to be made for (b) and
(c) (and the netfs would have to retain some sort of state).

How about, instead, for pioctl calls that require an inode, the AFS
multiplexor in the kernel invents a temporary file structure, points it at the
dentry and calls its open, ioctl and release calls on inode->f_op?

Or perhaps you'd consider adding an extra inode operation?

	struct inode_operations {
		...
		int (*inodectl) (struct dentry *, unsigned int, unsigned long);
	};

David

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[David Howells]

I selected a variadic method of argument passing because I didn't want to have
to copy the argument block several times in the course of passing from the
syscall stub through the multiplexor to the actual handler.

Of course, it might be better if the kernel itself sorted out the individual
subcalls and passed them to the filesystem individually or dealt with them
itself.

The four calls implemented by Linux are:

 (*) int setpag(void)

     Set Process Authentication Group number. This could easily be moved into
     the kernel proper, with the PAG being stored in or depending from the
     task structure somehow.

     This would then obviate the need for OpenAFS to mangle the setgroups and
     getgroups syscalls.

 (*) int pioctl(const char *path, int cmd, void *arg, int followsymlink)

     Al Viro's favourite:-) Do ioctl() on a file refered to by pathname. Can't
     be emulated by open/ioctl/close because:

     (a) it can operate directly on symbolic links.

     (b) some of its functions don't require a file and don't fail if one
	 can't be opened.

 (*) int afs_call(...)

     Local client control

 (*) int afs_icl(...)

     Local client status and logging control.

There are six more which linux doesn't actually support, even though the
multiplexor does:

 (*) icreate
 (*) iopen
 (*) idec
 (*) iinc
 (*) iread
 (*) iwrite

     Deal with file by inode number.

David

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Christoph Hellwig]

On Wed, Apr 30, 2003 at 04:30:20PM +0100, David Howells wrote:
> The four calls implemented by Linux are:
> 
>  (*) int setpag(void)
> 
>      Set Process Authentication Group number. This could easily be moved into
>      the kernel proper, with the PAG being stored in or depending from the
>      task structure somehow.

So please submit a patch for doing this in the kernel proper.

>  (*) int pioctl(const char *path, int cmd, void *arg, int followsymlink)
> 
>      Al Viro's favourite:-) Do ioctl() on a file refered to by pathname. Can't
>      be emulated by open/ioctl/close because:
> 
>      (a) it can operate directly on symbolic links.
> 
>      (b) some of its functions don't require a file and don't fail if one
> 	 can't be opened.

You don't expect we merge something that broken?  And then as multiplexer
inside a multiplexer?  This starts to look worse than sys_ipc()..

> 
>  (*) int afs_call(...)
> 
>      Local client control
> 
>  (*) int afs_icl(...)
> 
>      Local client status and logging control.

What's ...?

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Jan Harkes]

On Wed, Apr 30, 2003 at 04:30:20PM +0100, David Howells wrote:
> The four calls implemented by Linux are:
> 
>  (*) int setpag(void)
> 
>      Set Process Authentication Group number. This could easily be moved into
>      the kernel proper, with the PAG being stored in or depending from the
>      task structure somehow.
> 
>      This would then obviate the need for OpenAFS to mangle the setgroups and
>      getgroups syscalls.

Has been proposed many times in the context of Coda. Perhaps now that
there are 2 filesystems in the tree that want something like this we can
afford the extra int in the task structure. More likely we'll just have
to wait patiently for a credentials cache, or task ornaments, or
something might be possible with the LSM framework.

>  (*) int pioctl(const char *path, int cmd, void *arg, int followsymlink)
> 
>      Al Viro's favourite:-) Do ioctl() on a file refered to by pathname. Can't
>      be emulated by open/ioctl/close because:
> 
>      (a) it can operate directly on symbolic links.
>      (b) some of its functions don't require a file and don't fail if one
> 	 can't be opened.

Coda has the same abomination, but we use an ioctl on a special file in
the root of the Coda tree (/coda/.CONTROL). The advantage of this is
that if we have multiple clients and mountpoints we know which client is
supposed to handle the pioctl.

>  (*) int afs_call(...)
>  (*) int afs_icl(...)

We don't have anything like that, and most likely just stuffed them
into the pioctl multiplexor.

> There are six more which linux doesn't actually support, even though the
> multiplexor does:
> 
>  (*) icreate
>  (*) iopen
>  (*) idec
>  (*) iinc
>  (*) iread
>  (*) iwrite
> 
>      Deal with file by inode number.

You don't want to do that anyways, it will only work reliably on a few
filesystems. ReiserFS can possibly have inode collisions which are
resolved based on the parent directory inode. ext3 only journals writes
when a filehandle is closed, so opening by inode, writing to it and
closing it doesn't trigger a journal update. ramfs and tmpfs really
don't have any usable underlying inodes that can be opened with a
device/inode number pair because there is no 'device'.

Jan

Re: [PATCH] add a stub by which a module can bind to the AFS syscall

[Trond Myklebust]

>>>>> " " == Jan Harkes <jaharkes@cs.cmu.edu> writes:

     > On Wed, Apr 30, 2003 at 04:30:20PM +0100, David Howells wrote:
    >> The four calls implemented by Linux are:
    >>
    >> (*) int setpag(void)
    >>
    >> Set Process Authentication Group number. This could easily be
    >> moved into the kernel proper, with the PAG being stored in or
    >> depending from the task structure somehow.
    >>
    >> This would then obviate the need for OpenAFS to mangle the
    >> setgroups and getgroups syscalls.

     > Has been proposed many times in the context of Coda. Perhaps
     > now that there are 2 filesystems in the tree that want
     > something like this we can afford the extra int in the task
     > structure.

Make that 3. We would be able to make good use of the same feature for
strong authentication on NFS.

Cheers,
  Trond