[RFC] cr: pty: don't use required_id, change ptmx_open()

[RFC] cr: pty: don't use required_id, change ptmx_open()

[Serge E. Hallyn]

Ok so here is the patch to do unix98 pty restore by making
invasive changes to ptmx_open() instead of using
task_struct->required_id to pass an index.  Patch generated
against the cr tree with the original pty patches, as I
assume that will be easier to read.

Break ptmx_open() into a ptmx_create() which is used both
by ptmx_open itself, and fs/devpts/inode.c:open_create_pty().
The latter function is used only during sys_restart() when
a unix98 pty must be restored.

Either with or without this patch, the pty test at
	git://git.sr71.net/~hallyn/cr_tests.git
under the pty/ directory.  The question then is which is
a more likely approach to be acceptable upstream.  The
task->required_id approach is imo 'hacky', but it actually
does a much better job of re-using existing helpers, and
therefore is more maintainable.  So part of me DOES prefer
the original required_id approach.

This is purely RFC, so comments very much appreciated.

Signed-off-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
--
 checkpoint/sys.c           |    8 ---
 drivers/char/pty.c         |   16 +++---
 drivers/char/tty_io.c      |  112 +++++++++++++++++++++++++++++++++++----------
 fs/devpts/inode.c          |   55 ++++++++++++++++++++--
 include/linux/checkpoint.h |    2 
 include/linux/devpts_fs.h  |    2 
 include/linux/sched.h      |    1 
 7 files changed, 151 insertions(+), 45 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index 3db18f7..b1fdbd7 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -339,14 +339,6 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
 	return ret;
 }
 
-static int __init checkpoint_init(void)
-{
-	init_task.required_id = CKPT_REQUIRED_NONE;
-	return 0;
-}
-__initcall(checkpoint_init);
-
-
 /* 'ckpt_debug_level' controls the verbosity level of c/r code */
 #ifdef CONFIG_CHECKPOINT_DEBUG
 
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index afdab5e..79e86e4 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -28,6 +28,8 @@
 #include <linux/uaccess.h>
 #include <linux/bitops.h>
 #include <linux/devpts_fs.h>
+#include <linux/file.h>
+#include <linux/mount.h>
 
 #include <asm/system.h>
 
