libgeom: Avoid fixed remappings of the devstat device
ClosedPublic
Actions

Authored by markj on Aug 14 2024, 3:23 PM.

Details

Reviewers

brooks
asomers
imp

Commits

rG1e8b00a1cbc8: libgeom: Avoid fixed remappings of the devstat device
rGd06fe346eccf: libgeom: Avoid fixed remappings of the devstat device

Summary

libgeom maintains a quasi-private mapping of /dev/devstat, which might grow
over time if new devices appear. When the mapping needs to be expanded, the
old mapping is passed as a hint, but this appears to be unnecessary.

Thus:

stop passing a hint when remapping
don't create a mapping in geom_stats_open(), as geom_stats_resync() will create it for us,
check for errors from munmap().

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Aug 14 2024, 3:23 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 14 2024, 3:23 PM

markj requested review of this revision.Aug 14 2024, 3:23 PM

Harbormaster completed remote builds in B59020: Diff 142081.Aug 14 2024, 3:23 PM

jrtc27 added a subscriber: jrtc27.Aug 14 2024, 4:41 PM

jrtc27 added inline comments.

lib/libgeom/geom_stats.c
77	This isn't using MAP_FIXED? It's just hinting to use the old address, though you're still right it's unnecessary.

markj marked an inline comment as done.Aug 14 2024, 5:26 PM

markj added inline comments.

lib/libgeom/geom_stats.c
77	Oh, duh. I confused myself when trying to understand why this fails under CheriABI (a tagged hint pointer without MAP_FIXED results in EPROT). Actually, I find that behaviour somewhat surprising. But yes, there's no apparent reason to pass a hint at all. Perhaps, in an earlier iteration of this design, geom_stats_snapshot_get() didn't make a copy of the devstat mapping but instead returned a pointer to the mapping directly. Then, geom_stats_resync() would invalidate the pointer. But, without MAP_FIXED, that can happen anyway..

markj edited the summary of this revision. (Show Details)Aug 14 2024, 5:28 PM

This LGTM. And BTW, I successfully tested it with an out-of-tree libgeom consumer.

This revision is now accepted and ready to land.Aug 15 2024, 7:05 PM

Looks good to me, modulo the err() in a library thing... but it's already a thing for too many errors in libgeom and those are kinda hard to flush out.

Closed by commit rGd06fe346eccf: libgeom: Avoid fixed remappings of the devstat device (authored by markj). · Explain WhyAug 19 2024, 4:04 PM

This revision was automatically updated to reflect the committed changes.

markj marked an inline comment as done.

markj added a commit: rGd06fe346eccf: libgeom: Avoid fixed remappings of the devstat device.

brooks added inline comments.Aug 22 2024, 9:03 PM

lib/libgeom/geom_stats.c
77	Oh, duh. I confused myself when trying to understand why this fails under CheriABI (a tagged hint pointer without MAP_FIXED results in EPROT). Actually, I find that behaviour somewhat surprising. FWIW, the reason this is an error in CheriBSD is that we currently reject non-NULL derived, untagged capabilities and I don't want success to be racy in the face of revocation. It's possible we could be more relaxed here, but I mostly feel like it's better to be a bit too rigid to push people away from bogus use of hints.

markj added a commit: rG1e8b00a1cbc8: libgeom: Avoid fixed remappings of the devstat device.Sep 3 2024, 2:55 PM

Revision Contents
Changeset List

Path

Size

lib/

libgeom/

geom_stats.c

34 lines

Diff 142175

View Options

libgeom: Avoid fixed remappings of the devstat deviceClosedPublicActions