Page MenuHomeFreeBSD
Differential D32611

exec: provide right hardlink name in AT_EXECPATH
ClosedPublic
Actions

Authored by kib on Oct 23 2021, 1:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 5, 10:53 PM
Unknown Object (File)
Mon, Oct 5, 3:28 AM
Unknown Object (File)
Mon, Oct 5, 1:05 AM
Unknown Object (File)
Mon, Oct 5, 12:56 AM
Unknown Object (File)
Sun, Oct 4, 11:42 AM
Unknown Object (File)
Sun, Oct 4, 11:40 AM
Unknown Object (File)
Sun, Oct 4, 11:40 AM
Unknown Object (File)
Sun, Oct 4, 11:40 AM
View All Files
Subscribers
imp
merv_soundabstractions.io

Details

Reviewers
emaste
markj
mjg
Commits
rGee92c8a842d6: sysctl kern.proc.procname: report right hardlink name
rG351d5f7fc516: exec: store parent directory and hardlink name of the binary in struct proc
rG9a0bee9f6a77: Make vn_fullpath_hardlink() externally callable
rG0c10648fbb75: exec: provide right hardlink name in AT_EXECPATH
rG15bf81f354a4: struct image_params: use bool type for boolean members
rG9d58243fbc4a: do_execve(): switch boolean locals to use bool type
rG143dba3a9142: kern_exec.c: style
Summary
For this, use vn_fullpath_hardlink() to resolve executable name for
execve(2).

This should provide the right hardlink name, used for execution, instead
of random hardlink pointing to this binary.  Also this should make the
AT_EXECNAME reliable for execve(2), since kernel only needs to resolve
parent directory path, which should always succeed (except pathological
cases like unlinking a directory).

PR:     248184
exec: store parent directory and hardlink name of the binary in struct proc
sysctl kern.proc.procname: report right hardlink name

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Oct 23 2021, 1:06 AM
Herald added a subscriber: imp. · View Herald TranscriptOct 23 2021, 1:06 AM
kib requested review of this revision.Oct 23 2021, 1:06 AM
mjg added a comment.EditedOct 23 2021, 4:02 AM

Can this only add WANTPARENT if vn_fullpath_hardlink is going to be used to begin with? The extra ref/unref on containing directory would preferably be avoided if it can be helped.

kib added a comment.Oct 23 2021, 4:28 PM
In D32611#736134, @mjg wrote:

Can this only add WANTPARENT if vn_fullpath_hardlink is going to be used to begin with? The extra ref/unref on containing directory would preferably be avoided if it can be helped.

Isn't several jmps on modern CPUs cost same as atomic? I do not want to go with saving WANTPARENT in some var, it would be too ugly and really not save any jumps.

Anyway, below is update as you asked for. I also converted imgparam to bool as well. Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/execpath

kib updated this revision to Diff 97333.Oct 23 2021, 4:29 PM

Avoid WANTPARENT if not needed, i.e. fexecve(2) or path is absolute.
Convert imgparam to use bools, and group them together.

kib planned changes to this revision.Oct 23 2021, 6:05 PM
kib updated this revision to Diff 97341.Oct 23 2021, 8:05 PM

Restore unconditional WANTPARENT: the binary' dvp is now saved in the struct proc to allow for kern.proc.pathname to return right name as well.

kib edited the summary of this revision. (Show Details)Oct 23 2021, 9:35 PM
markj added inline comments.Oct 25 2021, 1:54 PM
sys/kern/kern_exec.c
442

Initializing oldtextdvp twice. I think it is technically safe to leave newtextvp uninitialized.

706

Don't we unconditionally set nd.ni_dvp = NULL above? i.e. the dvp reference is unconditionally transferred to newtextdvp after the namei() call.

kib marked 2 inline comments as done.Oct 25 2021, 2:20 PM
kib added inline comments.
sys/kern/kern_exec.c
442

Perhaps yes, but following the mix of success/failure paths on the function exit is risky.

706

So yes this is true there, I changed the vrele to assert that ni_dvp == NULL.

I want the vrele(ni_dvp) to be left as is on exec_fail_dealloc: label. I think that technically ni_dvp cannot be non-null there, but it is too fragile to assume on the recovery path.

kib updated this revision to Diff 97401.Oct 25 2021, 2:32 PM
kib marked 2 inline comments as done.

