Page MenuHomeFreeBSD
Differential D15048

call racct_proc_ucred_changed() under the proc lock
ClosedPublic
Actions

Authored by avg on Apr 12 2018, 9:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 4:50 AM
Unknown Object (File)
Thu, Nov 14, 6:05 AM
Unknown Object (File)
Tue, Oct 29, 6:43 PM
Unknown Object (File)
Fri, Oct 25, 7:21 AM
Unknown Object (File)
Thu, Oct 24, 4:56 PM
Unknown Object (File)
Thu, Oct 24, 4:56 PM
Unknown Object (File)
Thu, Oct 24, 4:56 PM
Unknown Object (File)
Thu, Oct 24, 4:56 PM
View All 97 Files
Subscribers
imp

Details

Reviewers
trasz
kib
Commits
rS332816: call racct_proc_ucred_changed() under the proc lock
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

Diff Detail

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

Event Timeline

avg created this revision.Apr 12 2018, 9:59 AM
Herald added a subscriber: imp. · View Herald TranscriptApr 12 2018, 9:59 AM
Harbormaster completed remote builds in B16114: Diff 41395.Apr 12 2018, 9:59 AM
kib added inline comments.Apr 12 2018, 11:26 AM
sys/kern/kern_racct.c
1078

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

avg added inline comments.Apr 12 2018, 12:16 PM
sys/kern/kern_racct.c
1078

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 added inline comments.Apr 12 2018, 12:40 PM
sys/kern/kern_racct.c
1078

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 updated this revision to Diff 41401.Apr 12 2018, 12:42 PM

don't call rctl_proc_ucred_change with proc lock because of M_WAITOK

Harbormaster completed remote builds in B16116: Diff 41401.Apr 12 2018, 12:42 PM
avg marked 3 inline comments as done.Apr 12 2018, 12:44 PM
kib added a comment.Apr 12 2018, 1:24 PM

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 added a comment.Apr 12 2018, 2:51 PM
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 added a comment.Apr 12 2018, 3:27 PM
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 added a comment.Apr 12 2018, 3:46 PM

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 added a comment.Apr 12 2018, 4:04 PM
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 updated this revision to Diff 41421.Apr 13 2018, 6:33 AM

call rctl_proc_ucred_changed explicitly

Harbormaster completed remote builds in B16123: Diff 41421.Apr 13 2018, 6:33 AM
kib added inline comments.Apr 13 2018, 9:09 AM
sys/kern/kern_jail.c
2410

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 added inline comments.Apr 16 2018, 11:18 AM
sys/kern/kern_jail.c
2410

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

kib added inline comments.Apr 16 2018, 12:24 PM
sys/kern/kern_jail.c
2410

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 added inline comments.Apr 16 2018, 12:48 PM
sys/kern/kern_jail.c
2410

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 added inline comments.Apr 16 2018, 1:11 PM
sys/kern/kern_jail.c
2410

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 added inline comments.Apr 16 2018, 1:46 PM
sys/kern/kern_jail.c
2410

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 updated this revision to Diff 41528.Apr 16 2018, 2:34 PM

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.

Harbormaster completed remote builds in B16167: Diff 41528.Apr 16 2018, 2:34 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 20 2018, 1:08 PM
Closed by commit rS332816: call racct_proc_ucred_changed() under the proc lock (authored by avg). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
kern/
4 lines
2 lines
6 lines
8 lines
3 lines

Diff 41401

View Options

sys/kern/kern_jail.c

Loading...
View Options

sys/kern/kern_loginclass.c

Loading...
View Options

sys/kern/kern_prot.c

Loading...
View Options

sys/kern/kern_racct.c

Loading...
View Options

sys/kern/kern_rctl.c

Loading...