xen: add xenfs to allow usermode <-> Xen interaction

[ Reviewers: This is in drivers/xen to keep it close to the code it is and will be using. Would people
prefer to see it in fs/xenfs? -J ]

From: Alex Zeffertt <[email protected]>

The xenfs filesystem exports various interfaces to usermode. Initially this exports a file to allow usermode to interact with xenbus/xenstore.

Traditionally this appeared in /proc/xen. Rather than extending procfs, this patch adds a backward-compat mountpoint on /proc/xen, and provides a xenfs filesystem which can be mounted there.

...

drivers/xen/xenfs/super.c new file +9 -0

@@ -0,0 +1,65 @@
+/*
+ *  xenfs.c - a filesystem for passing info between the a domain and
+ *  the hypervisor.
+ *
+ * 2008-10-07  Alex Zeffertt    Replaced /proc/xen/xenbus with xenfs filesystem
+ *                              and /proc/xen compatibility mount point.
+ *                              Turned xenfs into a loadable module.
+ */
+

drivers/xen/xenfs/super.c new file +31 -0

+static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
+{
+	static struct tree_descr xenfs_files[] = {
+		[2] = {"xenbus", &xenbus_file_ops, S_IRUSR|S_IWUSR},
+		{""},
+	};
+
+	return simple_fill_super(sb, XENFS_MAGIC, xenfs_files);
+}
+
 
...
 
+struct xenbus_transaction_holder {
+	struct list_head list;
+	struct xenbus_transaction handle;
+};
+
+struct read_buffer {
+	struct list_head list;
+	unsigned int cons;
+	unsigned int len;
+	char msg[];
+};
 
