[PATCH 1/6] xattr consolidation v2 - generic xattr API

[PATCH 1/6] xattr consolidation v2 - generic xattr API

[James Morris]

This patch consolidates common xattr handling logic into the core fs code,
with modifications suggested by Christoph Hellwig (hang off superblock,
remove locking, use generic code as methods), for use by ext2, ext3 and
devpts, as well as upcoming tmpfs xattr code.


 fs/xattr.c            |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h    |    1 
 include/linux/xattr.h |   16 ++++++++
 3 files changed, 115 insertions(+)


Signed-off-by: James Morris <jmorris@redhat.com>
Signed-off-by: Stephen Smalley <sds@epoch.ncsc.mil>


diff -purN -X dontdiff linux-2.6.9-rc2.p/fs/xattr.c linux-2.6.9-rc2.w/fs/xattr.c
--- linux-2.6.9-rc2.p/fs/xattr.c	2004-09-13 01:32:26.000000000 -0400
+++ linux-2.6.9-rc2.w/fs/xattr.c	2004-09-18 01:26:57.271414448 -0400
@@ -5,6 +5,7 @@
 
   Copyright (C) 2001 by Andreas Gruenbacher <a.gruenbacher@computer.org>
   Copyright (C) 2001 SGI - Silicon Graphics, Inc <linux-xfs@oss.sgi.com>
+  Copyright (c) 2004 Red Hat, Inc., James Morris <jmorris@redhat.com>
  */
 #include <linux/fs.h>
 #include <linux/slab.h>
@@ -13,6 +14,7 @@
 #include <linux/xattr.h>
 #include <linux/namei.h>
 #include <linux/security.h>
+#include <linux/module.h>
 #include <asm/uaccess.h>
 
 /*
@@ -347,3 +349,99 @@ sys_fremovexattr(int fd, char __user *na
 	fput(f);
 	return error;
 }
+
+static const char *strcmp_prefix(const char *a, const char *a_prefix)
+{
+	while (*a_prefix && *a == *a_prefix) {
+		a++;
+		a_prefix++;
+	}
+	return *a_prefix ? NULL : a;
+}
+
+#define for_each_xattr_handler(handlers, handler)		\
+		for ((handler) = *(handlers)++;			\
+			(handler) != NULL;			\
+			(handler) = *(handlers)++)	
+			
+static struct xattr_handler *xattr_resolve_name(struct xattr_handler **handlers, const char **name)
+{
+	struct xattr_handler *handler;
+
+	if (!*name)
+		return NULL;
+
+	for_each_xattr_handler(handlers, handler) {
+		const char *n = strcmp_prefix(*name, handler->prefix);
+		if (n) {
+			*name = n;
+			break;
+		}
+	}
+	return handler;
+}
+
+ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size)
+{
+	struct xattr_handler *handler;
+	struct inode *inode = dentry->d_inode;
+
+	handler = xattr_resolve_name(inode->i_sb->s_xattr, &name);
+	if (!handler)
+		return -EOPNOTSUPP;
+	return handler->get(inode, name, buffer, size);
+}
+
+ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
+{
+	struct inode *inode = dentry->d_inode;
+	struct xattr_handler *handler, **handlers = inode->i_sb->s_xattr;
+	unsigned int size = 0;
+	char *buf;
+
+	for_each_xattr_handler(handlers, handler)
+		size += handler->list(inode, NULL, NULL, 0);
+
+	if (!buffer)
+		return size;
+		
+	if (size > buffer_size)
+		return -ERANGE;
+
+	buf = buffer;
+	handlers = inode->i_sb->s_xattr;
+	
+	for_each_xattr_handler(handlers, handler)
+		buf += handler->list(inode, buf, NULL, 0);
+
+	return size;
+}
+
+int generic_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags)
+{
+	struct xattr_handler *handler;
+	struct inode *inode = dentry->d_inode;
+	
+	if (size == 0)
+		value = "";  /* empty EA, do not remove */
+	handler = xattr_resolve_name(inode->i_sb->s_xattr, &name);
+	if (!handler)
+		return -EOPNOTSUPP;
+	return handler->set(inode, name, value, size, flags);
+}
+
+int generic_removexattr(struct dentry *dentry, const char *name)
+{
+	struct xattr_handler *handler;
+	struct inode *inode = dentry->d_inode;
+
+	handler = xattr_resolve_name(inode->i_sb->s_xattr, &name);
+	if (!handler)
+		return -EOPNOTSUPP;
+	return handler->set(inode, name, NULL, 0, XATTR_REPLACE);
+}
+
+EXPORT_SYMBOL(generic_getxattr);
+EXPORT_SYMBOL(generic_listxattr);
+EXPORT_SYMBOL(generic_setxattr);
+EXPORT_SYMBOL(generic_removexattr);
diff -purN -X dontdiff linux-2.6.9-rc2.p/include/linux/fs.h linux-2.6.9-rc2.w/include/linux/fs.h
--- linux-2.6.9-rc2.p/include/linux/fs.h	2004-09-13 01:31:58.000000000 -0400
+++ linux-2.6.9-rc2.w/include/linux/fs.h	2004-09-18 01:26:57.273414144 -0400
@@ -758,6 +758,7 @@ struct super_block {
 	int			s_need_sync_fs;
 	atomic_t		s_active;
 	void                    *s_security;
+	struct xattr_handler	**s_xattr;
 
 	struct list_head	s_dirty;	/* dirty inodes */
 	struct list_head	s_io;		/* parked for writeback */
diff -purN -X dontdiff linux-2.6.9-rc2.p/include/linux/xattr.h linux-2.6.9-rc2.w/include/linux/xattr.h
--- linux-2.6.9-rc2.p/include/linux/xattr.h	2004-09-13 01:32:55.000000000 -0400
+++ linux-2.6.9-rc2.w/include/linux/xattr.h	2004-09-18 01:26:57.274413992 -0400
@@ -5,6 +5,7 @@
 
   Copyright (C) 2001 by Andreas Gruenbacher <a.gruenbacher@computer.org>
   Copyright (c) 2001-2002 Silicon Graphics, Inc.  All Rights Reserved.
+  Copyright (c) 2004 Red Hat, Inc., James Morris <jmorris@redhat.com>
 */
 #ifndef _LINUX_XATTR_H
 #define _LINUX_XATTR_H
@@ -14,4 +15,19 @@
 
 #define XATTR_SECURITY_PREFIX	"security."
 
+struct xattr_handler {
+	char *prefix;
+	size_t (*list)(struct inode *inode, char *list, const char *name,
+		       int name_len);
+	int (*get)(struct inode *inode, const char *name, void *buffer,
+		   size_t size);
+	int (*set)(struct inode *inode, const char *name, const void *buffer,
+		   size_t size, int flags);
+};
+
+ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
+ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size);
+int generic_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags);
+int generic_removexattr(struct dentry *dentry, const char *name);
+
 #endif	/* _LINUX_XATTR_H */

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[Andreas Gruenbacher]

