[PATCH 1/2] kernfs: update sysfs_init_inode_attrs()

[PATCH 1/2] kernfs: update sysfs_init_inode_attrs()

[Tejun Heo]

sysfs_init_inode_attrs() is a bit clumsy to use requiring the caller
to check whether @sd->s_iattr is already set or not.  Rename it to
sysfs_inode_attrs(), update it to check whether @sd->s_iattr is
already initialized before trying to initialize it and return
@sd->s_iattr.  This simplifies the callers.

While at it,

* Rename struct sysfs_inode_attrs pointer variables to "attrs".  As
  kernfs no longer deals with "struct attribute", this isn't confusing
  and makes it easier to distinguish from struct iattr pointers.

* A new field will be added to sysfs_inode_attrs.  Reindent in
  preparation.

This patch doesn't introduce any behavior changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/inode.c           | 60 ++++++++++++++++++++-------------------------
 fs/kernfs/kernfs-internal.h |  6 ++---
 2 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index b4cae6f..a1f8382 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -43,15 +43,17 @@ void __init sysfs_inode_init(void)
 		panic("failed to init sysfs_backing_dev_info");
 }
 
-static struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
+static struct sysfs_inode_attrs *sysfs_inode_attrs(struct sysfs_dirent *sd)
 {
-	struct sysfs_inode_attrs *attrs;
 	struct iattr *iattrs;
 
-	attrs = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
-	if (!attrs)
+	if (sd->s_iattr)
+		return sd->s_iattr;
+
+	sd->s_iattr = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
+	if (!sd->s_iattr)
 		return NULL;
-	iattrs = &attrs->ia_iattr;
+	iattrs = &sd->s_iattr->ia_iattr;
 
 	/* assign default attributes */
 	iattrs->ia_mode = sd->s_mode;
@@ -59,26 +61,20 @@ static struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
 	iattrs->ia_gid = GLOBAL_ROOT_GID;
 	iattrs->ia_atime = iattrs->ia_mtime = iattrs->ia_ctime = CURRENT_TIME;
 
-	return attrs;
+	return sd->s_iattr;
 }
 
 static int __kernfs_setattr(struct sysfs_dirent *sd, const struct iattr *iattr)
 {
-	struct sysfs_inode_attrs *sd_attrs;
+	struct sysfs_inode_attrs *attrs;
 	struct iattr *iattrs;
 	unsigned int ia_valid = iattr->ia_valid;
 
-	sd_attrs = sd->s_iattr;
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
 
-	if (!sd_attrs) {
-		/* setting attributes for the first time, allocate now */
-		sd_attrs = sysfs_init_inode_attrs(sd);
-		if (!sd_attrs)
-			return -ENOMEM;
-		sd->s_iattr = sd_attrs;
-	}
-	/* attributes were changed at least once in past */
-	iattrs = &sd_attrs->ia_iattr;
+	iattrs = &attrs->ia_iattr;
 
 	if (ia_valid & ATTR_UID)
 		iattrs->ia_uid = iattr->ia_uid;
@@ -143,22 +139,19 @@ out:
 static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata,
 			       u32 *secdata_len)
 {
-	struct sysfs_inode_attrs *iattrs;
+	struct sysfs_inode_attrs *attrs;
 	void *old_secdata;
 	size_t old_secdata_len;
 
-	if (!sd->s_iattr) {
-		sd->s_iattr = sysfs_init_inode_attrs(sd);
-		if (!sd->s_iattr)
-			return -ENOMEM;
-	}
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
 
-	iattrs = sd->s_iattr;
-	old_secdata = iattrs->ia_secdata;
-	old_secdata_len = iattrs->ia_secdata_len;
+	old_secdata = attrs->ia_secdata;
+	old_secdata_len = attrs->ia_secdata_len;
 
-	iattrs->ia_secdata = *secdata;
-	iattrs->ia_secdata_len = *secdata_len;
+	attrs->ia_secdata = *secdata;
+	attrs->ia_secdata_len = *secdata_len;
 
 	*secdata = old_secdata;
 	*secdata_len = old_secdata_len;
@@ -216,17 +209,16 @@ static inline void set_inode_attr(struct inode *inode, struct iattr *iattr)
 
 static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode)
 {
-	struct sysfs_inode_attrs *iattrs = sd->s_iattr;
+	struct sysfs_inode_attrs *attrs = sd->s_iattr;
 
 	inode->i_mode = sd->s_mode;
-	if (iattrs) {
+	if (attrs) {
 		/* sysfs_dirent has non-default attributes
 		 * get them from persistent copy in sysfs_dirent
 		 */
-		set_inode_attr(inode, &iattrs->ia_iattr);
-		security_inode_notifysecctx(inode,
-					    iattrs->ia_secdata,
-					    iattrs->ia_secdata_len);
+		set_inode_attr(inode, &attrs->ia_iattr);
+		security_inode_notifysecctx(inode, attrs->ia_secdata,
+					    attrs->ia_secdata_len);
 	}
 
 	if (sysfs_type(sd) == SYSFS_DIR)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index a8ec6ed..ff08b45 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -18,9 +18,9 @@
 #include <linux/kernfs.h>
 
 struct sysfs_inode_attrs {
-	struct iattr	ia_iattr;
-	void		*ia_secdata;
-	u32		ia_secdata_len;
+	struct iattr		ia_iattr;
+	void			*ia_secdata;
+	u32			ia_secdata_len;
 };
 
 #define SD_DEACTIVATED_BIAS		INT_MIN
-- 
1.8.3.1

[PATCHSET REPOST driver-core-next] kernfs: implement trusted.* xattr support

[Tejun Heo]

Hello,

This is repost of [L], rebased on top of the refreshed patches.

kernfs inherited "security.*" xattr support from sysfs but for it to
be useable for cgroupfs, it needs to support "trusted.*" too.  This
patchset adds "trusted.*" xattr support using simple_xattr*().

Note that the existing "security.*" support is gimped in that it
doesn't include remove/get/list.  This patchset implements all
operations for "trusted.*" but doesn't change "security.*" support.

David P. Quigley, can you please comment on this?  It doesn't seem
like adding it would be too difficult.  Why don't we have it yet?  Are
they unnecessary?

This patchset contains the following two patches.

 0001-kernfs-update-sysfs_init_inode_attrs.patch
 0002-kernfs-implement-trusted.-xattr-support.patch

0001 preps and 0002 implements.

This patchset is on top of

  v3.13-rc1 6ce4eac1f600 ("Linux 3.13-rc1")
+ [1] sysfs: handle duplicate removal attempts in sysfs_remove_group()
+ [2] sysfs: use a separate locking class for open files depending on mmap
+ [3] sysfs: separate out kernfs, take #3

and available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-kernfs-xattr

diffstat follows.  Thanks.

 fs/kernfs/dir.c             |   12 +++-
 fs/kernfs/inode.c           |  123 ++++++++++++++++++++++++++++----------------
 fs/kernfs/kernfs-internal.h |   13 +++-
 fs/kernfs/symlink.c         |    3 +
 4 files changed, 103 insertions(+), 48 deletions(-)

--
tejun

[L] https://lkml.kernel.org/g/1383617322-4386-1-git-send-email-tj@kernel.org
[1] https://lkml.kernel.org/g/20131123183508.GA28162@mtj.dyndns.org
[2] https://lkml.kernel.org/g/20131123183540.GB28162@mtj.dyndns.org
[3] https://lkml.kernel.org/g/1385245346-856-1-git-send-email-tj@kernel.org

[PATCH 1/2] kernfs: update sysfs_init_inode_attrs()

[Tejun Heo]

sysfs_init_inode_attrs() is a bit clumsy to use requiring the caller
to check whether @sd->s_iattr is already set or not.  Rename it to
sysfs_inode_attrs(), update it to check whether @sd->s_iattr is
already initialized before trying to initialize it and return
@sd->s_iattr.  This simplifies the callers.

While at it,

* Rename struct sysfs_inode_attrs pointer variables to "attrs".  As
  kernfs no longer deals with "struct attribute", this isn't confusing
  and makes it easier to distinguish from struct iattr pointers.

* A new field will be added to sysfs_inode_attrs.  Reindent in
  preparation.

This patch doesn't introduce any behavior changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/inode.c           | 60 ++++++++++++++++++++-------------------------
 fs/kernfs/kernfs-internal.h |  6 ++---
 2 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index b4cae6f..a1f8382 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -43,15 +43,17 @@ void __init sysfs_inode_init(void)
 		panic("failed to init sysfs_backing_dev_info");
 }
 
-static struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
+static struct sysfs_inode_attrs *sysfs_inode_attrs(struct sysfs_dirent *sd)
 {
-	struct sysfs_inode_attrs *attrs;
 	struct iattr *iattrs;
 
-	attrs = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
-	if (!attrs)
+	if (sd->s_iattr)
+		return sd->s_iattr;
+
+	sd->s_iattr = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
+	if (!sd->s_iattr)
 		return NULL;
-	iattrs = &attrs->ia_iattr;
+	iattrs = &sd->s_iattr->ia_iattr;
 
 	/* assign default attributes */
 	iattrs->ia_mode = sd->s_mode;
@@ -59,26 +61,20 @@ static struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
 	iattrs->ia_gid = GLOBAL_ROOT_GID;
 	iattrs->ia_atime = iattrs->ia_mtime = iattrs->ia_ctime = CURRENT_TIME;
 
-	return attrs;
+	return sd->s_iattr;
 }
 
 static int __kernfs_setattr(struct sysfs_dirent *sd, const struct iattr *iattr)
 {
-	struct sysfs_inode_attrs *sd_attrs;
+	struct sysfs_inode_attrs *attrs;
 	struct iattr *iattrs;
 	unsigned int ia_valid = iattr->ia_valid;
 
-	sd_attrs = sd->s_iattr;
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
 
-	if (!sd_attrs) {
-		/* setting attributes for the first time, allocate now */
-		sd_attrs = sysfs_init_inode_attrs(sd);
-		if (!sd_attrs)
-			return -ENOMEM;
-		sd->s_iattr = sd_attrs;
-	}
-	/* attributes were changed at least once in past */
-	iattrs = &sd_attrs->ia_iattr;
+	iattrs = &attrs->ia_iattr;
 
 	if (ia_valid & ATTR_UID)
 		iattrs->ia_uid = iattr->ia_uid;
@@ -143,22 +139,19 @@ out:
 static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata,
 			       u32 *secdata_len)
 {
-	struct sysfs_inode_attrs *iattrs;
+	struct sysfs_inode_attrs *attrs;
 	void *old_secdata;
 	size_t old_secdata_len;
 
-	if (!sd->s_iattr) {
-		sd->s_iattr = sysfs_init_inode_attrs(sd);
-		if (!sd->s_iattr)
-			return -ENOMEM;
-	}
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
 
-	iattrs = sd->s_iattr;
-	old_secdata = iattrs->ia_secdata;
-	old_secdata_len = iattrs->ia_secdata_len;
+	old_secdata = attrs->ia_secdata;
+	old_secdata_len = attrs->ia_secdata_len;
 
-	iattrs->ia_secdata = *secdata;
-	iattrs->ia_secdata_len = *secdata_len;
+	attrs->ia_secdata = *secdata;
+	attrs->ia_secdata_len = *secdata_len;
 
 	*secdata = old_secdata;
 	*secdata_len = old_secdata_len;
@@ -216,17 +209,16 @@ static inline void set_inode_attr(struct inode *inode, struct iattr *iattr)
 
 static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode)
 {
-	struct sysfs_inode_attrs *iattrs = sd->s_iattr;
+	struct sysfs_inode_attrs *attrs = sd->s_iattr;
 
 	inode->i_mode = sd->s_mode;
-	if (iattrs) {
+	if (attrs) {
 		/* sysfs_dirent has non-default attributes
 		 * get them from persistent copy in sysfs_dirent
 		 */
-		set_inode_attr(inode, &iattrs->ia_iattr);
-		security_inode_notifysecctx(inode,
-					    iattrs->ia_secdata,
-					    iattrs->ia_secdata_len);
+		set_inode_attr(inode, &attrs->ia_iattr);
+		security_inode_notifysecctx(inode, attrs->ia_secdata,
+					    attrs->ia_secdata_len);
 	}
 
 	if (sysfs_type(sd) == SYSFS_DIR)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index d1ff591..f25b354 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -18,9 +18,9 @@
 #include <linux/kernfs.h>
 
 struct sysfs_inode_attrs {
-	struct iattr	ia_iattr;
-	void		*ia_secdata;
-	u32		ia_secdata_len;
+	struct iattr		ia_iattr;
+	void			*ia_secdata;
+	u32			ia_secdata_len;
 };
 
 #define SD_DEACTIVATED_BIAS		INT_MIN
-- 
1.8.4.2

[PATCH 2/2] kernfs: implement "trusted.*" xattr support

[Tejun Heo]

kernfs inherited "security.*" xattr support from sysfs.  This patch
extends xattr support to "trusted.*" using simple_xattr_*().  As
trusted xattrs are restricted to CAP_SYS_ADMIN, simple_xattr_*() which
uses kernel memory for storage shouldn't be problematic.

Note that the existing "security.*" support doesn't implement
get/remove/list and the this patch only implements those ops for
"trusted.*".  We probably want to extend those ops to include support
for "security.*".

This patch will allow using kernfs from cgroup which requires
"trusted.*" xattr support.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/kernfs/dir.c             | 12 ++++++---
 fs/kernfs/inode.c           | 63 +++++++++++++++++++++++++++++++++++++++------
 fs/kernfs/kernfs-internal.h |  7 +++++
 fs/kernfs/symlink.c         |  3 +++
 4 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index f51e062..a441e3b 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -243,9 +243,12 @@ void kernfs_put(struct sysfs_dirent *sd)
 		kernfs_put(sd->s_symlink.target_sd);
 	if (sysfs_type(sd) & SYSFS_COPY_NAME)
 		kfree(sd->s_name);
-	if (sd->s_iattr && sd->s_iattr->ia_secdata)
-		security_release_secctx(sd->s_iattr->ia_secdata,
-					sd->s_iattr->ia_secdata_len);
+	if (sd->s_iattr) {
+		if (sd->s_iattr->ia_secdata)
+			security_release_secctx(sd->s_iattr->ia_secdata,
+						sd->s_iattr->ia_secdata_len);
+		simple_xattrs_free(&sd->s_iattr->xattrs);
+	}
 	kfree(sd->s_iattr);
 	ida_simple_remove(&root->ino_ida, sd->s_ino);
 	kmem_cache_free(sysfs_dir_cachep, sd);
@@ -718,6 +721,9 @@ const struct inode_operations sysfs_dir_inode_operations = {
 	.setattr	= sysfs_setattr,
 	.getattr	= sysfs_getattr,
 	.setxattr	= sysfs_setxattr,
+	.removexattr	= sysfs_removexattr,
+	.getxattr	= sysfs_getxattr,
+	.listxattr	= sysfs_listxattr,
 };
 
 static struct sysfs_dirent *sysfs_leftmost_descendant(struct sysfs_dirent *pos)
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index a1f8382..18ad431 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -35,6 +35,9 @@ static const struct inode_operations sysfs_inode_operations = {
 	.setattr	= sysfs_setattr,
 	.getattr	= sysfs_getattr,
 	.setxattr	= sysfs_setxattr,
+	.removexattr	= sysfs_removexattr,
+	.getxattr	= sysfs_getxattr,
+	.listxattr	= sysfs_listxattr,
 };
 
 void __init sysfs_inode_init(void)
@@ -61,6 +64,8 @@ static struct sysfs_inode_attrs *sysfs_inode_attrs(struct sysfs_dirent *sd)
 	iattrs->ia_gid = GLOBAL_ROOT_GID;
 	iattrs->ia_atime = iattrs->ia_mtime = iattrs->ia_ctime = CURRENT_TIME;
 
+	simple_xattrs_init(&sd->s_iattr->xattrs);
+
 	return sd->s_iattr;
 }
 
@@ -162,23 +167,25 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		size_t size, int flags)
 {
 	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct sysfs_inode_attrs *attrs;
 	void *secdata;
 	int error;
 	u32 secdata_len = 0;
 
-	if (!sd)
-		return -EINVAL;
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
 
 	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
 		error = security_inode_setsecurity(dentry->d_inode, suffix,
 						value, size, flags);
 		if (error)
-			goto out;
+			return error;
 		error = security_inode_getsecctx(dentry->d_inode,
 						&secdata, &secdata_len);
 		if (error)
-			goto out;
+			return error;
 
 		mutex_lock(&sysfs_mutex);
 		error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len);
@@ -186,10 +193,50 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 
 		if (secdata)
 			security_release_secctx(secdata, secdata_len);
-	} else
-		return -EINVAL;
-out:
-	return error;
+		return error;
+	} else if (!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN)) {
+		return simple_xattr_set(&attrs->xattrs, name, value, size,
+					flags);
+	}
+
+	return -EINVAL;
+}
+
+int sysfs_removexattr(struct dentry *dentry, const char *name)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct sysfs_inode_attrs *attrs;
+
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
+
+	return simple_xattr_remove(&attrs->xattrs, name);
+}
+
+ssize_t sysfs_getxattr(struct dentry *dentry, const char *name, void *buf,
+		       size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct sysfs_inode_attrs *attrs;
+
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
+
+	return simple_xattr_get(&attrs->xattrs, name, buf, size);
+}
+
+ssize_t sysfs_listxattr(struct dentry *dentry, char *buf, size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct sysfs_inode_attrs *attrs;
+
+	attrs = sysfs_inode_attrs(sd);
+	if (!attrs)
+		return -ENOMEM;
+
+	return simple_xattr_list(&attrs->xattrs, buf, size);
 }
 
 static inline void set_default_inode_attr(struct inode *inode, umode_t mode)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index f25b354..910e485 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -14,6 +14,7 @@
 #include <linux/lockdep.h>
 #include <linux/fs.h>
 #include <linux/mutex.h>
