Message-ID: <Ytl772fRS74eIneC@bombadil.infradead.org>
Date:   Thu, 21 Jul 2022 09:16:47 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Zhang Yuchen <zhangyuchen.lcr@...edance.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Howells <dhowells@...hat.com>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Christoph Hellwig <hch@....de>,
        Muchun Song <songmuchun@...edance.com>,
        linux-api@...r.kernel.org
Cc:     keescook@...omium.org, yzaikin@...gle.com,
        songmuchun@...edance.com, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [RFC] proc: fix create timestamp of files in proc

On Thu, Jul 21, 2022 at 04:16:17PM +0800, Zhang Yuchen wrote:
> A user has reported a problem that the /proc/{pid} directory
> creation timestamp is incorrect.

The directory?

> He believes that the directory was created when the process was
> started, so the timestamp of the directory creation should also
> be when the process was created.

A quick glance at Documentation/filesystems/proc.rst reveals there
is documentation that the process creation time is the start_time in
the stat file for the pid. It makes absolutely no mention of the
directory creation time.

The directory creation time has been the way it is since linux history [0]
commit fdb2f0a59a1c7 ("[PATCH] Linux-0.97.3 (September 5, 1992)) and so this
has been the way .. since the beginning.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/

The last change was by Deepa to correct y2038 considerations through
commit 078cd8279e659 ("fs: Replace CURRENT_TIME with current_time() for
inode timestamps").

Next time you try to report something like this please be very sure
to learn to use git blame, and then git blame foo.c <commit-id>~1 and
keep doing this until you get to the root commit, this will let you
determine *how long has this been this way*. When you run into a
commit history which lands to the first git commit on linux you can
use the above linux history.git to go back further as I did.

> The file timestamp in procfs is the timestamp when the inode was
> created. If the inode of a file in procfs is reclaimed, the inode
> will be recreated when it is opened again, and the timestamp will
> be changed to the time when it was recreated.

The commit log above starts off with a report of the directory
of a PID. When does the directory of a PID change dates when its
respective start_time does not? When does this reclaim happen exactly?
Under what situation?

And if that is not happening, can you name *one* file in a process
directory under proc which does get reclaimed for, for which this
does happen?

> In other file systems, this timestamp is typically recorded in
> the file system and assigned to the inode when the inode is created.

I don't understand, which files are we reclaiming in procfs which
get re-recreated which your *user* is having issues with? What did
they report exactly, I'm *super* curious what your user reported
exactly. Do you have a bug report somewhere? Or any information
about its bug report. Can you pass it on to Muchun for peer review?
What file were they monitoring and what tool were they using which
made them realize there was a sort of issue?

> This mechanism can be confusing to users who are not familiar with it.

Why are they monitoring it? Why would a *new* inode having a different
timestamp be an issue as per existing documentation?

> For users who know this mechanism, they will choose not to trust this time.
> So the timestamp now has no meaning other than to confuse the user.

That is unfair given this is the first *user* to report confusion since
the inception of Linux, don't you think?

> It needs fixing.

A fix is for when there is an issue. You are not reporting a bug or an
issue, but you seem to be reporting something a confused user sees and
perhaps lack of documentation for something which is not even tracked
or cared for. This is the way things have been done since the beginning.
It doesn't mean things can't change, but there needs to be a good reason.

The terminology of "fix" implies something is broken. The only thing
seriouly broken here is this patch you are suggesting and the mechanism
which is enabling you to send patches for what you think are issues and
seriously wasting people's time. That seriously needs to be fixed.

> There are three solutions. We don't have to make the timestamp
> meaningful, as long as the user doesn't trust the timestamp.
> 
> 1. Add to the kernel documentation that the timestamp in PROC is
>    not reliable and do not use this timestamp.
>    The problem with this solution is that most users don't read
>    the kernel documentation and it can still be misleading.
> 
> 2. Fix it, change the timestamp of /proc/pid to the timestamp of
>    process creation.
> 
>    This raises new questions.
> 
>    a. Users don't know which kernel version is correct.
> 
>    b. This problem exists not only in the pid directory, but also
>       in other directories under procfs. It would take a lot of
>       extra work to fix them all. There are also easier ways for
>       users to get the creation time information better than this.
> 
>    c. We need to describe the specific meaning of each file under
>       proc in the kernel documentation for the creation time.
>       Because the creation time of various directories has different
>       meanings. For example, PID directory is the process creation
>       time, FD directory is the FD creation time and so on.
> 
>    d. Some files have no associated entity, such as iomem.
>       Unable to give a meaningful time.
> 
> 3. Instead of fixing it, set the timestamp in all procfs to 0.
>    Users will see it as an error and will not use it.
> 
> I think 3 is better. Any other suggestions?
>
> Signed-off-by: Zhang Yuchen <zhangyuchen.lcr@...edance.com>

The logic behind this patch is way off track, a little effort
alone should have made you reach similar conflusions as I have.
Your patch does your suggested step 3), so no way! What you are
proposing can potentially break things! Have you put some effort
into evaluating the negative possible impacts of your patch? If
not, can you do that now?  Did you even *boot* test your patch?

It makes all of the proc files go dated back to Jan 1 1970.

How can this RFC in any way shape or form have been sent with
a serious intent?

Sadly the lack of any serious consideration of the past and then
for you to easily suggest to make a new change which could easily
break existing users makes me needing to ask you to please have
one of your peers at bytedance.com such as Muchun Song to please
review your patches prior to you posting them, because otherwise
this is creating noise and quite frankly make me wonder if you
are intentially trying to break things.

Muchun Song, sorry but can you please help here ensure that your
peers don't post this level of quality of patches again? It would be
seriously appreciated.

Users exist for years without issue and now you want to change things
for a user which finds something done which is not documented and want
to purposely *really* change things for *everyone* to ways which have
0 compatibility with what users may have been expecting before.

How can you conclude this?

This suggested patch is quite alarming.

  Luis

Below is just nonsense.

> ---
>  fs/proc/base.c        | 4 +++-
>  fs/proc/inode.c       | 3 ++-
>  fs/proc/proc_sysctl.c | 3 ++-
>  fs/proc/self.c        | 3 ++-
>  fs/proc/thread_self.c | 3 ++-
>  5 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 0b72a6d8aac3..af440ef13091 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1892,6 +1892,8 @@ struct inode *proc_pid_make_inode(struct super_block *sb,
>  	struct proc_inode *ei;
>  	struct pid *pid;
>  
> +	struct timespec64 ts_zero = {0, 0};
> +
>  	/* We need a new inode */
>  
>  	inode = new_inode(sb);
> @@ -1902,7 +1904,7 @@ struct inode *proc_pid_make_inode(struct super_block *sb,
>  	ei = PROC_I(inode);
>  	inode->i_mode = mode;
>  	inode->i_ino = get_next_ino();
> -	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero;
>  	inode->i_op = &proc_def_inode_operations;
>  
>  	/*
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index fd40d60169b5..efb1c935fa8d 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -642,6 +642,7 @@ const struct inode_operations proc_link_inode_operations = {
>  struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
>  {
>  	struct inode *inode = new_inode(sb);
> +	struct timespec64 ts_zero = {0, 0};
>  
>  	if (!inode) {
>  		pde_put(de);
> @@ -650,7 +651,7 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
>  
>  	inode->i_private = de->data;
>  	inode->i_ino = de->low_ino;
> -	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero;
>  	PROC_I(inode)->pde = de;
>  	if (is_empty_pde(de)) {
>  		make_empty_dir_inode(inode);
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 021e83fe831f..c670f9d3b871 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -455,6 +455,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  	struct ctl_table_root *root = head->root;
>  	struct inode *inode;
>  	struct proc_inode *ei;
> +	struct timespec64 ts_zero = {0, 0};
>  
>  	inode = new_inode(sb);
>  	if (!inode)
> @@ -476,7 +477,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  	head->count++;
>  	spin_unlock(&sysctl_lock);
>  
> -	inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero;
>  	inode->i_mode = table->mode;
>  	if (!S_ISDIR(table->mode)) {
>  		inode->i_mode |= S_IFREG;
> diff --git a/fs/proc/self.c b/fs/proc/self.c
> index 72cd69bcaf4a..b9e572fdc27c 100644
> --- a/fs/proc/self.c
> +++ b/fs/proc/self.c
> @@ -44,9 +44,10 @@ int proc_setup_self(struct super_block *s)
>  	self = d_alloc_name(s->s_root, "self");
>  	if (self) {
>  		struct inode *inode = new_inode(s);
> +		struct timespec64 ts_zero = {0, 0};
>  		if (inode) {
>  			inode->i_ino = self_inum;
> -			inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +			inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero;
>  			inode->i_mode = S_IFLNK | S_IRWXUGO;
>  			inode->i_uid = GLOBAL_ROOT_UID;
>  			inode->i_gid = GLOBAL_ROOT_GID;
> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> index a553273fbd41..964966387da2 100644
> --- a/fs/proc/thread_self.c
> +++ b/fs/proc/thread_self.c
> @@ -44,9 +44,10 @@ int proc_setup_thread_self(struct super_block *s)
>  	thread_self = d_alloc_name(s->s_root, "thread-self");
>  	if (thread_self) {
>  		struct inode *inode = new_inode(s);
> +		struct timespec64 ts_zero = {0, 0};
>  		if (inode) {
>  			inode->i_ino = thread_self_inum;
> -			inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> +			inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero;
>  			inode->i_mode = S_IFLNK | S_IRWXUGO;
>  			inode->i_uid = GLOBAL_ROOT_UID;
>  			inode->i_gid = GLOBAL_ROOT_GID;
> -- 
> 2.30.2
> 

Powered by blists - more mailing lists