Hello,

the patches look good except for a theoretical race in generic_listxattr:

> +ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t
> buffer_size) +{
> +	struct inode *inode = dentry->d_inode;
> +	struct xattr_handler *handler, **handlers = inode->i_sb->s_xattr;
> +	unsigned int size = 0;
> +	char *buf;
> +
> +	for_each_xattr_handler(handlers, handler)
> +		size += handler->list(inode, NULL, NULL, 0);

We might setxattr here and the name list might change between the two passes.

> [...]
> +	for_each_xattr_handler(handlers, handler)
> +		buf += handler->list(inode, buf, NULL, 0);
> +
> +	return size;
> +}

This currently is only relevant for the security attribute. Selinux always 
returns the same attribute name so it can't trigger this problem, but other 
LSMs might do something different.

We can add a list_size parameter to xattr_handler->list to get this fixed. We 
should change the name_len parameter of xattr_handler->list from int to 
size_t:

Index: linux-2.6.9-rc1/include/linux/xattr.h
===================================================================
--- linux-2.6.9-rc1.orig/include/linux/xattr.h
+++ linux-2.6.9-rc1/include/linux/xattr.h
@@ -17,8 +17,8 @@
 
 struct xattr_handler {
 	char *prefix;
-	size_t (*list)(struct inode *inode, char *list, const char *name,
-		       int name_len);
+	size_t (*list)(struct inode *inode, char *list, size_t list_size,
+		       const char *name, size_t name_len);
 	int (*get)(struct inode *inode, const char *name, void *buffer,
 		   size_t size);
 	int (*set)(struct inode *inode, const char *name, const void *buffer,
Index: linux-2.6.9-rc1/fs/xattr.c
===================================================================
--- linux-2.6.9-rc1.orig/fs/xattr.c
+++ linux-2.6.9-rc1/fs/xattr.c
@@ -397,23 +397,22 @@ ssize_t generic_listxattr(struct dentry 
 	struct inode *inode = dentry->d_inode;
 	struct xattr_handler *handler, **handlers = inode->i_sb->s_xattr;
 	unsigned int size = 0;
-	char *buf;
-
-	for_each_xattr_handler(handlers, handler)
-		size += handler->list(inode, NULL, NULL, 0);
-
-	if (!buffer)
-		return size;
-		
-	if (size > buffer_size)
-		return -ERANGE;
-
-	buf = buffer;
-	handlers = inode->i_sb->s_xattr;
-	
-	for_each_xattr_handler(handlers, handler)
-		buf += handler->list(inode, buf, NULL, 0);
 
+	if (!buffer) {
+		for_each_xattr_handler(handlers, handler)
+			size += handler->list(inode, NULL, 0, NULL, 0);
+	} else {
+		char *buf = buffer;
+
+		for_each_xattr_handler(handlers, handler) {
+			size = handler->list(inode, buf, buffer_size, NULL, 0);
+			if (size > buffer_size)
+				return -ERANGE;
+			buf += size;
+			buffer_size -= size;
+		}
+		size = buf - buffer;
+	}
 	return size;
 }

The current users can easily be converted. As a side effect, the 
ext2_xattr_list() and ext3_xattr_list() functions could also be changed to do 
a single iteration only.

I also noticed that your additions to fs/xattr.c use a slightly different 
coding style than the rest of the file. You might want to change that as 
well.

Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[James Morris]

On Sun, 19 Sep 2004, Andreas Gruenbacher wrote:

> This currently is only relevant for the security attribute. Selinux always 
> returns the same attribute name so it can't trigger this problem, but other 
> LSMs might do something different.
> 
> We can add a list_size parameter to xattr_handler->list to get this fixed. We 
> should change the name_len parameter of xattr_handler->list from int to 
> size_t:

Ahh, I thought we had the inode semaphore (never trust documentation).  
Why don't we take that instead in listxattr() ?  The name_len thing seems 
kludgy.

> I also noticed that your additions to fs/xattr.c use a slightly different 
> coding style than the rest of the file. You might want to change that as 
> well.

I was using Linus-recommended coding style, but it can be changed I guess.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[Andreas Gruenbacher]

On Sunday 19 September 2004 01:47, James Morris wrote:
> On Sun, 19 Sep 2004, Andreas Gruenbacher wrote:
> > This currently is only relevant for the security attribute. Selinux
> > always returns the same attribute name so it can't trigger this problem,
> > but other LSMs might do something different.
> >
> > We can add a list_size parameter to xattr_handler->list to get this
> > fixed. We should change the name_len parameter of xattr_handler->list
> > from int to size_t:
>
> Ahh, I thought we had the inode semaphore (never trust documentation).
> Why don't we take that instead in listxattr() ?

Documentation/filesystems/Locking seems to be accurate. Originally we were 
taking inode->i_sem for all four xattr operations. It turned out that this 
caused lock contention for acls. Selinux increases the frequency of xattr 
operations, so always taking i_sem would be even worse now.

> The name_len thing seems kludgy.

The old handler API was fine at the FS level where locking was guaranteed 
anyways. At the VFS level we should do better. Passing in the buffer and the 
buffer size at the same time gets us rid of the problem without requiring any 
locking.

> > I also noticed that your additions to fs/xattr.c use a slightly different
> > coding style than the rest of the file. You might want to change that as
> > well.
>
> I was using Linus-recommended coding style, but it can be changed I guess.

Both styles are being used in VFS. Choose one; I don't mind much.

-- Andreas.

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[James Morris]

On Sun, 19 Sep 2004, Andreas Gruenbacher wrote:

> Documentation/filesystems/Locking seems to be accurate.

There's an out of date comment in fs/devpts/xattr.c (which is no excuse 
for screwing it up though).

> The old handler API was fine at the FS level where locking was guaranteed 
> anyways. At the VFS level we should do better. Passing in the buffer and the 
> buffer size at the same time gets us rid of the problem without requiring any 
> locking.

Ok, I'll incorporate this and resubmit the patches soon.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[Will Dyson]

On Sat, 18 Sep 2004 03:20:37 -0400 (EDT), James Morris
<jmorris@redhat.com> wrote:
> This patch consolidates common xattr handling logic into the core fs code,
> with modifications suggested by Christoph Hellwig (hang off superblock,
> remove locking, use generic code as methods), for use by ext2, ext3 and
> devpts, as well as upcoming tmpfs xattr code.

Hi James,

I'd like to raise an issue with the documentation your patch
introduces. The lack of it, actually.

While it might be obvious to someone who is familiar with the ext2 and
ext3 xattr code, I had a rather hard time understanding how a
filesystem would make use of the generic functions that your patch
introduces. While I think I have it now, that is 30 minutes of my life
that I will never get back :) . 30 minutes is not a long time (I've
spent as much thinking about this message), but this sort of thing is
an issue all throughout the generic vfs code.

When I was working on the readonly befs filesystem, I may have spent
as much as 20 percent of my time actually thinking about on-disk data
and how to use it. Most of the rest of that time was spent reading
code from the vfs layer (or other existing filesystems), trying to
figure out how I should call helper functions or how they would call
my code. Some of that code is very clever and solves complex problems,
which makes it hard to grok on the first (or third) read-through.

I don't plan on holding my breath while waiting for "The linux vfs
layer for dummies" to come out. But a quick comment to explain the
purpose and use of a block of code can make a world of difference to
someone trying to come up to speed.

For example:

/*
In order to implement different sets of xattr operations for each
xattr prefix, a filesystem should create a null-terminated array of
struct xattr_handler (one for each prefix) and hang a pointer to it
off of the s_xattr field of the superblock. The generic_fooxattr
functions will search this list for a xattr_handler with a prefix
field that matches the prefix of the xattr we are dealing with and
call the apropriate function pointer from that xattr_handler.
*/

-- 
Will Dyson - Consultant
http://www.lucidts.com/

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[James Morris]

On Mon, 20 Sep 2004, Will Dyson wrote:

> I don't plan on holding my breath while waiting for "The linux vfs
> layer for dummies" to come out. But a quick comment to explain the
> purpose and use of a block of code can make a world of difference to
> someone trying to come up to speed.

