tty: Add get- ioctls to fetch tty status

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked. Just to be able to restore
this characteristics.

For this sake the following ioctl codes are introduced

- TIOGPKT to get packet mode state
- TIOGPTLCK to get Pty locked state
- TIOGEXCL to get Exclusive mode state

Quoted 5 lines · Cyrill Gorcunov

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked. Just to be able to restore
this characteristics.

Quoted 5 lines

For this sake the following ioctl codes are introduced

- TIOGPKT to get packet mode state
- TIOGPTLCK to get Pty locked state
- TIOGEXCL to get Exclusive mode state

Hi! I have only a comment regarding the naming. Why not to follow TIOC*?

drivers/tty/pty.c static int pty_set_lock(struct tty_struc · line 180

Quoted 5 lines

drivers/tty/pty.c +5 -0

+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng the lock setting ?

The other comment I have is that it might be better put these in now there are sysfs patches for the tty layer bouncing about to provide the needed infrastructure ?

Quoted 8 lines · Alan Cox

drivers/tty/pty.c static int pty_set_lock(struct tty_struc · line 180

Quoted 5 lines

drivers/tty/pty.c +5 -0

+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng the lock setting ?

It's the same as to set/clear this bit, isn't it? Please correct me if I'm wrong.

Quoted 3 lines

The other comment I have is that it might be better put these in now there are sysfs patches for the tty layer bouncing about to provide the needed infrastructure ?

Alan, could you please point me where these patches are living, so I would take a look and check them out

Quoted 10 lines · On Thu, 13 Sep 2012 16:54:01 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 8 lines · Alan Cox

drivers/tty/pty.c static int pty_set_lock(struct tty_struc · line 180

Quoted 5 lines

drivers/tty/pty.c +5 -0

+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng the lock setting ?

Quoted 2 lines

It's the same as to set/clear this bit, isn't it? Please correct me if I'm wrong.

So by the time you've finished the test bit and returned it to user space the answer may have changed?

Quoted 6 lines

Quoted 3 lines

The other comment I have is that it might be better put these in now there are sysfs patches for the tty layer bouncing about to provide the needed infrastructure ?

Alan, could you please point me where these patches are living, so I would take a look and check them out

linux-serial or check tty-next as I think Greg has now merged the test patch.

Quoted 10 lines · On Thu, 13 Sep 2012 16:54:01 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 8 lines · Alan Cox

Review

Quoted 5 lines

(quoted diff) +5 -0

+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng the lock setting ?

Quoted 2 lines

It's the same as to set/clear this bit, isn't it? Please correct me if I'm wrong.

So by the time you've finished the test bit and returned it to user space the answer may have changed ?

Quoted 4 lines

Quoted 3 lines

The other comment I have is that it might be better put these in now there are sysfs patches for the tty layer bouncing about to provide the needed infrastructure ?

Quoted 2 lines

Alan, could you please point me where these patches are living, so I would take a look and check them out

linux-serial or check tty-next as I think Greg has now merged the test patch.

Quoted 23 lines · Alan Cox

Quoted 10 lines · On Thu, 13 Sep 2012 16:54:01 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 8 lines · Alan Cox

Review

Quoted 5 lines

(quoted diff) +5 -0

+static int pty_get_lock(struct tty_struct *tty, int __user *arg)
+{
+	int locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	if (put_user(locked, arg))
+		return -EFAULT;

Now explain exactly how this doesn't race with another thread chanigng the lock setting ?

Quoted 2 lines

It's the same as to set/clear this bit, isn't it? Please correct me if I'm wrong.

So by the time you've finished the test bit and returned it to user space the answer may have changed ?

Quoted 4 lines

Quoted 3 lines

The other comment I have is that it might be better put these in now there are sysfs patches for the tty layer bouncing about to provide the needed infrastructure ?

Quoted 5 lines

Quoted 2 lines

Alan, could you please point me where these patches are living, so I would take a look and check them out

linux-serial or check tty-next as I think Greg has now merged the test patch.

Guys, you mean something like below? Look, I must admit I'm not really sure if I've done all locking right, and there is no need for additional kref counting on tty_struct. Could you please check if it looks more-less sane (I've tested it but still...)

---
From: Cyrill Gorcunov <[email protected]>
Subject: tty, pty: Add pty_state attribute to fetch tty flags

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked (just to be able to restore
this characteristics).

To serve this the pty_state attribute is introduced for pty devices. A typical output looks like

| [root@neptune ~]# cat /sys/devices/virtual/tty/ptmp0/pty_state | locked: 0 exclusive: 0 packet: 0

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Alan Cox <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

CC "H. Peter Anvin" <[email protected]>

CC Jiri Slaby <[email protected]>

CC Pavel Emelyanov <[email protected]>

---
 drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Index: tty.git/drivers/tty/pty.c
===================================================================

drivers/tty/pty.c +6 -0

@@ -283,6 +283,46 @@ done:
 	return 0;
 }
 
+static ssize_t pty_show_state(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int locked, exclusive, packet;

Quoted 17 lines

 drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Index: tty.git/drivers/tty/pty.c
===================================================================

drivers/tty/pty.c +6 -0

@@ -283,6 +283,46 @@ done:
 	return 0;
 }
 
+static ssize_t pty_show_state(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int locked, exclusive, packet;

drivers/tty/pty.c done: · line 301

Quoted 10 lines

drivers/tty/pty.c +10 -0

+
+	tty_lock(tty);
+	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
+	packet = tty->packet;
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
+			locked, exclusive, packet);
+}

Sysfs is one value per file, you have three values here, please make 3 files.

And document them in Documentation/ABI/.

Quoted 5 lines

Sysfs is one value per file, you have three values here, please make 3 files.

And document them in Documentation/ABI/.

Hmm, sure Greg, I'll update. Thanks!

Quoted 6 lines · Cyrill Gorcunov

Quoted 5 lines

Sysfs is one value per file, you have three values here, please make 3 files.

And document them in Documentation/ABI/.

Quoted 1 line

Hmm, sure Greg, I'll update. Thanks!

Something like below I suppose? Look, if there will be no complains on tech aspects on the patch (locking and tty refs) -- I'll update Documentation. Just want be sure I've made no mistakes here.

Another question -- would not it be worth to wrap code with CONFIG_CHECKPOINT_RESTORE?

Quoted 13 lines · Cyrill Gorcunov

Quoted 7 lines · Cyrill Gorcunov

Quoted 5 lines

Sysfs is one value per file, you have three values here, please make 3 files.

And document them in Documentation/ABI/.

Hmm, sure Greg, I'll update. Thanks!

Something like below I suppose? Look, if there will be no complains on tech aspects on the patch (locking and tty refs) -- I'll update Documentation. Just want be sure I've made no mistakes here.

Quoted 2 lines

Another question -- would not it be worth to wrap code with CONFIG_CHECKPOINT_RESTORE?

Alan's point stands, what's the use of this if it can instantly change after you read the value?

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 lines · Cyrill Gorcunov

Quoted 2 lines · Cyrill Gorcunov

Quoted 2 lines

Sysfs is one value per file, you have three values here, please

make 3

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

files.

And document them in Documentation/ABI/.

Hmm, sure Greg, I'll update. Thanks!

Quoted 8 lines

Quoted 6 lines

Something like below I suppose? Look, if there will be no complains on tech aspects on the patch (locking and tty refs) -- I'll update Documentation. Just want be sure I've made no mistakes here.

Another question -- would not it be worth to wrap code with CONFIG_CHECKPOINT_RESTORE?

Alan's point stands, what's the use of this if it can instantly

change

Quoted 1 line

after you read the value?

We use it when we do a checkpoint, ie when tasks are stopped. I think it's
close to data obtained from procfs (ie valid once you read it but can be
changed right after that operation). Maybe I should put everything to procfs, or stick back with ioctl calls?

Quoted 2 lines

changed right after that operation). Maybe I should put everything to procfs, or stick back with ioctl calls?

The problem as I see it is that you don't know if your process is the lock holder.

Quoted 22 lines · Cyrill Gorcunov

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 lines · Cyrill Gorcunov

Quoted 2 lines · Cyrill Gorcunov

Quoted 2 lines

Sysfs is one value per file, you have three values here, please

make 3

Quoted 15 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

files.

And document them in Documentation/ABI/.

Quoted 8 lines

Quoted 1 line

Hmm, sure Greg, I'll update. Thanks!

Something like below I suppose? Look, if there will be no complains on tech aspects on the patch (locking and tty refs) -- I'll update Documentation. Just want be sure I've made no mistakes here.

Another question -- would not it be worth to wrap code with CONFIG_CHECKPOINT_RESTORE?

Alan's point stands, what's the use of this if it can instantly change after you read the value?

Quoted 4 lines

We use it when we do a checkpoint, ie when tasks are stopped. I think it's close to data obtained from procfs (ie valid once you read it but can be changed right after that operation). Maybe I should put everything to procfs, or stick back with ioctl calls?

The problem as I see it is that you don't know if your process is the lock holder.

As to Alan's point on "what's the use of this if it can instantly change after you read the value" I guess it's the same as what we have when we simply set the value. Imagine we have two tasks fork'ed, first task do lock the pty while another task does unlock it immediately after that, as far as I see there is nothing preventing user space to do that, thus first task will think it has locked the peer while in real the peer remains unlocked. Same here with reading this value -- it's valid once read but can be changed right after. Isn't it?

Alan, Greg, what's opinion? This flags fetching is the same as say fetching of termios settings, once fetched they can be changed immediately, and it's up to caller what to do with termios settings. No?

Quoted 3 lines

Alan, Greg, what's opinion? This flags fetching is the same as say fetching of termios settings, once fetched they can be changed immediately, and it's up to caller what to do with termios settings. No?

I think you need to explain what you expect to be doing with it, and why it is safe in that application.

Quoted 4 lines · Alan Cox

Quoted 3 lines

Alan, Greg, what's opinion? This flags fetching is the same as say fetching of termios settings, once fetched they can be changed immediately, and it's up to caller what to do with termios settings. No?

Quoted 2 lines

I think you need to explain what you expect to be doing with it, and why it is safe in that application.

OK, it seems it was unclear from changelog. We need to know this parameters to be able to restore tty connection after checkpoint.

While we easily can fetch termios settings and such, there are few bits which are missed to expord. So this patch provides them to user-space.

Quoted 3 lines

While we easily can fetch termios settings and such, there are few bits which are missed to expord. So this patch provides them to user-space.

What bothers me (and the same applies to termios) is that you have NO idea if your particular process is the "owner" of that tty. tty users use out-of-band protocols, often implicit, to determine which process "owns" the tty state.

If you can't guarantee that ALL those processes are stopped and checkpointed/restarted, you have a huge problem.

Quoted 9 lines · H. Peter Anvin

Quoted 3 lines

While we easily can fetch termios settings and such, there are few bits which are missed to expord. So this patch provides them to user-space.

What bothers me (and the same applies to termios) is that you have NO idea if your particular process is the "owner" of that tty. tty users use out-of-band protocols, often implicit, to determine which process "owns" the tty state.

Quoted 2 lines

If you can't guarantee that ALL those processes are stopped and checkpointed/restarted, you have a huge problem.

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

Quoted 7 lines

Quoted 2 lines

If you can't guarantee that ALL those processes are stopped and checkpointed/restarted, you have a huge problem.

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

Sorry, no. I suspect you might be trying to do something that is impossible to do in a general fashion without application changes.

Quoted 8 lines · H. Peter Anvin

Quoted 7 lines

Quoted 2 lines

If you can't guarantee that ALL those processes are stopped and checkpointed/restarted, you have a huge problem.

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

Quoted 2 lines

Sorry, no. I suspect you might be trying to do something that is impossible to do in a general fashion without application changes.

I see. What if I wrap all this entries with CONFIG_CHECKPOINT_RESTORE and say add some sysctl which would enable/disbale this ioctls. Then it'll be up to user-space callers to make sure that data received is valid and can be used? The problem is that I need to fetch this bits somehow with minimal changes in kernel.

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

Quoted 4 lines

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

I suspect you can't deal with that. Which isn't to say you can't still build something useful. The query interfaces are trivial enough and beyond that it's simply a case of documenting that they are not intended to be generically "safe" but provide a spot sample.

Otherwise - TICOGPKT is specific to pty devices. Please therefore put it in the pty.c code and note that not all platforms use asm-geneic TTY ioctls so check carefully they all build.

Quoted 3 lines

Otherwise - TICOGPKT is specific to pty devices. Please therefore put it in the pty.c code and note that not all platforms use asm-geneic TTY ioctls so check carefully they all build.

I put it into tty_ioctl.c simply because TIOCPKT was there already so I thought it's good idea to keep them close to each other in code.

Quoted 6 lines

Quoted 3 lines

Otherwise - TICOGPKT is specific to pty devices. Please therefore put it in the pty.c code and note that not all platforms use asm-geneic TTY ioctls so check carefully they all build.

I put it into tty_ioctl.c simply because TIOCPKT was there already so I thought it's good idea to keep them close to each other in code.

Perhaps we should move TIOCPKT out as well?

Quoted 4 lines

Well, sure inside our tool before doing checkpoint we stop all tasks which are part of dumpee process tree. This unfortunatly doesn't apply to these ioctl calls. Peter, any idea how to deal with that?

I suspect you can't deal with that. Which isn't to say you can't still build something useful. The query interfaces are trivial enough and beyond that it's simply a case of documenting that they are not intended to be generically "safe" but provide a spot sample.

Otherwise - TICOGPKT is specific to pty devices. Please therefore put it in the pty.c code and note that not all platforms use asm-geneic TTY ioctls so check carefully they all build.

Quoted 5 lines

Guys, you mean something like below? Look, I must admit I'm not really sure if I've done all locking right, and there is no need for additional kref counting on tty_struct. Could you please check if it looks more-less sane (I've tested it but still...)

Guys, you mean something like below? Look, I must admit I'm not really sure if I've done all locking right, and there is no need for additional kref counting on tty_struct. Could you please check if it looks more-less sane (I've tested it but still...)

---
From: Cyrill Gorcunov <[email protected]>
Subject: tty, pty: Add pty_state attribute to fetch tty flags

For checkpoint/restore we need to know if tty has
exclusive or packet mode set, as well as if pty
is currently locked (just to be able to restore
this characteristics).

To serve this the pty_state attribute is introduced for pty devices. A typical output looks like

| [root@neptune ~]# cat /sys/devices/virtual/tty/ptmp0/pty_state | locked: 0 exclusive: 0 packet: 0

Signed-off-by Cyrill Gorcunov <[email protected]>

CC Alan Cox <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

CC "H. Peter Anvin" <[email protected]>

CC Jiri Slaby <[email protected]>

CC Pavel Emelyanov <[email protected]>

---
 drivers/tty/pty.c |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Index: tty.git/drivers/tty/pty.c
===================================================================

drivers/tty/pty.c +44 -2

@@ -283,6 +283,46 @@ done:
 	return 0;
 }
 
+static ssize_t pty_show_state(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tty_struct *tty = dev_get_drvdata(dev);
+	int locked, exclusive, packet;
+
+	tty_lock(tty);
+	locked = test_bit(TTY_PTY_LOCK, &tty->flags);
+	exclusive = test_bit(TTY_EXCLUSIVE, &tty->flags);
+	packet = tty->packet;
+	tty_unlock(tty);
+
+	return snprintf(buf, PAGE_SIZE, "locked: %d exclusive: %d packet: %d\n",
+			locked, exclusive, packet);
+}
+
+static DEVICE_ATTR(pty_state, S_IRUSR | S_IRGRP, pty_show_state, NULL);
+
+static struct attribute *pty_state_dev_attrs[] = {
+	&dev_attr_pty_state.attr,
+	NULL,
+};
+
+static const struct attribute_group pty_dev_attr_group = {
+	.attrs = pty_state_dev_attrs,
+};
+
+static const struct attribute_group *pty_dev_attr_groups[] = {
+	&pty_dev_attr_group,
+	NULL,
+};
+
+static int pty_register_attr(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct device *dev = tty_register_device_attr(driver, tty->index, tty->dev,
+						      (void *)tty, pty_dev_attr_groups);
+	return PTR_RET(dev);
+}
+
 /**
  *	pty_common_install		-	set up the pty pair
  *	@driver: the pty driver

@@ -351,7 +391,9 @@ static int pty_common_install(struct tty
 
 	tty_driver_kref_get(driver);
 	tty->count++;
-	return 0;
+
+	return pty_register_attr(driver, tty);
+
 err_free_termios:
 	if (legacy)
 		tty_free_termios(tty);

@@ -368,6 +410,7 @@ err:
 
 static void pty_cleanup(struct tty_struct *tty)
 {
+	tty_unregister_device(tty->driver, tty->index);
 	kfree(tty->port);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I don't know where this conversation comes from but putting ptys in sysfs in combination with the newinstance mount option is completely broken unless the device name and device number duplication is handled, which I don't see here.

Quoted 4 lines · Eric W. Biederman

I don't know where this conversation comes from but putting ptys in sysfs in combination with the newinstance mount option is completely broken unless the device name and device number duplication is

handled,

Quoted 1 line

which I don't see here.

Good point, Eric, thanks! I'll take a look on this aspect (I must admit I've missed this).

Funny... I gave you that feedback the last go around.