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

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/[email protected]>

--

 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(-)

checkpoint/sys.c +0 -8

index 3db18f7..b1fdbd7 100644
@@ -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
 

drivers/char/pty.c +8 -8

index afdab5e..79e86e4 100644
@@ -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;

drivers/char/tty_io.c +1 -0

index b8f8d79..d27ec36 100644
@@ -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>

drivers/char/tty_io.c +12 -0

+{
+	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;

drivers/char/tty_io.c +10 -0

+
+	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);
+

drivers/char/tty_io.c +10 -0

+
+	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)
+{

Quoted 8 lines

drivers/char/tty_io.c +8 -0

+	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());

drivers/char/tty_io.c static int restore_tty_ldisc(struct ckpt_ctx *ctx, · line 3025

Quoted 5 lines

drivers/char/tty_io.c +5 -0

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

Quoted 10 lines

drivers/char/tty_io.c +10 -0

+
+	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);
+

drivers/char/tty_io.c static int restore_tty_ldisc(struct ckpt_ctx *ctx, · line 3045

Quoted 10 lines

drivers/char/tty_io.c +10 -0

+	/*
+	 * 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

Quoted 15 lines

drivers/char/tty_io.c +15 -0

+
+	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

drivers/char/tty_io.c static int restore_tty_ldisc(struct ckpt_ctx *ctx, · line 3065

Quoted 5 lines

drivers/char/tty_io.c +5 -0

+	 * 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)

Quoted 8 lines

(quoted diff) +8 -0

+	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());

Review

Quoted 5 lines

(quoted diff) +5 -0

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