If you want to supply documentation patches, please feel free to do so.

> For example:
> 
> /*
> In order to implement different sets of xattr operations for each
> xattr prefix, a filesystem should create a null-terminated array of
> struct xattr_handler (one for each prefix) and hang a pointer to it
> off of the s_xattr field of the superblock. The generic_fooxattr
> functions will search this list for a xattr_handler with a prefix
> field that matches the prefix of the xattr we are dealing with and
> call the apropriate function pointer from that xattr_handler.
> */

The above is inaccurate.  e.g. not all of the generic functions search for
a matching xattr handler.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH 1/6] xattr consolidation v2 - generic xattr API

[Will Dyson]

On Mon, 20 Sep 2004 19:07:21 -0400 (EDT), James Morris
<jmorris@redhat.com> wrote:

> > For example:
> >
> > /*
> > In order to implement different sets of xattr operations for each
> > xattr prefix, a filesystem should create a null-terminated array of
> > struct xattr_handler (one for each prefix) and hang a pointer to it
> > off of the s_xattr field of the superblock. The generic_fooxattr
> > functions will search this list for a xattr_handler with a prefix
> > field that matches the prefix of the xattr we are dealing with and
> > call the apropriate function pointer from that xattr_handler.
> > */
> 
> The above is inaccurate.  e.g. not all of the generic functions search for
> a matching xattr handler.

Ok. You see the difficulty of documenting code that someone else
wrote.  Please consider the following:

--- xattr.c.old	2004-09-20 22:39:18.000000000 -0400
+++ xattr.c	2004-09-20 23:13:53.000000000 -0400
@@ -359,11 +359,24 @@
 	return *a_prefix ? NULL : a;
 }
 
+/*
+ * In order to implement different sets of xattr operations for each xattr
+ * prefix, a filesystem should create a null-terminated array of struct
+ * xattr_handler (one for each prefix) and hang a pointer to it off of the
+ * s_xattr field of the superblock.
+ *
+ * The generic_fooxattr() functions will use this list to dispatch xattr
+ * operations to the correct xattr_handler.
+ */
+
 #define for_each_xattr_handler(handlers, handler)		\
 		for ((handler) = *(handlers)++;			\
 			(handler) != NULL;			\
 			(handler) = *(handlers)++)	
 			
+/*
+ * Find the xattr_handler with the matching prefix
+ */
 static struct xattr_handler *xattr_resolve_name(struct xattr_handler
**handlers, const char **name)
 {
 	struct xattr_handler *handler;
@@ -381,6 +394,9 @@
 	return handler;
 }
 
+/*
+ * Find the handler for the prefix and dispatch the operation through it
+ */
 ssize_t generic_getxattr(struct dentry *dentry, const char *name,
void *buffer, size_t size)
 {
 	struct xattr_handler *handler;
@@ -392,6 +408,10 @@
 	return handler->get(inode, name, buffer, size);
 }
 
+/*
+ * Combine the results of the list() function from every xattr_handler in the
+ * list.
+ */
 ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t
buffer_size)
 {
 	struct inode *inode = dentry->d_inode;
@@ -417,6 +437,9 @@
 	return size;
 }
 
+/*
+ * Find the handler for the prefix and dispatch the operation through it
+ */
 int generic_setxattr(struct dentry *dentry, const char *name, const
void *value, size_t size, int flags)
 {
 	struct xattr_handler *handler;
@@ -430,6 +453,9 @@
 	return handler->set(inode, name, value, size, flags);
 }
 
+/*
+ * Find the handler for the prefix and dispatch the operation through it
+ */
 int generic_removexattr(struct dentry *dentry, const char *name)
 {
 	struct xattr_handler *handler;


-- 
Will Dyson - Consultant
http://www.lucidts.com/