linux-kernel - Re: [PATCH] proc: Show the mountid associated with exe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250511174153.GB2023217@ZenIV>
Date: Sun, 11 May 2025 18:41:53 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Chen Linxuan <chenlinxuan@...ontech.com>
Cc: WangYuli <wangyuli@...ontech.com>, brauner@...nel.org, jack@...e.cz,
	akpm@...ux-foundation.org, tglx@...utronix.de, jlayton@...nel.org,
	frederic@...nel.org, xu.xin16@....com.cn,
	adrian.ratiu@...labora.com, lorenzo.stoakes@...cle.com,
	mingo@...nel.org, felix.moessbauer@...mens.com,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	zhanjun@...ontech.com, niecheng1@...ontech.com,
	guanwentao@...ontech.com
Subject: Re: [PATCH] proc: Show the mountid associated with exe

On Mon, May 12, 2025 at 12:19:01AM +0800, Chen Linxuan wrote:
> On Sun, May 11, 2025 at 10:22 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> 
> > Excuse me, just what is that path_get/path_put for?  If you have
> > an opened file, you do have its ->f_path pinned and unchanging.
> > Otherwise this call of path_get() would've itself been unsafe...
> 
> I am not very familiar with how these functions should be used.
> I just copied similar logic from proc_exe_link and added a path_put.
> Maybe I made a stupid mistake...

path_get() grabs an extra reference to mount and dentry; that is enough
to guarantee that their refcount will not drop to zero at least until
we drop the references.

To use it safely you need to be sure that currently refcounts are non-zero
and won't be dropped to zero right under you - for rather obvious reasons.

Your code also clearly relies upon ->f_path being unchanging - otherwise
you would've risked to drop the references not to the objects you've
grabbed earlier.

If both conditions are satisfied, what's the point of grabbing and dropping
these references in the first place?

*IF* there was a chance of mount going away under you, what would prevent
that happening right before that path_get()?

IOW, these dances with path_get()/path_put() are either nowhere near enough
or not needed at all.  As it is, ->f_path of an open file *IS* stable and
mount and dentry references in it do contribute towards mount and dentry
refcounts.  But my point is, that code does not pass the basic "how could
that possibly be right?" test.

PS: since you've mentioned proc_exe_link()... the difference there is that
we want the reference to be used by the caller.  If the file happens to
be closed just as we return (that can't happen until proc_exe_link() drops
the reference to struct file it got from get_task_exe_file(), but as soon
as fput() had been called, there's nothing to guarantee the safety),
file's contributions to refcounts are dropped, so we can't count upon them
to stay for as long as we need.  So in that case we fetch the references
out of ->f_path and pin them before we drop the file reference with fput().

PPS: I'm still not convinced regarding the usefulness of having that
information; "just because we can display it" isn't a strong argument
on its own...