Message-ID: <c91c458e-58d0-f13a-9adb-a48a19f82107@kernel.org>
Date:   Mon, 6 Mar 2023 09:03:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Content-Language: en-US
To:     Michal Sekletar <msekleta@redhat.com>, gregkh@linuxfoundation.org
Cc:     arozansk@redhat.com, shuah@kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
References: <20230303133606.227934-1-msekleta@redhat.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH 1/2] tty: tty_io: update timestamps on all device nodes
In-Reply-To: <20230303133606.227934-1-msekleta@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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