[PATCH v4 0/3] libfs,xenfs: replace /proc/xen/xenbus with a symlink

[PATCH v4 0/3] libfs,xenfs: replace /proc/xen/xenbus with a symlink

[David Vrabel]

Using /proc/xen/xenbus can cause deadlocks on the atomic file position
mutex since this file should behave like a character device and not a
regular file.  This is easiest to achive by making it a symlink to the
existing /dev/xen/xenbus device.

This requires extending simple_fill_super() to add symlinks as well as
regular files.

David

Changes in v4:
- Switch on file type in simple_fill_super()
- Make xen_xenus_fops and xen_privcmd_fops static
- Rebased on v4.9-rc2.
- Add patch from Seth for namespace support.

Changes in v3:
- Rebased on v4.7-rc5.

Changes in v2:
- Simplified simple_fill_super() change.

[PATCHv4 1/3] libfs: allow simple_fill_super() to add symlinks

[David Vrabel]

simple_fill_super() will add symlinks if an entry has mode & S_IFLNK.
The target is provided in the new "link" field.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v4:
- Use switch for file type.

v2:
- simplified.
---
 fs/libfs.c         | 27 ++++++++++++++++++++++++---
 include/linux/fs.h |  2 +-
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 48826d4..8eabcb1 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -509,6 +509,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 	struct dentry *root;
 	struct dentry *dentry;
 	int i;
+	int ret = -ENOMEM;
 
 	s->s_blocksize = PAGE_SIZE;
 	s->s_blocksize_bits = PAGE_SHIFT;
@@ -550,9 +551,29 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 			dput(dentry);
 			goto out;
 		}
-		inode->i_mode = S_IFREG | files->mode;
+		switch (files->mode & S_IFMT) {
+		case S_IFLNK:
+			inode->i_mode = files->mode;
+			inode->i_op = &simple_symlink_inode_operations;
+			inode->i_link = kstrdup(files->link, GFP_KERNEL);
+			if (!inode->i_link) {
+				iput(inode);
+				dput(dentry);
+				goto out;
+			}
+			break;
+		case S_IFREG:
+		case 0:
+			inode->i_mode = S_IFREG | files->mode;
+			inode->i_fop = files->ops;
+			break;
+		default:
+			iput(inode);
+			dput(dentry);
+			ret = -EINVAL;
+			goto out;
+		}
 		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
-		inode->i_fop = files->ops;
 		inode->i_ino = i;
 		d_add(dentry, inode);
 	}
@@ -562,7 +583,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 	d_genocide(root);
 	shrink_dcache_parent(root);
 	dput(root);
-	return -ENOMEM;
+	return ret;
 }
 EXPORT_SYMBOL(simple_fill_super);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 16d2b6e..4b64fbb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2988,7 +2988,7 @@ extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 extern void make_empty_dir_inode(struct inode *inode);
 extern bool is_empty_dir_inode(struct inode *inode);
-struct tree_descr { char *name; const struct file_operations *ops; int mode; };
+struct tree_descr { char *name; const struct file_operations *ops; int mode; char *link; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
 extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
-- 
2.1.4

[PATCHv4 2/3] xenfs: replace xenbus and privcmd with symlinks

[David Vrabel]

/proc/xen/xenbus does not work correctly.  A read blocked waiting for
a xenstore message holds the mutex needed for atomic file position
updates.  This blocks any writes on the same file handle, which can
deadlock if the write is needed to unblock the read.

/proc/xen/xenbus is supposed to be identical to the character device
/dev/xen/xenbus so replace the file with a symlink.

Similarly, replace /proc/xen/privcmd with a symlink since it should be
the same as /dev/xen/privcmd.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v4:
- Make xen_xenbus_fops and xen_privcmd_fops static.
---
 drivers/xen/privcmd.c                    |  5 +----
 drivers/xen/privcmd.h                    |  3 ---
 drivers/xen/xenbus/xenbus_comms.h        |  2 --
 drivers/xen/xenbus/xenbus_dev_frontend.c |  3 +--
 drivers/xen/xenfs/super.c                | 10 ++++------
 5 files changed, 6 insertions(+), 17 deletions(-)
 delete mode 100644 drivers/xen/privcmd.h

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 702040f..12ece8d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -37,8 +37,6 @@
 #include <xen/xen-ops.h>
 #include <xen/balloon.h>
 
-#include "privcmd.h"
-
 MODULE_LICENSE("GPL");
 
 #define PRIV_VMA_LOCKED ((void *)1)
@@ -644,12 +642,11 @@ static int privcmd_vma_range_is_mapped(
 				   is_mapped_fn, NULL) != 0;
 }
 
-const struct file_operations xen_privcmd_fops = {
+const static struct file_operations xen_privcmd_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = privcmd_ioctl,
 	.mmap = privcmd_mmap,
 };
