Page MenuHomeFreeBSD

procctl.2: Editing pass
ClosedPublic

Authored by jhb on Nov 27 2024, 4:22 PM.
Tags
None
Referenced Files
F107121849: D47782.diff
Fri, Jan 10, 11:08 AM
Unknown Object (File)
Mon, Dec 30, 7:31 PM
Unknown Object (File)
Fri, Dec 27, 9:14 AM
Unknown Object (File)
Fri, Dec 27, 8:46 AM
Unknown Object (File)
Thu, Dec 26, 9:18 PM
Unknown Object (File)
Thu, Dec 26, 9:44 AM
Unknown Object (File)
Thu, Dec 26, 9:10 AM
Unknown Object (File)
Thu, Dec 26, 7:20 AM
Subscribers

Details

Summary
  • Add some missing .Pp macros after the end of literal blocks and some lists to ensure there is a blank line before the following text.
  • Use an indent of Ds for nested lists to reduce excessive indentation and make the bodies of the nested list items easier to read.
  • Various and sundry rewordings and clarifications.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60816
Build 57700: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Nov 27 2024, 4:22 PM

I had started with just trying to add the missing blank lines, but then the indentation thing was something man was warning about when rendering as postscript and I ended up down a rabbit hole of other tweaks. I think many of these tweaks are slightly more readable or simpler to understand, but that may just be a matter of taste.

Some minor notes

lib/libsys/procctl.2
150

there's a lengthy list in betwen, so maybe it's worth being a bit verbose and having some repetition: "If this control is enabled," instead of otherwise.

231

We should mention the end of the chain perhaps

473
478
531–532

maybe unwrap this

579
581

I'm not sure what the canonical way to describe VA mapping in this context but "filled" maybe is a bit odd.

584

or "upon access to a page" maybe, one access gets you one signal right?

This revision is now accepted and ready to land.Nov 27 2024, 4:51 PM

I think many of these tweaks are slightly more readable or simpler to understand, but that may just be a matter of taste.

I think overall the tweaks improve readability / understanding. Agree some of them are a matter of taste; your edits are an improvement IMO but I've commented on a couple of things that I think still have a reasonable improvement over your take.

lib/libsys/procctl.2
86

unsplit this line?

166

Perhaps apply the same way to explain that there is the elfctl flag for the binary, as was used for ASLR?

231

It is the next sentence.

242
293
339

un-split the line

432

I would not say that dumping core is abnormal exit, but perhaps my view is too kernel-focused. Also, wouldn't this open question about dumping core in other cases? (I know that there is no other cases, but it makes an impression for reader).

497

It does not disable signal delivery, it only stops delivering the SIGTRAP described above.

579

Might be the 'end' needs some explanation.
It is bottom for grow-down, and top for grow-up.

581

backed

717

un-split

923
jhb marked 11 inline comments as done.Nov 29 2024, 4:12 PM
jhb added inline comments.
lib/libsys/procctl.2
150

The issue here is that it's more like:

If this control is enabled, the prot argument to mmap(2) is not PROT_NONE, and the elfctl(1) noprotomax flag is not set, the implicit protection is equal...

That is, in code it's:

 if (x || y || z)
    maxprot = _PROT_ALL;
else
    maxprot = prot;

That said, maybe it's worth reworking this a bit further to first identify the two cases (RWX vs == prot) and then outline when those cases are used.

293

Hmm, why the leading \? I wasn't aware of needing that (and there are several other "-1" literals in this file I'll need to fix if so).

432

Hmmm, I guess PT_COREDUMP is already covered by ptrace(2) (that is another case of coredumps that is presumably also prevented by this control though it is implicit by blocking PT_ATTACH). It is true that for a userspace programmer using this API, "abnormal exit" is redundant with a core dump so I can drop that part.

497

Yes, I will add an explicit SIGTRAP to clarify.

579

I think "the end of the growth area" is sufficient. I'm not sure we have any stacks that grow up on FreeBSD after the removal of ia64?

584

Yes, each access raises a synchronous signal. I can maybe make that a bit more explicit.

927

I wonder if it is worth adding a note up above about ASLR and just saying "ASLR support was added" here. The other note might read something like:

ASLR support provides minimal security in practice as it is based on secrets and is non-deterministic and comes at the cost of address space fragmentation.

One could also perhaps say that ASLR makes debugging harder, but I think that's not really true anymore for the most part? Is there any performance overhead from having PIE binaries instead of PDE binaries?

Suggestions and feedback from Konstantin and Ed

This revision now requires review to proceed.Nov 29 2024, 4:13 PM
lib/libsys/procctl.2
293

In the Heirloom troff user manual, 2. Font and Character Size Control 2.1. Character set.

ASCII Input             Character
−                          minus
Printed by troff      Character
–                         hyphen
The characters ´, `, and – may be input by \´, \`, and \– respectively or by their names.
579

s/end/bottom/. Apparently, grow-up stacks are not exposed to userspace.

kib added inline comments.
lib/libsys/procctl.2
86

Do you plan to fix this?

This revision is now accepted and ready to land.Dec 2 2024, 1:57 AM
emaste added inline comments.
lib/libsys/procctl.2
150

Oh yes, of course.

159

I.e., it's not possible to create a non-upgradable PROT_NONE

jhb marked 9 inline comments as done.Dec 2 2024, 2:35 PM
jhb added inline comments.
lib/libsys/procctl.2
86

Since I didn't change these lines at all I was planning to leave it alone to avoid adding noise in the diff.

159

I believe that is true, yes. However, you can use MAP_GUARD for a mapping you can't upgrade protections on via mprotect(2) alone (you have to install a new mapping via mmap(2) with MAP_FIXED).

jhb marked an inline comment as done.Dec 2 2024, 2:37 PM
jhb added inline comments.
lib/libsys/procctl.2
579

So maybe just "A stack gap is one or more virtual pages at the bottom of a MAP_STACK mapped region..."?

lib/libsys/procctl.2
86

Can be a separate commit in the series.

579

Sounds good. Do we all agree that addresses grow from smaller to larger, up?

Might be 'at the bottom (lower address) of a MAP_STACK...'

lib/libsys/procctl.2
579

The larger world of computer science does not agree and uses top/bottom for both ends at times (in particular non-call stacks tend to use top as the newest item). In some ways "end of the growth area" is probably clearer and less ambiguous than either "top" or "bottom".

lib/libsys/procctl.2
579

I am fine with any wording you prefer there.

This revision was automatically updated to reflect the committed changes.