Page MenuHomeFreeBSD
Differential D58117

kern: add fget_procdesc()
ClosedPublic
Actions

Authored by kib on Wed, Jul 8, 8:34 PM.
Tags
None
Referenced Files
F162793912: D58117.id181771.diff
Thu, Jul 16, 11:29 PM
F162778772: D58117.id181772.diff
Thu, Jul 16, 7:25 PM
F162776128: D58117.id181905.diff
Thu, Jul 16, 6:42 PM
F162775695: D58117.id181772.diff
Thu, Jul 16, 6:36 PM
F162774910: D58117.id181772.diff
Thu, Jul 16, 6:27 PM
F162763601: D58117.diff
Thu, Jul 16, 2:55 PM
Unknown Object (File)
Wed, Jul 15, 3:10 PM
Unknown Object (File)
Wed, Jul 15, 3:07 PM
View All 58 Files
Subscribers
imp
olce

Details

Reviewers
markj
kevans
Commits
rG97de8330e168: kern: add fget_procdesc()
Summary
Convert several callers to use fget_procdesc().
Eliminate procdesc_find() and directly use fget_procdesc() in
sys_pdkill().  Previous code structure required to fdrop() procdesc
while the process is locked.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Wed, Jul 8, 8:34 PM
Herald added subscribers: olce, imp. · View Herald TranscriptWed, Jul 8, 8:34 PM
kib requested review of this revision.Wed, Jul 8, 8:34 PM
kib mentioned this in D58094: ptrace(PT_ATTACH_PD).
markj added inline comments.Thu, Jul 9, 2:02 PM
sys/kern/sys_procdesc.c
117–118

Can we also use the new helper here?

kib marked an inline comment as done.Thu, Jul 9, 2:47 PM
kib added inline comments.
sys/kern/sys_procdesc.c
117–118

The current structure makes the fdrop() call under the process lock. I eliminated procdesc_find() altogether.

kib updated this revision to Diff 181604.Thu, Jul 9, 2:49 PM
kib marked an inline comment as done.
kib edited the summary of this revision. (Show Details)

Remove procdesc_find(). Use fget_procdesc() in sys_pdkill().

kib updated this revision to Diff 181617.Thu, Jul 9, 4:38 PM

Fix all places that called fdrop() under proctree_lock.
Add herald comment for fget_procdesc().

kib updated this revision to Diff 181618.Thu, Jul 9, 4:43 PM

Slightly reduce the diff size.

markj added inline comments.Thu, Jul 9, 5:45 PM
sys/kern/sys_procdesc.c
674

Shouldn't it just be, "If the process return is requested, proctree_lock must be owned"?

705

You can store to *pp before acquiring the lock.

734

After sleeping, how do you know that p hasn't been recycled?

kib updated this revision to Diff 181633.Thu, Jul 9, 7:20 PM
kib marked 3 inline comments as done.

Hold target around pddupfd() core.
Simplify the function.

Update assert about proctree_lock in fget_procdesc().

markj added inline comments.Fri, Jul 10, 2:40 PM
sys/kern/sys_procdesc.c
698

So the caller may need to fdrop() even if fget_procdesc() returns an error. Why not just make fget_procdesc() call fdrop() directly in this error case? That makes callers a bit simpler.

For instance there is a bug in sys_pdkill(): if the fget_procdesc()->fget() call fails, sys_pdkill() calls fdrop(NULL).

sys/sys/procdesc.h
114–116

The indentation here looks inconsistent.

kib marked 2 inline comments as done.Fri, Jul 10, 3:18 PM
kib added inline comments.
sys/kern/sys_procdesc.c
674

I think it should be both.

698

Because the drop might need to take the proctree_lock. I updated the comment and fixed pdkill.

kib updated this revision to Diff 181713.Fri, Jul 10, 3:19 PM
kib marked an inline comment as done.

Check for NULL fp in pdkill.
Update comment explaining why callers must drop *pfp.

kib updated this revision to Diff 181771.Sat, Jul 11, 2:23 PM

Convert kern_pdwait()

kib updated this revision to Diff 181772.Sat, Jul 11, 2:28 PM

Better conversion for kern_pdwait(), drop the req to lock proctree_lock when the process is not requested.

markj accepted this revision.Mon, Jul 13, 1:18 PM
markj added inline comments.
sys/kern/sys_procdesc.c
674

I would combine the last two sentences:

"If requested to return the process, the proctree lock must be held, and the process will be returned locked."

677
759

Extra newline?

This revision is now accepted and ready to land.Mon, Jul 13, 1:18 PM
kib marked 3 inline comments as done.Mon, Jul 13, 1:27 PM
kib added inline comments.
sys/kern/sys_procdesc.c
759

It was intended. The multi-line comment describes the execve_block+p_candebug() guarantees. After the block for which the comment is applicable, there should be a blank line.

Closed by commit rG97de8330e168: kern: add fget_procdesc() (authored by kib). · Explain WhyMon, Jul 13, 1:42 PM
This revision was automatically updated to reflect the committed changes.
kib marked an inline comment as done.
kib added a commit: rG97de8330e168: kern: add fget_procdesc().

Revision Contents
Changeset List

PathSize
sys/
kern/
10 lines
126 lines
sys/
6 lines

Diff 181604

View Options

sys/kern/kern_sig.c

Loading...
View Options

sys/kern/sys_procdesc.c

Loading...
View Options

sys/sys/procdesc.h

Loading...