Page MenuHomeFreeBSD

Several hwpmc(4) fixes mostly for logging.
ClosedPublic

Authored by kib on Oct 31 2017, 8:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 29, 8:49 AM
Unknown Object (File)
Mar 7 2024, 4:56 PM
Unknown Object (File)
Dec 25 2023, 9:39 AM
Unknown Object (File)
Dec 20 2023, 1:45 AM
Unknown Object (File)
Dec 13 2023, 3:56 AM
Unknown Object (File)
Nov 10 2023, 5:04 AM
Unknown Object (File)
Nov 8 2023, 5:11 AM
Unknown Object (File)
Oct 7 2023, 4:01 AM
Subscribers

Details

Summary

Patch makes some number of random fixes for hwpmc(4) after pho applied fuzzing to the hpwmc syscall. I intend to try to split the patch into logically separated commits, but it is too hard to do now if reviewers request changes.

First, r195005 is perhaps worse than the bug it fixed. The pmclog_configure_log() runs without pmx_sx protection at all, causing too many failure modes, esp. with the flush or closelog running in parallel. So I reverted r195005 and instead I pre-create the logging process, allowing it to run after the set up succeeded, otherwise the process terminates itself.

Second, hwpmc(4) must not voluntarily call fo_close(), this causes double-close of the file. It seems to almost avoid bad consequences for pipes, but other types of files demonstrate random memory access. So remove fo_close() calls, which also do not provide the declared wake-up of waiters consistently. Instead, send a signal to the logger and configure the logger process to not block it. Since logger never returns to userspace, the signal only causes termination of the interruptible sleeps in fo_write().

Remove unneeded Giant drop inside hwpmc syscall handler.

Use designated initializers for sysent.

Do some style adjustments in the nearby code.

Reported and tested by: pho

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib added a subscriber: pho.
sys/dev/hwpmc/hwpmc_logging.c
330 ↗(On Diff #34564)

I'd consider setting ia = NULL here to avoid leaving a dangling pointer.

618 ↗(On Diff #34564)

Is there anything preventing a privileged user from sending SIGHUP to the thread from userland?

642 ↗(On Diff #34564)

This comment is bogus now.

kib marked 2 inline comments as done.Oct 31 2017, 9:43 PM
kib added inline comments.
sys/dev/hwpmc/hwpmc_logging.c
618 ↗(On Diff #34564)

I do not think so. Such user can signal the logger.

Should we care ? For instance, logging can be initiated with the pipe as a sink, and then pipe could be closed, causing the same effect. So if we should, this is a separate bug and fix.

markj added inline comments.
sys/dev/hwpmc/hwpmc_logging.c
618 ↗(On Diff #34564)

Probably it would be sufficient to ensure that we don't crash, hang or leak memory in that case. Of course, fo_write() can return an error for a variety of reasons besides SIGHUP delivery, and I can't see any issues with the error handling. So I don't think there's anything more to do.

This revision is now accepted and ready to land.Nov 1 2017, 3:36 AM
This revision was automatically updated to reflect the committed changes.