Re: [PATCH 1/2] tty: tty_io: update timestamps on all device nodes

Jiri Slaby
2023-03-06T08:04:11Z

On 03. 03. 23, 14:36, Michal Sekletar wrote:
> User space applications watch for timestamp changes on character device
> files in order to determine idle time of a given terminal session. For
> example, "w" program uses this information to populate the IDLE column
> of its output [1]. Similarly, systemd-logind has optional feature where
> it uses atime of the tty character device to determine if there was
> activity on the terminal associated with the logind's session object. If
> there was no activity for a configured period of time then logind will
> terminate such session [2].
>
> Now, usually (e.g. bash running on the terminal) the use of the terminal
> will update timestamps (atime and mtime) on the corresponding terminal
> character device. However, if access to the terminal, e.g. /dev/pts/0,
> is performed through magic character device /dev/tty then such access
> obviously changes the state of the terminal, however timestamps on the
> device that correspond to the terminal (/dev/pts/0) are not updated.
>
> This patch makes sure that we update timestamps on *all* character
> devices that correspond to the given tty, because outside observers (w,
> systemd-logind) are maybe checking these timestamps. Obviously, they can
> not check timestamps on /dev/tty as that has per-process meaning.
>
> [1] https://gitlab.com/procps-ng/procps/-/blob/v4.0.0/w.c#L286
> [2] https://github.com/systemd/systemd/blob/v252/NEWS#L477
>
> Signed-off-by: Michal Sekletar <msekleta@redhat.com>
> ---
> drivers/tty/tty_io.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 36fb945fdad4..48e0148b0f3e 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -101,6 +101,7 @@
> #include <linux/compat.h>
> #include <linux/uaccess.h>
> #include <linux/termios_internal.h>
> +#include <linux/fs.h>
>
> #include <linux/kbd_kern.h>
> #include <linux/vt_kern.h>
> @@ -811,18 +812,27 @@ void start_tty(struct tty_struct *tty)
> }
> EXPORT_SYMBOL(start_tty);
>
> -static void tty_update_time(struct timespec64 *time)
> +static void tty_update_time(struct tty_struct *tty, int tstamp)

Why not enum file_time_flags then?

And "tstamp" sounds weird for what it is. It should be something like
"time" or "time_flag". Or make it simply "bool mtime". And call it with
true/false.

> {
> + struct tty_file_private *priv;
> time64_t sec = ktime_get_real_seconds();

Likely should be switched to have a reverse xmas tree.

>
> - /*
> - * We only care if the two values differ in anything other than the
> - * lower three bits (i.e every 8 seconds). If so, then we can update
> - * the time of the tty device, otherwise it could be construded as a
> - * security leak to let userspace know the exact timing of the tty.
> - */
> - if ((sec ^ time->tv_sec) & ~7)
> - time->tv_sec = sec;
> + spin_lock(&tty->files_lock);

Note: this should be fine wrt write lock. Have you tried running w/
lockdep enabled?

> + list_for_each_entry(priv, &tty->tty_files, list) {
> + struct file *filp = priv->file;

I think you can inline the above ^^ to the bellow vv.

> + struct inode *inode = file_inode(filp);
> + struct timespec64 *time = tstamp == S_MTIME ? &inode->i_mtime : &inode->i_atime;

So you'd have:
struct inode *inode = file_inode(priv->file);
struct timespec64 *time = mtime ? &inode->i_mtime : &inode->i_atime;

> +
> + /*
> + * We only care if the two values differ in anything other than the
> + * lower three bits (i.e every 8 seconds). If so, then we can update
> + * the time of the tty device, otherwise it could be construded as a
> + * security leak to let userspace know the exact timing of the tty.
> + */
> + if ((sec ^ time->tv_sec) & ~7)
> + time->tv_sec = sec;
> + }
> + spin_unlock(&tty->files_lock);
> }
>
> /*
> @@ -928,7 +938,7 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
> tty_ldisc_deref(ld);
>
> if (i > 0)
> - tty_update_time(&inode->i_atime);
> + tty_update_time(tty, S_ATIME);
>
> return i;
> }
> @@ -1036,7 +1046,7 @@ static inline ssize_t do_tty_write(
> cond_resched();
> }
> if (written) {
> - tty_update_time(&file_inode(file)->i_mtime);
> + tty_update_time(tty, S_MTIME);
> ret = written;
> }
> out:

--
js
suse labs