Page MenuHomeFreeBSD
Differential D19874

Use shared vnode locks for ELF interpreter
ClosedPublic
Actions

Authored by trasz on Apr 10 2019, 11:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
May 18 2024, 5:21 PM
Unknown Object (File)
May 18 2024, 3:19 PM
Unknown Object (File)
May 12 2024, 10:28 AM
Unknown Object (File)
May 5 2024, 2:13 AM
Unknown Object (File)
Apr 24 2024, 10:42 PM
Unknown Object (File)
Jan 7 2024, 9:00 AM
Unknown Object (File)
Jan 7 2024, 9:00 AM
Unknown Object (File)
Jan 2 2024, 12:56 PM
View All 29 Files
Subscribers
imp

Details

Reviewers
kib
Commits
rS346120: Use shared vnode locks for the ELF interpreter.
Summary

Use shared vnode locks for the ELF interpreter.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23613
Build 22599: arc lint + arc unit

Event Timeline

trasz created this revision.Apr 10 2019, 11:51 AM
Herald added a subscriber: imp. · View Herald TranscriptApr 10 2019, 11:51 AM
Harbormaster completed remote builds in B23613: Diff 56043.Apr 10 2019, 11:51 AM
trasz retitled this revision from Use shared lookups for ELF interpreter to Use shared vnode locks for ELF interpreter.Apr 10 2019, 11:51 AM
trasz edited the summary of this revision. (Show Details)
kib added a comment.Apr 10 2019, 1:52 PM

Basically the relock invalidates all benefits of the shared locking.

I thought about taking excl in lookup(), and then downgrade to shared after VV_TEXT is set, but again this is nonsensical and perhormance is worse.

sys/kern/imgact_elf.c
769

This drops the vnode lock, so exec_check_permissions() above results are no longer valid.

trasz added a comment.Apr 10 2019, 2:12 PM

The relock happens pretty much never, since the function is used for loading the interpreter - which is busy all the time. I had a printf there for testing, and it's called just once, early during boot.

trasz updated this revision to Diff 56050.Apr 10 2019, 2:34 PM

Handle the fact that LK_UPGRADE might drop the lock.

Harbormaster completed remote builds in B23616: Diff 56050.Apr 10 2019, 2:34 PM
trasz marked an inline comment as done.Apr 10 2019, 2:35 PM
kib added a comment.Apr 10 2019, 2:43 PM
In D19874#426663, @trasz wrote:

The relock happens pretty much never, since the function is used for loading the interpreter - which is busy all the time. I had a printf there for testing, and it's called just once, early during boot.

It depends on the interpreter. You only see it for the default ld-elf.so.1.

sys/kern/imgact_elf.c
778

In fact again should include lookup.

trasz added a comment.Apr 10 2019, 2:57 PM
In D19874#426683, @kib wrote:
In D19874#426663, @trasz wrote:

The relock happens pretty much never, since the function is used for loading the interpreter - which is busy all the time. I had a printf there for testing, and it's called just once, early during boot.

It depends on the interpreter. You only see it for the default ld-elf.so.1.

Well, yes, it's an optimization for the common case.

trasz added inline comments.Apr 10 2019, 3:01 PM
sys/kern/imgact_elf.c
778

I'm not sure why. What is a scenario that could break it?

kib added inline comments.Apr 10 2019, 3:50 PM
sys/kern/imgact_elf.c
778

Imagine that the interpreter is being removed or renamed/overwritten while you unlocked the vnode

I hate to get aborted processes when I do 'make install' in parallel.

trasz added inline comments.Apr 10 2019, 4:21 PM
sys/kern/imgact_elf.c
778

But we keep the usecount bumped, so if it got replaced/renamed we still can access the "previous version of the file", right?

I can imagine a situation where the binary depends on the new interpreter to work (wouldn't work with the old interpreter under the same path), but if we tried to run it in parallel with upgrading it could still fail without this patch.

trasz added inline comments.Apr 10 2019, 4:38 PM
sys/kern/imgact_elf.c
778

Also, from my understanding of lookup() this LK_UPGRADE happens even without the patch - at the end of lookup() itself, and we don't do anything special to handle that.

kib added inline comments.Apr 10 2019, 5:32 PM
sys/kern/imgact_elf.c
778

Usermode has to provide relatively straight-forward guarantee: it never writes into the canonical interpreter, for everything to work. Your patch breaks this guarantee, if userspace renamed the interpreter and then wrote to the old file, it might break some process doing execve().

lookup() could need LK_UPGRADE only if something weird occured like dot lookup as the last path. Otherwise, if you look at the code right before VOP_LOOKUP() invocation, you note that last component is locked exclusively if lookup obtains a new lock (and it does for regular files).

I do not understand your desire to keep more complicated and buggy code, where restart to namei() with changed locking flag is actually simpler. IMO you do not need even to downgrade the lock after VV_TEXT is set,

trasz updated this revision to Diff 56065.Apr 10 2019, 7:09 PM

Improve after feedback from kib@.

Harbormaster completed remote builds in B23622: Diff 56065.Apr 10 2019, 7:09 PM
trasz marked 3 inline comments as done.Apr 10 2019, 7:10 PM
trasz added inline comments.
sys/kern/imgact_elf.c
778

Ah, now I get it. Thanks!

I'm not opposed to what you're saying, it's just I like to understand the "why".

trasz marked an inline comment as done.Apr 10 2019, 7:10 PM
kib accepted this revision.Apr 10 2019, 7:23 PM

Did you tested this with DEBUG_VFS_LOCKS ?

This revision is now accepted and ready to land.Apr 10 2019, 7:23 PM
trasz added a comment.Apr 11 2019, 11:16 AM
In D19874#426822, @kib wrote:

Did you tested this with DEBUG_VFS_LOCKS ?

Yes.

Closed by commit rS346120: Use shared vnode locks for the ELF interpreter. (authored by trasz). · Explain WhyApr 11 2019, 11:22 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
kern/
18 lines

Diff 56043

View Options

sys/kern/imgact_elf.c

Loading...