@@ -629,20 +631,13 @@ static const struct tty_operations pty_unix98_ops = {
  *		allocated_ptys_lock handles the list of free pty numbers
  */
 
-static int __ptmx_open(struct inode *inode, struct file *filp)
+int ptmx_create(struct inode *inode, struct file *filp, int index)
 {
 	struct tty_struct *tty;
 	int retval;
-	int index = -1;
 
 	nonseekable_open(inode, filp);
 
-#ifdef CONFIG_CHECKPOINT
-	/* when restarting, request specific index */
-	if (current->flags & PF_RESTARTING)
-		index = (int) current->required_id;
-#endif
-	/* find a device that is not in use. */
 	index = devpts_new_index(inode, index);
 	if (index < 0)
 		return index;
@@ -675,6 +670,11 @@ out:
 	return retval;
 }
 
+static int __ptmx_open(struct inode *inode, struct file *filp)
+{
+	return ptmx_create(inode, filp, UNSPECIFIED_PTY_INDEX);
+}
+
 static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	int ret;
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index b8f8d79..d27ec36 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -79,6 +79,7 @@
 #include <linux/devpts_fs.h>
 #include <linux/file.h>
 #include <linux/fdtable.h>
+#include <linux/namei.h>
 #include <linux/console.h>
 #include <linux/timer.h>
 #include <linux/ctype.h>
@@ -2982,13 +2983,97 @@ static int restore_tty_ldisc(struct ckpt_ctx *ctx,
 	return 0;
 }
 
-#define CKPT_PTMX_PATH  "/dev/ptmx"
+#define PTMXNAME "ptmx"
+struct tty_struct *pty_open_by_master_in_ns(struct ckpt_ctx *ctx,
+		struct ckpt_hdr_tty *h, struct file *fdir)
+{
+	struct tty_struct *tty;
+	struct file *file;
+	int ret;
+	struct qstr ptmxname;
+	struct dentry *ptmxdentry;
+	struct nameidata nd;
+
+	ret = file_permission(fdir, MAY_EXEC);
+	if (ret)
+		return ERR_PTR(ret);
+	nd.path = fdir->f_path;
+	path_get(&nd.path);
+	ptmxname.name = PTMXNAME;
+	ptmxname.len = strlen(ptmxname.name);
+	mutex_lock(&fdir->f_dentry->d_inode->i_mutex);
+	ptmxdentry = d_hash_and_lookup(fdir->f_dentry, &ptmxname);
+	mutex_unlock(&fdir->f_dentry->d_inode->i_mutex);
+	if (!ptmxdentry) {
+		path_put(&nd.path);
+		return ERR_PTR(-ENOENT);
+	}
+
+	/* should get mode from the file handle... */
+	file = alloc_file(nd.path.mnt, ptmxdentry, FMODE_READ|FMODE_WRITE, &tty_fops);
+	path_put(&nd.path);
+	if (!file) {
+		dput(ptmxdentry);
+		return ERR_PTR(-EINVAL);
+	}
+
+	ret = security_dentry_open(file, current_cred());
+	if (ret) {
+		fput(file);
+		return ERR_PTR(ret);
+	}
+	/* check write access perms to file */
+
+	lock_kernel();  /* tty_open does it... */
+	ret = open_create_pty(fdir, h->index, file);
+	unlock_kernel();
+	if (ret) {
+		fput(file);
+		return ERR_PTR(ret);
+	}
+	ckpt_debug("master file %p (obj %d)\n", file, h->file_objref);
+
+	/*
+	 * Add file to objhash to ensure proper cleanup later
+	 * (it isn't referenced elsewhere). Use h->file_objref
+	 * which was explicitly during checkpoint for this.
+	 */
+	ret = ckpt_obj_insert(ctx, file, h->file_objref, CKPT_OBJ_FILE);
+	if (ret < 0) {
+		fput(file);
+		return ERR_PTR(ret);
+	}
+
+	tty = file->private_data;
+	fput(file);   /* objhash took a ref */
+
+	return tty;
+}
+
+struct tty_struct *pty_open_by_master(struct ckpt_ctx *ctx,
+		struct ckpt_hdr_tty *h)
+{
+	struct file *fdir;
+	struct tty_struct *tty;
+	/*
+	 * we need to pick a way to specify which devpts
+	 * mountpoint to use.  For now, we'll just use
+	 * whatever /dev/ptmx points to
+	 */
+	fdir = filp_open("/dev/pts", O_RDONLY, 0);
+	if (IS_ERR(fdir))
+		return ERR_PTR(PTR_ERR(fdir));
+
+	tty = pty_open_by_master_in_ns(ctx, h, fdir);
+	fput(fdir);
+
+	return tty;
+}
 
 static struct tty_struct *do_restore_tty(struct ckpt_ctx *ctx)
 {
 	struct ckpt_hdr_tty *h;
 	struct tty_struct *tty = ERR_PTR(-EINVAL);
-	struct file *file = NULL;
 	int master, ret;
 
 	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TTY);
@@ -3025,28 +3110,9 @@ static struct tty_struct *do_restore_tty(struct ckpt_ctx *ctx)
 	 * specific (and probably called via tty_operations).
 	 */
 	if (master) {
-		current->required_id = h->index;
-		file = filp_open(CKPT_PTMX_PATH, O_RDWR | O_NOCTTY, 0);
-		current->required_id = CKPT_REQUIRED_NONE;
-		ckpt_debug("master file %p (obj %d)\n", file, h->file_objref);
-		if (IS_ERR(file)) {
-			tty = (struct tty_struct *) file;
-			goto out;
-		}
-
-		/*
-		 * Add file to objhash to ensure proper cleanup later
-		 * (it isn't referenced elsewhere). Use h->file_objref
-		 * which was explicitly during checkpoint for this.
-		 */
-		ret = ckpt_obj_insert(ctx, file, h->file_objref, CKPT_OBJ_FILE);
-		if (ret < 0) {
-			tty = ERR_PTR(ret);
-			fput(file);
+		tty = pty_open_by_master(ctx, h);
+		if (IS_ERR(tty))
 			goto out;
-		}
-
-		tty = file->private_data;
 	} else {
 		tty = ckpt_obj_fetch(ctx, h->link_objref, CKPT_OBJ_TTY);
 		if (IS_ERR(tty))
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 8921726..f76083f 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -14,6 +14,9 @@
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
+#include <linux/fs.h>
+#include <linux/smp_lock.h>
+#include <linux/file.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
 #include <linux/tty.h>
@@ -431,13 +434,18 @@ static struct file_system_type devpts_fs_type = {
 /*
  * The normal naming convention is simply /dev/pts/<number>; this conforms
  * to the System V naming convention
+ *
+ * @ptmx_inode: inode of the /dev/ptmx file - each pty namepace has its
+ * own
+ * @req_idx: requested specific index (i.e. 5 for /dev/pts/5).  If -1,
+ * then return first available.
  */
 
 int devpts_new_index(struct inode *ptmx_inode, int req_idx)
 {
 	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
-	int index;
+	int index = req_idx;
 	int ida_ret;
 
 retry:
@@ -445,7 +453,8 @@ retry:
 		return -ENOMEM;
 
 	mutex_lock(&allocated_ptys_lock);
-	index = (req_idx >= 0 ? req_idx : 0);
+	if (index == UNSPECIFIED_PTY_INDEX)
+		index = 0;
 	ida_ret = ida_get_new_above(&fsi->allocated_ptys, index, &index);
 	if (ida_ret < 0) {
 		mutex_unlock(&allocated_ptys_lock);
@@ -454,7 +463,7 @@ retry:
 		return -EIO;
 	}
 
-	if (req_idx >= 0 && index != req_idx) {
+	if (req_idx != UNSPECIFIED_PTY_INDEX && index != req_idx) {
 		ida_remove(&fsi->allocated_ptys, index);
 		mutex_unlock(&allocated_ptys_lock);
 		return -EBUSY;
@@ -557,6 +566,46 @@ out:
 	mutex_unlock(&root->d_inode->i_mutex);
 }
 
+struct inode *get_ptmx_inode(struct super_block *sb)
+{
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct dentry *dentry = fsi->ptmx_dentry;
+	struct inode *inode = dentry->d_inode;
+
+	return igrab(inode);
+}
+
+/* defined in drivers/char/pty.c */
+extern int ptmx_create(struct inode *inode, struct file *filp, int index);
+
+/*
+ * fn caller (in restart code) has checked for the rights on the
+ * part of the userspace task to open the ptmx file.
+ * @pdir: an open file handle to the /dev/pts directory, which we
+ * will use to determine the devpts namespace, and to confirm that
+ * the resulting file is in the right directory.
+ * @file is a file for /dev/ptmx
+ */
+int open_create_pty(struct file *fdir, int idx, struct file *file)
+{
+	struct inode *ptmx_ino;
+	struct dentry *pdir = fdir->f_dentry;
+	struct super_block *sb;
+	int ret;
+
+	if (!pdir || !pdir->d_inode)
+		return -EINVAL;
+
+	sb = pts_sb_from_inode(pdir->d_inode);
+	ptmx_ino = get_ptmx_inode(sb);
+	if (!ptmx_ino)
+		return -EINVAL;
+
+	ret = ptmx_create(ptmx_ino, file, idx);
+	iput(ptmx_ino);
+	return ret;
+}
+
 static int __init init_devpts_fs(void)
 {
 	int err = register_filesystem(&devpts_fs_type);
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 5e90ef9..0cbe8c4 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -46,8 +46,6 @@
 #define CHECKPOINT_USER_FLAGS		CHECKPOINT_SUBTREE
 #define RESTART_USER_FLAGS		(RESTART_TASKSELF | RESTART_FROZEN)
 
-#define CKPT_REQUIRED_NONE  ((unsigned long) -1L)
-
 extern void exit_checkpoint(struct task_struct *tsk);
 
 extern int ckpt_kwrite(struct ckpt_ctx *ctx, void *buf, int count);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 14948ee..a15d23c 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -15,10 +15,12 @@
 
 #include <linux/errno.h>
 
+#define UNSPECIFIED_PTY_INDEX -1
 #ifdef CONFIG_UNIX98_PTYS
 
 int devpts_new_index(struct inode *ptmx_inode, int req_idx);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
+int open_create_pty(struct file *fdir, int idx, struct file *file);
 /* mknod in devpts */
 int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty);
 /* get tty structure */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f6f1350..0e67de7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1481,7 +1481,6 @@ struct task_struct {
 #endif /* CONFIG_TRACING */
 #ifdef CONFIG_CHECKPOINT
 	struct ckpt_ctx *checkpoint_ctx;
-	unsigned long required_id;
 #endif
 };

Re: [RFC] cr: pty: don't use required_id, change ptmx_open()

[Oren Laadan]

I assume this will be the preferred approach. See comments
below.

Serge E. Hallyn wrote:
> Ok so here is the patch to do unix98 pty restore by making
> invasive changes to ptmx_open() instead of using
> task_struct->required_id to pass an index.  Patch generated
> against the cr tree with the original pty patches, as I
> assume that will be easier to read.
> 
> Break ptmx_open() into a ptmx_create() which is used both
> by ptmx_open itself, and fs/devpts/inode.c:open_create_pty().
> The latter function is used only during sys_restart() when
> a unix98 pty must be restored.
> 
> Either with or without this patch, the pty test at
> 	git://git.sr71.net/~hallyn/cr_tests.git
> under the pty/ directory.  The question then is which is
> a more likely approach to be acceptable upstream.  The
> task->required_id approach is imo 'hacky', but it actually
> does a much better job of re-using existing helpers, and
> therefore is more maintainable.  So part of me DOES prefer
> the original required_id approach.
> 
> This is purely RFC, so comments very much appreciated.
> 
> Signed-off-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> --
>  checkpoint/sys.c           |    8 ---
>  drivers/char/pty.c         |   16 +++---
>  drivers/char/tty_io.c      |  112 +++++++++++++++++++++++++++++++++++----------
>  fs/devpts/inode.c          |   55 ++++++++++++++++++++--
>  include/linux/checkpoint.h |    2 
>  include/linux/devpts_fs.h  |    2 
>  include/linux/sched.h      |    1 
>  7 files changed, 151 insertions(+), 45 deletions(-)
> 
> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index 3db18f7..b1fdbd7 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -339,14 +339,6 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
>  	return ret;
>  }
>  
> -static int __init checkpoint_init(void)
> -{
> -	init_task.required_id = CKPT_REQUIRED_NONE;
> -	return 0;
> -}
> -__initcall(checkpoint_init);
> -
> -
>  /* 'ckpt_debug_level' controls the verbosity level of c/r code */
>  #ifdef CONFIG_CHECKPOINT_DEBUG
>  
> diff --git a/drivers/char/pty.c b/drivers/char/pty.c
> index afdab5e..79e86e4 100644
> --- a/drivers/char/pty.c
> +++ b/drivers/char/pty.c
> @@ -28,6 +28,8 @@
>  #include <linux/uaccess.h>
>  #include <linux/bitops.h>
>  #include <linux/devpts_fs.h>
> +#include <linux/file.h>
> +#include <linux/mount.h>
>  
>  #include <asm/system.h>
>  
> @@ -629,20 +631,13 @@ static const struct tty_operations pty_unix98_ops = {
>   *		allocated_ptys_lock handles the list of free pty numbers
>   */
>  
> -static int __ptmx_open(struct inode *inode, struct file *filp)
> +int ptmx_create(struct inode *inode, struct file *filp, int index)
>  {
>  	struct tty_struct *tty;
>  	int retval;
> -	int index = -1;
>  
>  	nonseekable_open(inode, filp);
>  
> -#ifdef CONFIG_CHECKPOINT
> -	/* when restarting, request specific index */
> -	if (current->flags & PF_RESTARTING)
> -		index = (int) current->required_id;
> -#endif
> -	/* find a device that is not in use. */
>  	index = devpts_new_index(inode, index);
>  	if (index < 0)
>  		return index;
> @@ -675,6 +670,11 @@ out:
>  	return retval;
>  }
>  
> +static int __ptmx_open(struct inode *inode, struct file *filp)
> +{
> +	return ptmx_create(inode, filp, UNSPECIFIED_PTY_INDEX);
> +}
> +
>  static int ptmx_open(struct inode *inode, struct file *filp)
>  {
>  	int ret;
> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index b8f8d79..d27ec36 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -79,6 +79,7 @@
>  #include <linux/devpts_fs.h>
>  #include <linux/file.h>
>  #include <linux/fdtable.h>
> +#include <linux/namei.h>
>  #include <linux/console.h>
>  #include <linux/timer.h>
>  #include <linux/ctype.h>
> @@ -2982,13 +2983,97 @@ static int restore_tty_ldisc(struct ckpt_ctx *ctx,
>  	return 0;
>  }
>  
> -#define CKPT_PTMX_PATH  "/dev/ptmx"
> +#define PTMXNAME "ptmx"
> +struct tty_struct *pty_open_by_master_in_ns(struct ckpt_ctx *ctx,
> +		struct ckpt_hdr_tty *h, struct file *fdir)

I assume you split this to two functions so that in the future you
could feed a different "base" directory (for a different devpts mount).
To simplify this patch, perhaps we can defer it until we do that ?

Nit: s/pty_open_by_master_in_ns/pty_open_by_index/ ?

Instead of passing ckpt_hdr_tty, maybe instead make it return the
file pointer, and pass the desired index instead:
	struct file *pty_open_by_index(char *path, int index)

By letting the caller decide on the path, and what to do with the
resulting file pointer, I hope the c/r code will be more separate
and therefore easier to follow ?

> +{
> +	struct tty_struct *tty;
> +	struct file *file;
> +	int ret;
> +	struct qstr ptmxname;
> +	struct dentry *ptmxdentry;
> +	struct nameidata nd;
> +
> +	ret = file_permission(fdir, MAY_EXEC);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	nd.path = fdir->f_path;
> +	path_get(&nd.path);
> +	ptmxname.name = PTMXNAME;
> +	ptmxname.len = strlen(ptmxname.name);
> +	mutex_lock(&fdir->f_dentry->d_inode->i_mutex);
> +	ptmxdentry = d_hash_and_lookup(fdir->f_dentry, &ptmxname);
> +	mutex_unlock(&fdir->f_dentry->d_inode->i_mutex);
> +	if (!ptmxdentry) {
> +		path_put(&nd.path);
> +		return ERR_PTR(-ENOENT);
> +	}

Need to verify that the resulting dentry points to a ptmx device.
(With current code, user can arrange for something else in /dev/pts)

> +
> +	/* should get mode from the file handle... */

Should be ok, since restore_file_common() should restore the mode...

> +	file = alloc_file(nd.path.mnt, ptmxdentry, FMODE_READ|FMODE_WRITE, &tty_fops);
> +	path_put(&nd.path);
> +	if (!file) {
> +		dput(ptmxdentry);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	ret = security_dentry_open(file, current_cred());
> +	if (ret) {
> +		fput(file);
> +		return ERR_PTR(ret);
> +	}
> +	/* check write access perms to file */

I bet the above can be done by reusing lookup and __dentry_open()
functions from fs/open.c and fs/namei.c ...

> +
> +	lock_kernel();  /* tty_open does it... */
> +	ret = open_create_pty(fdir, h->index, file);
> +	unlock_kernel();
> +	if (ret) {
> +		fput(file);
> +		return ERR_PTR(ret);
> +	}
> +	ckpt_debug("master file %p (obj %d)\n", file, h->file_objref);
> +
> +	/*
> +	 * Add file to objhash to ensure proper cleanup later
> +	 * (it isn't referenced elsewhere). Use h->file_objref
> +	 * which was explicitly during checkpoint for this.
> +	 */
> +	ret = ckpt_obj_insert(ctx, file, h->file_objref, CKPT_OBJ_FILE);
> +	if (ret < 0) {
> +		fput(file);
> +		return ERR_PTR(ret);
> +	}

Can do this in caller. So

> +
> +	tty = file->private_data;
> +	fput(file);   /* objhash took a ref */
> +
> +	return tty;
> +}
> +
> +struct tty_struct *pty_open_by_master(struct ckpt_ctx *ctx,
> +		struct ckpt_hdr_tty *h)
> +{
> +	struct file *fdir;
> +	struct tty_struct *tty;
> +	/*
> +	 * we need to pick a way to specify which devpts
> +	 * mountpoint to use.  For now, we'll just use
> +	 * whatever /dev/ptmx points to
> +	 */
> +	fdir = filp_open("/dev/pts", O_RDONLY, 0);
> +	if (IS_ERR(fdir))
> +		return ERR_PTR(PTR_ERR(fdir));

Does this mean that to restore a subtree on a system without
devpts-ns it won't work ?  (no /dev/pts/ptmx)

[...]

Oren.

Re: [RFC] cr: pty: don't use required_id, change ptmx_open()

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> > +	file = alloc_file(nd.path.mnt, ptmxdentry, FMODE_READ|FMODE_WRITE, &tty_fops);
> > +	path_put(&nd.path);
> > +	if (!file) {
> > +		dput(ptmxdentry);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	ret = security_dentry_open(file, current_cred());
> > +	if (ret) {
> > +		fput(file);
> > +		return ERR_PTR(ret);
> > +	}
> > +	/* check write access perms to file */
> 
> I bet the above can be done by reusing lookup and __dentry_open()
> functions from fs/open.c and fs/namei.c ...

With lookup_create()?  Hmm, yeah - I think there are reasons I can't do
that, but I'll just try and see where it gets us.

> > +
> > +	lock_kernel();  /* tty_open does it... */
> > +	ret = open_create_pty(fdir, h->index, file);
> > +	unlock_kernel();
> > +	if (ret) {
> > +		fput(file);
> > +		return ERR_PTR(ret);
> > +	}
> > +	ckpt_debug("master file %p (obj %d)\n", file, h->file_objref);
> > +
> > +	/*
> > +	 * Add file to objhash to ensure proper cleanup later
> > +	 * (it isn't referenced elsewhere). Use h->file_objref
> > +	 * which was explicitly during checkpoint for this.
> > +	 */
> > +	ret = ckpt_obj_insert(ctx, file, h->file_objref, CKPT_OBJ_FILE);
> > +	if (ret < 0) {
> > +		fput(file);
> > +		return ERR_PTR(ret);
> > +	}
> 
> Can do this in caller. So
> 
> > +
> > +	tty = file->private_data;
> > +	fput(file);   /* objhash took a ref */
> > +
> > +	return tty;
> > +}
> > +
> > +struct tty_struct *pty_open_by_master(struct ckpt_ctx *ctx,
> > +		struct ckpt_hdr_tty *h)
> > +{
> > +	struct file *fdir;
> > +	struct tty_struct *tty;
> > +	/*
> > +	 * we need to pick a way to specify which devpts
> > +	 * mountpoint to use.  For now, we'll just use
> > +	 * whatever /dev/ptmx points to
> > +	 */
> > +	fdir = filp_open("/dev/pts", O_RDONLY, 0);
> > +	if (IS_ERR(fdir))
> > +		return ERR_PTR(PTR_ERR(fdir));
> 
> Does this mean that to restore a subtree on a system without
> devpts-ns it won't work ?  (no /dev/pts/ptmx)

Yes.  I figured once we settle on devpts ns handling, we can
trivially special-case the DEVPTS_MULTIPLE_INSTANCES=n case
to use /dev/ptmx.

Since we're punting on devpts namespaces anyway I guess the
pty_open_by_master() can just open /dev/ptmx directly for
now.

Or, maybe we're better off handling the namespaces.  Because
without doing that, we are hard-coding pathnames in the kernel
left and right, and that's Bad(tm).

thanks,
-serge

Re: [RFC] cr: pty: don't use required_id, change ptmx_open()

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> > +	file = alloc_file(nd.path.mnt, ptmxdentry, FMODE_READ|FMODE_WRITE, &tty_fops);
> > +	path_put(&nd.path);
> > +	if (!file) {
> > +		dput(ptmxdentry);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	ret = security_dentry_open(file, current_cred());
> > +	if (ret) {
> > +		fput(file);
> > +		return ERR_PTR(ret);
> > +	}
> > +	/* check write access perms to file */
> 
> I bet the above can be done by reusing lookup and __dentry_open()
> functions from fs/open.c and fs/namei.c ...

All right, so I tried the below patch (lots of debugging left in) but
the master end doesn't seem to end up right.  After restart, pty_write()
continues to succeed on the re-opened /dev/pts/N, but no reads happen
on the master.

What I'm doing is passing a new flag LOOKUP_CALLER_OPEN to filp_open(),
to tell it not to actually call the fops->open() routine (ptmx_open).
Then I call ptmx_create() (the renamed ptmx_open) by hand.  Seems like
it certainly ought to be working...  But I'm getting nowhere figuring
out what's happening to the major file, so putting it aside for a bit.

Following is the patch against the tree with Oren's original pty patches.

-serge

---
 checkpoint/sys.c           |    8 -----
 drivers/char/pty.c         |   43 ++++++++++++++++++++--------
 drivers/char/tty_io.c      |   69 ++++++++++++++++++++++++++++++++++++---------
 fs/devpts/inode.c          |   60 +++++++++++++++++++++++++++++++++++++--
 fs/open.c                  |   19 +++++++++++-
 include/linux/checkpoint.h |    2 -
 include/linux/devpts_fs.h  |    3 +
 include/linux/namei.h      |    1 
 include/linux/sched.h      |    1 
 9 files changed, 167 insertions(+), 39 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index 3db18f7..b1fdbd7 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -339,14 +339,6 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
 	return ret;
 }
 
-static int __init checkpoint_init(void)
-{
-	init_task.required_id = CKPT_REQUIRED_NONE;
-	return 0;
-}
-__initcall(checkpoint_init);
-
-
 /* 'ckpt_debug_level' controls the verbosity level of c/r code */
 #ifdef CONFIG_CHECKPOINT_DEBUG
 
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index afdab5e..6ea8afb 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -28,6 +28,8 @@
 #include <linux/uaccess.h>
 #include <linux/bitops.h>
 #include <linux/devpts_fs.h>
+#include <linux/file.h>
+#include <linux/mount.h>
 
 #include <asm/system.h>
 
@@ -114,6 +116,13 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf,
 {
 	struct tty_struct *to = tty->link;
 	int c;
+	int debug = (tty->index > 0);
+
+	if (debug)
+		printk(KERN_NOTICE "%s: called on %s idx %d buf .%s. count %d\n",
+			__func__,
+			tty->driver->subtype == PTY_TYPE_MASTER ? "master" : "slave",
+			tty->index, buf, count);
 
 	if (tty->stopped)
 		return 0;
@@ -122,6 +131,8 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf,
 	   big deal */
 
 	c = pty_space(to);
+	if (debug)
+		printk(KERN_NOTICE "%s: space was %d\n", __func__, c);
 	if (c > count)
 		c = count;
 	if (c > 0) {
@@ -131,6 +142,8 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf,
 		tty_flip_buffer_push(to);
 		tty_wakeup(tty);
 	}
+	if (debug)
+		printk(KERN_NOTICE "%s: returning %d\n", __func__, c);
 	return c;
 }
 
@@ -196,13 +209,21 @@ static int pty_open(struct tty_struct *tty, struct file *filp)
 	if (!tty || !tty->link)
 		goto out;
 
+	printk(KERN_NOTICE "%s: tty is %pS link %pS\n",
+		__func__, tty, tty->link);
 	retval = -EIO;
-	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
+	if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+		printk(KERN_NOTICE "%s: other end closed\n", __func__);
 		goto out;
-	if (test_bit(TTY_PTY_LOCK, &tty->link->flags))
+	}
+	if (test_bit(TTY_PTY_LOCK, &tty->link->flags)) {
+		printk(KERN_NOTICE "%s: ptylock\n", __func__);
 		goto out;
-	if (tty->link->count != 1)
+	}
+	if (tty->link->count != 1) {
+		printk(KERN_NOTICE "%s: count is %d (!=1)\n", __func__, tty->link->count);
 		goto out;
+	}
 
 	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
 	set_bit(TTY_THROTTLED, &tty->flags);
@@ -629,27 +650,22 @@ static const struct tty_operations pty_unix98_ops = {
  *		allocated_ptys_lock handles the list of free pty numbers
  */
 
-static int __ptmx_open(struct inode *inode, struct file *filp)
+int ptmx_create(struct inode *inode, struct file *filp, int index)
 {
 	struct tty_struct *tty;
 	int retval;
-	int index = -1;
 
 	nonseekable_open(inode, filp);
 
-#ifdef CONFIG_CHECKPOINT
-	/* when restarting, request specific index */
-	if (current->flags & PF_RESTARTING)
-		index = (int) current->required_id;
-#endif
-	/* find a device that is not in use. */
 	index = devpts_new_index(inode, index);
+	printk(KERN_NOTICE "%s: index %d\n", __func__, index);
 	if (index < 0)
 		return index;
 
 	mutex_lock(&tty_mutex);
 	tty = tty_init_dev(ptm_driver, index, 1);
 	mutex_unlock(&tty_mutex);
+	printk(KERN_NOTICE "%s: tty is %pS\n", __func__, tty);
 
 	if (IS_ERR(tty)) {
 		retval = PTR_ERR(tty);
@@ -675,6 +691,11 @@ out:
 	return retval;
 }
 
+static int __ptmx_open(struct inode *inode, struct file *filp)
+{
+	return ptmx_create(inode, filp, UNSPECIFIED_PTY_INDEX);
+}
+
 static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	int ret;
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index b8f8d79..d363c0e 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -79,6 +79,7 @@
 #include <linux/devpts_fs.h>
 #include <linux/file.h>
 #include <linux/fdtable.h>
+#include <linux/namei.h>
 #include <linux/console.h>
 #include <linux/timer.h>
 #include <linux/ctype.h>
@@ -891,8 +892,19 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	struct tty_struct *tty;
 	struct inode *inode;
 	struct tty_ldisc *ld;
+	int debug = 0;
 
 	tty = (struct tty_struct *)file->private_data;
+	if ((tty->driver->subtype == PTY_TYPE_MASTER ||
+			tty->driver->subtype == PTY_TYPE_SLAVE) &&
+			tty->index > 0)
+		debug = 1;
+
+	if (debug)
+		printk(KERN_NOTICE "%s: called on %s idx %d count %ld\n",
+			__func__,
+			tty->driver->subtype == PTY_TYPE_MASTER ? "master" : "slave",
+			tty->index, count);
 	inode = file->f_path.dentry->d_inode;
 	if (tty_paranoia_check(tty, inode, "tty_read"))
 		return -EIO;
@@ -909,6 +921,8 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	tty_ldisc_deref(ld);
 	if (i > 0)
 		inode->i_atime = current_fs_time(inode->i_sb);
+	if (debug)
+		printk(KERN_NOTICE "%s: returning %d\n", __func__, i);
 	return i;
 }
 
@@ -2907,6 +2921,7 @@ struct file *tty_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 		clear_bit(TTY_PTY_LOCK, &tty->link->flags);
 		file = filp_open(slavepath, O_RDWR | O_NOCTTY, 0);
 		ckpt_debug("slave file %p (idnex %d)\n", file, tty->index);
+		ckpt_debug("slavelock was %d\n", slavelock);
 		if (IS_ERR(file))
 			return file;
 		if (slavelock)
@@ -2982,13 +2997,43 @@ static int restore_tty_ldisc(struct ckpt_ctx *ctx,
 	return 0;
 }
 
-#define CKPT_PTMX_PATH  "/dev/ptmx"
+struct file *pty_open_by_index(char *ptmxpath, int index)
+{
+	struct file *ptmxfile;
+	int ret;
+
+	/*
+	 * we need to pick a way to specify which devpts
+	 * mountpoint to use.  For now, we'll just use
+	 * whatever /dev/ptmx points to
+	 */
+	ptmxfile = filp_open(ptmxpath, O_RDWR|O_NOCTTY|LOOKUP_CALLER_OPEN, 0);
+	if (IS_ERR(ptmxfile))
+		return ptmxfile;
+
+	if (!file_is_ptmx(ptmxfile)) {
+		printk(KERN_NOTICE "%s: dentry was not ptmx\n", __func__);
+		fput(ptmxfile);
+		return ERR_PTR(-ENOENT);
+	}
+
+	lock_kernel();
+	ret = open_create_pty(ptmxfile, index);
+	unlock_kernel();
+	printk(KERN_NOTICE "%s: open_create_pty returned %d\n", __func__, ret);
+	if (ret) {
+		fput(ptmxfile);
+		return ERR_PTR(ret);
+	}
+	printk(KERN_NOTICE "%s: done\n", __func__);
+
+	return ptmxfile;
+}
 
 static struct tty_struct *do_restore_tty(struct ckpt_ctx *ctx)
 {
 	struct ckpt_hdr_tty *h;
 	struct tty_struct *tty = ERR_PTR(-EINVAL);
-	struct file *file = NULL;
 	int master, ret;
 
 	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TTY);
@@ -3025,30 +3070,25 @@ static struct tty_struct *do_restore_tty(struct ckpt_ctx *ctx)
 	 * specific (and probably called via tty_operations).
 	 */
 	if (master) {
-		current->required_id = h->index;
-		file = filp_open(CKPT_PTMX_PATH, O_RDWR | O_NOCTTY, 0);
-		current->required_id = CKPT_REQUIRED_NONE;
-		ckpt_debug("master file %p (obj %d)\n", file, h->file_objref);
-		if (IS_ERR(file)) {
-			tty = (struct tty_struct *) file;
+		struct file *f = pty_open_by_index("/dev/ptmx", h->index);
+		if (IS_ERR(f))
 			goto out;
-		}
+		tty = f->private_data;
 
 		/*
 		 * Add file to objhash to ensure proper cleanup later
 		 * (it isn't referenced elsewhere). Use h->file_objref
 		 * which was explicitly during checkpoint for this.
 		 */
-		ret = ckpt_obj_insert(ctx, file, h->file_objref, CKPT_OBJ_FILE);
+		ret = ckpt_obj_insert(ctx, f, h->file_objref, CKPT_OBJ_FILE);
+		fput(f);  /* objhash took an extra reference */
 		if (ret < 0) {
 			tty = ERR_PTR(ret);
-			fput(file);
 			goto out;
 		}
-
-		tty = file->private_data;
 	} else {
 		tty = ckpt_obj_fetch(ctx, h->link_objref, CKPT_OBJ_TTY);
+		ckpt_debug("restoring slave");
 		if (IS_ERR(tty))
 			goto out;
 		tty = tty->link;
@@ -3077,15 +3117,18 @@ static struct tty_struct *do_restore_tty(struct ckpt_ctx *ctx)
 	tty->winsize.ws_xpixel = h->winsize.ws_xpixel;
 	mutex_unlock(&tty->termios_mutex);
 
+	ckpt_debug("after mutex_unlock");
 	if (test_bit(TTY_HUPPED, (unsigned long *) &h->flags))
 		tty_vhangup(tty);
 	else {
 		ret = restore_tty_ldisc(ctx, tty, h);
+		ckpt_debug("restore_tty_ldisc returnd %d\n", ret);
 		if (ret < 0) {
 			tty = ERR_PTR(ret);
 			goto out;
 		}
 	}
+	ckpt_debug("at end tty is %pS", tty);
 
 	tty_kref_get(tty);
  out:
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 8921726..cfadb22 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -14,6 +14,9 @@
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
+#include <linux/fs.h>
+#include <linux/smp_lock.h>
+#include <linux/file.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
 #include <linux/tty.h>
@@ -431,13 +434,18 @@ static struct file_system_type devpts_fs_type = {
 /*
  * The normal naming convention is simply /dev/pts/<number>; this conforms
  * to the System V naming convention
+ *
+ * @ptmx_inode: inode of the /dev/ptmx file - each pty namepace has its
+ * own
+ * @req_idx: requested specific index (i.e. 5 for /dev/pts/5).  If -1,
+ * then return first available.
  */
 
 int devpts_new_index(struct inode *ptmx_inode, int req_idx)
 {
 	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
-	int index;
+	int index = req_idx;
 	int ida_ret;
 
 retry:
@@ -445,7 +453,8 @@ retry:
 		return -ENOMEM;
 
 	mutex_lock(&allocated_ptys_lock);
-	index = (req_idx >= 0 ? req_idx : 0);
+	if (index == UNSPECIFIED_PTY_INDEX)
+		index = 0;
 	ida_ret = ida_get_new_above(&fsi->allocated_ptys, index, &index);
 	if (ida_ret < 0) {
 		mutex_unlock(&allocated_ptys_lock);
@@ -454,7 +463,7 @@ retry:
 		return -EIO;
 	}
 
-	if (req_idx >= 0 && index != req_idx) {
+	if (req_idx != UNSPECIFIED_PTY_INDEX && index != req_idx) {
 		ida_remove(&fsi->allocated_ptys, index);
 		mutex_unlock(&allocated_ptys_lock);
 		return -EBUSY;
@@ -557,6 +566,51 @@ out:
 	mutex_unlock(&root->d_inode->i_mutex);
 }
 
+struct inode *get_ptmx_inode(struct super_block *sb)
+{
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct dentry *dentry = fsi->ptmx_dentry;
+	struct inode *inode = dentry->d_inode;
+
+	return igrab(inode);
+}
+
+/* defined in drivers/char/pty.c */
+extern int ptmx_create(struct inode *inode, struct file *filp, int index);
+
+/*
+ * fn caller (in restart code) has checked for the rights on the
+ * part of the userspace task to open the ptmx file.
+ * @pdir: an open file handle to the /dev/pts directory, which we
+ * will use to determine the devpts namespace, and to confirm that
+ * the resulting file is in the right directory.
+ * @file is a file for /dev/ptmx
+ */
+int open_create_pty(struct file *ptmxfile, int idx)
+{
+	return  ptmx_create(ptmxfile->f_dentry->d_inode, ptmxfile, idx);
+}
+
+int file_is_ptmx(struct file *file)
+{
+	dev_t device;
+	struct inode *inode;
+
+	if (!file->f_dentry)
+		return 0;
+	inode = file->f_dentry->d_inode;
+	if (!inode)
+		return 0;
+	device = inode->i_rdev;
+	if (!device)
+		return 0;
+	if (imajor(inode) != TTYAUX_MAJOR)
+		return 0;
+	if (iminor(inode) != 2)
+		return 0;
+	return 1;
+}
+
 static int __init init_devpts_fs(void)
 {
 	int err = register_filesystem(&devpts_fs_type);
diff --git a/fs/open.c b/fs/open.c
index dd98e80..c65229a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -924,6 +924,17 @@ out_err:
 }
 EXPORT_SYMBOL_GPL(lookup_instantiate_filp);
 
+/*
+ * Passed to nameidata_to_filp to keep it from running
+ * the file-specified open routine.
+ * We pass this along if LOOKUP_CALLER_OPEN was passed to
+ * filp_open, meaning the caller will open the file by hand
+ */
+static int __dummy_open(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
 /**
  * nameidata_to_filp - convert a nameidata to an open filp.
  * @nd: pointer to nameidata
@@ -935,13 +946,19 @@ struct file *nameidata_to_filp(struct nameidata *nd, int flags)
 {
 	const struct cred *cred = current_cred();
 	struct file *filp;
+	int (*open)(struct inode *, struct file *) = NULL;
+
+	if (flags & LOOKUP_CALLER_OPEN) {
+		printk(KERN_NOTICE "%s: I was passed LOOKUP_CALLER_OPEN\n", __func__);
+		open = __dummy_open;
+	}
 
 	/* Pick up the filp from the open intent */
 	filp = nd->intent.open.file;
 	/* Has the filesystem initialised the file for us? */
 	if (filp->f_path.dentry == NULL)
 		filp = __dentry_open(nd->path.dentry, nd->path.mnt, flags, filp,
-				     NULL, cred);
+				     open, cred);
 	else
 		path_put(&nd->path);
 	return filp;
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 5e90ef9..0cbe8c4 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -46,8 +46,6 @@
 #define CHECKPOINT_USER_FLAGS		CHECKPOINT_SUBTREE
 #define RESTART_USER_FLAGS		(RESTART_TASKSELF | RESTART_FROZEN)
 
-#define CKPT_REQUIRED_NONE  ((unsigned long) -1L)
-
 extern void exit_checkpoint(struct task_struct *tsk);
 
 extern int ckpt_kwrite(struct ckpt_ctx *ctx, void *buf, int count);
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 14948ee..ddb418d 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -15,10 +15,13 @@
 
 #include <linux/errno.h>
 
+#define UNSPECIFIED_PTY_INDEX -1
 #ifdef CONFIG_UNIX98_PTYS
 
 int devpts_new_index(struct inode *ptmx_inode, int req_idx);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
+int open_create_pty(struct file *ptmxfile, int idx);
+int file_is_ptmx(struct file *file);
 /* mknod in devpts */
 int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty);
 /* get tty structure */
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d870ae2..fae5159 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -56,6 +56,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_CREATE		0x0200
 #define LOOKUP_EXCL		0x0400
 #define LOOKUP_RENAME_TARGET	0x0800
+#define LOOKUP_CALLER_OPEN	0x1000
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f6f1350..0e67de7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1481,7 +1481,6 @@ struct task_struct {
 #endif /* CONFIG_TRACING */
 #ifdef CONFIG_CHECKPOINT
 	struct ckpt_ctx *checkpoint_ctx;
-	unsigned long required_id;
 #endif
 };