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@xxxxxxxxxx>
> --
> 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.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
