syslogd: Create syslogd libcasper service

[jfree]

Summary

Once in capability mode, resource acquisition is not possible. To
mitigate this, introduce a libcasper service that is responsible for
fetching and returning new, requested resources to syslogd.

Some resources must be packed into an nvlist to be properly transferred
between syslogd and the libcasper process. The filed_to_nvlist() and
nvlist_to_filed() functions are included to ease the packing process for
filed structures.

Two additional syslogd.h and syslogd_cap.h header files are included
with shared declarations.

[jfree]

Make sure that filed->f_type is F_FILE before adding descriptor to nvlist

[markj]

usr.sbin/syslogd/syslogd_cap.c

103

So, we're assuming that we can serialize a compiled regexp by simply copying the binary to a different process? That's fragile: it assumes that the structure doesn't contain any pointers. (And this assumption appears to be false, at a glance.)

Better would be to only ship the regexp string to syslogd and have syslogd compile the regexp locally. That is, I think we should be more selective about which pieces of the filed are sent from the casper service.

139

This line is too long.

209

Is there any reason it shouldn't be called "syslogd.config" or so? I am vaguely uneasy about putting a "*" in a process title.

[jfree]

usr.sbin/syslogd/syslogd_cap.c

209

Is there any reason it shouldn't be called "syslogd.config" or so? I am vaguely uneasy about putting a "*" in a process title.

I used the glob asterisk because this single service is used as a libcasper interface for all "compartments" of syslogd (configuration parsing and logging), not just configuration.

[jfree]

usr.sbin/syslogd/syslogd_cap.c

103

So, we're assuming that we can serialize a compiled regexp by simply copying the binary to a different process? That's fragile: it assumes that the structure doesn't contain any pointers. (And this assumption appears to be false, at a glance.)

Better would be to only ship the regexp string to syslogd and have syslogd compile the regexp locally. That is, I think we should be more selective about which pieces of the filed are sent from the casper service.

Good catch. I am astounded that I missed this and all of my prop_filter tests were still passed. Those pointer components must not be essential. I agree, though. This current solution is fragile. I will update it.

[jfree]

usr.sbin/syslogd/syslogd_cap.c

209

Changed to syslogd.casper

[jfree]

• Change service name to syslogd.casper
• Compile the regex exp in nvlist_to_prop_filter()

[markj]

usr.sbin/syslogd/syslogd.c

2447–2448

Static analyzers will complain about this unless you add an explicit /* FALLTHROUGH */ comment.

2738

There should be a blank line between declarations and code.

2754

Missing a check for memory allocation failure here.

usr.sbin/syslogd/syslogd_cap.c

73

Can this condition ever be false? We always copy pfilter->pflt_strval now.

104

There should be a blank line following declarations.

112

I dislike that we have two places in the syslogd code which compile the regex. The config parser does it, but it's not necessary, and the logic is duplicated here. I understand that it's simpler to reuse the existing code and struct filed, but I think the cleanest design is to have the config parsing code live in the casper daemon (for example, readconfigfile() and callees should live in syslog_cap_config.c) and make it return nvlists directly, instead of creating a filed which gets converted to an nvlist and then back to a filed.

I think this patch is ok as an intermediate step but we should really aim to simplify this with followup patches.

[jfree]

usr.sbin/syslogd/syslogd_cap.c

73

Can this condition ever be false? We always copy pfilter->pflt_strval now.

Yes. If the pfilter argument is set to * in cfline(), then we allocate and return an empty prop_filter in prop_filter_compile(). We could just set the f->f_prop_filter member to NULL, but there are only so many changes I wanted to make to this program.

112

I dislike that we have two places in the syslogd code which compile the regex. The config parser does it, but it's not necessary, and the logic is duplicated here. I understand that it's simpler to reuse the existing code and struct filed, but I think the cleanest design is to have the config parsing code live in the casper daemon (for example, readconfigfile() and callees should live in syslog_cap_config.c) and make it return nvlists directly, instead of creating a filed which gets converted to an nvlist and then back to a filed.

I think this patch is ok as an intermediate step but we should really aim to simplify this with followup patches.

I had originally written the configuration parsing this way. All configuration parsing code was handled in syslogd_cap_config.c and a new parseconfigfile() created an nvlist filed directly. It worked well, but you are missing an incredibly important asterisk...

You're assuming that we have libcasper support. What happens if we don't?

I ended up having two copies for everything. One parseconfigfile() that generated nvlists and one parseconfigfile() that generated fileds for builds without WITH_CASPER. Whenever I found a bug, I needed to update both copies. This was a pain in the ass, and was a mess, to say the least.

I think this solution is much more elegant when we must factor in clients that are built without WITH_CASPER. I was very careful to preserve support for builds without libcasper, but it seems that building is failing now. I should fix that...

[markj]

usr.sbin/syslogd/syslogd_cap.c

112

I ended up having two copies for everything. One parseconfigfile() that generated nvlists and one parseconfigfile() that generated fileds for builds without WITH_CASPER. Whenever I found a bug, I needed to update both copies. This was a pain in the ass, and was a mess, to say the least.

The data flow I have in mind is parseconfigfile() -> nvlist -> cap_xfer_nvlist() -> nvlist -> filed array. If casper support is compiled out, then the cap_xfer_nvlist() step doesn't happen, but the rest can stay the same. This requires more refactoring of existing syslogd code, but less code overall, I think.

That's how I tried to structure rtsold, which admittedly is simpler than syslogd: each cap_* function called by syslogd is an RPC if casper is enabled, and a direct function call if casper is disabled. The code running in the sandbox doesn't know the difference, it's just calling cap_*.

To be clear, I think what you've done is reasonable.

[jfree]

• Address Mark's style(9) comments
• Add error checking for strdup() in prop_filter_compile()

[jfree]

usr.sbin/syslogd/syslogd_cap.c

112

The data flow I have in mind is parseconfigfile() -> nvlist -> cap_xfer_nvlist() -> nvlist -> filed array. If casper support is compiled out, then the cap_xfer_nvlist() step doesn't happen, but the rest can stay the same. This requires more refactoring of existing syslogd code, but less code overall, I think.

In the end, I think what you're describing would like very similar to this patch. You're just removing the filed -> nvlist processing. You'd still need to populate a global filed list in libcasper if we wanted p_open() to work.

That's how I tried to structure rtsold, which admittedly is simpler than syslogd: each cap_* function called by syslogd is an RPC if casper is enabled, and a direct function call if casper is disabled. The code running in the sandbox doesn't know the difference, it's just calling cap_*.

Yes. I noted this pattern. D41467 does the same cap_* aliasing with macros. I thought that was clever.

[jfree]

Fix incorrect handling of the nvlist size argument for in nvlist_get_nvlist_array() and nvlist_get_binary()

Also, add assertions to make sure the returned sizes are correct.

[markj]

usr.sbin/syslogd/syslogd_cap.c

112

In the end, I think what you're describing would like very similar to this patch. You're just removing the filed -> nvlist processing. You'd still need to populate a global filed list in libcasper if we wanted p_open() to work.

Because the p_open() interface uses an integer index, the casper service just needs to cache *some* representation of the logging destinations. It doesn't need to be represented by a filed. You could just take the nvlists returned by a readconfigfile() call and cache them in a global variable.