Page MenuHomeFreeBSD

Run most of rtsold(8) and rtsol(8) in capability mode.
ClosedPublic

Authored by markj on Oct 15 2018, 7:19 PM.

Details

Summary

This involves a fair amount of refactoring and will be difficult to
review; apologies in advance. I will try to describe the major changes
here.

rtsold performs five operations that don't work in capability mode:

1 Using sendmsg(2) with a non-NULL msg_name to send Router
Solicitations to the all-routers multicast address of each interface.

This is solved by pre-opening a socket for each interface and pre-setting
the destination address using connect(2). This is the ifi_sock field of
each struct ifinfo. Since the set of interfaces managed by rtsol(d) is
static, this works reasonably well. These sockets are used for nothing
except to send Router Solicitations.

2 When -m is specified, reading the ND6 default router list (exposed by
a sysctl) and sending probes (IPv6 messages with no content) to them
using sendmsg(2) and msg_name.

This is solved using a libcasper service. This is implemented in
cap_probe.c. The probe capability is needed only when -m is specified.

3 Invoking resolvconf(8) and potentially another script (specified with
-O) in response to certain Router Advertisements.

This is solved using a libcasper service. The service is pretty
simplistic; rtsold will only ever invoke two different scripts (just one
if -O is not specified), so the service creates a pipe, does a
fork()+execve(), and returns the write end of the pipe to the main
process. The main process writes its input to the pipe, closes it, and
calls back into the service to reap the child. This service is
implemented in cap_script.c.

I have a way of doing this without the extra wait() capability and will
try implementing that shortly.

4 Calling syslog(3) and related functions.

This is solved using the cap_syslog capability provided by libcasper.

5 In response to a SIGUSR1, rtsold will open /var/run/rtsold.dump and
write its state there.

This is solved by pre-opening the dump file during initialization. One
downside of this is that a dump file will always be left behind after an
rtsol(d) invocation. I think this is a minor defect.

In the process I've done some code cleanup, though I've tried to refrain
from excessively polluting the review. I also switched the main loop
from poll(2) to kevent(2) since I think the result is a bit neater. I
got rid of the #ifndef SMALL conditionals which are used to omit code
from rtsol(8); the savings are minuscule, and the ifdefs hurt
readability and confuse Coverity.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Oct 15 2018, 7:19 PM
markj edited the summary of this revision. (Show Details)Oct 15 2018, 7:20 PM
markj edited the summary of this revision. (Show Details)Oct 15 2018, 7:39 PM
markj updated this revision to Diff 49233.Oct 16 2018, 5:15 PM
  • Link cap_syslog into rtsol(8)
  • Use the right name for the syslog service
oshogbo requested changes to this revision.Oct 17 2018, 10:44 PM
oshogbo added a subscriber: oshogbo.

Wow Amezing job! Thank you for working on that!