-EXPORT_SYMBOL_GPL(xen_privcmd_fops);
 
 static struct miscdevice privcmd_dev = {
 	.minor = MISC_DYNAMIC_MINOR,
diff --git a/drivers/xen/privcmd.h b/drivers/xen/privcmd.h
deleted file mode 100644
index 14facae..0000000
--- a/drivers/xen/privcmd.h
+++ /dev/null
@@ -1,3 +0,0 @@
-#include <linux/fs.h>
-
-extern const struct file_operations xen_privcmd_fops;
diff --git a/drivers/xen/xenbus/xenbus_comms.h b/drivers/xen/xenbus/xenbus_comms.h
index e74f9c1..39efb85 100644
--- a/drivers/xen/xenbus/xenbus_comms.h
+++ b/drivers/xen/xenbus/xenbus_comms.h
@@ -47,6 +47,4 @@ extern struct xenstore_domain_interface *xen_store_interface;
 extern int xen_store_evtchn;
 extern enum xenstore_init xen_store_domain_type;
 
-extern const struct file_operations xen_xenbus_fops;
-
 #endif /* _XENBUS_COMMS_H */
diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index c1010f01..a45e7f2 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -598,7 +598,7 @@ static unsigned int xenbus_file_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
-const struct file_operations xen_xenbus_fops = {
+const static struct file_operations xen_xenbus_fops = {
 	.read = xenbus_file_read,
 	.write = xenbus_file_write,
 	.open = xenbus_file_open,
@@ -606,7 +606,6 @@ const struct file_operations xen_xenbus_fops = {
 	.poll = xenbus_file_poll,
 	.llseek = no_llseek,
 };
-EXPORT_SYMBOL_GPL(xen_xenbus_fops);
 
 static struct miscdevice xenbus_dev = {
 	.minor = MISC_DYNAMIC_MINOR,
diff --git a/drivers/xen/xenfs/super.c b/drivers/xen/xenfs/super.c
index 8559a71..0f2e2cd 100644
--- a/drivers/xen/xenfs/super.c
+++ b/drivers/xen/xenfs/super.c
@@ -18,8 +18,6 @@
 #include <xen/xen.h>
 
 #include "xenfs.h"
-#include "../privcmd.h"
-#include "../xenbus/xenbus_comms.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -45,16 +43,16 @@ static const struct file_operations capabilities_file_ops = {
 static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr xenfs_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{""},
 	};
 
 	static struct tree_descr xenfs_init_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{ "xsd_kva", &xsd_kva_file_ops, S_IRUSR|S_IWUSR},
 		{ "xsd_port", &xsd_port_file_ops, S_IRUSR|S_IWUSR},
 #ifdef CONFIG_XEN_SYMS
-- 
2.1.4

[PATCHv4 3/3] xenfs: Use proc_create_mount_point() to create /proc/xen

[David Vrabel]

From: Seth Forshee <seth.forshee@canonical.com>

Mounting proc in user namespace containers fails if the xenbus
filesystem is mounted on /proc/xen because this directory fails
the "permanently empty" test. proc_create_mount_point() exists
specifically to create such mountpoints in proc but is currently
proc-internal. Export this interface to modules, then use it in
xenbus when creating /proc/xen.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/xenbus/xenbus_probe.c | 2 +-
 fs/proc/generic.c                 | 1 +
 fs/proc/internal.h                | 1 -
 include/linux/proc_fs.h           | 2 ++
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 33a31cf..b5c1dec 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -826,7 +826,7 @@ static int __init xenbus_init(void)
 	 * Create xenfs mountpoint in /proc for compatibility with
 	 * utilities that expect to find "xenbus" under "/proc/xen".
 	 */
-	proc_mkdir("xen", NULL);
+	proc_create_mount_point("xen");
 #endif
 
 out_error:
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 5f2dc20..7eb3cef 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -479,6 +479,7 @@ struct proc_dir_entry *proc_create_mount_point(const char *name)
 	}
 	return ent;
 }
+EXPORT_SYMBOL(proc_create_mount_point);
 
 struct proc_dir_entry *proc_create_data(const char *name, umode_t mode,
 					struct proc_dir_entry *parent,
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 5378441..7de6795 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -195,7 +195,6 @@ static inline bool is_empty_pde(const struct proc_dir_entry *pde)
 {
 	return S_ISDIR(pde->mode) && !pde->proc_iops;
 }
-struct proc_dir_entry *proc_create_mount_point(const char *name);
 
 /*
  * inode.c
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index b97bf2e..8bd2f72 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -21,6 +21,7 @@ extern struct proc_dir_entry *proc_mkdir_data(const char *, umode_t,
 					      struct proc_dir_entry *, void *);
 extern struct proc_dir_entry *proc_mkdir_mode(const char *, umode_t,
 					      struct proc_dir_entry *);
+struct proc_dir_entry *proc_create_mount_point(const char *name);
  
 extern struct proc_dir_entry *proc_create_data(const char *, umode_t,
 					       struct proc_dir_entry *,
@@ -56,6 +57,7 @@ static inline struct proc_dir_entry *proc_symlink(const char *name,
 		struct proc_dir_entry *parent,const char *dest) { return NULL;}
 static inline struct proc_dir_entry *proc_mkdir(const char *name,
 	struct proc_dir_entry *parent) {return NULL;}
+static inline struct proc_dir_entry *proc_create_mount_point(const char *name) { return NULL; }
 static inline struct proc_dir_entry *proc_mkdir_data(const char *name,
 	umode_t mode, struct proc_dir_entry *parent, void *data) { return NULL; }
 static inline struct proc_dir_entry *proc_mkdir_mode(const char *name,
-- 
2.1.4

Re: [PATCH v4 0/3] libfs,xenfs: replace /proc/xen/xenbus with a symlink

[Al Viro]

On Fri, Oct 28, 2016 at 04:52:36PM +0100, David Vrabel wrote:
> Using /proc/xen/xenbus can cause deadlocks on the atomic file position
> mutex since this file should behave like a character device and not a
> regular file.  This is easiest to achive by making it a symlink to the
> existing /dev/xen/xenbus device.

What.  The.  Hell?  What's wrong with simply adding
	file->f_mode &= ~FMODE_ATOMIC_POS;	/* cdev-style semantics */

in the ->open() of those files, rather than going through all those
convolutions?