[PATCH] PAG support only

[PATCH] PAG support only

[David Howells]

Hi Linus,

Due to the slight unpopularity of the AFS multiplexor, here's a patch with
only the PAG support.

David

diff -uNr linux-2.5.69/arch/i386/kernel/entry.S linux-2.5.69-cred/arch/i386/kernel/entry.S
--- linux-2.5.69/arch/i386/kernel/entry.S	2003-05-06 15:06:47.000000000 +0100
+++ linux-2.5.69-cred/arch/i386/kernel/entry.S	2003-05-13 11:16:17.000000000 +0100
@@ -852,6 +852,7 @@
  	.long sys_clock_gettime		/* 265 */
  	.long sys_clock_getres
  	.long sys_clock_nanosleep
- 
+	.long sys_setpag
+	.long sys_getpag
  
 nr_syscalls=(.-sys_call_table)/4
diff -uNr linux-2.5.69/fs/file_table.c linux-2.5.69-cred/fs/file_table.c
--- linux-2.5.69/fs/file_table.c	2003-05-06 15:04:45.000000000 +0100
+++ linux-2.5.69-cred/fs/file_table.c	2003-05-13 13:54:58.000000000 +0100
@@ -166,6 +166,7 @@
 	if (file->f_op && file->f_op->release)
 		file->f_op->release(inode, file);
 	security_file_free(file);
+	if (file->f_token) vfs_token_put(file->f_token);
 	fops_put(file->f_op);
 	if (file->f_mode & FMODE_WRITE)
 		put_write_access(inode);
diff -uNr linux-2.5.69/fs/open.c linux-2.5.69-cred/fs/open.c
--- linux-2.5.69/fs/open.c	2003-05-06 15:04:45.000000000 +0100
+++ linux-2.5.69-cred/fs/open.c	2003-05-13 11:28:08.000000000 +0100
@@ -46,7 +46,7 @@
 	struct nameidata nd;
 	int error;
 
