call racct_proc_ucred_changed() under the proc lock

[avg]

Summary

The lock is required to ensure that the switch to the new credentials and the
transfer of the process's accounting data from the old credentials to the new
ones is done atomically. Otherwise, some updates may be applied to the new
credentials and the additionally transferred from the old credentials if the
updates happen after proc_set_cred() and before racct_proc_ucred_changed().

The problem is especially pronounced for RACCT_RSS because

• there is a strict accounting for this resource (it's reclaimable)
• it's updated asynchronously from the vm daemon
• it's updated by setting an absolute value instead of applying a delta

Test Plan

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=222027

[kib]

sys/kern/kern_racct.c

1078 ↗

(On Diff #41395)

Why added by commented out ? Is it due to racct_lock ?

[avg]

sys/kern/kern_racct.c

1078 ↗

(On Diff #41395)

Oh, sorry, I will remove this.
It's a left-over from my earlier tests.
I was puzzled why this function asserted MA_NOTOWNED on the proc lock. I looked at the code and could not find any reason for it. I thought that maybe it was because of something in rctl_proc_ucred_changed, so initially I dropped the lock around the call to that function. But then I commented out PROC_UNLOCK / PROC_LOCK and everything still worked, at least in my tests.

Maybe @trasz has a recollection of why he added the original assert.

[avg]

sys/kern/kern_racct.c

1078 ↗

(On Diff #41395)

Probably I just got lucky... I now see that rctl_proc_ucred_changed drops the RACCT lock in order to be able to use M_WAITOK, so I guess that holing the proc lock would also be a problem.

[avg]

don't call rctl_proc_ucred_change with proc lock because of M_WAITOK

[kib]

would it be more explicit to require racct_proc_ucred_changed() to enter with the proc locked, but to return with the proc unlocked. So that innocent user of the function did not relied on the proc lock not being dropped inside.

[avg]

In D15048#317024, @kib wrote:

would it be more explicit to require racct_proc_ucred_changed() to enter with the proc locked, but to return with the proc unlocked. So that innocent user of the function did not relied on the proc lock not being dropped inside.

I am not sure...
Your suggestion makes sense, but on the other hand I do not like the pattern where a function must be called with a lock held but returns with the lock unlocked.
Reading and maintaining such code is harder.

[kib]

In D15048#317038, @avg wrote:

In D15048#317024, @kib wrote:

would it be more explicit to require racct_proc_ucred_changed() to enter with the proc locked, but to return with the proc unlocked. So that innocent user of the function did not relied on the proc lock not being dropped inside.

I am not sure...
Your suggestion makes sense, but on the other hand I do not like the pattern where a function must be called with a lock held but returns with the lock unlocked.
Reading and maintaining such code is harder.

Maintaining the code which silently drops locks under you is much worse. I would even go as far as to say that a call to rctl_proc_ucred_changed() from proc-lock protected region is bug.

[avg]

Then, maybe we should make all current callers of racct_proc_ucred_changed call rctl_proc_ucred_changed explicitly, so that the latter is called while the proc lock is held and the former is called after the lock is dropped...

[kib]

In D15048#317048, @avg wrote:

Then, maybe we should make all current callers of racct_proc_ucred_changed call rctl_proc_ucred_changed explicitly, so that the latter is called while the proc lock is held and the former is called after the lock is dropped...

This sounds as a good idea.

[avg]

call rctl_proc_ucred_changed explicitly

[kib]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

So this change clearly demonstrate the problem: after you dropped the process lock, other thread might drop the refcount on the newcred and free the memory.

[avg]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

Ah, so the newcred is owned by the process and not this thread?
So, this thread should do crhold / crfree for the newcred?

[kib]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

Process credentials are only protected by the refcount. If you do not own neither the reference nor the process lock, other thread may change and free the p_cred structure.

td_cred is a cached reference to the p_cred, taken at the start of the syscall. It is guaranteed to be stable for the whole duration of the syscall, but might become stale. This is usually acceptable.

In the line commented, newcred is assigned as the process cred, and its only guaranteed reference is moved to the p_cred. So after the lock is dropped, you dereference the pointer for which you do not own a reference.

[avg]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

Thank you for the explanation!

In the line commented, newcred is assigned as the process cred, and its only guaranteed reference is moved to the p_cred. So after the lock is dropped, you dereference the pointer for which you do not own a reference.

So, crhold(newcred) ?

[kib]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

This, if done under the proc lock, would garantee that you operate on the non-freed memory. I have no idea is it acceptable to operate on the stale credentials.

[avg]

sys/kern/kern_jail.c

2411 ↗

(On Diff #41421)

Me neither... At least, my change should not make anything worse than before.
It seems like there is a design problem, maybe this discussion will inspire @trasz to revisit the code in question.

[avg]

hold the new cred before dropping the proc lock to ensure that
rctl_proc_ucred_changed is called with a valid (but potentially stale)
cred.

This is not perfect but is a bit of an improvement over the previous
code.