Page MenuHomeFreeBSD

libgeom: Avoid fixed remappings of the devstat device
ClosedPublic

Authored by markj on Aug 14 2024, 3:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 6, 1:58 AM
Unknown Object (File)
Thu, Dec 5, 8:42 PM
Unknown Object (File)
Thu, Dec 5, 4:49 PM
Unknown Object (File)
Thu, Nov 14, 10:26 PM
Unknown Object (File)
Oct 16 2024, 11:33 PM
Unknown Object (File)
Oct 2 2024, 5:53 PM
Unknown Object (File)
Oct 2 2024, 10:55 AM
Unknown Object (File)
Oct 2 2024, 2:53 AM
Subscribers

Details

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 requested review of this revision.Aug 14 2024, 3:23 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 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..

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.

This revision was automatically updated to reflect the committed changes.
markj marked an inline comment as done.
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.