| Message ID | 20230303133606.227934-1-msekleta@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] tty: tty_io: update timestamps on all device nodes | expand |
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:
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) { + struct tty_file_private *priv; time64_t sec = ktime_get_real_seconds(); - /* - * 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); + list_for_each_entry(priv, &tty->tty_files, list) { + struct file *filp = priv->file; + struct inode *inode = file_inode(filp); + struct timespec64 *time = tstamp == S_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:
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(-)