-	error = user_path_walk(path, &nd);
+	error = user_path_walk(path,&nd);
 	if (!error) {
 		struct statfs tmp;
 		error = vfs_statfs(nd.dentry->d_inode->i_sb, &tmp);
diff -uNr linux-2.5.69/fs/proc/array.c linux-2.5.69-cred/fs/proc/array.c
--- linux-2.5.69/fs/proc/array.c	2003-05-06 15:07:08.000000000 +0100
+++ linux-2.5.69-cred/fs/proc/array.c	2003-05-13 10:58:56.000000000 +0100
@@ -154,13 +154,14 @@
 	read_lock(&tasklist_lock);
 	buffer += sprintf(buffer,
 		"State:\t%s\n"
+		"Pag:\t%d\n"
 		"Tgid:\t%d\n"
 		"Pid:\t%d\n"
 		"PPid:\t%d\n"
 		"TracerPid:\t%d\n"
 		"Uid:\t%d\t%d\t%d\t%d\n"
 		"Gid:\t%d\t%d\t%d\t%d\n",
-		get_task_state(p), p->tgid,
+		get_task_state(p), p->vfspag ? p->vfspag->pag : 0, p->tgid,
 		p->pid, p->pid ? p->real_parent->pid : 0,
 		p->pid && p->ptrace ? p->parent->pid : 0,
 		p->uid, p->euid, p->suid, p->fsuid,
diff -uNr linux-2.5.69/include/asm-i386/posix_types.h linux-2.5.69-cred/include/asm-i386/posix_types.h
--- linux-2.5.69/include/asm-i386/posix_types.h	2003-05-06 15:04:37.000000000 +0100
+++ linux-2.5.69-cred/include/asm-i386/posix_types.h	2003-05-12 10:19:15.000000000 +0100
@@ -13,6 +13,7 @@
 typedef unsigned short	__kernel_nlink_t;
 typedef long		__kernel_off_t;
 typedef int		__kernel_pid_t;
+typedef int		__kernel_pag_t;
 typedef unsigned short	__kernel_ipc_pid_t;
 typedef unsigned short	__kernel_uid_t;
 typedef unsigned short	__kernel_gid_t;
diff -uNr linux-2.5.69/include/asm-i386/unistd.h linux-2.5.69-cred/include/asm-i386/unistd.h
--- linux-2.5.69/include/asm-i386/unistd.h	2003-05-06 15:04:37.000000000 +0100
+++ linux-2.5.69-cred/include/asm-i386/unistd.h	2003-05-13 10:47:59.000000000 +0100
@@ -273,8 +273,10 @@
 #define __NR_clock_gettime	(__NR_timer_create+6)
 #define __NR_clock_getres	(__NR_timer_create+7)
 #define __NR_clock_nanosleep	(__NR_timer_create+8)
+#define __NR_setpag		268
+#define __NR_getpag		269
 
-#define NR_syscalls 268
+#define NR_syscalls 270
 
 /* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */
 
diff -uNr linux-2.5.69/include/linux/cred.h linux-2.5.69-cred/include/linux/cred.h
--- linux-2.5.69/include/linux/cred.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.69-cred/include/linux/cred.h	2003-05-13 13:20:06.000000000 +0100
@@ -0,0 +1,79 @@
+#ifndef _LINUX_CRED_H
+#define _LINUX_CRED_H
+
+#ifdef __KERNEL__
+
+#include <linux/param.h>
+#include <linux/types.h>
+#include <linux/list.h>
+#include <linux/rbtree.h>
+#include <asm/atomic.h>
+
+/*
+ * VFS session authentication token cache
+ *
+ * This is used to store the data required for extra levels of filesystem
+ * security (such as AFS/NFS kerberos keys, Samba workgroup/user/pass, or NTFS
+ * ACLs).
+ *
+ * VFS authentication tokens contain a single blob of data, consisting of three
+ * parts, all next to each other:
+ *   (1) An FS name
+ *   (2) A key
+ *   (3) An arbitrary chunk of data
+ *
+ * Token blobs must not be changed once passed to the core kernel for management
+ */
+struct vfs_pag
+{
+	struct rb_node		node;
+	atomic_t		usage;
+	pag_t			pag;	/* Process Authentication Group ID */
+	struct list_head	tokens;	/* authentication tokens */
+	rwlock_t		lock;
+};
+
+struct vfs_token
+{
+	atomic_t		usage;
+	struct list_head	link;	/* link in pag's list */
+	unsigned short		k_off;	/* offset of key in blob */
+	unsigned short		d_off;	/* offset of data in blob */
+	size_t			size;	/* size of blob */
+	void			*blob;	/* blob containing key + data */
+};
+
+extern int sys_setpag(pag_t);
+extern int sys_getpag(void);
+extern void vfs_unpag(const char *fsname);
+
+extern void vfs_pag_put(struct vfs_pag *);
+
+static inline struct vfs_pag *vfs_pag_get(struct vfs_pag *vfspag)
+{
+	atomic_inc(&vfspag->usage);
+	return vfspag;
+}
+
+static inline int is_vfs_token_valid(struct vfs_token *vtoken)
+{
+	return !list_empty(&vtoken->link);
+}
+
+extern int vfs_pag_add_token(const char *fsname,
+			     unsigned short klen,
+			     const void *key,
+			     size_t dlen,
+			     const void *data,
+			     struct vfs_token **_token);
+
+extern struct vfs_token *vfs_pag_find_token(const char *fsname,
+					    unsigned short klen,
+					    const void *key);
+
+extern void vfs_pag_withdraw_token(struct vfs_token *vtoken);
+
+extern void vfs_token_put(struct vfs_token *vtoken);
+
+#endif /* __KERNEL__ */
+#endif /* _LINUX_CRED_H */
diff -uNr linux-2.5.69/include/linux/fs.h linux-2.5.69-cred/include/linux/fs.h
--- linux-2.5.69/include/linux/fs.h	2003-05-13 11:02:22.000000000 +0100
+++ linux-2.5.69-cred/include/linux/fs.h	2003-05-13 11:02:35.000000000 +0100
@@ -430,6 +430,7 @@
 	mode_t			f_mode;
 	loff_t			f_pos;
 	struct fown_struct	f_owner;
+	struct vfs_token	*f_token;	/* governing credential */
 	unsigned int		f_uid, f_gid;
 	int			f_error;
 	struct file_ra_state	f_ra;
diff -uNr linux-2.5.69/include/linux/sched.h linux-2.5.69-cred/include/linux/sched.h
--- linux-2.5.69/include/linux/sched.h	2003-05-06 15:07:12.000000000 +0100
+++ linux-2.5.69-cred/include/linux/sched.h	2003-05-13 10:29:18.000000000 +0100
@@ -28,6 +28,7 @@
 #include <linux/completion.h>
 #include <linux/pid.h>
 #include <linux/percpu.h>
+#include <linux/cred.h>
 
 struct exec_domain;
 
@@ -387,6 +388,7 @@
 	gid_t gid,egid,sgid,fsgid;
 	int ngroups;
 	gid_t	groups[NGROUPS];
+	struct vfs_pag *vfspag;
 	kernel_cap_t   cap_effective, cap_inheritable, cap_permitted;
 	int keep_capabilities:1;
 	struct user_struct *user;
diff -uNr linux-2.5.69/include/linux/types.h linux-2.5.69-cred/include/linux/types.h
--- linux-2.5.69/include/linux/types.h	2003-05-06 15:04:31.000000000 +0100
+++ linux-2.5.69-cred/include/linux/types.h	2003-05-12 10:19:08.000000000 +0100
@@ -24,6 +24,7 @@
 typedef __kernel_nlink_t	nlink_t;
 typedef __kernel_off_t		off_t;
 typedef __kernel_pid_t		pid_t;
+typedef __kernel_pag_t		pag_t;
 typedef __kernel_daddr_t	daddr_t;
 typedef __kernel_key_t		key_t;
 typedef __kernel_suseconds_t	suseconds_t;
diff -uNr linux-2.5.69/init/main.c linux-2.5.69-cred/init/main.c
--- linux-2.5.69/init/main.c	2003-05-06 15:07:12.000000000 +0100
+++ linux-2.5.69-cred/init/main.c	2003-05-13 14:08:11.000000000 +0100
@@ -80,6 +80,7 @@
 extern void pidhash_init(void);
 extern void pidmap_init(void);
 extern void pte_chain_init(void);
+extern void credentials_init(void);
 extern void radix_tree_init(void);
 extern void free_initmem(void);
 extern void populate_rootfs(void);
@@ -434,6 +435,7 @@
 	pidmap_init();
 	pgtable_cache_init();
 	pte_chain_init();
+	credentials_init();
 	fork_init(num_physpages);
 	proc_caches_init();
 	security_scaffolding_startup();
diff -uNr linux-2.5.69/kernel/cred.c linux-2.5.69-cred/kernel/cred.c
--- linux-2.5.69/kernel/cred.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.69-cred/kernel/cred.c	2003-05-13 13:21:06.000000000 +0100
@@ -0,0 +1,367 @@
+/* cred.c: authentication credentials 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/config.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/string.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
+
+static kmem_cache_t *vfs_token_cache;
+static kmem_cache_t *vfs_pag_cache;
+
+static struct rb_root	vfs_pag_tree;
+static spinlock_t	vfs_pag_lock = SPIN_LOCK_UNLOCKED;
+static pag_t		vfs_pag_next = 1;
+
+static void vfs_pag_init_once(void *_vfspag, kmem_cache_t *cachep, unsigned long flags)
+{
+	struct vfs_pag *vfspag = _vfspag;
+
+	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR) {
+		memset(vfspag,0,sizeof(*vfspag));
+		INIT_LIST_HEAD(&vfspag->tokens);
+		rwlock_init(&vfspag->lock);
+	}
+}
+
+static void vfs_token_init_once(void *_vtoken, kmem_cache_t *cachep, unsigned long flags)
+{
+	struct vfs_token *vtoken = _vtoken;
+
+	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR) {
+		memset(vtoken,0,sizeof(*vtoken));
+		INIT_LIST_HEAD(&vtoken->link);
+	}
+}
+
+void __init credentials_init(void)
+{
+	vfs_pag_cache = kmem_cache_create("vfs_pag",
+					      sizeof(struct vfs_pag),
+					      0,
+					      SLAB_HWCACHE_ALIGN,
+					      vfs_pag_init_once, NULL);
+	if (!vfs_pag_cache)
+		panic("Cannot create vfs pag SLAB cache");
+
+	vfs_token_cache = kmem_cache_create("vfs_token",
+					    sizeof(struct vfs_token),
+					    0,
+					    SLAB_HWCACHE_ALIGN,
+					    vfs_token_init_once, NULL);
+	if (!vfs_token_cache)
+		panic("Cannot create vfs token SLAB cache");
+}
+
+/*****************************************************************************/
+/*
+ * join an existing PAG (+ve), run without PAG (0), or create and join new PAG (-1)
+ * - returns ID of PAG joined or 0 if now running without a PAG
+ */
+int sys_setpag(pag_t pag)
+{
+	struct task_struct *tsk = current;
+	struct vfs_pag *vfspag, *xvfspag;
+	struct rb_node **p, *parent;
+
+	if (pag>0) {
+		/* join existing PAG */
+		if (tsk->vfspag->pag &&
+		    tsk->vfspag->pag==pag)
+			return pag;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		spin_lock(&vfs_pag_lock);
+
+		parent = NULL;
+		p = &vfs_pag_tree.rb_node;
+
+		while (*p) {
+			parent = *p;
+			vfspag = rb_entry(parent,struct vfs_pag,node);
+
+			if (pag < vfspag->pag)
+				p = &(*p)->rb_left;
+			else if (pag > vfspag->pag)
+				p = &(*p)->rb_right;
+			else
+				goto pag_found;
+		}
+
+		spin_unlock(&vfs_pag_lock);
+		return -ENOENT;
+
+	pag_found:
+		xvfspag = xchg(&current->vfspag,vfs_pag_get(vfspag));
+		spin_unlock(&vfs_pag_lock);
+
+		if (xvfspag) vfs_pag_put(xvfspag);
+		return pag;
+	}
+	else if (pag==0) {
+		/* run without a PAG */
+		xvfspag = xchg(&current->vfspag,NULL);
+
+		if (xvfspag) vfs_pag_put(xvfspag);
+		return 0;
+	}
+	else if (pag!=-1) {
+		     return -EINVAL;
+	}
+
+	/* start new PAG */
+	vfspag = kmem_cache_alloc(vfs_pag_cache,SLAB_KERNEL);
+	if (!vfspag)
+		return -ENOMEM;
+
+	atomic_set(&vfspag->usage,1);
+
+	/* PAG IDs must be +ve, >0 and unique */
+	spin_lock(&vfs_pag_lock);
+
+	vfspag->pag = vfs_pag_next++;
+	if (vfspag->pag<1)
+		vfspag->pag = 1;
+
+	parent = NULL;
+	p = &vfs_pag_tree.rb_node;
+
+	while (*p) {
+		parent = *p;
+		xvfspag = rb_entry(parent,struct vfs_pag,node);
+
+		if (vfspag->pag < xvfspag->pag)
+			p = &(*p)->rb_left;
+		else if (vfspag->pag > xvfspag->pag)
+			p = &(*p)->rb_right;
+		else
+			goto pag_exists;
+	}
+	goto insert_here;
+
+	/* we found a PAG of the same ID - walk the tree from that point
+	 * looking for the next unused PAG */
+ pag_exists:
+	for (;;) {
+		vfspag->pag = vfs_pag_next++;
+		if (vfspag->pag<1)
+			vfspag->pag = 1;
+
+		if (!parent->rb_parent)
+			p = &vfs_pag_tree.rb_node;
+		else if (parent->rb_parent->rb_left==parent)
+			p = &parent->rb_parent->rb_left;
+		else
+			p = &parent->rb_parent->rb_right;
+
+		parent = rb_next(parent);
+		if (!parent)
+			break;
+
+		xvfspag = rb_entry(parent,struct vfs_pag,node);
+		if (vfspag->pag < xvfspag->pag)
+			goto insert_here;
+	}
+
+ insert_here:
+	rb_link_node(&vfspag->node,parent,p);
+	rb_insert_color(&vfspag->node,&vfs_pag_tree);
+	spin_unlock(&vfs_pag_lock);
+
+	xvfspag = xchg(&current->vfspag,vfspag);
+	if (xvfspag) vfs_pag_put(xvfspag);
+
+	return vfspag->pag;
+} /* end sys_setpag() */
+
+/*****************************************************************************/
+/*
+ * get the PAG of the current process, or 0 if it doesn't have one
+ */
+int sys_getpag(void)
+{
+	struct vfs_pag *vfspag = current->vfspag;
+
+	return vfspag ? vfspag->pag : 0;
+
+} /* end sys_getpag() */
+
+/*****************************************************************************/
+/*
+ * dispose of a VFS pag
+ */
+void vfs_pag_put(struct vfs_pag *vfspag)
+{
+	struct vfs_token *vtoken;
+
+	if (atomic_dec_and_lock(&vfspag->usage,&vfs_pag_lock)) {
+		rb_erase(&vfspag->node,&vfs_pag_tree);
+		spin_unlock(&vfs_pag_lock);
+
+		while (!list_empty(&vfspag->tokens)) {
+			vtoken = list_entry(vfspag->tokens.next,struct vfs_token,link);
+			list_del_init(&vtoken->link);
+			vfs_token_put(vtoken);
+		}
+
+		kmem_cache_free(vfs_pag_cache,vfspag);
+	}
+
+} /* end vfs_pag_put() */
+
+/*****************************************************************************/
+/*
+ * dispose of a VFS token
+ */
+void vfs_token_put(struct vfs_token *vtoken)
+{
+	if (atomic_dec_and_test(&vtoken->usage)) {
+		kfree(vtoken->blob);
+		kmem_cache_free(vfs_pag_cache,vtoken);
+	}
+
+} /* end vfs_token_put() */
+
+/*****************************************************************************/
+/*
+ * add an authentication token to a pag list
+ */
+int vfs_pag_add_token(const char *fsname,
+		      unsigned short klen,
+		      const void *key,
+		      size_t dlen,
+		      const void *data,
+		      struct vfs_token **_vtoken)
+{
+	struct vfs_token *vtoken;
+	struct vfs_pag *vfspag = current->vfspag;
+
+	*_vtoken = NULL;
+
+	if (!vfspag)
+		return -EACCES;
+
+	vtoken = kmem_cache_alloc(vfs_token_cache,SLAB_KERNEL);
+	if (!vtoken)
+		return -ENOMEM;
+
+	vtoken->k_off	= strlen(fsname) + 1;
+	vtoken->d_off	= vtoken->k_off + klen;
+	vtoken->size	= vtoken->d_off + dlen;
+
+	vtoken->blob = kmalloc(vtoken->size,SLAB_KERNEL);
+	if (!vtoken->blob) {
+		kfree(vtoken);
+		return -ENOMEM;
+	}
+
+	atomic_set(&vtoken->usage,1);
+
+	memcpy(vtoken->blob,fsname,vtoken->k_off);
+	memcpy(vtoken->blob+vtoken->k_off,key,klen);
+	memcpy(vtoken->blob+vtoken->d_off,key,dlen);
+
+	write_lock(&vfspag->lock);
+	list_add_tail(&vtoken->link,&vfspag->tokens);
+	write_unlock(&vfspag->lock);
+
+	*_vtoken = vtoken;
+	return 0;
+} /* end vfs_pag_add_token() */
+
+EXPORT_SYMBOL(vfs_pag_add_token);
+
+/*****************************************************************************/
+/*
+ * search for a token covering a particular filesystem key in the specified pag list
+ */
+struct vfs_token *vfs_pag_find_token(const char *fsname,
+				     unsigned short klen,
+				     const void *key)
+{
+	struct vfs_token *vtoken;
+	struct vfs_pag *vfspag = current->vfspag;
+
+	if (!vfspag) return NULL;
+
+	read_lock(&vfspag->lock);
+
+	list_for_each_entry(vtoken,&vfspag->tokens,link) {
+		if (vtoken->d_off-vtoken->k_off == klen &&
+		    strcmp(vtoken->blob,fsname)==0 &&
+		    memcmp(vtoken->blob+vtoken->k_off,key,klen)==0)
+			goto found;		    
+	}
+
+	read_unlock(&vfspag->lock);
+	return NULL;
+
+ found:
+	read_unlock(&vfspag->lock);
+	return vtoken;
+} /* end vfs_pag_find_token() */
+
+EXPORT_SYMBOL(vfs_pag_find_token);
+
+/*****************************************************************************/
+/*
+ * withdraw a token from a pag list
+ */
+void vfs_pag_withdraw_token(struct vfs_token *vtoken)
+{
+	struct vfs_pag *vfspag = current->vfspag;
+
+	if (!vfspag)
+		return;
+
+	write_lock(&vfspag->lock);
+	list_del_init(&vtoken->link);
+	write_unlock(&vfspag->lock);
+
+	vfs_token_put(vtoken);
+
+} /* end vfs_pag_withdraw_token() */
+
+EXPORT_SYMBOL(vfs_pag_withdraw_token);
+
+/*****************************************************************************/
+/*
+ * withdraw all tokens for the named filesystem from the current PAG
+ */
+void vfs_unpag(const char *fsname)
+{
+	struct list_head *_n, *_p;
+	struct vfs_token *vtoken;
+	struct vfs_pag *vfspag = current->vfspag;
+
+	if (!vfspag)
+		return;
+
+	write_lock(&vfspag->lock);
+
+	list_for_each_safe(_p,_n,&vfspag->tokens) {
+		vtoken = list_entry(_p,struct vfs_token,link);
+
+		if (strcmp(fsname,vtoken->blob)==0) {
+			list_del_init(&vtoken->link);
+			vfs_token_put(vtoken);
+		}
+	}
+
+	write_unlock(&vfspag->lock);
+
+} /* end vfs_unpag() */
diff -uNr linux-2.5.69/kernel/fork.c linux-2.5.69-cred/kernel/fork.c
--- linux-2.5.69/kernel/fork.c	2003-05-06 15:07:12.000000000 +0100
+++ linux-2.5.69-cred/kernel/fork.c	2003-05-13 10:39:37.000000000 +0100
@@ -884,6 +884,9 @@
 
 	if (clone_flags & CLONE_CHILD_SETTID)
 		p->set_child_tid = child_tidptr;
+
+	if (p->vfspag) vfs_pag_get(p->vfspag);
+
 	/*
 	 * Clear TID on mm_release()?
 	 */
diff -uNr linux-2.5.69/kernel/Makefile linux-2.5.69-cred/kernel/Makefile
--- linux-2.5.69/kernel/Makefile	2003-05-06 15:04:56.000000000 +0100
+++ linux-2.5.69-cred/kernel/Makefile	2003-05-13 10:45:27.000000000 +0100
@@ -3,7 +3,7 @@
 #
 
 obj-y     = sched.o fork.o exec_domain.o panic.o printk.o profile.o \
-	    exit.o itimer.o time.o softirq.o resource.o \
+	    cred.o 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

Re: [PATCH] PAG support only

[Jeff Garzik]

On Tue, May 13, 2003 at 05:33:11PM +0100, David Howells wrote:
> Due to the slight unpopularity of the AFS multiplexor, here's a patch with

I think of it as "bitter pill we will be forced to swallow", and then
hope and pray that we can use your AFS stuff as a transition step to --
after many years -- migrate people away from AFS altogether.  ;-)

Or maybe that's just wishful thinking.


> diff -uNr linux-2.5.69/fs/open.c linux-2.5.69-cred/fs/open.c
> --- linux-2.5.69/fs/open.c	2003-05-06 15:04:45.000000000 +0100
> +++ linux-2.5.69-cred/fs/open.c	2003-05-13 11:28:08.000000000 +0100
> @@ -46,7 +46,7 @@
>  	struct nameidata nd;
>  	int error;
>  
> -	error = user_path_walk(path, &nd);
> +	error = user_path_walk(path,&nd);

a bit of noise


> --- linux-2.5.69/include/asm-i386/posix_types.h	2003-05-06 15:04:37.000000000 +0100
> +++ linux-2.5.69-cred/include/asm-i386/posix_types.h	2003-05-12 10:19:15.000000000 +0100
> @@ -13,6 +13,7 @@
>  typedef unsigned short	__kernel_nlink_t;
>  typedef long		__kernel_off_t;
>  typedef int		__kernel_pid_t;
> +typedef int		__kernel_pag_t;
>  typedef unsigned short	__kernel_ipc_pid_t;
>  typedef unsigned short	__kernel_uid_t;
>  typedef unsigned short	__kernel_gid_t;
[...]
> +int sys_setpag(pag_t pag)
> +int sys_getpag(void)

Surely you want s/int/long/ here?

Two other comments:

* even though you're referencing 'current', I'm a bit surprised you
  don't need any locking at all in sys_getpag.  Is that guaranteed
  through judicious use of xchg()?

* is it reasonable to make credentials support a config option?
  Long term I worry about Linux kernel becoming another Irix, supporting
  thousands of rarely used syscalls unconditionally.

	Jeff



P.S.  Looking forward to the "cachefs" code you have in your cvs repo
hitting mainline.  That will be fun for NFS.  <grin>

Re: [PATCH] PAG support only

[Christoph Hellwig]

> @@ -166,6 +166,7 @@
>  	if (file->f_op && file->f_op->release)
>  		file->f_op->release(inode, file);
>  	security_file_free(file);
> +	if (file->f_token) vfs_token_put(file->f_token);

Please split this into 2 lines as per Documentation/CodingStyle.  Even better
make vfs_token_put handle a NULL argument.

> diff -uNr linux-2.5.69/fs/open.c linux-2.5.69-cred/fs/open.c
> --- linux-2.5.69/fs/open.c	2003-05-06 15:04:45.000000000 +0100
> +++ linux-2.5.69-cred/fs/open.c	2003-05-13 11:28:08.000000000 +0100
> @@ -46,7 +46,7 @@
>  	struct nameidata nd;
>  	int error;
>  
> -	error = user_path_walk(path, &nd);
> +	error = user_path_walk(path,&nd);

Random whitespace change - and even a wrong one..

> +static inline int is_vfs_token_valid(struct vfs_token *vtoken)
> +{
> +	return !list_empty(&vtoken->link);
> +}

This one is not used - and the name would imply it would be used without taking
a lock and thus racy..

> +static kmem_cache_t *vfs_token_cache;
> +static kmem_cache_t *vfs_pag_cache;

How many of those will be around for a typical AFS client?  I have the vague
feeling the slabs are overkill..

> +	if (pag>0) {
> +		/* join existing PAG */
> +		if (tsk->vfspag->pag &&
> +		    tsk->vfspag->pag==pag)
> +			return pag;

Please try to get your code in conformance with Documentation/CodingStyle.

> +} /* end vfs_pag_put() */

Re: [OpenAFS-devel] Re: [PATCH] PAG support only

[Nathan Neulinger]

> > +static kmem_cache_t *vfs_token_cache;
> > +static kmem_cache_t *vfs_pag_cache;
> 
> How many of those will be around for a typical AFS client?  I have the vague
> feeling the slabs are overkill..

What's a "typical client"?

I have machines that typically have 0 pags and tokens in kernel, and I
have machines that typically have a few hundred to a thousand.

Per pag on most clients will only have a single token, but number of
pags will depend totally on the nature of the machine. 

I also have machines that have a few hundred pags around that don't have
any tokens in them, but could at any time.

-- Nathan

------------------------------------------------------------
Nathan Neulinger                       EMail:  nneul@umr.edu
University of Missouri - Rolla         Phone: (573) 341-4841
Computing Services                       Fax: (573) 341-4216

Re: Re: [PATCH] PAG support only

[Nathan Neulinger]

> > +static kmem_cache_t *vfs_token_cache;
> > +static kmem_cache_t *vfs_pag_cache;
> 
> How many of those will be around for a typical AFS client?  I have the vague
> feeling the slabs are overkill..

What's a "typical client"?

I have machines that typically have 0 pags and tokens in kernel, and I
have machines that typically have a few hundred to a thousand.

Per pag on most clients will only have a single token, but number of
pags will depend totally on the nature of the machine. 

I also have machines that have a few hundred pags around that don't have
any tokens in them, but could at any time.

-- Nathan

------------------------------------------------------------
Nathan Neulinger                       EMail:  nneul@umr.edu
University of Missouri - Rolla         Phone: (573) 341-4841
Computing Services                       Fax: (573) 341-4216

Re: [OpenAFS-devel] Re: [PATCH] PAG support only

[Christoph Hellwig]

[openafs-devel dropped from the Cc-list due to stupid subscriber only policy]

On Tue, May 13, 2003 at 05:27:20PM -0500, Nathan Neulinger wrote:
> > > +static kmem_cache_t *vfs_token_cache;
> > > +static kmem_cache_t *vfs_pag_cache;
> > 
> > How many of those will be around for a typical AFS client?  I have the vague
> > feeling the slabs are overkill..
> 
> What's a "typical client"?

The case we wan to optimize for.  The question here is whether we really want
a separate slab or whether it makes more senze to just use the new kmalloc
slab (usually power of two sized).  

Re: [OpenAFS-devel] Re: [PATCH] PAG support only

[Derek Atkins]

Nathan Neulinger <nneul@umr.edu> writes:

> Per pag on most clients will only have a single token, but number of
> pags will depend totally on the nature of the machine. 

Uh, no, at least around MIT per-pag users usually have about 2-3
tokens on average.  I've run with as many as 10.  Please do not assume
some max. number of tokens in a pag.

> I also have machines that have a few hundred pags around that don't have
> any tokens in them, but could at any time.
> 
> -- Nathan

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord@MIT.EDU                        PGP key available

Re: [PATCH] PAG support only

[David Howells]

Christoph Hellwig wrote:

> Please split this into 2 lines as per Documentation/CodingStyle.  Even better
> make vfs_token_put handle a NULL argument.

Done the latter.

> Random whitespace change

Gone.

> - and even a wrong one..

I disagree, but no matter.

> > +static kmem_cache_t *vfs_token_cache;
> > +static kmem_cache_t *vfs_pag_cache;
> 
> How many of those will be around for a typical AFS client?  I have the vague
> feeling the slabs are overkill..

And then there's the people who said I shouldn't use kmalloc but should create
a slab instead...

> > +	if (pag>0) {
> > +		/* join existing PAG */
> > +		if (tsk->vfspag->pag &&
> > +		    tsk->vfspag->pag==pag)
> > +			return pag;
> 
> Please try to get your code in conformance with Documentation/CodingStyle.

You are suggesting what changes exactly?

David

Re: [PATCH] PAG support only

[Christoph Hellwig]

On Wed, May 14, 2003 at 09:17:43AM +0100, David Howells wrote:
> > - and even a wrong one..
> 
> I disagree, but no matter.

It's not a matter of your (or my) personal opinion of coding style.  Linux
is a big project and it's only maintainable if everyone sticks to a slightly
similar style.  


> > > +static kmem_cache_t *vfs_token_cache;
> > > +static kmem_cache_t *vfs_pag_cache;
> > 
> > How many of those will be around for a typical AFS client?  I have the vague
> > feeling the slabs are overkill..
> 
> And then there's the people who said I shouldn't use kmalloc but should create
> a slab instead...

That's why I ask how much it is used.  Could you answer the question maybe?

> 
> > > +	if (pag>0) {
> > > +		/* join existing PAG */
> > > +		if (tsk->vfspag->pag &&
> > > +		    tsk->vfspag->pag==pag)
> > > +			return pag;
> > 
> > Please try to get your code in conformance with Documentation/CodingStyle.
> 
> You are suggesting what changes exactly?

spaces between operators to start with.  It's not that difficult to read the
above document and other core kernel code, though.

Re: [PATCH] PAG support only

[Muli Ben-Yehuda]

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

On Tue, May 13, 2003 at 05:33:11PM +0100, David Howells wrote:
> 
> Hi Linus,
> 
> Due to the slight unpopularity of the AFS multiplexor, here's a patch with
> only the PAG support.
> 
> David

Hi David, 

Some quetions/comments about the code: 

> diff -uNr linux-2.5.69/include/linux/cred.h linux-2.5.69-cred/include/linux/cred.h
> --- linux-2.5.69/include/linux/cred.h	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.5.69-cred/include/linux/cred.h	2003-05-13 13:20:06.000000000 +0100
> @@ -0,0 +1,79 @@
> +#ifndef _LINUX_CRED_H
> +#define _LINUX_CRED_H
> +
> +#ifdef __KERNEL__
> +
> +#include <linux/param.h>
> +#include <linux/types.h>
> +#include <linux/list.h>
> +#include <linux/rbtree.h>
> +#include <asm/atomic.h>
> +
> +/*
> + * VFS session authentication token cache
> + *
> + * This is used to store the data required for extra levels of filesystem
> + * security (such as AFS/NFS kerberos keys, Samba workgroup/user/pass, or NTFS
> + * ACLs).
> + *
> + * VFS authentication tokens contain a single blob of data, consisting of three
> + * parts, all next to each other:
> + *   (1) An FS name
> + *   (2) A key
> + *   (3) An arbitrary chunk of data
> + *
> + * Token blobs must not be changed once passed to the core kernel for management

If you know the type of the data, why keep it all in one binary blob,
instead of a struct? cache effects? 

> diff -uNr linux-2.5.69/kernel/cred.c linux-2.5.69-cred/kernel/cred.c
> --- linux-2.5.69/kernel/cred.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.5.69-cred/kernel/cred.c	2003-05-13 13:21:06.000000000 +0100
> @@ -0,0 +1,367 @@
> +/* cred.c: authentication credentials 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/config.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +#include <linux/string.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>
> +
> +static kmem_cache_t *vfs_token_cache;
> +static kmem_cache_t *vfs_pag_cache;
> +
> +static struct rb_root	vfs_pag_tree;
> +static spinlock_t	vfs_pag_lock = SPIN_LOCK_UNLOCKED;
> +static pag_t		vfs_pag_next = 1;
> +
> +static void vfs_pag_init_once(void *_vfspag, kmem_cache_t *cachep, unsigned long flags)
> +{
> +	struct vfs_pag *vfspag = _vfspag;
> +
> +	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR) {
> +		memset(vfspag,0,sizeof(*vfspag));
> +		INIT_LIST_HEAD(&vfspag->tokens);
> +		rwlock_init(&vfspag->lock);
> +	}
> +}
> +
> +static void vfs_token_init_once(void *_vtoken, kmem_cache_t *cachep, unsigned long flags)
> +{
> +	struct vfs_token *vtoken = _vtoken;
> +
> +	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR) {
> +		memset(vtoken,0,sizeof(*vtoken));
> +		INIT_LIST_HEAD(&vtoken->link);
> +	}
> +}
> +
> +void __init credentials_init(void)
> +{
> +	vfs_pag_cache = kmem_cache_create("vfs_pag",
> +					      sizeof(struct vfs_pag),
> +					      0,
> +					      SLAB_HWCACHE_ALIGN,
> +					      vfs_pag_init_once, NULL);
> +	if (!vfs_pag_cache)
> +		panic("Cannot create vfs pag SLAB cache");
> +
> +	vfs_token_cache = kmem_cache_create("vfs_token",
> +					    sizeof(struct vfs_token),
> +					    0,
> +					    SLAB_HWCACHE_ALIGN,
> +					    vfs_token_init_once, NULL);
> +	if (!vfs_token_cache)
> +		panic("Cannot create vfs token SLAB cache");
> +}
> +
> +/*****************************************************************************/
> +/*
> + * join an existing PAG (+ve), run without PAG (0), or create and join new PAG (-1)
> + * - returns ID of PAG joined or 0 if now running without a PAG
> + */
> +int sys_setpag(pag_t pag)
> +{
> +	struct task_struct *tsk = current;
> +	struct vfs_pag *vfspag, *xvfspag;
> +	struct rb_node **p, *parent;
> +
> +	if (pag>0) {
> +		/* join existing PAG */
> +		if (tsk->vfspag->pag &&

Isn't the check here meant to be against tsk->vfspag? If not, there
should be. 

> +		    tsk->vfspag->pag==pag)
> +			return pag;
> +
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +
> +		spin_lock(&vfs_pag_lock);
> +
> +		parent = NULL;
> +		p = &vfs_pag_tree.rb_node;
> +
> +		while (*p) {
> +			parent = *p;
> +			vfspag = rb_entry(parent,struct vfs_pag,node);
> +
> +			if (pag < vfspag->pag)
> +				p = &(*p)->rb_left;
> +			else if (pag > vfspag->pag)
> +				p = &(*p)->rb_right;
> +			else
> +				goto pag_found;
> +		}
> +
> +		spin_unlock(&vfs_pag_lock);
> +		return -ENOENT;
> +
> +	pag_found:
> +		xvfspag = xchg(&current->vfspag,vfs_pag_get(vfspag));
> +		spin_unlock(&vfs_pag_lock);
> +
> +		if (xvfspag) vfs_pag_put(xvfspag);
> +		return pag;
> +	}
> +	else if (pag==0) {
> +		/* run without a PAG */
> +		xvfspag = xchg(&current->vfspag,NULL);
> +
> +		if (xvfspag) vfs_pag_put(xvfspag);
> +		return 0;
> +	}
> +	else if (pag!=-1) {
> +		     return -EINVAL;
> +	}
> +
> +	/* start new PAG */
> +	vfspag = kmem_cache_alloc(vfs_pag_cache,SLAB_KERNEL);
> +	if (!vfspag)
> +		return -ENOMEM;
> +
> +	atomic_set(&vfspag->usage,1);
> +
> +	/* PAG IDs must be +ve, >0 and unique */
> +	spin_lock(&vfs_pag_lock);
> +
> +	vfspag->pag = vfs_pag_next++;
> +	if (vfspag->pag<1)
> +		vfspag->pag = 1;

If vfs_pag_next wraps around, you will get two pag's with the value
'1'. 

> +
> +	parent = NULL;
> +	p = &vfs_pag_tree.rb_node;
> +
> +	while (*p) {
> +		parent = *p;
> +		xvfspag = rb_entry(parent,struct vfs_pag,node);
> +
> +		if (vfspag->pag < xvfspag->pag)
> +			p = &(*p)->rb_left;
> +		else if (vfspag->pag > xvfspag->pag)
> +			p = &(*p)->rb_right;
> +		else
> +			goto pag_exists;
> +	}
> +	goto insert_here;
> +
> +	/* we found a PAG of the same ID - walk the tree from that point
> +	 * looking for the next unused PAG */
> + pag_exists:
> +	for (;;) {
> +		vfspag->pag = vfs_pag_next++;
> +		if (vfspag->pag<1)
> +			vfspag->pag = 1;

Likewise. 

> +struct vfs_token *vfs_pag_find_token(const char *fsname,
> +				     unsigned short klen,
> +				     const void *key)
> +{
> +	struct vfs_token *vtoken;
> +	struct vfs_pag *vfspag = current->vfspag;
> +
> +	if (!vfspag) return NULL;
> +
> +	read_lock(&vfspag->lock);
> +
> +	list_for_each_entry(vtoken,&vfspag->tokens,link) {
> +		if (vtoken->d_off-vtoken->k_off == klen &&
> +		    strcmp(vtoken->blob,fsname)==0 &&
> +		    memcmp(vtoken->blob+vtoken->k_off,key,klen)==0)
> +			goto found;		    
> +	}
> +
> +	read_unlock(&vfspag->lock);
> +	return NULL;
> +
> + found:
> +	read_unlock(&vfspag->lock);
> +	return vtoken;
> +} /* end vfs_pag_find_token() */

Nothing in this patch appears to be using it. You aren't taking a
reference to the token here, what's protecting it from disappearing
after the return and before the caller gets a chance to do something
with it?

> +
> +EXPORT_SYMBOL(vfs_pag_find_token);
> +
> +/*****************************************************************************/
> +/*
> + * withdraw a token from a pag list
> + */
> +void vfs_pag_withdraw_token(struct vfs_token *vtoken)
> +{
> +	struct vfs_pag *vfspag = current->vfspag;
> +
> +	if (!vfspag)
> +		return;
> +
> +	write_lock(&vfspag->lock);
> +	list_del_init(&vtoken->link);
> +	write_unlock(&vfspag->lock);
> +
> +	vfs_token_put(vtoken);
> +
> +} /* end vfs_pag_withdraw_token() */
> +
> +EXPORT_SYMBOL(vfs_pag_withdraw_token);
> +
> +/*****************************************************************************/
> +/*
> + * withdraw all tokens for the named filesystem from the current PAG
> + */
> +void vfs_unpag(const char *fsname)
> +{
> +	struct list_head *_n, *_p;
> +	struct vfs_token *vtoken;
> +	struct vfs_pag *vfspag = current->vfspag;
> +
> +	if (!vfspag)
> +		return;
> +
> +	write_lock(&vfspag->lock);
> +
> +	list_for_each_safe(_p,_n,&vfspag->tokens) {
> +		vtoken = list_entry(_p,struct vfs_token,link);
> +
> +		if (strcmp(fsname,vtoken->blob)==0) {
> +			list_del_init(&vtoken->link);
> +			vfs_token_put(vtoken);
> +		}
> +	}
> +
> +	write_unlock(&vfspag->lock);
> +
> +} /* end vfs_unpag() */

-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] PAG support only

[David Howells]

Muli Ben-Yehuda wrote:

> > + * VFS session authentication token cache
> ...
>
> If you know the type of the data, why keep it all in one binary blob,
> instead of a struct? cache effects? 

No. We _don't_ know the type of the data. A filesystem entrusts us with a
token to keep in the PAG on its behalf. However, since this is meant to be a
generic mechanism, it's entirely dependent on the fs as to what's in the blob.

> > +		if (tsk->vfspag->pag &&
> 
> Isn't the check here meant to be against tsk->vfspag? If not, there
> should be. 

Good point. Fixed.

> If vfs_pag_next wraps around, you will get two pag's with the value
> '1'. 

Not so:

 +		else
 +			goto pag_exists;
 +	}
 +	goto insert_here;
 +
 +	/* we found a PAG of the same ID - walk the tree from that point
 +	 * looking for the next unused PAG */
 + pag_exists:

If we find the target PAG already present, we keep incrementing the counter
(making sure it doesn't go -ve or 0) and searching from the current node in
the tree until we find a gap in the currently instantiated PAG number space.

We can make use of the properties of rb trees to do this more efficiently than
just incrementing and repeating the search every time. rb_next() will return
the node with next highest instantiated PAG number (or NULL when it reaches
the end of the tree).

> Nothing in this patch appears to be using it. You aren't taking a
> reference to the token here, what's protecting it from disappearing
> after the return and before the caller gets a chance to do something
> with it?

Thanks. Fixed.

David

Re: [PATCH] PAG support only

[Muli Ben-Yehuda]

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

On Wed, May 14, 2003 at 10:54:36AM +0100, David Howells wrote:
> 
> Muli Ben-Yehuda wrote:
> 
> > > + * VFS session authentication token cache
> > ...
> >
> > If you know the type of the data, why keep it all in one binary blob,
> > instead of a struct? cache effects? 
> 
> No. We _don't_ know the type of the data. A filesystem entrusts us with a
> token to keep in the PAG on its behalf. However, since this is meant to be a
> generic mechanism, it's entirely dependent on the fs as to what's in
> the blob. 

But you do know the type of the data in the blob... it's char* fsname,
const void* key and const void* data, according to your code. 

You do

void* blob = kmalloc(fsname size + key size + data size)

memcpy(blob, fsname, fsname size)
memcpy(blob + fsname offset, key, key size)
mempcy(blob + fsname offset + key offset, data, data size)

I suggest

struct fsblob { 
       const char* fsname; 
       const void* key; 
       size_t keysize; 
       const void* data; 
       size_t datasize; 
}; 

struct fsblob b; 

b->fsname = kmalloc(fsname size)
memcpy(b->fsname, fsname, fsname size)

etc. 

Your method loses on additional complexity, and wins on
speed. However, unless it's really, really speed sensitive code, I
would go for KISS any day.

> > Nothing in this patch appears to be using it. You aren't taking a
> > reference to the token here, what's protecting it from disappearing
> > after the return and before the caller gets a chance to do something
> > with it?
> 
> Thanks. Fixed.

My pleasure. 
-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] PAG support only

[David Howells]

> > No. We _don't_ know the type of the data. A filesystem entrusts us with a
> > token to keep in the PAG on its behalf. However, since this is meant to be
> > a generic mechanism, it's entirely dependent on the fs as to what's in the
> > blob.
> 
> But you do know the type of the data in the blob... it's char* fsname,
> const void* key and const void* data, according to your code. 

We only partially know the data type (btw you should ignore the const's).

> struct fsblob { 
>        const char* fsname; 
>        const void* key; 
>        size_t keysize; 
>        const void* data; 
>        size_t datasize; 
> }; 

There shouldn't be any consts in there, or else the pointers have to be cast
prior to freeing.

So I'd have to do three kmalloc's instead of just the one (assuming struct
fsblob was folded back into struct vfs_token)...

> Your method loses on additional complexity, and wins on speed.

Yours may use more memory overhead, though that's not easy to judge. Also yours
is more complex in that there's more bits of memory to keep track of. OTOH,
your suggestion may permit sharing of fsname and maybe also key under some
circumstances.

> However, unless it's really, really speed sensitive code, I would go for
> KISS any day.

OTOH, resources are very precious, and the complexity is actually
straightforward and can be wrapped with macros or inline funcs.

David

Re: [PATCH] PAG support only

[David Howells]

> David, please stick with Linus-style.  That means commas
> after spaces and never, ever, ever:
>
> 	if (foo) bar

So you'd rather have:

	long sys_setpag(pag_t pag)
	{
		if (pag > 0)
			return vfs_join_pag(pag);
		else if (pag == 0)
			return vfs_leave_pag();
		else if (pag == -1)
			return vfs_new_pag();
		else
			return -EINVAL;
	}

Than:

	long sys_setpag(pag_t pag)
	{
		if (pag > 0)		return vfs_join_pag(pag);
		else if (pag == 0)	return vfs_leave_pag();
		else if (pag == -1)	return vfs_new_pag();
		else			return -EINVAL;
	}

When the former is _far_ less readable at a glance? And also consumes nearly
twice as many screen lines [see CodingStyle: "Thus, as the supply of new-lines
on your screen is not a renewable resource (think 25-line terminal screens
here), you have more empty lines to put comments on."].

> and syscalls should return long, not int.

Fair enough, but in arch/i386/kernel/process.c:

	asmlinkage int sys_fork(struct pt_regs regs)
	asmlinkage int sys_clone(struct pt_regs regs)
	asmlinkage int sys_vfork(struct pt_regs regs)
	asmlinkage int sys_execve(struct pt_regs regs)
	etc...

Should these be fixed too (the i386 arch is referred to quite a lot)?

David

Re: [PATCH] PAG support only

[Andrew Morton]

David Howells <dhowells@warthog.cambridge.redhat.com> wrote:
>
> 
> So you'd rather have:

Doesn't matter.

We want all the code looking the same, and that is more important than the
arguable benefit of the minor changes which you propose.

> 	long sys_setpag(pag_t pag)
> 	{
> 		if (pag > 0)		return vfs_join_pag(pag);
> 		else if (pag == 0)	return vfs_leave_pag();
> 		else if (pag == -1)	return vfs_new_pag();
> 		else			return -EINVAL;
> 	}

If someone else comes along and makes changes to this, you'll end up with a
mix of styles.

> > and syscalls should return long, not int.
> 
> Fair enough, but in arch/i386/kernel/process.c:
> 
> 	asmlinkage int sys_fork(struct pt_regs regs)
> 	asmlinkage int sys_clone(struct pt_regs regs)
> 	asmlinkage int sys_vfork(struct pt_regs regs)
> 	asmlinkage int sys_execve(struct pt_regs regs)
> 	etc...
> 
> Should these be fixed too (the i386 arch is referred to quite a lot)?

I don't know really.  David M-T said "syscalls should return long".  I'm
not sure why - perhaps it's only those which need 32-bit wrappers (guess).

Re: [PATCH] PAG support only

[Miles Bader]

David Howells <dhowells@warthog.cambridge.redhat.com> writes:
> So you'd rather have:
> 
> 		if (pag > 0)
> 			return vfs_join_pag(pag);
> 		else if (pag == 0)
> 			return vfs_leave_pag();
> 		else if (pag == -1)
> 			return vfs_new_pag();
> 		else
> 			return -EINVAL;
>
> Than:
>
> 		if (pag > 0)		return vfs_join_pag(pag);
> 		else if (pag == 0)	return vfs_leave_pag();
> 		else if (pag == -1)	return vfs_new_pag();
> 		else			return -EINVAL;
>
> When the former is _far_ less readable at a glance?

Keep in mind that `readable' also means that someone used to the
kernel's coding style, but not to your particular portion, should
naturally be able to follow the code with as little trouble as possible.
While I agree that your second example is more clear once one sees
what's going on (and it's certainly prettier in some sense), the `shape'
is unusual, so it takes some adaptation on the part of the reader to
make this jump, and that's something to be avoided if possible.

My personal feeling is that (1) there are exceptional cases where the
rules should be broken, and (2) but not that many.

These exceptional cases usually stick out like a sore thumb, and if
there's any doubt, it's probably not one.

-Miles
-- 
We live, as we dream -- alone....

Re: [PATCH] PAG support only

[Chuck Ebbert]

David Howells wrote:

> Fair enough, but in arch/i386/kernel/process.c:
>
>       asmlinkage int sys_fork(struct pt_regs regs)
>       asmlinkage int sys_clone(struct pt_regs regs)
>       asmlinkage int sys_vfork(struct pt_regs regs)
>       asmlinkage int sys_execve(struct pt_regs regs)
>       etc...
>
> Should these be fixed too (the i386 arch is referred to quite a lot)?


$ grep 'asmlinkage int sys_' arch/i386/kernel/*.c | wc -l
     19


  And what is this all about?

$ grep -A 2 'unused)' arch/i386/kernel/*.c | grep -B 2 regs
arch/i386/kernel/ioport.c:asmlinkage int sys_iopl(unsigned long unused)
arch/i386/kernel/ioport.c-{
arch/i386/kernel/ioport.c-      volatile struct pt_regs * regs = (struct pt_regs *) &unused;
--
arch/i386/kernel/signal.c:asmlinkage int sys_sigreturn(unsigned long __unused)
arch/i386/kernel/signal.c-{
arch/i386/kernel/signal.c-      struct pt_regs *regs = (struct pt_regs *) &__unused;
--
arch/i386/kernel/signal.c:asmlinkage int sys_rt_sigreturn(unsigned long __unused)
arch/i386/kernel/signal.c-{
arch/i386/kernel/signal.c-      struct pt_regs *regs = (struct pt_regs *) &__unused;

Re: [PATCH] PAG support only

[William Lee Irwin III]

On Wed, May 14, 2003 at 12:04:38PM -0400, Chuck Ebbert wrote:
>   And what is this all about?

Crusty, ancient code.


-- wli