Fix typo in NULL initialization.
Assert that ni_dvp is NULL, instead of releasing it, in the switch to interpreter' binary.

markj added inline comments.Oct 25 2021, 3:28 PM
sys/kern/kern_exec.c
442

I prefer the explicit initialization as well. I just wasn't sure if it was intentional.

706

Actually I do not understand how this works. Doesn't the use of vn_fullpath_hardlink() assume that ni_dvp != NULL?

sys/kern/kern_exit.c
437

Consider setting p->p_binname = NULL here, like we're doing for other fields.

kib marked 3 inline comments as done.Oct 25 2021, 5:50 PM
kib added inline comments.
sys/kern/kern_exec.c
706

Right, there were too many moves around.

So I did one more, lets calculate the execpath right after namei(). I like the move on its own, because it naturally fit into fexecve(2) path of calculating execpath, also removing one vnode relock. Then ni_dvp is not used anywhere except this args->fname != NULL path.

Also I think it is possible to eliminate second NDINIT() by moving first one under the same if() branch.

kib updated this revision to Diff 97418.Oct 25 2021, 6:00 PM
kib marked an inline comment as done.

Coalesce most of args->fname != NULL processing, including interpreter NDINIT() reset of nd.
As consequence, audit interpreter path and remove XXX comment.
Use nd.ni_dvp locally and only care about reference on newtextdvp.
Set p_binname to NULL on exit.

merv_soundabstractions.io added a subscriber: merv_soundabstractions.io.Oct 25 2021, 6:11 PM
markj added inline comments.Oct 25 2021, 8:26 PM
sys/kern/kern_exec.c
704–714

It took me a while to convince myself that we do not need to set imgp->vp = NULL here, but maybe it would be worthwhile anyway.

884
sys/kern/kern_proc.c
2280

There is some layering violation here I think, we are assuming that vn_fullpath_hardlink() only accesses these fields. I'm not sure how best to resolve it.

2291

If namei() fails we leak freebuf. It'll be overwritten by vn_fullpath().

kib marked 4 inline comments as done.Oct 26 2021, 1:59 AM
kib added inline comments.
sys/kern/kern_exec.c
704–714

imgp is fully reset on interpreter recycle, I will add the reset.

kib updated this revision to Diff 97446.Oct 26 2021, 2:04 AM
kib marked an inline comment as done.

Change interface for vn_fullpath_hardlink: take vp/dvp/name instead of struct nameidata *
Leaks fixes
More reshuffling

kib updated this revision to Diff 97472.Oct 26 2021, 11:42 AM

Fix vp/dvp juggling in vn_fullpath_hardlink()

markj accepted this revision.Oct 26 2021, 1:08 PM
This revision is now accepted and ready to land.Oct 26 2021, 1:08 PM
Closed by commit rG143dba3a9142: kern_exec.c: style (authored by kib). · Explain WhyOct 28 2021, 5:51 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rG143dba3a9142: kern_exec.c: style.
kib added a commit: rG9d58243fbc4a: do_execve(): switch boolean locals to use bool type.
kib added a commit: rG15bf81f354a4: struct image_params: use bool type for boolean members.
kib added a commit: rG0c10648fbb75: exec: provide right hardlink name in AT_EXECPATH.
kib added a commit: rG9a0bee9f6a77: Make vn_fullpath_hardlink() externally callable.
kib added a commit: rG351d5f7fc516: exec: store parent directory and hardlink name of the binary in struct proc.
kib added a commit: rGee92c8a842d6: sysctl kern.proc.procname: report right hardlink name.

Revision Contents
Changeset List

PathSize
sys/
kern/
125 lines
10 lines
12 lines
54 lines
6 lines
31 lines
sys/
26 lines
2 lines
3 lines

Diff 97446

View Options

sys/kern/kern_exec.c

Loading...
View Options

sys/kern/kern_exit.c

Loading...
View Options

sys/kern/kern_fork.c

Loading...
View Options

sys/kern/kern_proc.c

Loading...
View Options

sys/kern/kern_thread.c

Loading...
View Options

sys/kern/vfs_cache.c

Loading...
View Options

sys/sys/imgact.h

Loading...
View Options

sys/sys/proc.h

Loading...
View Options

sys/sys/vnode.h

Loading...