Page MenuHomeFreeBSD

nlsysevent: add a genetlink(4) module to report kernel events
ClosedPublic

Authored by bapt on Nov 30 2022, 5:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 2:55 AM
Unknown Object (File)
Thu, Mar 14, 2:59 PM
Unknown Object (File)
Thu, Mar 14, 2:59 PM
Unknown Object (File)
Thu, Mar 14, 10:31 AM
Unknown Object (File)
Mon, Mar 11, 2:54 AM
Unknown Object (File)
Mon, Mar 11, 2:54 AM
Unknown Object (File)
Sun, Mar 10, 11:51 PM
Unknown Object (File)
Sun, Mar 10, 11:51 PM

Details

Summary

Hooked to devctl_notify, this allows consumers to received events
by subscribing to a system over a generic netlink protocol

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bapt requested review of this revision.Nov 30 2022, 5:06 PM

note that it requires: https://reviews.freebsd.org/D37573
Also independant from this, it show inconsistencies in the devctl_notify "systems"
Userland example of usage will follow later

Generally LGTM!
Please see some minor comments inline.

sys/netlink/netlink_sysevent.c
39

That's an internal header, which shouldn't be used here.
Are there any places where it's actually required? I'm happy to help remove any dependencies.

88

I'd suggest using NL_LOG() for consistency in all reporting. It's easy to setup:

#define DEBUG_MOD_NAME  nl_sysevent
#define DEBUG_MAX_LEVEL LOG_DEBUG3
#include <netlink/netlink_debug.h>
_DECLARE_DEBUG(LOG_DEBUG3);

Also, given it's not a hot path, I'd probably skip predictors to improve readability.

91

Maybe it's worth splitting the function in two, with the first part getting the proper group, setting vnet & calling the second?

97

nlmsg_type should be equal to the family id allocated by the module (e.g. ctrl_family_id).
nlmsg_flags is already initialized to zero - that's guaranteed by the {} initializer.

110

I'd still allocate some CMD (NLS_NEWEVENT? ) to make it non-zero.

126

I'd probably consider renaming CTRL_ prefix to something like NLEVT_ or similar (or even remove it),
so it won't clash with genetlink control family.

bapt marked 6 inline comments as done.Dec 1 2022, 8:11 AM

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

sys/netlink/netlink_sysevent.c
86

I'd suggest either removing it or converting it to NL_LOG().

94

Nit: the message will be prefixed by [nl_sysevent] sysevent_send_to_group , so probably worth skipping 'nlsysevent: '.

Also: each memory allocation failure will be reported by the underlying nlmsg_refill_buffer(). Currently, it's not verbose, but this will change.

123

Nit: no prefix needed, '\n' is redundant

136
sys/netlink/netlink_sysevent.h
40

Nit: maybe NLSE_ATTR_MAX ?

This revision is now accepted and ready to land.Dec 1 2022, 11:29 AM

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

I don't know, so far I don't see any message going through devctl_notify which should be vnet aware, but let's see how this get used in the future

In D37574#854409, @bapt wrote:

Thank you for addressing the comments!
Added a couple of nitpicks that can be addressed at the commit time.

Q: what do you think of providing these notifications to each VNET? This can be addressed later, especially given the support should be implemented in the core netlink code. For now, I'm more curious about the concept.

I don't know, so far I don't see any message going through devctl_notify which should be vnet aware, but let's see how this get used in the future

Oh, sorry, let me be a bit more specific.
I'm talking about the case when a user wants to put some notification consumer to jail (for example, as a part of jail-service-by-default) with VNET. Currently, such a service won't be able to receive any notifications as it's not in the default VNET anymore.
So the questions is that if this behaviour is desired or not. If not, how should we proceed (have per-jail tunable to allow events passing etc) ?

I don't think putting some notification per jail (if not in the default vnet) is a desired behaviour.

imp requested changes to this revision.Dec 1 2022, 2:35 PM
In D37574#854445, @bapt wrote:

I don't think putting some notification per jail (if not in the default vnet) is a desired behaviour.

sys/netlink/netlink_sysevent.c
74

Hard no on this table.
devd hasn't needed it in the past and there's a lot of pain in having this list in a centralized place.
It prevents people from loading modules that have their own system class (which people have done in the past).

If you want to make this an 'enum', I'd do it old-school X11 way with
#define DEVCTL_SYSTEM_ACPI "ACPI"
while you won't get enforcement, it will be extensible.

This revision now requires changes to proceed.Dec 1 2022, 2:35 PM
sys/netlink/netlink_sysevent.c
74

that said, I'd be OK on the table if there were some plan to have a way to register all the names and call devctl_notify with a token from such a registration rather than just a bare string.

lwhsu added inline comments.
sys/netlink/netlink_sysevent.h
3

We use BSD-2-Clause now, no -FreeBSD needed.

bapt marked an inline comment as done.Jun 1 2023, 3:10 PM
bapt added inline comments.
sys/netlink/netlink_sysevent.c
74

Yes I think we do need a discussion before I push this anywhere, the way we do define "systems" here is inconsistent (see my email to hackers@) we do need to find something imho more robust. (I had no plan to push this table as is, without a discussion)

bapt marked an inline comment as done and an inline comment as not done.

address some of the melifaro's comments

more of melifaro's comments

Remove use of internal header

bapt marked 2 inline comments as done.

split the main function to simplify code and restoration of vnet

bapt marked 3 inline comments as done.Jun 1 2023, 3:48 PM
bapt marked an inline comment as done.
sys/netlink/netlink_sysevent.c
41

Is there any specific reason to have it different from the other headers?

52

Personally I read it as "maximum sysevents I can send". Maybe it's worth renaming it to something like "SYSTEVENT_TYPES" or similar?

79

I'd set some CMD here to adhere to the protocol.

96

Maybe it's worth having a separate function for finding/creating the event group. Like,

sysevent_get_group() that does exactly that. In this case there'll be no name clash between "sysevent_send" and "sysevent_write", it'll be just sysevent_write + sysevent_get_group

98

I'd probably factor out the code for creating new group into separate function to avoid duplication.

sys/netlink/netlink_sysevent.h
34

Nit: I'd add the comments to the attributes on their types and usage e.g. /* string, reporting system name */

I think this approach is an innovative way to cope with things.

This revision is now accepted and ready to land.Jun 1 2023, 6:07 PM