Re: [PATCH resend v2] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Peter Hurley]

Hi Cyrill,

On 04/07/2016 05:53 AM, Cyrill Gorcunov wrote:
> Currently when we checkpoint PTY connections we ignore data queued inside
> read buffer of ldisk, such data is barely lost after the restore. So we
> would like to have an ability to dump and restore it.

Do you want to be able to restore the exact state (like column and
newline markers)?

I'm wondering if a for-purpose checkpoint/restore ioctl would be more
appropriate?

A generic peek() may find other uses which could make future improvements
difficult.

Plus this ioctl() is only reading the 4k line discipline read buffer, and
not the tty buffers which could contain up to another 8k of unprocessed
input.


> Here is a new ioctl code which simply copies data from read buffer into
> the userspace without any additional processing (just like terminal is
> sitting in a raw mode).
> 
> The idea behind is when we checkpointing PTY pairs we peek this unread
> data into images on disk and on restore phase we find the linked peer
> and write it back thus reading peer will see it in read buffer.

Have you frozen every process with either end open? I could see how this would
be easy for an entire process group but what if the processes are unrelated?
How do you ensure you have the correct linked peer with multiple devpts instances?


> I tried several approaches (including simply reuse of canon_copy_from_read_buf
> and copy_from_read_buf) but it makes code more complicated, so I end up
> in plain copying of buffer data.
> 
> Strictly speaking, we could try handle it inside CRIU itself, as
> Peter Hurley proposed (switch terminal to raw mode, read data,
> and return original termios back) but in this case we may hit
> the scenario when we read data and failed to restore it back
> (due to any error) leaving the task we're dumping without
> read buffer, thus peeking data looks like be the only
> safe way.

Code comments below.


> v2:
>  - Use @char in ioctl definition.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@xxxxxxxxxxxxx>
> CC: Jiri Slaby <jslaby@xxxxxxxx>
> CC: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> CC: Andrey Vagin <avagin@xxxxxxxxxxxxx>
> CC: Pavel Emelianov <xemul@xxxxxxxxxxxxx>
> CC: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx>
> CC: Konstantin Khorenko <khorenko@xxxxxxxxxxxxx>
> ---
>  drivers/tty/n_tty.c               |   22 ++++++++++++++++++++++
>  include/uapi/asm-generic/ioctls.h |    1 +
>  2 files changed, 23 insertions(+)
> 
> Index: linux-ml.git/drivers/tty/n_tty.c
> ===================================================================
> --- linux-ml.git.orig/drivers/tty/n_tty.c
> +++ linux-ml.git/drivers/tty/n_tty.c
> @@ -2420,6 +2420,26 @@ static unsigned long inq_canon(struct n_
>  	return nr;
>  }
>  
> +static ssize_t n_tty_peek_raw(struct tty_struct *tty, unsigned char __user *buf)

function name is redundant; n_tty_peek() is sufficient.

Needs a buffer-length parameter; could return the size required to completely
copy the ldisc read buffer, if smaller than required.


> +{
> +	struct n_tty_data *ldata = tty->disc_data;
> +	ssize_t retval;
> +
> +	if (!mutex_trylock(&ldata->atomic_read_lock))
> +		return -EAGAIN;
> +
> +	down_read(&tty->termios_rwsem);

Why take these two locks if parallel operations are not expected?
Conversely, I would expect this to assert current state is TASK_TRACED.

Assuming the other pty end is halted, this ioctl would also need to
wait for the input kworker to stop:

--- >% ---
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index a946e49..744cb87 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -614,3 +614,8 @@ bool tty_buffer_cancel_work(struct tty_port *port)
 {
 	return cancel_work_sync(&port->buf.work);
 }
+
+void tty_buffer_flush_work(struct tty_port *port)
+{
+	flush_work(&port->buf.work);
+}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bf1bcdb..a541132 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -480,6 +480,7 @@ extern void tty_buffer_init(struct tty_port *port);
 extern void tty_buffer_set_lock_subclass(struct tty_port *port);
 extern bool tty_buffer_restart_work(struct tty_port *port);
 extern bool tty_buffer_cancel_work(struct tty_port *port);
+extern void tty_buffer_flush_work(struct tty_port *port);
 extern speed_t tty_termios_baud_rate(struct ktermios *termios);
 extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
 extern void tty_termios_encode_baud_rate(struct ktermios *termios,





	tty_buffer_flush_work(tty->port);

> +	retval = read_cnt(ldata);
> +	if (retval) {
> +		const unsigned char *from = read_buf_addr(ldata, ldata->read_tail);
> +		if (copy_to_user(buf, from, retval))
> +			retval = -EFAULT;
> +	}
> +	up_read(&tty->termios_rwsem);
> +	mutex_unlock(&ldata->atomic_read_lock);
> +	return retval;
> +}
> +
>  static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
>  		       unsigned int cmd, unsigned long arg)
>  {
> @@ -2437,6 +2457,8 @@ static int n_tty_ioctl(struct tty_struct
>  			retval = read_cnt(ldata);
>  		up_write(&tty->termios_rwsem);
>  		return put_user(retval, (unsigned int __user *) arg);
> +	case TIOCPEEKRAW:

As with the function name, the ioctl symbol is redundant; simply TIOCPEEK
is ok.

> +		return n_tty_peek_raw(tty, (unsigned char __user *) arg);
>  	default:
>  		return n_tty_ioctl_helper(tty, file, cmd, arg);
>  	}
> Index: linux-ml.git/include/uapi/asm-generic/ioctls.h
> ===================================================================
> --- linux-ml.git.orig/include/uapi/asm-generic/ioctls.h
> +++ linux-ml.git/include/uapi/asm-generic/ioctls.h
> @@ -77,6 +77,7 @@
>  #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
>  #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
>  #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
> +#define TIOCPEEKRAW	_IOR('T', 0x41, char)
>  
>  #define FIONCLEX	0x5450
>  #define FIOCLEX		0x5451
>