+#include <linux/xattr.h>
 
 #include <linux/kernfs.h>
 
@@ -21,6 +22,8 @@ struct sysfs_inode_attrs {
 	struct iattr		ia_iattr;
 	void			*ia_secdata;
 	u32			ia_secdata_len;
+
+	struct simple_xattrs	xattrs;
 };
 
 #define SD_DEACTIVATED_BIAS		INT_MIN
@@ -81,6 +84,10 @@ int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry,
 		  struct kstat *stat);
 int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		   size_t size, int flags);
+int sysfs_removexattr(struct dentry *dentry, const char *name);
+ssize_t sysfs_getxattr(struct dentry *dentry, const char *name, void *buf,
+		       size_t size);
+ssize_t sysfs_listxattr(struct dentry *dentry, char *buf, size_t size);
 void sysfs_inode_init(void);
 
 /*
diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
index 12569a7..adf2875 100644
--- a/fs/kernfs/symlink.c
+++ b/fs/kernfs/symlink.c
@@ -140,6 +140,9 @@ static void sysfs_put_link(struct dentry *dentry, struct nameidata *nd,
 
 const struct inode_operations sysfs_symlink_inode_operations = {
 	.setxattr	= sysfs_setxattr,
+	.removexattr	= sysfs_removexattr,
+	.getxattr	= sysfs_getxattr,
+	.listxattr	= sysfs_listxattr,
 	.readlink	= generic_readlink,
 	.follow_link	= sysfs_follow_link,
 	.put_link	= sysfs_put_link,
-- 
1.8.4.2

Re: [PATCHSET REPOST driver-core-next] kernfs: implement trusted.* xattr support

[Tejun Heo]

FYI, these two patches still apply as-is on top of the current
driver-core-next e756bc5670d0 ("kobject: fix kset sample error path").

Thanks.

-- 
tejun

Re: [PATCHSET REPOST driver-core-next] kernfs: implement trusted.* xattr support

[Greg KH]

On Wed, Dec 04, 2013 at 10:24:55AM -0500, Tejun Heo wrote:
> FYI, these two patches still apply as-is on top of the current
> driver-core-next e756bc5670d0 ("kobject: fix kset sample error path").

Ick, sorry about that, thanks for the poke, I forgot about them (they
are burried in my "todo" mbox.  They aren't lost, I'll get to them
soon...

thanks,

greg k-h