Use shared vnode locks for ELF interpreter

[trasz]

Summary

Use shared vnode locks for the ELF interpreter.

[kib]

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 ↗

(On Diff #56043)

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

[trasz]

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]

Handle the fact that LK_UPGRADE might drop the lock.

[kib]

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

779 ↗

(On Diff #56050)

In fact again should include lookup.

[trasz]

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]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

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

[kib]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

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]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

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]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

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]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

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]

Improve after feedback from kib@.

[trasz]

sys/kern/imgact_elf.c

779 ↗

(On Diff #56050)

Ah, now I get it. Thanks!

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

[kib]

Did you tested this with DEBUG_VFS_LOCKS ?

[trasz]

In D19874#426822, @kib wrote:

Did you tested this with DEBUG_VFS_LOCKS ?

Yes.