sbin/rtsol/Makefile
25 ↗(On Diff #49233)

What in case when sb build it without CASPER support?

usr.sbin/rtsold/cap_probe.c
168 ↗(On Diff #49233)

Maybe just dnvlist ?

error = (int)dnvlist_get_number(nvl, "error", 0);

231 ↗(On Diff #49233)

You can set just NULL in case of probe_limit.

usr.sbin/rtsold/cap_script.c
60 ↗(On Diff #49233)

I think correct style is:

for (argc = 0; argv[argc] != NULL; argc++)
   ;
70 ↗(On Diff #49233)

Maybe dnvlist just?

75 ↗(On Diff #49233)

Do you want to set errno in case when error = 0 ?

93 ↗(On Diff #49233)

Maybe just dnvlist ?

161 ↗(On Diff #49233)

Why _exit and not exit ?

162 ↗(On Diff #49233)

closefrom(3) ?
You probably leaking some fd.

203 ↗(On Diff #49233)

Why you want CASPER_SERVICE_STDIO? It's not looking like you are using it.

usr.sbin/rtsold/dump.c
75 ↗(On Diff #49233)

Is this necessary ?

usr.sbin/rtsold/rtsold.c
267 ↗(On Diff #49233)

What if casper is not enabled?

This revision now requires changes to proceed.Oct 17 2018, 10:44 PM
markj marked 15 inline comments as done.Oct 20 2018, 9:55 PM

Wow Amezing job! Thank you for working on that!

Thank you for the review.

sbin/rtsol/Makefile
25 ↗(On Diff #49233)

Sorry, not sure what you mean.

cap_syslog is not technically used by rtsol, only by rtsold. I could add ifdefs I suppose.

usr.sbin/rtsold/cap_script.c
75 ↗(On Diff #49233)

No reason to I guess.

161 ↗(On Diff #49233)

Because this code is running in the child process.

203 ↗(On Diff #49233)

Right, I'm just duping over the stdio descriptors anyway.

usr.sbin/rtsold/dump.c
75 ↗(On Diff #49233)

No. :)

usr.sbin/rtsold/rtsold.c
267 ↗(On Diff #49233)

Sorry, I don't follow. What does that have to do with caph_enter()?

markj updated this revision to Diff 49365.Oct 20 2018, 9:57 PM
markj marked 3 inline comments as done.

Address Mariusz's feedback.

Few more comments.

sbin/rtsol/Makefile
25 ↗(On Diff #49233)

As we discussed system can be build without Casper, so the binaries dosen't exists.

usr.sbin/rtsold/cap_script.c
161–162 ↗(On Diff #49365)

The STDERR_FILENO and STDOUT_FILENO is done already for you: See lib/libcasper/libcasper/service.c function stdnull.

usr.sbin/rtsold/rtsock.c
83 ↗(On Diff #49365)

You should check errnoS or use new functions or wait a moment when I will commit the caph_rights_limit.

84 ↗(On Diff #49365)

Are you interested in keeping the errno?

usr.sbin/rtsold/rtsol.c
147 ↗(On Diff #49365)

errno or caph_rights_limit.

196 ↗(On Diff #49365)

errno or caph.

usr.sbin/rtsold/rtsold.c
267 ↗(On Diff #49233)

When the Casper is not build you still enter the capability mode. cap_syslog will be changed to use syslog, but it will fail in sandbox.

You should probably use the caph_enter_with_casper().

markj updated this revision to Diff 49382.Oct 21 2018, 12:46 AM
markj marked 7 inline comments as done.

Handle feedback:

  • Provide implementations for script and probe APIs when casper isn't available.
  • Use caph_rights_limit().

I haven't tested this yet.

markj updated this revision to Diff 50030.Nov 5 2018, 5:35 PM

Fixes based on testing and feedback:

  • use caph_rights_limit()
  • remove empty .else blocks in Makefiles
  • fix the rescue build
oshogbo accepted this revision.Nov 18 2018, 11:45 AM
oshogbo added inline comments.
usr.sbin/rtsold/cap_script.c
60 ↗(On Diff #50030)

I would feel bather passing here const, and __DECONST on execve if still needed.

usr.sbin/rtsold/rtsock.c
84 ↗(On Diff #49365)

Ping. Close can change the errno right?

usr.sbin/rtsold/rtsol.c
207 ↗(On Diff #50030)

Are you interested in keeping errno?

usr.sbin/rtsold/rtsold.c
327 ↗(On Diff #50030)

Shouldn't all this succeed even when casper is not avilable?

This revision is now accepted and ready to land.Nov 18 2018, 11:45 AM
markj marked 7 inline comments as done.Nov 22 2018, 8:24 PM

Unfortunately, this still requires work - in some cases we need to call getifaddrs(3), which requires a sysctl.

usr.sbin/rtsold/cap_script.c
60 ↗(On Diff #50030)

It's kind of ugly to do it that way because the casper handler has to make a temporary copy of the argv before calling script_run() (because string arrays in libnv cannot contain NULL, so we have to add a terminating NULL ourselves).

usr.sbin/rtsold/rtsol.c
207 ↗(On Diff #50030)

No, we already printed the warning.

usr.sbin/rtsold/rtsold.c
327 ↗(On Diff #50030)

It does - we return 0 at the end.

markj updated this revision to Diff 50933.Nov 22 2018, 8:25 PM
markj marked an inline comment as done.
  • Address feedback.
This revision now requires review to proceed.Nov 22 2018, 8:25 PM
markj updated this revision to Diff 51547.Dec 3 2018, 7:39 PM

Address remaining issues:

  • Generalize the probe service to all unconnected sendmsg() calls. I had been creating one pre-connected socket per interface, but that only works if the interface is up when rtsold starts. If it comes up after we enter capability mode, we're stuck.
  • We can't call getifaddrs() in capability mode. (The SIOCGIFINFO ioctl does effectively the same thing, though, and is permitted in capability mode.) For now, just add yet another casper service to get the address flags of the link-local address on a specified interface.
oshogbo accepted this revision.Jan 2 2019, 4:29 PM
This revision is now accepted and ready to land.Jan 2 2019, 4:29 PM
This revision was automatically updated to reflect the committed changes.