+struct xenbus_file_priv {
+	/* In-progress transaction. */
+	struct list_head transactions;
+
+	/* Active watches. */
+	struct list_head watches;
+
+	/* Partial request. */
+	unsigned int len;
+	union {

drivers/xen/xenfs/super.c new file +12 -0

+
+static ssize_t xenbus_file_read(struct file *filp,
+			       char __user *ubuf,
+			       size_t len, loff_t *ppos)
+{
+	struct xenbus_file_priv *u = filp->private_data;
+	struct read_buffer *rb;
+	int i, ret;
+
+	mutex_lock(&u->reply_mutex);
+	while (list_empty(&u->read_buffers)) {
+		mutex_unlock(&u->reply_mutex);

drivers/xen/xenfs/super.c new file +5 -0

+		if (ret) {
+			mutex_unlock(&u->reply_mutex);
+			return ret;
+		}
+		i++;

drivers/xen/xenfs/super.c new file +9 -0

+	return 0;
+}
+
+/* Free all the read_buffer s on a list.
+ * Caller must have sole reference to list.
+ */
+static void queue_cleanup(struct list_head  *list)
+{
+	struct read_buffer *rb;

drivers/xen/xenfs/super.c new file +14 -0

+	struct xenbus_watch watch;
+	struct xenbus_file_priv *dev_data;
+	char *token;
+};
+
 
...
 
+static LIST_HEAD(watch_list);
+
+static ssize_t xenbus_file_write(struct file *filp,
+				const char __user *ubuf,
+				size_t len, loff_t *ppos)
+{
+	struct xenbus_file_priv *u = filp->private_data;
+	struct xenbus_transaction_holder *trans = NULL;
+	uint32_t msg_type;

drivers/xen/xenfs/super.c new file +144 -0

+	if (copy_from_user(u->u.buffer + u->len, ubuf, len) != 0) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	u->len += len;
+	if ((u->len < sizeof(u->u.msg)) ||
+	    (u->len < (sizeof(u->u.msg) + u->u.msg.len)))
+		return rc;
+
+	msg_type = u->u.msg.type;
+
+	switch (msg_type) {
+	case XS_TRANSACTION_START:
+	case XS_TRANSACTION_END:
+	case XS_DIRECTORY:
+	case XS_READ:
+	case XS_GET_PERMS:
+	case XS_RELEASE:
+	case XS_GET_DOMAIN_PATH:
+	case XS_WRITE:
+	case XS_MKDIR:
+	case XS_RM:
+	case XS_SET_PERMS:
+		if (msg_type == XS_TRANSACTION_START) {
+			trans = kmalloc(sizeof(*trans), GFP_KERNEL);
+			if (!trans) {
+				rc = -ENOMEM;
+				goto out;
+			}
+		}
+
+		reply = xenbus_dev_request_and_reply(&u->u.msg);
+		if (IS_ERR(reply)) {
+			kfree(trans);
+			rc = PTR_ERR(reply);
+			goto out;
+		}
+
+		if (msg_type == XS_TRANSACTION_START) {
+			trans->handle.id = simple_strtoul(reply, NULL, 0);
+			list_add(&trans->list, &u->transactions);
+		} else if (msg_type == XS_TRANSACTION_END) {
+			list_for_each_entry(trans, &u->transactions, list)
+				if (trans->handle.id == u->u.msg.tx_id)
+					break;
+			BUG_ON(&trans->list == &u->transactions);
+			list_del(&trans->list);
+			kfree(trans);
+		}
+		mutex_lock(&u->reply_mutex);
+		ret = queue_reply(&staging_q,
+				  (char *)&u->u.msg, sizeof(u->u.msg));
+		if (!ret)
+			ret = queue_reply(&staging_q,
+					  (char *)reply, u->u.msg.len);
+		if (!ret) {
+			list_splice_tail(&staging_q, &u->read_buffers);
+			wake_up(&u->read_waitq);
+		} else {
+			queue_cleanup(&staging_q);
+			rc = ret;
+		}
+		mutex_unlock(&u->reply_mutex);
+		kfree(reply);
+		break;
+
+	case XS_WATCH:
+	case XS_UNWATCH: {
+		struct watch_adapter *watch, *tmp_watch;
+		static const char XS_RESP[] = "OK";
+		struct xsd_sockmsg hdr;
+
+		path = u->u.buffer + sizeof(u->u.msg);
+		token = memchr(path, 0, u->u.msg.len);
+		if (token == NULL) {
+			rc = -EILSEQ;
+			goto out;
+		}
+		token++;
+
+		if (msg_type == XS_WATCH) {
+			watch = kzalloc(sizeof(*watch), GFP_KERNEL);
+			if (watch == NULL) {
+				rc = -ENOMEM;
+				goto out;
+			}
+			watch->watch.node =
+				kmalloc(strlen(path)+1, GFP_KERNEL);
+			if (watch->watch.node == NULL) {
+				kfree(watch);
+				rc = -ENOMEM;
+				goto out;
+			}
+			strcpy((char *)watch->watch.node, path);
+			watch->watch.callback = watch_fired;
+			watch->token =
+				kmalloc(strlen(token)+1, GFP_KERNEL);
+			if (watch->token == NULL) {
+				kfree(watch->watch.node);
+				kfree(watch);
+				rc = -ENOMEM;
+				goto out;
+			}
+			strcpy(watch->token, token);
+			watch->dev_data = u;
+
+			err = register_xenbus_watch(&watch->watch);
+			if (err) {
+				free_watch_adapter(watch);
+				rc = err;
+				goto out;
+			}
+
+			list_add(&watch->list, &u->watches);
+		} else {
+			list_for_each_entry_safe(watch, tmp_watch,
+						 &u->watches, list) {
+				if (!strcmp(watch->token, token) &&
+				    !strcmp(watch->watch.node, path)) {
+					unregister_xenbus_watch(&watch->watch);
+					list_del(&watch->list);
+					free_watch_adapter(watch);
+					break;
+				}
+			}
+		}
+
+		hdr.type = msg_type;
+		hdr.len = sizeof(XS_RESP);
+		mutex_lock(&u->reply_mutex);
+		ret = queue_reply(&staging_q, (char *)&hdr, sizeof(hdr));
+		if (!ret)
+			ret = queue_reply(&staging_q, (char *)XS_RESP, hdr.len);
+		if (!ret) {
+			list_splice_tail(&staging_q, &u->read_buffers);
+			wake_up(&u->read_waitq);
+		} else {
+			queue_cleanup(&staging_q);
+			rc = ret;
+		}
+		mutex_unlock(&u->reply_mutex);
+		break;
+	}

drivers/xen/xenfs/super.c new file +3 -0

+	return 0;
+}
+
 
...

Quoted 3 lines · On Tue, 16 Dec 2008 12:27:37 -0800 Jeremy Fitzhardinge <[email protected]> wrote:

[ Reviewers: This is in drivers/xen to keep it close to the code it is and will be using. Would people
prefer to see it in fs/xenfs? -J ]

Well it would be nice to keep filesystems under ./fs, but `grep -rl register_filesystem .' says we screwed that pooch.

Quoted 29 lines

From: Alex Zeffertt <[email protected]>

The xenfs filesystem exports various interfaces to usermode. Initially this exports a file to allow usermode to interact with xenbus/xenstore.

Traditionally this appeared in /proc/xen. Rather than extending procfs, this patch adds a backward-compat mountpoint on /proc/xen, and provides a xenfs filesystem which can be mounted there.

...

drivers/xen/xenfs/super.c new file +14 -0

@@ -0,0 +1,65 @@
+/*
+ *  xenfs.c - a filesystem for passing info between the a domain and
+ *  the hypervisor.
+ *
+ * 2008-10-07  Alex Zeffertt    Replaced /proc/xen/xenbus with xenfs filesystem
+ *                              and /proc/xen compatibility mount point.
+ *                              Turned xenfs into a loadable module.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+

drivers/xen/xenfs/super.c line 19

Quoted 6 lines

drivers/xen/xenfs/super.c new file +5 -0

+#include "xenfs.h"
+
+#include <asm/xen/hypervisor.h>
+
+#define XENFS_MAGIC	0xabba1974

Move to include/linux/magic.h, please.

drivers/xen/xenfs/super.c line 22

Quoted 4 lines

drivers/xen/xenfs/super.c new file +2 -0

+MODULE_DESCRIPTION("Xen filesystem");
+MODULE_LICENSE("GPL");

MODULE_AUTHOR()?

./MAINTAINERS?

Quoted 41 lines

drivers/xen/xenfs/super.c new file +36 -0

+static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
+{
+	static struct tree_descr xenfs_files[] = {
+		[2] = {"xenbus", &xenbus_file_ops, S_IRUSR|S_IWUSR},
+		{""},
+	};
+
+	return simple_fill_super(sb, XENFS_MAGIC, xenfs_files);
+}
+
 
...
 
+struct xenbus_transaction_holder {
+	struct list_head list;
+	struct xenbus_transaction handle;
+};
+
+struct read_buffer {
+	struct list_head list;
+	unsigned int cons;
+	unsigned int len;
+	char msg[];
+};
 
+struct xenbus_file_priv {
+	/* In-progress transaction. */
+	struct list_head transactions;
+
+	/* Active watches. */
+	struct list_head watches;
+
+	/* Partial request. */
+	unsigned int len;
+	union {
+		struct xsd_sockmsg msg;
+		char buffer[PAGE_SIZE];
+	} u;
+
+	/* Response queue. */

drivers/xen/xenfs/super.c

Quoted 6 lines

drivers/xen/xenfs/super.c new file +5 -0

+	struct list_head read_buffers;
+	wait_queue_head_t read_waitq;
+
+	struct mutex reply_mutex;
+};

It would be useful to document the locking for the list_head's (at least).

Quoted 18 lines

drivers/xen/xenfs/super.c new file +18 -0

+
+static ssize_t xenbus_file_read(struct file *filp,
+			       char __user *ubuf,
+			       size_t len, loff_t *ppos)
+{
+	struct xenbus_file_priv *u = filp->private_data;
+	struct read_buffer *rb;
+	int i, ret;
+
+	mutex_lock(&u->reply_mutex);
+	while (list_empty(&u->read_buffers)) {
+		mutex_unlock(&u->reply_mutex);
+		ret = wait_event_interruptible(u->read_waitq,
+					       !list_empty(&u->read_buffers));
+		if (ret)
+			return ret;
+		mutex_lock(&u->reply_mutex);
+	}

drivers/xen/xenfs/super.c

Quoted 5 lines

drivers/xen/xenfs/super.c new file +4 -0

+
+	rb = list_entry(u->read_buffers.next, struct read_buffer, list);
+	for (i = 0; i < len;) {
+		ret = put_user(rb->msg[rb->cons], ubuf + i);

So mmap_sem nests inside ->reply_mutex. Has this all been carefully tested with lockdep?

Quoted 7 lines

drivers/xen/xenfs/super.c new file +5 -0

+		if (ret) {
+			mutex_unlock(&u->reply_mutex);
+			return ret;
+		}
+		i++;

drivers/xen/xenfs/super.c

Quoted 10 lines

drivers/xen/xenfs/super.c new file +10 -0

+		rb->cons++;
+		if (rb->cons == rb->len) {
+			list_del(&rb->list);
+			kfree(rb);
+			if (list_empty(&u->read_buffers))
+				break;
+			rb = list_entry(u->read_buffers.next,
+					struct read_buffer, list);
+		}
+	}

This code will misbehave if it ever receives a read_buffer with len==0.

drivers/xen/xenfs/super.c

Quoted 12 lines

drivers/xen/xenfs/super.c new file +11 -0

+	mutex_unlock(&u->reply_mutex);
+
+	return i;
+}
+
+static int queue_reply(struct list_head *list, char *data, unsigned int len)
+{
+	struct read_buffer *rb;
+
+	if (len == 0)
+		return 0;

That should fix it.

drivers/xen/xenfs/super.c

Quoted 11 lines

drivers/xen/xenfs/super.c new file +10 -0

+	rb = kmalloc(sizeof(*rb) + len, GFP_KERNEL);
+	if (rb == NULL)
+		return -ENOMEM;
+
+	rb->cons = 0;
+	rb->len = len;
+
+	memcpy(rb->msg, data, len);
+
+	list_add_tail(&rb->list, list);

So the caller of this function must hold ->reply_mutex.

You thought that was secret but I found it out!

Quoted 15 lines

drivers/xen/xenfs/super.c new file +14 -0

+	return 0;
+}
+
+/* Free all the read_buffer s on a list.
+ * Caller must have sole reference to list.
+ */
+static void queue_cleanup(struct list_head  *list)
+{
+	struct read_buffer *rb;
+
+	while (!list_empty(list)) {
+		rb = list_entry(list->next, struct read_buffer, list);
+		list_del(list->next);
+		kfree(rb);

drivers/xen/xenfs/super.c

Quoted 6 lines

drivers/xen/xenfs/super.c new file +5 -0

+	}
+}
+
+struct watch_adapter {
+	struct list_head list;

locking is?

Quoted 18 lines

drivers/xen/xenfs/super.c new file +14 -0

+	struct xenbus_watch watch;
+	struct xenbus_file_priv *dev_data;
+	char *token;
+};
+
 
...
 
+static LIST_HEAD(watch_list);
+
+static ssize_t xenbus_file_write(struct file *filp,
+				const char __user *ubuf,
+				size_t len, loff_t *ppos)
+{
+	struct xenbus_file_priv *u = filp->private_data;
+	struct xenbus_transaction_holder *trans = NULL;
+	uint32_t msg_type;

drivers/xen/xenfs/super.c

Quoted 10 lines

drivers/xen/xenfs/super.c new file +10 -0

+	void *reply;
+	char *path, *token;
+	int err, rc = len;
+	int ret;
+	LIST_HEAD(staging_q);
+
+	if ((len + u->len) > sizeof(u->u.buffer)) {
+		rc = -EINVAL;
+		goto out;
+	}

Now what's this doing?

One would expect a large write to get chunked up into smaller writes by the kernel.

This code is poorly documented.

Quoted 151 lines

drivers/xen/xenfs/super.c new file +150 -0

+	if (copy_from_user(u->u.buffer + u->len, ubuf, len) != 0) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	u->len += len;
+	if ((u->len < sizeof(u->u.msg)) ||
+	    (u->len < (sizeof(u->u.msg) + u->u.msg.len)))
+		return rc;
+
+	msg_type = u->u.msg.type;
+
+	switch (msg_type) {
+	case XS_TRANSACTION_START:
+	case XS_TRANSACTION_END:
+	case XS_DIRECTORY:
+	case XS_READ:
+	case XS_GET_PERMS:
+	case XS_RELEASE:
+	case XS_GET_DOMAIN_PATH:
+	case XS_WRITE:
+	case XS_MKDIR:
+	case XS_RM:
+	case XS_SET_PERMS:
+		if (msg_type == XS_TRANSACTION_START) {
+			trans = kmalloc(sizeof(*trans), GFP_KERNEL);
+			if (!trans) {
+				rc = -ENOMEM;
+				goto out;
+			}
+		}
+
+		reply = xenbus_dev_request_and_reply(&u->u.msg);
+		if (IS_ERR(reply)) {
+			kfree(trans);
+			rc = PTR_ERR(reply);
+			goto out;
+		}
+
+		if (msg_type == XS_TRANSACTION_START) {
+			trans->handle.id = simple_strtoul(reply, NULL, 0);
+			list_add(&trans->list, &u->transactions);
+		} else if (msg_type == XS_TRANSACTION_END) {
+			list_for_each_entry(trans, &u->transactions, list)
+				if (trans->handle.id == u->u.msg.tx_id)
+					break;
+			BUG_ON(&trans->list == &u->transactions);
+			list_del(&trans->list);
+			kfree(trans);
+		}
+		mutex_lock(&u->reply_mutex);
+		ret = queue_reply(&staging_q,
+				  (char *)&u->u.msg, sizeof(u->u.msg));
+		if (!ret)
+			ret = queue_reply(&staging_q,
+					  (char *)reply, u->u.msg.len);
+		if (!ret) {
+			list_splice_tail(&staging_q, &u->read_buffers);
+			wake_up(&u->read_waitq);
+		} else {
+			queue_cleanup(&staging_q);
+			rc = ret;
+		}
+		mutex_unlock(&u->reply_mutex);
+		kfree(reply);
+		break;
+
+	case XS_WATCH:
+	case XS_UNWATCH: {
+		struct watch_adapter *watch, *tmp_watch;
+		static const char XS_RESP[] = "OK";
+		struct xsd_sockmsg hdr;
+
+		path = u->u.buffer + sizeof(u->u.msg);
+		token = memchr(path, 0, u->u.msg.len);
+		if (token == NULL) {
+			rc = -EILSEQ;
+			goto out;
+		}
+		token++;
+
+		if (msg_type == XS_WATCH) {
+			watch = kzalloc(sizeof(*watch), GFP_KERNEL);
+			if (watch == NULL) {
+				rc = -ENOMEM;
+				goto out;
+			}
+			watch->watch.node =
+				kmalloc(strlen(path)+1, GFP_KERNEL);
+			if (watch->watch.node == NULL) {
+				kfree(watch);
+				rc = -ENOMEM;
+				goto out;
+			}
+			strcpy((char *)watch->watch.node, path);
+			watch->watch.callback = watch_fired;
+			watch->token =
+				kmalloc(strlen(token)+1, GFP_KERNEL);
+			if (watch->token == NULL) {
+				kfree(watch->watch.node);
+				kfree(watch);
+				rc = -ENOMEM;
+				goto out;
+			}
+			strcpy(watch->token, token);
+			watch->dev_data = u;
+
+			err = register_xenbus_watch(&watch->watch);
+			if (err) {
+				free_watch_adapter(watch);
+				rc = err;
+				goto out;
+			}
+
+			list_add(&watch->list, &u->watches);
+		} else {
+			list_for_each_entry_safe(watch, tmp_watch,
+						 &u->watches, list) {
+				if (!strcmp(watch->token, token) &&
+				    !strcmp(watch->watch.node, path)) {
+					unregister_xenbus_watch(&watch->watch);
+					list_del(&watch->list);
+					free_watch_adapter(watch);
+					break;
+				}
+			}
+		}
+
+		hdr.type = msg_type;
+		hdr.len = sizeof(XS_RESP);
+		mutex_lock(&u->reply_mutex);
+		ret = queue_reply(&staging_q, (char *)&hdr, sizeof(hdr));
+		if (!ret)
+			ret = queue_reply(&staging_q, (char *)XS_RESP, hdr.len);
+		if (!ret) {
+			list_splice_tail(&staging_q, &u->read_buffers);
+			wake_up(&u->read_waitq);
+		} else {
+			queue_cleanup(&staging_q);
+			rc = ret;
+		}
+		mutex_unlock(&u->reply_mutex);
+		break;
+	}
+
+	default:
+		rc = -EINVAL;
+		break;
+	}
+

drivers/xen/xenfs/super.c

Quoted 5 lines

+ out:

drivers/xen/xenfs/super.c new file +3 -0

+	u->len = 0;
+	return rc;
+}

This function implements a new kernel ABI. How are reviewers to review the design of this proposed ABI?

drivers/xen/xenfs/super.c

Quoted 10 lines

...

drivers/xen/xenfs/super.c new file +7 -0

+static unsigned int xenbus_file_poll(struct file *file, poll_table *wait)
+{
+	struct xenbus_file_priv *u = file->private_data;
+
+	poll_wait(file, &u->read_waitq, wait);
+	if (!list_empty(&u->read_buffers))
+		return POLLIN | POLLRDNORM;

I wonder if we needed the lock or some open-coded barrier to safely run list_empty().

The xenfs filesystem exports various interfaces to usermode. Initially this exports a file to allow usermode to interact with xenbus/xenstore.

Traditionally this appeared in /proc/xen. Rather than extending procfs, this patch adds a backward-compat mountpoint on /proc/xen, and provides a xenfs filesystem which can be mounted there.

[
I did quite a lot of work to this code as a result of review, which is why this is a repost rather than a delta. The changes are:

- Moved the XENFS_SUPER_MAGIC to linux/magic.h
- Added comments to answer the "what's this for?" questions (I hope)
- Split things out into smaller functions
- Cleaned up type of queue_reply(), removed casts
- Added a mutex to protect struct xenbus_file_priv. This protects the list heads, and the partial message buffer. There were several ways in which usermode could overwrite the kernel's memory via races without this locking.
- Fixed a bug in which usermode could start sending a message which can never be sent, leaving the file descriptor in a useless state.

I'm bored of your filesystem, so I think I'll just merge it, unless someone else wants to ;)

Was this noticed and considered?

WARNING: consider using strict_strtoul in preference to simple_strtoul #543: FILE: drivers/xen/xenfs/xenbus.c:315:
+ trans->handle.id = simple_strtoul(reply, NULL, 0);

Quoted 11 lines · On Wed, 17 Dec 2008 13:24:39 -0800 Jeremy Fitzhardinge <[email protected]> wrote:

The xenfs filesystem exports various interfaces to usermode. Initially this exports a file to allow usermode to interact with xenbus/xenstore.

Traditionally this appeared in /proc/xen. Rather than extending procfs, this patch adds a backward-compat mountpoint on /proc/xen, and provides a xenfs filesystem which can be mounted there.

[
I did quite a lot of work to this code as a result of review, which is why this is a repost rather than a delta. The changes are:

Quoted 11 lines

- Moved the XENFS_SUPER_MAGIC to linux/magic.h
- Added comments to answer the "what's this for?" questions (I hope)
- Split things out into smaller functions
- Cleaned up type of queue_reply(), removed casts
- Added a mutex to protect struct xenbus_file_priv. This protects the list heads, and the partial message buffer. There were several ways in which usermode could overwrite the kernel's memory via races without this locking.
- Fixed a bug in which usermode could start sending a message which can never be sent, leaving the file descriptor in a useless state.

I'm bored of your filesystem, so I think I'll just merge it, unless someone else wants to ;)