[PATCH v5 0/2] xenfs: xenbus and container related bug fixes

[PATCH v5 0/2] xenfs: xenbus and container related bug fixes

[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 clearing FMODE_ATOMIC_POS.

David

Changes in v5:
- Clear FMODE_ATOMIC_POS instead of using symlinks.

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCHv5 1/2] xenbus: fix deadlock on writes to /proc/xen/xenbus

[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.

Clear FMODE_ATOMIC_POS when opening this device to always get
character device like sematics.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/xenbus/xenbus_dev_frontend.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index 1e8be12..ce389b4 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -536,6 +536,8 @@ static int xenbus_file_open(struct inode *inode, struct file *filp)
 	if (xen_store_evtchn == 0)
 		return -ENOENT;
 
+	filp->f_mode &= ~FMODE_ATOMIC_POS; /* cdev-style semantics */
+
 	nonseekable_open(inode, filp);
 
 	u = kzalloc(sizeof(*u), GFP_KERNEL);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCHv5 1/2] xenbus: fix deadlock on writes to /proc/xen/xenbus

[Jan Beulich]

>>> On 14.11.16 at 12:12, <david.vrabel@citrix.com> wrote:
> /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.
> 
> Clear FMODE_ATOMIC_POS when opening this device to always get
> character device like sematics.

Interesting. I'm pretty sure that back in March/April, when I had
to deal with this for our kernels, I was told the upstream kernel is
unaffected. In any event I continue to think that
https://patchwork.kernel.org/patch/8752901/
is the better (because more generic) solution here.

> --- a/drivers/xen/xenbus/xenbus_dev_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
> @@ -536,6 +536,8 @@ static int xenbus_file_open(struct inode *inode, struct file *filp)
>  	if (xen_store_evtchn == 0)
>  		return -ENOENT;
>  
> +	filp->f_mode &= ~FMODE_ATOMIC_POS; /* cdev-style semantics */
> +
>  	nonseekable_open(inode, filp);

In any event I think the adjustment should be placed after the call
to nonseekable_open(), despite it being unlikely that the function
might set the bit again.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCHv5 2/2] 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCHv5 2/2] xenfs: Use proc_create_mount_point() to create /proc/xen

[Juergen Gross]

On 14/11/16 12:12, David Vrabel wrote:
> 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>

Applied to xen/tip.git for-linus-4.10


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel