[RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Cyrill Gorcunov]

Currently when we checkpoint PTY connections we ignore data queued inside
read buffer of ldisk, such data us barely lost after the restore. So we
would like to improve the situation and being able to dump and restore
it as well.

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 pair 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 obtain it then.

I tried several approaches (including simply reuse of
canon_copy_from_read_buf and copy_from_read_buf) but
it not always do the trick: for example if data queued
doesn't have \n symbol the input_available_p helper
returns false and I shouldn't call for the helpers
above.

Another proposal was to try to implement something
like sys_tee but for tty layer (proposed by xemul@)
but I drop this idea because it will require a lot
of rework as far as I can say (including moving
read buffer outside of the n_tty_data structure
and such).

Still this ioctl looks somewhat ugly to me so
if someone has better idea how to fetch queued
data from ldisk this would be awesome, please
share. _Any_ comments are highly appreciated.

Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
CC: Jiri Slaby <jslaby@suse.com>
CC: Peter Hurley <peter@hurleysoftware.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Andrey Vagin <avagin@virtuozzo.com>
CC: Pavel Emelianov <xemul@virtuozzo.com>
CC: Vladimir Davydov <vdavydov@virtuozzo.com>
CC: Konstantin Khorenko <khorenko@virtuozzo.com>
---
 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
@@ -2485,6 +2485,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)
+{
+	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);
+	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)
 {
@@ -2502,6 +2522,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:
+		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, void *)
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451

Re: [RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Jiri Slaby]

On 03/12/2016, 03:18 PM, Cyrill Gorcunov wrote:
> --- 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, void *)

Hi, this is not 32/64bit compatible and would need a compat ioctl number
and a wrapper. Since it is char *, could you make it just 'char'?

thanks,
-- 
js
suse labs

Re: [RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Cyrill Gorcunov]

On Sun, Mar 13, 2016 at 11:14:46AM +0100, Jiri Slaby wrote:
> On 03/12/2016, 03:18 PM, Cyrill Gorcunov wrote:
> > --- 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, void *)
> 
> Hi, this is not 32/64bit compatible and would need a compat ioctl number
> and a wrapper. Since it is char *, could you make it just 'char'?

Hi Jiri, sure, thanks! Lets wait for more feedback, maybe there some
other more elegant solution possible, which I failed to invent.

	Cyrill

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

[Cyrill Gorcunov]

On Sun, Mar 13, 2016 at 01:35:23PM +0300, Cyrill Gorcunov wrote:
> On Sun, Mar 13, 2016 at 11:14:46AM +0100, Jiri Slaby wrote:
> > On 03/12/2016, 03:18 PM, Cyrill Gorcunov wrote:
> > > --- 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, void *)
> > 
> > Hi, this is not 32/64bit compatible and would need a compat ioctl number
> > and a wrapper. Since it is char *, could you make it just 'char'?
> 
> Hi Jiri, sure, thanks! Lets wait for more feedback, maybe there some
> other more elegant solution possible, which I failed to invent.

Here is an updated one. Hopefully I didn't miss anything. Any comments
are highly appreciated.
---
From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
Subject: [PATCH v2] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

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.

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.

I tried several approaches (including simply reuse of
canon_copy_from_read_buf and copy_from_read_buf) but
it not always do the trick: for example if data queued
doesn't have \n symbol the input_available_p helper
returns false and I shouldn't call for the helpers
above. So I end up in plain copying.

v2:
 - Use @char in ioctl definition.

Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
CC: Jiri Slaby <jslaby@suse.com>
CC: Peter Hurley <peter@hurleysoftware.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Andrey Vagin <avagin@virtuozzo.com>
CC: Pavel Emelianov <xemul@virtuozzo.com>
CC: Vladimir Davydov <vdavydov@virtuozzo.com>
CC: Konstantin Khorenko <khorenko@virtuozzo.com>
---
 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
@@ -2485,6 +2485,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)
+{
+	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);
+	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)
 {
@@ -2502,6 +2522,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:
+		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

Re: [RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Peter Hurley]

Hi Cyrill,

On 03/12/2016 06:18 AM, Cyrill Gorcunov wrote:
> Currently when we checkpoint PTY connections we ignore data queued inside
> read buffer of ldisk, such data us barely lost after the restore. So we
> would like to improve the situation and being able to dump and restore
> it as well.
> 
> 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).

Maybe I'm overlooking something obvious, but why not do just that;
ie., save the termios, reset termios to raw mode and read the entire
ldisc read buffer?

Note that saving and resetting termios to raw mode is only necessary
for the slave side, as the master side is always in raw mode.

Then, two options for restore are:
1) set termios to raw mode, write to peer, restore the saved termios; or
2) restore the saved termios and write to peer.

option 1 will preserve the contents as read but not preserve the line
termination state; ie., it will be possible to read multiple lines
with a single canonical read.

option 2 will preserve the line termination state (for the most part)
but not necessarily the contents which might be re-interpreted.

These two options are not necessarily exclusive; it may be possible
to construct a mixed mode for restore based on the original saved
termios that reconstitutes both line termination state and read buffer
contents.

One thing not accounted for is the column position.

Regards,
Peter Hurley

> The idea behind is when we checkpointing PTY pair 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 obtain it then.
> 
> I tried several approaches (including simply reuse of
> canon_copy_from_read_buf and copy_from_read_buf) but
> it not always do the trick: for example if data queued
> doesn't have \n symbol the input_available_p helper
> returns false and I shouldn't call for the helpers
> above.
> 
> Another proposal was to try to implement something
> like sys_tee but for tty layer (proposed by xemul@)
> but I drop this idea because it will require a lot
> of rework as far as I can say (including moving
> read buffer outside of the n_tty_data structure
> and such).
> 
> Still this ioctl looks somewhat ugly to me so
> if someone has better idea how to fetch queued
> data from ldisk this would be awesome, please
> share. _Any_ comments are highly appreciated.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> CC: Jiri Slaby <jslaby@suse.com>
> CC: Peter Hurley <peter@hurleysoftware.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Andrey Vagin <avagin@virtuozzo.com>
> CC: Pavel Emelianov <xemul@virtuozzo.com>
> CC: Vladimir Davydov <vdavydov@virtuozzo.com>
> CC: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
>  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
> @@ -2485,6 +2485,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)
> +{
> +	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);
> +	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)
>  {
> @@ -2502,6 +2522,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:
> +		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, void *)
>  
>  #define FIONCLEX	0x5450
>  #define FIOCLEX		0x5451
> 

Re: [RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Cyrill Gorcunov]

On Wed, Mar 16, 2016 at 09:05:06PM -0700, Peter Hurley wrote:
> > 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).
> 
> Maybe I'm overlooking something obvious, but why not do just that;
> ie., save the termios, reset termios to raw mode and read the entire
> ldisc read buffer?
> 
> Note that saving and resetting termios to raw mode is only necessary
> for the slave side, as the master side is always in raw mode.

Hi Peter! Yes we can do that, but there is one significant problem:
when we read the buffer (draining it) and then during later stages
of checkpoint something fails -- we need to bring the dumping program
back to the former state so it won't notice that someone has been
dumping it. Which means code flow like

 - read ldisk buffer
 - ... some other work for checkpoint sake ...
 - ... obtained some error ...
 - write ldisk buffer back to restore original content

and if here write fails (for any reason), the program being
dupmed will loose ldisk buffer content. we simply can't allow
this to happen because otherwise checkpoint will work in
destructive way.

Moreover there is an option in criu where we allow to dump
program and leave it in running state, say someone needs
a snapshot of a state, thus we always must work in non-
destructive manner.

Sure, not modifying the kernel would be a preferred way
for me too.

> 
> Then, two options for restore are:
> 1) set termios to raw mode, write to peer, restore the saved termios; or
> 2) restore the saved termios and write to peer.
> 
> option 1 will preserve the contents as read but not preserve the line
> termination state; ie., it will be possible to read multiple lines
> with a single canonical read.
> 
> option 2 will preserve the line termination state (for the most part)
> but not necessarily the contents which might be re-interpreted.
> 
> These two options are not necessarily exclusive; it may be possible
> to construct a mixed mode for restore based on the original saved
> termios that reconstitutes both line termination state and read buffer
> contents.
> 
> One thing not accounted for is the column position.

Re: [RFC] tty: n_tty -- Add new TIOCPEEKRAW ioctl to peek unread data

[Cyrill Gorcunov]

On Thu, Mar 17, 2016 at 10:32:44AM +0300, Cyrill Gorcunov wrote:
> On Wed, Mar 16, 2016 at 09:05:06PM -0700, Peter Hurley wrote:
> > > 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).
> > 
> > Maybe I'm overlooking something obvious, but why not do just that;
> > ie., save the termios, reset termios to raw mode and read the entire
> > ldisc read buffer?
> > 
> > Note that saving and resetting termios to raw mode is only necessary
> > for the slave side, as the master side is always in raw mode.
> 
> Hi Peter! Yes we can do that, but there is one significant problem:
> when we read the buffer (draining it) and then during later stages
> of checkpoint something fails -- we need to bring the dumping program
> back to the former state so it won't notice that someone has been
> dumping it. Which means code flow like
> 
>  - read ldisk buffer
>  - ... some other work for checkpoint sake ...
>  - ... obtained some error ...
>  - write ldisk buffer back to restore original content
> 
> and if here write fails (for any reason), the program being
> dupmed will loose ldisk buffer content. we simply can't allow
> this to happen because otherwise checkpoint will work in
> destructive way.
> 
> Moreover there is an option in criu where we allow to dump
> program and leave it in running state, say someone needs
> a snapshot of a state, thus we always must work in non-
> destructive manner.
> 
> Sure, not modifying the kernel would be a preferred way
> for me too.

Ping? Guys, are there some more arguments against this ioctl?
Or some way which I missed suitable for nondestructive read
from a buffer?

> 
> > 
> > Then, two options for restore are:
> > 1) set termios to raw mode, write to peer, restore the saved termios; or
> > 2) restore the saved termios and write to peer.
> > 
> > option 1 will preserve the contents as read but not preserve the line
> > termination state; ie., it will be possible to read multiple lines
> > with a single canonical read.
> > 
> > option 2 will preserve the line termination state (for the most part)
> > but not necessarily the contents which might be re-interpreted.
> > 
> > These two options are not necessarily exclusive; it may be possible
> > to construct a mixed mode for restore based on the original saved
> > termios that reconstitutes both line termination state and read buffer
> > contents.
> > 
> > One thing not accounted for is the column position.

	Cyrill